[PATCH] perf build: Restore {0} initializer since GCC-15

[PATCH] perf build: Restore {0} initializer since GCC-15

[Leo Yan]

GCC-15 release claims [1]:

 {0} initializer in C or C++ for unions no longer guarantees clearing
 of the whole union (except for static storage duration initialization),
 it just initializes the first union member to zero. If initialization
 of the whole union including padding bits is desirable, use {} (valid
 in C23 or C++) or use -fzero-init-padding-bits=unions option to
 restore old GCC behavior.

This new behaviour might cause stale and unexpected data we defined in
Perf.  Add the -fzero-init-padding-bits=unions option for entirely
zeroing union structures.

[1] https://gcc.gnu.org/gcc-15/changes.html

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/Makefile.config | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index a148ca9efca9..f767ab634022 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -323,6 +323,14 @@ FEATURE_CHECK_LDFLAGS-libaio = -lrt
 FEATURE_CHECK_LDFLAGS-disassembler-four-args = -lbfd -lopcodes -ldl
 FEATURE_CHECK_LDFLAGS-disassembler-init-styled = -lbfd -lopcodes -ldl
 
+# As described in GCC-15 release: https://gcc.gnu.org/gcc-15/changes.html:
+#   "{0} initializer in C or C++ for unions no longer guarantees clearing
+#    of the whole union (except for static storage duration initialization),
+#    it just initializes the first union member to zero. ... use {} (valid
+#    in C23 or C++) or use -fzero-init-padding-bits=unions option to
+#    restore old GCC behavior."
+CORE_CFLAGS += $(call cc-option,-fzero-init-padding-bits=unions)
+
 CORE_CFLAGS += -fno-omit-frame-pointer
 CORE_CFLAGS += -Wall
 CORE_CFLAGS += -Wextra
-- 
2.34.1

Re: [PATCH] perf build: Restore {0} initializer since GCC-15

[James Clark]

On 19/03/2025 11:04 am, Leo Yan wrote:
> GCC-15 release claims [1]:
> 
>   {0} initializer in C or C++ for unions no longer guarantees clearing
>   of the whole union (except for static storage duration initialization),
>   it just initializes the first union member to zero. If initialization
>   of the whole union including padding bits is desirable, use {} (valid
>   in C23 or C++) or use -fzero-init-padding-bits=unions option to
>   restore old GCC behavior.
> 
> This new behaviour might cause stale and unexpected data we defined in
> Perf.  Add the -fzero-init-padding-bits=unions option for entirely
> zeroing union structures.
> 

Do we need this? I don't see any unions initialized in that way. In fact 
there is only one struct initialized with {0}, the other handful are 
char*s but I don't think either are affected.

Adding options that allow people to add more non standard code doesn't 
feel very portable or in the spirit of doing it the right way. Maybe 
there's an argument that it guards against future mistakes, but it's not 
mentioned in the commit message.

Re: [PATCH] perf build: Restore {0} initializer since GCC-15

[Leo Yan]

On Wed, Mar 19, 2025 at 11:19:38AM +0000, James Clark wrote:
> 
> On 19/03/2025 11:04 am, Leo Yan wrote:
> > GCC-15 release claims [1]:
> > 
> >   {0} initializer in C or C++ for unions no longer guarantees clearing
> >   of the whole union (except for static storage duration initialization),
> >   it just initializes the first union member to zero. If initialization
> >   of the whole union including padding bits is desirable, use {} (valid
> >   in C23 or C++) or use -fzero-init-padding-bits=unions option to
> >   restore old GCC behavior.
> > 
> > This new behaviour might cause stale and unexpected data we defined in
> > Perf.  Add the -fzero-init-padding-bits=unions option for entirely
> > zeroing union structures.
> > 
> 
> Do we need this? I don't see any unions initialized in that way. In fact
> there is only one struct initialized with {0}, the other handful are char*s
> but I don't think either are affected.

Though I did not found a straightforward case in Perf for initializing
union with "{0}", the result I got:

