Patchwork [U-Boot] cmd_bdinfo: simplify local static funcs a bit

login
register
mail settings
Submitter Mike Frysinger
Date Oct. 31, 2011, 12:54 a.m.
Message ID <1320022463-26410-1-git-send-email-vapier@gentoo.org>
Download mbox | patch
Permalink /patch/122696/
State Accepted
Delegated to: Wolfgang Denk
Headers show

Comments

Mike Frysinger - Oct. 31, 2011, 12:54 a.m.
If we move the local funcs to the top of the file, and use the
__maybe_unused define, we can drop a lot of ugly ifdef logic and
duplicated prototypes.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 common/cmd_bdinfo.c |   89 ++++++++++++++++++--------------------------------
 1 files changed, 32 insertions(+), 57 deletions(-)
Simon Glass - Oct. 31, 2011, 7:11 p.m.
On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> If we move the local funcs to the top of the file, and use the
> __maybe_unused define, we can drop a lot of ugly ifdef logic and
> duplicated prototypes.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

This is much cleaner - is the correct style to put attribute tags on
the previous line?

Acked-by: Simon Glass <sjg@chromium.org>

> ---
>  common/cmd_bdinfo.c |   89 ++++++++++++++++++--------------------------------
>  1 files changed, 32 insertions(+), 57 deletions(-)
>
Mike Frysinger - Oct. 31, 2011, 8:44 p.m.
On Monday 31 October 2011 15:11:35 Simon Glass wrote:
> On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > If we move the local funcs to the top of the file, and use the
> > __maybe_unused define, we can drop a lot of ugly ifdef logic and
> > duplicated prototypes.
> > 
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> This is much cleaner - is the correct style to put attribute tags on
> the previous line?

when responding to add your own, there isn't any real protocol.  just normal 
e-mail etiquette (no top posting/etc...).  patchwork/humans will do the right 
thing when manually updating the changelog.
-mike
Simon Glass - Oct. 31, 2011, 8:47 p.m.
Hi Mike,

On Mon, Oct 31, 2011 at 1:44 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 31 October 2011 15:11:35 Simon Glass wrote:
>> On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> > If we move the local funcs to the top of the file, and use the
>> > __maybe_unused define, we can drop a lot of ugly ifdef logic and
>> > duplicated prototypes.
>> >
>> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>>
>> This is much cleaner - is the correct style to put attribute tags on
>> the previous line?
>
> when responding to add your own, there isn't any real protocol.  just normal
> e-mail etiquette (no top posting/etc...).  patchwork/humans will do the right
> thing when manually updating the changelog.
> -mike
>

Actually I meant the __maybe_unused tag before the function name.

Regards,
Simon
Tabi Timur-B04825 - Oct. 31, 2011, 9:15 p.m.
On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>
>
> -static void print_num(const char *, ulong);
> +__maybe_unused
> +static void print_num(const char *name, ulong value)
> +{
> +       printf("%-12s= 0x%08lX\n", name, value);
> +}
>

Will the linker remove the functions from the binary if they are unusued?
Mike Frysinger - Oct. 31, 2011, 9:38 p.m.
On Monday 31 October 2011 16:47:08 Simon Glass wrote:
> On Mon, Oct 31, 2011 at 1:44 PM, Mike Frysinger wrote:
> > On Monday 31 October 2011 15:11:35 Simon Glass wrote:
> >> On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger wrote:
> >> > If we move the local funcs to the top of the file, and use the
> >> > __maybe_unused define, we can drop a lot of ugly ifdef logic and
> >> > duplicated prototypes.
> >> > 
> >> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> >> 
> >> This is much cleaner - is the correct style to put attribute tags on
> >> the previous line?
> > 
> > when responding to add your own, there isn't any real protocol.  just
> > normal e-mail etiquette (no top posting/etc...).  patchwork/humans will
> > do the right thing when manually updating the changelog.
> 
> Actually I meant the __maybe_unused tag before the function name.

ah.  i'm not sure there is a hard rule here.  i did that because one line 
would make the func def too long to fit.
-mike
Simon Glass - Oct. 31, 2011, 9:49 p.m.
On Mon, Oct 31, 2011 at 2:15 PM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
> On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>>
>>
>> -static void print_num(const char *, ulong);
>> +__maybe_unused
>> +static void print_num(const char *name, ulong value)
>> +{
>> +       printf("%-12s= 0x%08lX\n", name, value);
>> +}
>>
>
> Will the linker remove the functions from the binary if they are unusued?

If built with -ffunction-sections and --gc-sections,, then the linker
can do this sort of thing. Otherwise it can't, but the compiler can. I
just tested Mike's code on my ARM compiler to make sure and it happily
removed print_eth() when it was not used.

Regards,
Simon

>
> --
> Timur Tabi
> Linux kernel developer at Freescale
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Mike Frysinger - Oct. 31, 2011, 10:37 p.m.
On Monday 31 October 2011 17:49:58 Simon Glass wrote:
> On Mon, Oct 31, 2011 at 2:15 PM, Tabi Timur-B04825 wrote:
> > On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger wrote:
> >> -static void print_num(const char *, ulong);
> >> +__maybe_unused
> >> +static void print_num(const char *name, ulong value)
> >> +{
> >> +       printf("%-12s= 0x%08lX\n", name, value);
> >> +}
> > 
> > Will the linker remove the functions from the binary if they are unusued?
> 
> If built with -ffunction-sections and --gc-sections,, then the linker
> can do this sort of thing. Otherwise it can't, but the compiler can. I
> just tested Mike's code on my ARM compiler to make sure and it happily
> removed print_eth() when it was not used.

