| Message ID | 1320022463-26410-1-git-send-email-vapier@gentoo.org |
|---|---|
| State | Accepted |
| Delegated to: | Wolfgang Denk |
| Headers | show |
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(-) >
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
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
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?
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
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 >
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
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
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(
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(-)