$ git grep -E "\{ *0 *\}" tools/perf/
tools/perf/arch/x86/tests/insn-x86.c:   {{0}, 0, 0, NULL, NULL, NULL},
tools/perf/arch/x86/tests/insn-x86.c:   {{0}, 0, 0, NULL, NULL, NULL},
tools/perf/arch/x86/tests/intel-pt-test.c:      {1, {0}, 0, {INTEL_PT_PAD, 0, 0}, 0, 1 },
tools/perf/arch/x86/tests/intel-pt-test.c:      {0, {0}, 0, {0, 0, 0}, 0, 0 },
tools/perf/arch/x86/util/perf_regs.c:   char new_reg[SDT_REG_NAME_SIZE] = {0};
tools/perf/arch/x86/util/perf_regs.c:   char prefix[3] = {0};
tools/perf/builtin-kwork.c:             .nr_skipped_events   = { 0 },
tools/perf/builtin-record.c:    u8 pad[8] = {0};
tools/perf/python/twatch.py:                    print("cpu: {0}, pid: {1}, tid: {2} {3}".format(event.sample_cpu,
tools/perf/tests/code-reading.c:        unsigned char buf1[BUFSZ] = {0};
tools/perf/tests/code-reading.c:        unsigned char buf2[BUFSZ] = {0};
tools/perf/util/bpf_counter.c:  struct bpf_map_info map_info = {0};
tools/perf/util/bpf_kwork.c:    char name[MAX_KWORKNAME] = { 0 };
tools/perf/util/bpf_skel/bperf_follower.bpf.c:  struct bperf_filter_value child_fval = { 0 };
tools/perf/util/lzma.c: char buf[6] = { 0 };
tools/perf/util/mem-events.c:bool perf_mem_record[PERF_MEM_EVENTS__MAX] = { 0 };
tools/perf/util/mem-events.c:   char hit_miss[5] = {0};
tools/perf/util/trace-event-scripting.c:        char xs[16] = {0};
tools/perf/util/zlib.c: char buf[2] = { 0 };

We can see the bpf structures (bpf_map_info/bperf_filter_value) are
initialized with {0}.  For a more complex case, {0} is used for
initialize a specific field in a structure (see the results in
insn-x86.c and intel-pt-test.c).

> Adding options that allow people to add more non standard code doesn't feel
> very portable or in the spirit of doing it the right way. Maybe there's an
> argument that it guards against future mistakes, but it's not mentioned in
> the commit message.

I think Linux perf shares the same understanding with "we do expect
initializers that always initialize the whole variable fully" (quote
in [1]).  Furthermore, the reply mentioned:

 The exact same problem happens with "{ 0 }" as happens with "{ }".
 The bug is literally that some versions of clang seem to implement
 BOTH of these as "initialize the first member of the union", which
 then means that if the first member isn't the largest member, the
 rest of the union is entirely undefined.

So I think it is reasonable to imposes a compiler option to make
compiler's behavouir consistent.

Thanks,
Leo

[1] https://www.spinics.net/lists/netdev/msg1007244.html

Re: [PATCH] perf build: Restore {0} initializer since GCC-15

[Ian Rogers]

On Wed, Mar 19, 2025 at 6:31 AM Leo Yan <leo.yan@arm.com> wrote:
>
> On Wed, Mar 19, 2025 at 11:19:38AM +0000, James Clark wrote:
> >
> > On 19/03/2025 11:04 am, Leo Yan wrote:
> > > GCC-15 release claims [1]:
> > >
> > >   {0} initializer in C or C++ for unions no longer guarantees clearing
> > >   of the whole union (except for static storage duration initialization),
> > >   it just initializes the first union member to zero. If initialization
> > >   of the whole union including padding bits is desirable, use {} (valid
> > >   in C23 or C++) or use -fzero-init-padding-bits=unions option to
> > >   restore old GCC behavior.
> > >
> > > This new behaviour might cause stale and unexpected data we defined in
> > > Perf.  Add the -fzero-init-padding-bits=unions option for entirely
> > > zeroing union structures.
> > >
> >
> > Do we need this? I don't see any unions initialized in that way. In fact
> > there is only one struct initialized with {0}, the other handful are char*s
> > but I don't think either are affected.
>
> Though I did not found a straightforward case in Perf for initializing
> union with "{0}", the result I got:
>
> $ git grep -E "\{ *0 *\}" tools/perf/
> tools/perf/arch/x86/tests/insn-x86.c:   {{0}, 0, 0, NULL, NULL, NULL},
> tools/perf/arch/x86/tests/insn-x86.c:   {{0}, 0, 0, NULL, NULL, NULL},
> tools/perf/arch/x86/tests/intel-pt-test.c:      {1, {0}, 0, {INTEL_PT_PAD, 0, 0}, 0, 1 },
> tools/perf/arch/x86/tests/intel-pt-test.c:      {0, {0}, 0, {0, 0, 0}, 0, 0 },
> tools/perf/arch/x86/util/perf_regs.c:   char new_reg[SDT_REG_NAME_SIZE] = {0};
> tools/perf/arch/x86/util/perf_regs.c:   char prefix[3] = {0};
> tools/perf/builtin-kwork.c:             .nr_skipped_events   = { 0 },
> tools/perf/builtin-record.c:    u8 pad[8] = {0};
> tools/perf/python/twatch.py:                    print("cpu: {0}, pid: {1}, tid: {2} {3}".format(event.sample_cpu,
> tools/perf/tests/code-reading.c:        unsigned char buf1[BUFSZ] = {0};
> tools/perf/tests/code-reading.c:        unsigned char buf2[BUFSZ] = {0};
> tools/perf/util/bpf_counter.c:  struct bpf_map_info map_info = {0};
> tools/perf/util/bpf_kwork.c:    char name[MAX_KWORKNAME] = { 0 };
> tools/perf/util/bpf_skel/bperf_follower.bpf.c:  struct bperf_filter_value child_fval = { 0 };
> tools/perf/util/lzma.c: char buf[6] = { 0 };
> tools/perf/util/mem-events.c:bool perf_mem_record[PERF_MEM_EVENTS__MAX] = { 0 };
> tools/perf/util/mem-events.c:   char hit_miss[5] = {0};
> tools/perf/util/trace-event-scripting.c:        char xs[16] = {0};
> tools/perf/util/zlib.c: char buf[2] = { 0 };
>
> We can see the bpf structures (bpf_map_info/bperf_filter_value) are
> initialized with {0}.  For a more complex case, {0} is used for
> initialize a specific field in a structure (see the results in
> insn-x86.c and intel-pt-test.c).
>
> > Adding options that allow people to add more non standard code doesn't feel
> > very portable or in the spirit of doing it the right way. Maybe there's an
> > argument that it guards against future mistakes, but it's not mentioned in
> > the commit message.
>
> I think Linux perf shares the same understanding with "we do expect
> initializers that always initialize the whole variable fully" (quote
> in [1]).  Furthermore, the reply mentioned:
>
>  The exact same problem happens with "{ 0 }" as happens with "{ }".
>  The bug is literally that some versions of clang seem to implement
>  BOTH of these as "initialize the first member of the union", which
>  then means that if the first member isn't the largest member, the
>  rest of the union is entirely undefined.
>
> So I think it is reasonable to imposes a compiler option to make
> compiler's behavouir consistent.

We have encountered this problem, here is a fix for a case:
https://lore.kernel.org/r/20241119230033.115369-1-irogers@google.com
It would be nice if rather than -fzero-init-padding-bits=unions there
were some kind of warning flag we could enable, or worse use a tool
like clang-tidy to identify these problems. In the linked change the
problem was identified with -fsanitize=undefined but IIRC perf didn't
quit with a sanitizer warning message, just things broke and needed
fixing.

Thanks,
Ian

> Thanks,
> Leo
>
> [1] https://www.spinics.net/lists/netdev/msg1007244.html

Re: [PATCH] perf build: Restore {0} initializer since GCC-15

[Leo Yan]

On Wed, Mar 19, 2025 at 08:26:10AM -0700, Ian Rogers wrote:

[...]

> > > Adding options that allow people to add more non standard code doesn't feel
> > > very portable or in the spirit of doing it the right way. Maybe there's an
> > > argument that it guards against future mistakes, but it's not mentioned in
> > > the commit message.
> >
> > I think Linux perf shares the same understanding with "we do expect
> > initializers that always initialize the whole variable fully" (quote
> > in [1]).  Furthermore, the reply mentioned:
> >
> >  The exact same problem happens with "{ 0 }" as happens with "{ }".
> >  The bug is literally that some versions of clang seem to implement
> >  BOTH of these as "initialize the first member of the union", which
> >  then means that if the first member isn't the largest member, the
> >  rest of the union is entirely undefined.
> >
> > So I think it is reasonable to imposes a compiler option to make
> > compiler's behavouir consistent.
> 
> We have encountered this problem, here is a fix for a case:
> https://lore.kernel.org/r/20241119230033.115369-1-irogers@google.com
> It would be nice if rather than -fzero-init-padding-bits=unions there
> were some kind of warning flag we could enable, or worse use a tool
> like clang-tidy to identify these problems. In the linked change the
> problem was identified with -fsanitize=undefined but IIRC perf didn't
> quit with a sanitizer warning message, just things broke and needed
> fixing.

I searched the GCC online doc [2], I found below options but none of
them is used for such kind of warning:

  -Wmissing-braces
  -Wuninitialized
  -Wmissing-field-initializers

For the "-Wmissing-field-initializers" option, it says "In C this
option does not warn about the universal zero initializer ‘{ 0 }’".

Linux kernel has added the "-fzero-init-padding-bits=all" option in
the commit:

  dce4aab8441d kbuild: Use -fzero-init-padding-bits=all

Maybe we can do the same thing for perf?  This could help developers
and maintainers avoid endlessly struggling with potential bugs caused
by "{0}".

Thanks,
Leo

[1] https://www.spinics.net/lists/netdev/msg1007244.html
[2] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

Re: [PATCH] perf build: Restore {0} initializer since GCC-15

[Ian Rogers]

On Wed, Mar 19, 2025 at 11:28 AM Leo Yan <leo.yan@arm.com> wrote:
>
> On Wed, Mar 19, 2025 at 08:26:10AM -0700, Ian Rogers wrote:
>
> [...]
>
> > > > Adding options that allow people to add more non standard code doesn't feel
> > > > very portable or in the spirit of doing it the right way. Maybe there's an
> > > > argument that it guards against future mistakes, but it's not mentioned in
> > > > the commit message.
> > >
> > > I think Linux perf shares the same understanding with "we do expect
> > > initializers that always initialize the whole variable fully" (quote
> > > in [1]).  Furthermore, the reply mentioned:
> > >
> > >  The exact same problem happens with "{ 0 }" as happens with "{ }".
> > >  The bug is literally that some versions of clang seem to implement
> > >  BOTH of these as "initialize the first member of the union", which
> > >  then means that if the first member isn't the largest member, the
> > >  rest of the union is entirely undefined.
> > >
> > > So I think it is reasonable to imposes a compiler option to make
> > > compiler's behavouir consistent.
> >
> > We have encountered this problem, here is a fix for a case:
> > https://lore.kernel.org/r/20241119230033.115369-1-irogers@google.com
> > It would be nice if rather than -fzero-init-padding-bits=unions there
> > were some kind of warning flag we could enable, or worse use a tool
> > like clang-tidy to identify these problems. In the linked change the
> > problem was identified with -fsanitize=undefined but IIRC perf didn't
> > quit with a sanitizer warning message, just things broke and needed
> > fixing.
>
> I searched the GCC online doc [2], I found below options but none of
> them is used for such kind of warning:
>
>   -Wmissing-braces
>   -Wuninitialized
>   -Wmissing-field-initializers
>
> For the "-Wmissing-field-initializers" option, it says "In C this
> option does not warn about the universal zero initializer ‘{ 0 }’".
>
> Linux kernel has added the "-fzero-init-padding-bits=all" option in
> the commit:
>
>   dce4aab8441d kbuild: Use -fzero-init-padding-bits=all
>
> Maybe we can do the same thing for perf?  This could help developers
> and maintainers avoid endlessly struggling with potential bugs caused
> by "{0}".

It looks like clang may be just changing the default to zero
initialize everything:
https://github.com/llvm/llvm-project/pull/110051/files
https://github.com/llvm/llvm-project/pull/97121

Thanks,
Ian



> Thanks,
> Leo
>
> [1] https://www.spinics.net/lists/netdev/msg1007244.html
> [2] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

Re: [PATCH] perf build: Restore {0} initializer since GCC-15

[Namhyung Kim]

Hello,

On Wed, Mar 19, 2025 at 02:05:17PM -0700, Ian Rogers wrote:
> On Wed, Mar 19, 2025 at 11:28 AM Leo Yan <leo.yan@arm.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 08:26:10AM -0700, Ian Rogers wrote:
> >
> > [...]
> >
> > > > > Adding options that allow people to add more non standard code doesn't feel
> > > > > very portable or in the spirit of doing it the right way. Maybe there's an
> > > > > argument that it guards against future mistakes, but it's not mentioned in
> > > > > the commit message.
> > > >
> > > > I think Linux perf shares the same understanding with "we do expect
> > > > initializers that always initialize the whole variable fully" (quote
> > > > in [1]).  Furthermore, the reply mentioned:
> > > >
> > > >  The exact same problem happens with "{ 0 }" as happens with "{ }".
> > > >  The bug is literally that some versions of clang seem to implement
> > > >  BOTH of these as "initialize the first member of the union", which
> > > >  then means that if the first member isn't the largest member, the
> > > >  rest of the union is entirely undefined.
> > > >
> > > > So I think it is reasonable to imposes a compiler option to make
> > > > compiler's behavouir consistent.
> > >
> > > We have encountered this problem, here is a fix for a case:
> > > https://lore.kernel.org/r/20241119230033.115369-1-irogers@google.com
> > > It would be nice if rather than -fzero-init-padding-bits=unions there
> > > were some kind of warning flag we could enable, or worse use a tool
> > > like clang-tidy to identify these problems. In the linked change the
> > > problem was identified with -fsanitize=undefined but IIRC perf didn't
> > > quit with a sanitizer warning message, just things broke and needed
> > > fixing.
> >
> > I searched the GCC online doc [2], I found below options but none of
> > them is used for such kind of warning:
> >
> >   -Wmissing-braces
> >   -Wuninitialized
> >   -Wmissing-field-initializers
> >
> > For the "-Wmissing-field-initializers" option, it says "In C this
> > option does not warn about the universal zero initializer ‘{ 0 }’".
> >
> > Linux kernel has added the "-fzero-init-padding-bits=all" option in
> > the commit:
> >
> >   dce4aab8441d kbuild: Use -fzero-init-padding-bits=all
> >
> > Maybe we can do the same thing for perf?  This could help developers
> > and maintainers avoid endlessly struggling with potential bugs caused
> > by "{0}".
> 
> It looks like clang may be just changing the default to zero
> initialize everything:
> https://github.com/llvm/llvm-project/pull/110051/files
> https://github.com/llvm/llvm-project/pull/97121

Then I think it's better to add "-fzero-init-padding-bits=all" for GCC
to minimize surprises.

Thanks,
Namhyung

Re: [PATCH] perf build: Restore {0} initializer since GCC-15

[James Clark]

On 19/03/2025 11:19 am, James Clark wrote:
> 
> 
> On 19/03/2025 11:04 am, Leo Yan wrote:
>> GCC-15 release claims [1]:
>>
>>   {0} initializer in C or C++ for unions no longer guarantees clearing
>>   of the whole union (except for static storage duration initialization),
>>   it just initializes the first union member to zero. If initialization
>>   of the whole union including padding bits is desirable, use {} (valid
>>   in C23 or C++) or use -fzero-init-padding-bits=unions option to
>>   restore old GCC behavior.
>>
>> This new behaviour might cause stale and unexpected data we defined in
>> Perf.  Add the -fzero-init-padding-bits=unions option for entirely
>> zeroing union structures.
>>
> 
> Do we need this? I don't see any unions initialized in that way. In fact 
> there is only one struct initialized with {0}, the other handful are 
> char*s but I don't think either are affected.
> 
> Adding options that allow people to add more non standard code doesn't 
> feel very portable or in the spirit of doing it the right way. Maybe 
> there's an argument that it guards against future mistakes, but it's not 
> mentioned in the commit message.
> 


After reading in a bit more detail and seeing all the extra 
inconsistencies between compilers/versions/syntaxes etc I retract this 
and agree that -fzero-init-padding-bits=all would be harmless and only 
make things better.