i don't think you need function-sections to make this happen.  since it is 
marked "static", gcc should do DCE on it.  the __maybe_unused markings is just 
to kill off any warnings about the func not being used (which might occur in 
the #ifdef jungle below).  that attribute does not tell gcc to retain the 
function even if it isn't referenced in this file (as far as gcc can tell).
-mike
Simon Glass - Oct. 31, 2011, 10:51 p.m.
Hi Mike,

On Mon, Oct 31, 2011 at 3:37 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 31 October 2011 17:49:58 Simon Glass wrote:
>> On Mon, Oct 31, 2011 at 2:15 PM, Tabi Timur-B04825 wrote:
>> > On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger wrote:
>> >> -static void print_num(const char *, ulong);
>> >> +__maybe_unused
>> >> +static void print_num(const char *name, ulong value)
>> >> +{
>> >> +       printf("%-12s= 0x%08lX\n", name, value);
>> >> +}
>> >
>> > Will the linker remove the functions from the binary if they are unusued?
>>
>> If built with -ffunction-sections and --gc-sections,, then the linker
>> can do this sort of thing. Otherwise it can't, but the compiler can. I
>> just tested Mike's code on my ARM compiler to make sure and it happily
>> removed print_eth() when it was not used.
>
> i don't think you need function-sections to make this happen.  since it is
> marked "static", gcc should do DCE on it.  the __maybe_unused markings is just
> to kill off any warnings about the func not being used (which might occur in
> the #ifdef jungle below).  that attribute does not tell gcc to retain the
> function even if it isn't referenced in this file (as far as gcc can tell).
> -mike
>

That's right, you don't need function-sections for the compiler to
eliminate the code - my point was that the linker can't do this sort
of thing...luckily it doesn't need to.

There might be an option to control this, a bit like
-fkeep-static-consts, but I can't see it. Can't see it being very
useful anyway.

Regards,
Simon

Patch

diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
index 688b238..67cb0da 100644
--- a/common/cmd_bdinfo.c
+++ b/common/cmd_bdinfo.c
@@ -26,24 +26,45 @@ 
  */
 #include <common.h>
 #include <command.h>
+#include <linux/compiler.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static void print_num(const char *, ulong);
+__maybe_unused
+static void print_num(const char *name, ulong value)
+{
+	printf("%-12s= 0x%08lX\n", name, value);
+}
 
-#if !(defined(CONFIG_ARM) || defined(CONFIG_M68K) || defined(CONFIG_SANDBOX)) \
-	|| defined(CONFIG_CMD_NET)
-#define HAVE_PRINT_ETH
-static void print_eth(int idx);
-#endif
+__maybe_unused
+static void print_eth(int idx)
+{
+	char name[10], *val;
+	if (idx)
+		sprintf(name, "eth%iaddr", idx);
+	else
+		strcpy(name, "ethaddr");
+	val = getenv(name);
+	if (!val)
+		val = "(not set)";
+	printf("%-12s= %s\n", name, val);
+}
 
-#if (!defined(CONFIG_ARM) && !defined(CONFIG_X86) && !defined(CONFIG_SANDBOX))
-#define HAVE_PRINT_LNUM
-static void print_lnum(const char *, u64);
-#endif
+__maybe_unused
+static void print_lnum(const char *name, u64 value)
+{
+	printf("%-12s= 0x%.8llX\n", name, value);
+}
+
+__maybe_unused
+static void print_mhz(const char *name, unsigned long hz)
+{
+	char buf[32];
+
+	printf("%-12s= %6s MHz\n", name, strmhz(buf, hz));
+}
 
 #if defined(CONFIG_PPC)
-static void print_mhz(const char *, unsigned long);
 
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
@@ -208,8 +229,6 @@  int do_bdinfo(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_M68K)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	bd_t *bd = gd->bd;
@@ -257,8 +276,6 @@  int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_BLACKFIN)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	bd_t *bd = gd->bd;
@@ -377,8 +394,6 @@  int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_X86)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i;
@@ -464,46 +479,6 @@  int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  #error "a case for this architecture does not exist!"
 #endif
 
-static void print_num(const char *name, ulong value)
-{
-	printf("%-12s= 0x%08lX\n", name, value);
-}
-
-#ifdef HAVE_PRINT_ETH
-static void print_eth(int idx)
-{
-	char name[10], *val;
-	if (idx)
-		sprintf(name, "eth%iaddr", idx);
-	else
-		strcpy(name, "ethaddr");
-	val = getenv(name);
-	if (!val)
-		val = "(not set)";
-	printf("%-12s= %s\n", name, val);
-}
-#endif
-
-#ifdef HAVE_PRINT_LNUM
-static void print_lnum(const char *name, u64 value)
-{
-	printf("%-12s= 0x%.8llX\n", name, value);
-}
-#endif
-
-#if	defined(CONFIG_PPC) || \
-	defined(CONFIG_M68K) || \
-	defined(CONFIG_BLACKFIN) || \
-	defined(CONFIG_X86)
-static void print_mhz(const char *name, unsigned long hz)
-{
-	char buf[32];
-
-	printf("%-12s= %6s MHz\n", name, strmhz(buf, hz));
-}
-#endif	/* CONFIG_PPC */
-
-
 /* -------------------------------------------------------------------- */
 
 U_BOOT_CMD(