Message-ID: <CH3PR11MB8751EEEAD1E6C970F68FF2CBF3C92@CH3PR11MB8751.namprd11.prod.outlook.com>
Date: Fri, 21 Jun 2024 11:24:55 +0000
From: "Ballman, Aaron" <aaron.ballman@...el.com>
To: Nick Desaulniers <ndesaulniers@...gle.com>, Yabin Cui <yabinc@...gle.com>
CC: Steffen Klassert <steffen.klassert@...unet.com>, Herbert Xu
	<herbert@...dor.apana.org.au>, "David S. Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Nathan Chancellor <nathan@...nel.org>, Bill Wendling
	<morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"llvm@...ts.linux.dev" <llvm@...ts.linux.dev>
Subject: RE: [PATCH] Fix initializing a static union variable

Sadly, this is not a bug in Clang but is the terrible way C still works in C23. ☹ See C23 6.7.11p11, the last bullet:

if it is a union, the first named member is initialized (recursively) according to these rules, and any padding is initialized to zero bits.

So the padding gets initialized as does the first member, but if the first member is not the largest member in the union, that leaves other bits uninitialized. This happened late during NB comment stages, which is why N2900 and N3011 give the impression that the largest union member should be initialized.

That said, maybe there's appetite within the community to initialize the largest member as a conforming extension. The downside is that people not playing tricks with their unions may end up paying additional initialization cost that's useless, in pathological cases. e.g.,

int main() {
  union U {
    int i;
    struct {
      long double huge[1000];
    } s;
  } u = {}; // This is not a brilliant use of unions
  return u.i;
}

But how often does this matter for performance -- I have to imagine that most unions make use of most of the memory needed for their members instead of being lopsided like that. If we find some important use case that has significantly worse performance, we could always add a driver flag to control the behavior to let people opt into/out of the extension.

~Aaron

-----Original Message-----
From: Nick Desaulniers <ndesaulniers@...gle.com> 
Sent: Thursday, June 20, 2024 3:54 PM
To: Yabin Cui <yabinc@...gle.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>; Herbert Xu <herbert@...dor.apana.org.au>; David S. Miller <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; Nathan Chancellor <nathan@...nel.org>; Bill Wendling <morbo@...gle.com>; Justin Stitt <justinstitt@...gle.com>; netdev@...r.kernel.org; linux-kernel@...r.kernel.org; llvm@...ts.linux.dev; Ballman, Aaron <aaron.ballman@...el.com>
Subject: Re: [PATCH] Fix initializing a static union variable

On Thu, Jun 20, 2024 at 12:47 PM Nick Desaulniers <ndesaulniers@...gle.com> wrote:
>
> On Thu, Jun 20, 2024 at 12:31 PM Nick Desaulniers 
> <ndesaulniers@...gle.com> wrote:
> >
> > On Thu, Jun 20, 2024 at 11:17 AM Yabin Cui <yabinc@...gle.com> wrote:
> > >
> > > saddr_wildcard is a static union variable initialized with {}.
> > > But c11 standard doesn't guarantee initializing all fields as zero 
> > > for this case. As in https://godbolt.org/z/rWvdv6aEx,
> >
> > Specifically, it sounds like C99+ is just the first member of the 
> > union, which is dumb since that may not necessarily be the largest 
> > variant.  Can you find the specific relevant wording from a pre-c23 
> > spec?
> >
> > > clang only initializes the first field as zero, but the bits 
> > > corresponding to other (larger) members are undefined.
> >
> > Oh, that sucks!
> >
> > Reading through the internal report on this is fascinating!  Nice 
> > job tracking down the issue!  It sounds like if we can aggressively 
> > inline the users of this partially initialized value, then the UB 
> > from control flow on the partially initialized value can result in 
> > Android's kernel network tests failing.  It might be good to include 
> > more info on "why this is a problem" in the commit message.
> >
> > https://godbolt.org/z/hxnT1PTWo more clearly demonstrates the issue, IMO.
> >
> > TIL that C23 clarifies this, but clang still doesn't have the 
> > correct codegen then for -std=c23.  Can you please find or file a 
> > bug about this, then add a link to it in the commit message?
> >
> > It might be interesting to link to the specific section of n3096 
> > that clarifies this, or if there was a corresponding defect report 
> > sent to ISO about this.  Maybe something from 
> > https://www.open-std.org/jtc1/sc22/wg14/www/wg14_document_log.htm
> > discusses this?
>
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3011.htm
>
> https://clang.llvm.org/c_status.html mentions that n3011 was addressed 
> by clang-17, but based on my godbolt link above, it seems perhaps not?

Sorry, n3011 was a minor revision to
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm
which is a better citation for this bug, IMO.  I still think the clang status page is wrong (for n2900) and that is a bug against clang that should be fixed (for c23), but this kernel patch still has merit (since the issue I'm referring to in clang is not what's leading to the test case failures).

>
> 6.7.10.2 of n3096 (c23) defines "empty initialization" (which wasn't 
> defined in older standards).
>
> Ah, reading
>
> n2310 (c17) 6.7.9.10:
>
> ```
> If an object that has static or thread storage duration is not 
> initialized explicitly, then:
> ...
> — if it is a union, the first named member is initialized
> (recursively) according to these rules, and any padding is initialized 
> to zero bits; ```
>
> Specifically, "the first named member" was a terrible mistake in the language.
>
> Yikes! Might want to quote that in the commit message.
>
> >
> > Can you also please (find or) file a bug against clang about this? A 
> > compiler diagnostic would be very very helpful here, since `= {};` 
> > is such a common idiom.
> >
> > Patch LGTM, but I think more context can be provided in the commit 
> > message in a v2 that helps reviewers follow along with what's going 
> > on here.
> >
> > >
> > > Signed-off-by: Yabin Cui <yabinc@...gle.com>
> > > ---
> > >  net/xfrm/xfrm_state.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 
> > > 649bb739df0d..9bc69d703e5c 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > > @@ -1139,7 +1139,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > >                 struct xfrm_policy *pol, int *err,
> > >                 unsigned short family, u32 if_id)  {
> > > -       static xfrm_address_t saddr_wildcard = { };
> > > +       static const xfrm_address_t saddr_wildcard;
> > >         struct net *net = xp_net(pol);
> > >         unsigned int h, h_wildcard;
> > >         struct xfrm_state *x, *x0, *to_put;
> > > --
> > > 2.45.2.741.gdbec12cfda-goog
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
>
>
> --
> Thanks,
> ~Nick Desaulniers



--
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists