Re: [bug report] cpumask: gcc 13.x emits compilation error on PowerPC

[Yury Norov <yury.norov@gmail.com>]

On Wed, Nov 06, 2024 at 06:32:23PM +0530, Nilay Shroff wrote:
> Hi,
> 
> Of late, I've been encountering the following compilation error while using GCC 13.x and latest upstream code:
> 
> Compilation error:
> ==================
>   <snip>
>   CC      kernel/padata.o
> In file included from ./include/linux/string.h:390,
>                  from ./arch/powerpc/include/asm/paca.h:16,
>                  from ./arch/powerpc/include/asm/current.h:13,
>                  from ./include/linux/thread_info.h:23,
>                  from ./include/asm-generic/preempt.h:5,
>                  from ./arch/powerpc/include/generated/asm/preempt.h:1,
>                  from ./include/linux/preempt.h:79,
>                  from ./include/linux/spinlock.h:56,
>                  from ./include/linux/swait.h:7,
>                  from ./include/linux/completion.h:12,
>                  from kernel/padata.c:14:
> In function ‘bitmap_copy’,
>     inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2,
>     inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2:
> ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread]
>   114 | #define __underlying_memcpy     __builtin_memcpy
>       |                                 ^
> ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
>   633 |         __underlying_##op(p, q, __fortify_size);                        \
>       |         ^~~~~~~~~~~~~
> ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
>   678 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>       |                          ^~~~~~~~~~~~~~~~~~~~
> ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’
>   259 |                 memcpy(dst, src, len);
>       |                 ^~~~~~
> kernel/padata.c: In function ‘__padata_set_cpumasks’:
> kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256]
>   713 |                                  cpumask_var_t pcpumask,
>       |                                  ~~~~~~~~~~~~~~^~~~~~~~
> In function ‘bitmap_copy’,
>     inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2,
>     inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2:
> ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread]
>   114 | #define __underlying_memcpy     __builtin_memcpy
>       |                                 ^
> ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
>   633 |         __underlying_##op(p, q, __fortify_size);                        \
>       |         ^~~~~~~~~~~~~
> ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
>   678 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>       |                          ^~~~~~~~~~~~~~~~~~~~
> ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’
>   259 |                 memcpy(dst, src, len);
>       |                 ^~~~~~
> kernel/padata.c: In function ‘__padata_set_cpumasks’:
> kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256]
>   713 |                                  cpumask_var_t pcpumask,
>       |                                  ~~~~~~~~~~~~~~^~~~~~~~
> cc1: all warnings being treated as errors
> make[3]: *** [scripts/Makefile.build:229: kernel/padata.o] Error 1
> make[2]: *** [scripts/Makefile.build:478: kernel] Error 2
> make[1]: *** [/root/linux/Makefile:1936: .] Error 2
> make: *** [Makefile:224: __sub-make] Error 2
> 
> # gcc --version 
> gcc (GCC) 13.2.1 20231205 (Red Hat 13.2.1-6)
> Copyright (C) 2023 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> Note:
> =====
> I don't encounter above error using GCC 11.x and 12.x on PowerPC.
> Moreover, I don't encounter above error using GCC 11.x or 12.x or 13.x on x86_64.
> 
> Git bisect:
> ===========
> The git bisect points to the following commit causing the above compilation error:
> 
> commit ab6b1010dab68f6d4bf063517db4ce2d63554bc6 (HEAD)
> Author: Brian Norris <briannorris@chromium.org>
> Date:   Thu Jul 18 17:50:39 2024 -0700
> 
>     cpumask: Switch from inline to __always_inline
>     
>     On recent (v6.6+) builds with Clang (based on Clang 18.0.0) and certain
>     configurations [0], I'm finding that (lack of) inlining decisions may
>     lead to section mismatch warnings like the following:
>     
>       WARNING: modpost: vmlinux.o: section mismatch in reference:
>       cpumask_andnot (section: .text) ->
>       cpuhp_bringup_cpus_parallel.tmp_mask (section: .init.data) ERROR:
>       modpost: Section mismatches detected.
>     
>     or more confusingly:
>     
>       WARNING: modpost: vmlinux: section mismatch in reference:
>       cpumask_andnot+0x5f (section: .text) -> efi_systab_phys (section:
>       .init.data)
>     
>     The first warning makes a little sense, because
>     cpuhp_bringup_cpus_parallel() (an __init function) calls
>     cpumask_andnot() on tmp_mask (an __initdata symbol). If the compiler
>     doesn't inline cpumask_andnot(), this may appear like a mismatch.
>     
>     The second warning makes less sense, but might be because efi_systab_phys
>     and cpuhp_bringup_cpus_parallel.tmp_mask are laid out near each other,
>     and the latter isn't a proper C symbol definition.
>     
>     In any case, it seems a reasonable solution to suggest more strongly to
>     the compiler that these cpumask macros *must* be inlined, as 'inline' is
>     just a recommendation.
>     
>     This change has been previously proposed in the past as:
>       Subject: [PATCH 1/3] bitmap: switch from inline to __always_inline
>       https://lore.kernel.org/all/20221027043810.350460-2-yury.norov@gmail.com/
>     
>     But the change has been split up, to separately justify the cpumask
>     changes (which drive my work) and the bitmap/const optimizations (that
>     Yury separately proposed for other reasons). This ends up as somewhere
>     between a "rebase" and "rewrite" -- I had to rewrite most of the patch.
>     
>     According to bloat-o-meter, vmlinux decreases minimally in size (-0.00%
>     to -0.01%, depending on the version of GCC or Clang and .config in
>     question) with this series of changes:
>     
>     gcc 13.2.0, x86_64_defconfig
>     -3005 bytes, Before=21944501, After=21941496, chg -0.01%
>     
>     clang 16.0.6, x86_64_defconfig
>     -105 bytes, Before=22571692, After=22571587, chg -0.00%
>     
>     gcc 9.5.0, x86_64_defconfig
>     -1771 bytes, Before=21557598, After=21555827, chg -0.01%
>     
>     clang 18.0_pre516547 (ChromiumOS toolchain), x86_64_defconfig
>     -191 bytes, Before=22615339, After=22615148, chg -0.00%
>     
>     clang 18.0_pre516547 (ChromiumOS toolchain), based on ChromiumOS config + gcov
>     -979 bytes, Before=76294783, After=76293804, chg -0.00%
>     
>     [0] CONFIG_HOTPLUG_PARALLEL=y ('select'ed for x86 as of [1]) and
>         CONFIG_GCOV_PROFILE_ALL.
>     
>     [1] commit 0c7ffa32dbd6 ("x86/smpboot/64: Implement
>         arch_cpuhp_init_parallel_bringup() and enable it")
>     
>     Co-developed-by: Brian Norris <briannorris@chromium.org>
>     Signed-off-by: Brian Norris <briannorris@chromium.org>
>     Reviewed-by: Kees Cook <kees@kernel.org>
>     Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>     Signed-off-by: Yury Norov <yury.norov@gmail.com>
> 
> So it appears that changing cpumask_copy() from inline to __always_inline causing the 
> above error using Gcc 13.x. I am not gcc expert but it seems some issue with GCC 13.x?
> 
> I tried the following patch which helps fix the above error but I'm not sure if this
> is the proper fix or do we need to fix it differently.
> 
> Patch:
> ======
> diff --git a/kernel/padata.c b/kernel/padata.c
> index d899f34558af..86aad2f71890 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -710,8 +710,8 @@ static bool padata_validate_cpumask(struct padata_instance *pinst,
>  }
>  
>  static int __padata_set_cpumasks(struct padata_instance *pinst,
> -                                cpumask_var_t pcpumask,
> -                                cpumask_var_t cbcpumask)
> +                                struct cpumask *pcpumask,
> +                                struct cpumask *cbcpumask)
>  {
>         int valid;
>         int err;
 
This only works if CONFIG_CPUMASK_OFFSTACK=y. Otherwise, cpumask_var_t
is declared as:

typedef struct cpumask cpumask_var_t[1];

and your hack wouldn't work. You can read a comment starting with "Oh, the
wicked games we play!" in include/linux/cpumask_types.h for details. :)
 
> Please let me know if you need any further information.

config usually helps. Is it defconfig? What instrumentation is
enabled? Can you try the same without *ASAN and friends?

Thanks,
Yury