| Message ID | 1380295718-10700-1-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
On Fri, 27 Sep 2013 17:28:38 +0200 Gerhard Sittig <gsi@denx.de> wrote: > a disabled Kconfig option results in a reference to a not implemented > routine when the IS_ENABLED() macro is used for both conditional > implementation of the routine as well as a C language source code test > at the call site -- the "if (0) func();" construct only gets eliminated > later by the optimizer, while the compiler already has emitted its > warning about "func()" being undeclared applied, thanks!
Hi all, (Please keep me on the CC list, as I'm not subscribed) On Fri, Sep 27, 2013 at 05:28:38PM +0200, Gerhard Sittig wrote: > a disabled Kconfig option results in a reference to a not implemented > routine when the IS_ENABLED() macro is used for both conditional > implementation of the routine as well as a C language source code test > at the call site -- the "if (0) func();" construct only gets eliminated > later by the optimizer, while the compiler already has emitted its > warning about "func()" being undeclared > > provide an empty implementation for the mpc512x_setup_diu() and > mpc512x_init_diu() routines in case of the disabled option, to avoid the > compiler warning which is considered fatal and breaks compilation > > the bug appeared with commit 2abbbb63c90ab55ca3f054772c2e5ba7df810c48 > "powerpc/mpc512x: move common code to shared.c file", how to reproduce: > > make mpc512x_defconfig > echo CONFIG_FB_FSL_DIU=n >> .config && make olddefconfig > make > > CC arch/powerpc/platforms/512x/mpc512x_shared.o > .../arch/powerpc/platforms/512x/mpc512x_shared.c: In function 'mpc512x_init_early': > .../arch/powerpc/platforms/512x/mpc512x_shared.c:456:3: error: implicit declaration of function 'mpc512x_init_diu' [-Werror=implicit-function-declaration] > .../arch/powerpc/platforms/512x/mpc512x_shared.c: In function 'mpc512x_setup_arch': > .../arch/powerpc/platforms/512x/mpc512x_shared.c:469:3: error: implicit declaration of function 'mpc512x_setup_diu' [-Werror=implicit-function-declaration] > cc1: all warnings being treated as errors > make[4]: *** [arch/powerpc/platforms/512x/mpc512x_shared.o] Error 1 I just ran across this same compile issue, and I have a few thoughts below. > Signed-off-by: Gerhard Sittig <gsi@denx.de> > CC: <stable@vger.kernel.org> # v3.11 > > --- > arch/powerpc/platforms/512x/mpc512x_shared.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c > index a82a41b..1a7b1d0 100644 > --- a/arch/powerpc/platforms/512x/mpc512x_shared.c > +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c > @@ -303,6 +303,9 @@ void __init mpc512x_setup_diu(void) > diu_ops.release_bootmem = mpc512x_release_bootmem; > } > > +#else > +void __init mpc512x_setup_diu(void) { /* EMPTY */ } > +void __init mpc512x_init_diu(void) { /* EMPTY */ } > #endif > > void __init mpc512x_init_IRQ(void) I see an alternative solution: Can't almost all of the code in mpc512x_shared.c be declared 'static'? Then, you can get the real benefit of IS_ENABLED() by removing the #if IS_ENABLED(CONFIG_FB_FSL_DIU) from around all the DIU code, and it will automatically be removed by the compiler when it is not used. I think the current patch is necessary for immediate use, and it can be sent to stable. But I might suggest a follow-up patch or 2 that makes the functions static and kills the #ifdef entirely. Thanks, Brian
Hi, On Wed, 9 Oct 2013 12:29:31 -0700 Brian Norris <computersforpeace@gmail.com> wrote: ... > > +#else > > +void __init mpc512x_setup_diu(void) { /* EMPTY */ } > > +void __init mpc512x_init_diu(void) { /* EMPTY */ } > > #endif > > > > void __init mpc512x_init_IRQ(void) > > I see an alternative solution: > > Can't almost all of the code in mpc512x_shared.c be declared 'static'? making mpc512x_setup_diu(), mpc512x_release_bootmem(), mpc512x_valid_monitor_port() and void mpc512x_set_pixel_clock() should be okay. > Then, you can get the real benefit of IS_ENABLED() by removing the > > #if IS_ENABLED(CONFIG_FB_FSL_DIU) > > from around all the DIU code, and it will automatically be removed by > the compiler when it is not used. > > I think the current patch is necessary for immediate use, and it can be > sent to stable. But I might suggest a follow-up patch or 2 that makes > the functions static and kills the #ifdef entirely. Yes, we also have to remove CONFIG_FB_FSL_DIU ifdef in arch/powerpc/sysdev/fsl_soc.h and building should work then. Thanks, Anatolij
Hello, On Thu, Oct 10, 2013 at 9:09 AM, Anatolij Gustschin <agust@denx.de> wrote: > On Wed, 9 Oct 2013 12:29:31 -0700 > Brian Norris <computersforpeace@gmail.com> wrote: > ... >> > +#else >> > +void __init mpc512x_setup_diu(void) { /* EMPTY */ } >> > +void __init mpc512x_init_diu(void) { /* EMPTY */ } >> > #endif >> > >> > void __init mpc512x_init_IRQ(void) >> >> I see an alternative solution: >> >> Can't almost all of the code in mpc512x_shared.c be declared 'static'? > > making mpc512x_setup_diu(), mpc512x_release_bootmem(), > mpc512x_valid_monitor_port() and void mpc512x_set_pixel_clock() > should be okay. And mpc512x_init_diu()? >> Then, you can get the real benefit of IS_ENABLED() by removing the >> >> #if IS_ENABLED(CONFIG_FB_FSL_DIU) >> >> from around all the DIU code, and it will automatically be removed by >> the compiler when it is not used. >> >> I think the current patch is necessary for immediate use, and it can be >> sent to stable. But I might suggest a follow-up patch or 2 that makes >> the functions static and kills the #ifdef entirely. > > Yes, we also have to remove CONFIG_FB_FSL_DIU ifdef in > arch/powerpc/sysdev/fsl_soc.h and building should work then. Still want it around this line, probably, so we'll get compiler errors and not linker errors if someone tries to use it? extern struct platform_diu_data_ops diu_ops; But otherwise that looks OK to me. Shall I send a patch? Brian
Hello, On Thu, 10 Oct 2013 11:23:55 -0700 Brian Norris <computersforpeace@gmail.com> wrote: ... > > making mpc512x_setup_diu(), mpc512x_release_bootmem(), > > mpc512x_valid_monitor_port() and void mpc512x_set_pixel_clock() > > should be okay. > > And mpc512x_init_diu()? yes, it can be static, too. > > >> Then, you can get the real benefit of IS_ENABLED() by removing the > >> > >> #if IS_ENABLED(CONFIG_FB_FSL_DIU) > >> > >> from around all the DIU code, and it will automatically be removed by > >> the compiler when it is not used. > >> > >> I think the current patch is necessary for immediate use, and it can be > >> sent to stable. But I might suggest a follow-up patch or 2 that makes > >> the functions static and kills the #ifdef entirely. > > > > Yes, we also have to remove CONFIG_FB_FSL_DIU ifdef in > > arch/powerpc/sysdev/fsl_soc.h and building should work then. > > Still want it around this line, probably, so we'll get compiler errors > and not linker errors if someone tries to use it? > > extern struct platform_diu_data_ops diu_ops; > > But otherwise that looks OK to me. Shall I send a patch? yes, please. Thanks, Anatolij
Hi, On Thu, Oct 10, 2013 at 12:37 PM, Anatolij Gustschin <agust@denx.de> wrote: > On Thu, 10 Oct 2013 11:23:55 -0700 > Brian Norris <computersforpeace@gmail.com> wrote: >> > Yes, we also have to remove CONFIG_FB_FSL_DIU ifdef in >> > arch/powerpc/sysdev/fsl_soc.h and building should work then. >> >> Still want it around this line, probably, so we'll get compiler errors >> and not linker errors if someone tries to use it? >> >> extern struct platform_diu_data_ops diu_ops; I was wrong about this: we need to remove the #ifdef for this extern as well, because the 'diu_ops' symbol is used in either case with my patch. Sending out my patch now. Brian
diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c index a82a41b..1a7b1d0 100644 --- a/arch/powerpc/platforms/512x/mpc512x_shared.c +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c @@ -303,6 +303,9 @@ void __init mpc512x_setup_diu(void) diu_ops.release_bootmem = mpc512x_release_bootmem; } +#else +void __init mpc512x_setup_diu(void) { /* EMPTY */ } +void __init mpc512x_init_diu(void) { /* EMPTY */ } #endif void __init mpc512x_init_IRQ(void)
a disabled Kconfig option results in a reference to a not implemented routine when the IS_ENABLED() macro is used for both conditional implementation of the routine as well as a C language source code test at the call site -- the "if (0) func();" construct only gets eliminated later by the optimizer, while the compiler already has emitted its warning about "func()" being undeclared provide an empty implementation for the mpc512x_setup_diu() and mpc512x_init_diu() routines in case of the disabled option, to avoid the compiler warning which is considered fatal and breaks compilation the bug appeared with commit 2abbbb63c90ab55ca3f054772c2e5ba7df810c48 "powerpc/mpc512x: move common code to shared.c file", how to reproduce: make mpc512x_defconfig echo CONFIG_FB_FSL_DIU=n >> .config && make olddefconfig make CC arch/powerpc/platforms/512x/mpc512x_shared.o .../arch/powerpc/platforms/512x/mpc512x_shared.c: In function 'mpc512x_init_early': .../arch/powerpc/platforms/512x/mpc512x_shared.c:456:3: error: implicit declaration of function 'mpc512x_init_diu' [-Werror=implicit-function-declaration] .../arch/powerpc/platforms/512x/mpc512x_shared.c: In function 'mpc512x_setup_arch': .../arch/powerpc/platforms/512x/mpc512x_shared.c:469:3: error: implicit declaration of function 'mpc512x_setup_diu' [-Werror=implicit-function-declaration] cc1: all warnings being treated as errors make[4]: *** [arch/powerpc/platforms/512x/mpc512x_shared.o] Error 1 Signed-off-by: Gerhard Sittig <gsi@denx.de> CC: <stable@vger.kernel.org> # v3.11 --- arch/powerpc/platforms/512x/mpc512x_shared.c | 3 +++ 1 file changed, 3 insertions(+)