Hi David, Thanks for looking into the patch and apologies for a late response. Please find my comments inline below:
On 2026/03/12 06:56 PM, David Laight wrote: > On Thu, 12 Mar 2026 13:16:26 +0000 > Amit Machhiwal <[email protected]> wrote: > > > Hi Christhophe, > > > > Thanks for looking at the patch. Please find my comments inline:j > > > > On 2026/03/10 11:54 AM, Christophe Leroy (CS GROUP) wrote: > > > > > > > > > Le 10/03/2026 à 11:15, Amit Machhiwal a écrit : > > > > GCC 15 reports the below false positive '-Wmaybe-uninitialized' warning > > > > in vphn_unpack_associativity() when building the powerpc selftests. > > > > > > > > # make -C tools/testing/selftests TARGETS="powerpc" > > > > [...] > > > > CC test-vphn > > > > In file included from test-vphn.c:3: > > > > In function ‘vphn_unpack_associativity’, > > > > inlined from ‘test_one’ at test-vphn.c:371:2, > > > > inlined from ‘test_vphn’ at test-vphn.c:399:9: > > > > test-vphn.c:10:33: error: ‘be_packed’ may be used uninitialized > > > > [-Werror=maybe-uninitialized] > > > > 10 | #define be16_to_cpup(x) bswap_16(*x) > > > > | ^~~~~~~~ > > > > vphn.c:42:27: note: in expansion of macro ‘be16_to_cpup’ > > > > 42 | u16 new = be16_to_cpup(field++); > > > > | ^~~~~~~~~~~~ > > > > In file included from test-vphn.c:19: > > > > vphn.c: In function ‘test_vphn’: > > > > vphn.c:27:16: note: ‘be_packed’ declared here > > > > 27 | __be64 be_packed[VPHN_REGISTER_COUNT]; > > > > | ^~~~~~~~~ > > > > cc1: all warnings being treated as errors > > > > > > > > When vphn_unpack_associativity() is called from hcall_vphn(), this error > > > > is not seen during compilation because GCC 15 seems to consider 'retbuf' > > > > always populated from the hypervisor which is eventually referred by > > > > 'be_packed'. However, GCC 15's dataflow analysis can’t prove the same > > > > before the first dereference when vphn_unpack_associativity() is called > > > > from test_one() with pre-initialized array of 'struct test'. This > > > > results in a false positive warning which is promoted to an error under > > > > '-Werror'. This problem is not seen when the compilation is performed > > > > with GCC 13 and 14. > > > > > > > > Suppress the warning locally around the offending statement when > > > > building with GCC 15 using a diagnostic pragma. This keeps the build > > > > working while limiting the scope of the suppression to the specific > > > > statement that triggers the false positive. An issue [1] has also been > > > > created on GCC bugzilla. > > > > > > Usually when we get this kind of warning this is because the code is too > > > complex. We should try to make it more obvious instead of just hiding the > > > warning. > > > > The real issue here is that GCC 15 emits '-Wmaybe-uninitialized' due to > > type punning between __be64[] and __b16* when accessing the buffer via > > be16_to_cpup(). The underlying object is fully initialized but GCC 15 > > fails to track the aliasing due to the strict aliasing violation here. > > Nope, I think it is tracking it correctly. > The writes to be_packed[] are of 64bit values. > The only reads of that memory are 16bit ones through field[]. > With 'strict aliasing' the compiler doesn't have to order those accesses. > Indeed, it is allowed to completely optimise away the first loop. Quoting the below statement from the discussion with GCC folks at [1] "This code has aliasing violations in it. The uninitialized happens due to the undefined code due to the alias violations." Having mentioned that and looking at the discussion happened at [2], the '-Wmaybe-uninitialized' seems to be a case of a bad diagnostic instead where the actual warning should have been pointing to a Wstrict-aliasing issue which already is being tracked in the issue. > If you cast to 'unsigned char *' then the accesses do have to be ordered. > gcc will also treat accesses to different members of a union as being ordered > (the C stand doesn't require this, IIRC s/union/struct/ is valid). True, an union could be used to avoid this type punning problem but I think its easier (and probablty better at this point) to avoid the warning while building the vphn.c in test with '-fno-strict-aliasing' immitating the way its compiled in kernel. ~Amit > David > > > Please refer [1] and [2]. > > > > The selftest compiles fine with '-fno-strict-aliasing'. I see that when > > we build vphn.c while compiling the kernel, the top level Makefile > > includes '-fno-strict-aliasing' flag always. > > > > So, I believe the same flag should be used to build vphn tests when > > compiling vphn.c via the selftests. I'll send the v2 to achieve this > > thus avoiding the compilation failure. > > > > Please let me know you have different thoughts. > > > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124427 > > [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99768 > > > > ~Amit > > > > > > > > Here the for loop is a bit misleading. > > > > > > > > > > > [1] > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fbugzilla%2Fshow_bug.cgi%3Fid%3D124427&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C06a4d55b55f24c5cf00208de7e8e3676%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C639087346428583316%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=xEfO94N6IfGYhmmapNFduv3OrMarxpjTpZR6B38uR1s%3D&reserved=0 > > > > > > > > Fixes: 58dae82843f5 ("selftests/powerpc: Add test for VPHN") > > > > Reviewed-by: Vaibhav Jain <[email protected]> > > > > Signed-off-by: Amit Machhiwal <[email protected]> > > > > --- > > > > arch/powerpc/platforms/pseries/vphn.c | 15 +++++++++++++++ > > > > 1 file changed, 15 insertions(+) > > > > > > > > diff --git a/arch/powerpc/platforms/pseries/vphn.c > > > > b/arch/powerpc/platforms/pseries/vphn.c > > > > index 3f85ece3c872..9bc891143fec 100644 > > > > --- a/arch/powerpc/platforms/pseries/vphn.c > > > > +++ b/arch/powerpc/platforms/pseries/vphn.c > > > > @@ -39,7 +39,22 @@ static int vphn_unpack_associativity(const long > > > > *packed, __be32 *unpacked) > > > > be_packed[i] = cpu_to_be64(packed[i]); > > > > for (i = 1; i < VPHN_ASSOC_BUFSIZE; i++) { > > > > +/* > > > > + * When this function is called from hcall_vphn(), GCC 15 seems to > > > > consider > > > > + * 'retbuf' always populated from the hypervisor which is eventually > > > > referred by > > > > + * 'be_packed'. However, GCC 15's dataflow analysis can’t prove the > > > > same before > > > > + * the first dereference when this function is called from test_one() > > > > with > > > > + * pre-initialized array of 'struct test'. This results in a false > > > > positive > > > > + * '-Wmaybe-uninitialized' warning which is promoted to an error under > > > > + * '-Werror'. This problem is not seen when the compilation is > > > > performed with > > > > + * older GCC versions. > > > > + */ > > > > +#pragma GCC diagnostic push > > > > +#if defined(__GNUC__) && __GNUC__ >= 15 > > > > +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" > > > > +#endif > > > > u16 new = be16_to_cpup(field++); > > > > +#pragma GCC diagnostic pop > > > > if (is_32bit) { > > > > /* > > > > > > > > base-commit: 1f318b96cc84d7c2ab792fcc0bfd42a7ca890681 > > > > > >
