aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaInit.cpp:808
unsigned NumElems = numStructUnionElements(ILE->getType());
- if (RDecl->hasFlexibleArrayMember())
+ if (!RDecl->isUnion() && RDecl->hasFlexibleArrayMember())
++NumElems;
----------------
Fznamznon wrote:
> shafik wrote:
> > Fznamznon wrote:
> > > Just for some context, numStructUnionElements checks that there is a
> > > flexible array member and returns number_of_initializable_fields-1 for
> > > structs. For unions it just returns 1 or 0, so flexible array member
> > > caused adding one more element to initlistexpr that was never properly
> > > handled.
> > >
> > > Instead of doing this change, we could probably never enter
> > > initialization since the record (union) declaration is not valid, but
> > > that is not the case even for other types of errors in code, for example,
> > > I've tried declaring field of struct with a typo:
> > >
> > > ```
> > > struct { cha x[]; } r = {1};
> > > ```
> > > Initialization is still performed by clang.
> > > Also, it seems MSVC considers flexible array member inside union as
> > > valid, so the test code is probably not always invalid.
> > I am not sure what to think here, looking at gcc documentation for this
> > extension: https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> >
> > and using the following code:
> >
> > ```
> > struct f1 {
> > int x; int y[];
> > } f1 = { 1, { 2, 3, 4 } }; // #1
> >
> > struct f2 {
> > struct f1 f1; int data[3];
> > } f2 = { { 1 }, { 2, 3, 4 } }; // #2
> >
> > struct { char x[]; } r = {1}; // #3
> > ```
> >
> > gcc rejects 2 and 3 even though 2 comes from their documentation. Clang
> > warns on 2 and MSVC rejects 2
> >
> > CC @aaron.ballman @rsmith
> Yes, I also had a feeling that we probably need to refine how these
> extensions are supported by clang, that is probably a bit out of scope of the
> fix though.
The GCC extension differs based on C vs C++: https://godbolt.org/z/E14Yz37To
As does the extension in Clang, but differently than GCC:
https://godbolt.org/z/zYznaYPf5
So I agree that there's work to be done on this extension, but it's outside of
the scope of the crash fix for this patch. For this patch, GCC rejects allowing
a flexible array member in a union in both C and C++, but it looks like Clang
rejects in C and perhaps accepts in C++: https://godbolt.org/z/bTsPn3G7b So how
about we add a C++ test case for this as well -- if that still crashes, that
should be fixed, but if the code is accepted, we should either decide we want
to start rejecting it or we should ensure the codegen for it is correct.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147626/new/
https://reviews.llvm.org/D147626
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
