Re: [PATCH v2] Makefile.extrawarn: enable -Wmissing-variable-declarations for W=1

[Nathan Chancellor]

On Tue, Aug 08, 2023 at 09:01:38AM -0700, Nick Desaulniers wrote:
> On Tue, Aug 8, 2023 at 1:03 AM kernel test robot <lkp@xxxxxxxxx> wrote:
> >
> > Hi Nick,
> >
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on 52a93d39b17dc7eb98b6aa3edb93943248e03b2f]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Nick-Desaulniers/Makefile-extrawarn-enable-Wmissing-variable-declarations-for-W-1/20230808-005859
> > base:   52a93d39b17dc7eb98b6aa3edb93943248e03b2f
> > patch link:    https://lore.kernel.org/r/20230807-missing_proto-v2-1-3ae2e188bb0c%40google.com
> > patch subject: [PATCH v2] Makefile.extrawarn: enable -Wmissing-variable-declarations for W=1
> > config: arm64-randconfig-r013-20230807 (https://download.01.org/0day-ci/archive/20230808/202308081508.EI3CRzQo-lkp@xxxxxxxxx/config)
> > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> > reproduce: (https://download.01.org/0day-ci/archive/20230808/202308081508.EI3CRzQo-lkp@xxxxxxxxx/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202308081508.EI3CRzQo-lkp@xxxxxxxxx/
> >
> > All errors (new ones prefixed by >>):
> >
> >    In file included from lib/test_bitops.c:9:
> >    In file included from include/linux/module.h:13:
> >    In file included from include/linux/stat.h:19:
> >    In file included from include/linux/time.h:60:
> >    In file included from include/linux/time32.h:13:
> >    In file included from include/linux/timex.h:67:
> >    In file included from arch/arm64/include/asm/timex.h:8:
> >    In file included from arch/arm64/include/asm/arch_timer.h:18:
> >    In file included from include/linux/smp.h:110:
> >    In file included from include/linux/preempt.h:79:
> >    In file included from arch/arm64/include/asm/preempt.h:6:
> >    In file included from include/linux/thread_info.h:60:
> >    In file included from arch/arm64/include/asm/thread_info.h:18:
> > >> arch/arm64/include/asm/stack_pointer.h:8:24: error: no previous extern declaration for non-static variable 'current_stack_pointer' [-Werror,-Wmissing-variable-declarations]
> >        8 | register unsigned long current_stack_pointer asm ("sp");
> >          |                        ^
> >    arch/arm64/include/asm/stack_pointer.h:8:10: note: declare 'static' if the variable is not intended to be used outside of this translation unit
> >        8 | register unsigned long current_stack_pointer asm ("sp");
> >          |          ^
> 
> I actually don't think that either compiler should warn for variables
> with register storage.  I spoke briefly with some GCC folks on IRC and
> the initial assesment was agreed.  I've filed
> - https://github.com/llvm/llvm-project/issues/64509
> - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110947
> 
> Also, I've received 3 emails from zero day; this is expected as the
> tree is not W=1 clean (actually, I think Arnd has been a lot of
> cleanup around these groups of warnings, so I take that back).  What's
> more curious to me is that none are GCC builds. I wonder if 0day bot
> team is only testing W=1 with clang and not GCC?  That would seem like
> perhaps the bar is higher for LLVM?

As far as I am aware, the 0day bot tests both compilers with W=1. I
think the more likely explanation is that the robot is not testing with
prerelease versions of GCC, which is currently 14.x, which is the only
version of GCC that has this warning implemented.

> Masahiro, Nathan,
> What are your thoughts on how to proceed here? Do we need the tree to
> be free of warnings before it can be added to W=1? Hopefully not; I

No, otherwise we wouldn't be adding it to W=1 ;)

> would think that's the criteria for promoting a warning from being
> hidden behind W=1 to being on by default in the top level Makefile.
> What are your thoughts?

I think the register storage issue should be resolved in at least clang
before this patch is accepted, as that seems to be where the majority of
warnings are coming from so far. Like we talked about, I'll take a shot
at fixing that. Once that is fixed, I'll build mainline with
-Wmissing-variable-declarations to see how many instances there are and
if there are any other interesting edge cases that should be fixed in
the compiler. After that, I think this should be good to go in. Does
that sound reasonable?

Cheers,
Nathan