Fznamznon marked an inline comment as not done.
Fznamznon 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:
> aaron.ballman wrote:
> > 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.
> While experimenting with C++ test, I've noticed the following thing:
> ```
> union { char x[]; } r = {0}; // when in global scope generates no IR (without
> my patch crashes)
>
> void foo() {
> union { char x[]; } r = {0}; // fails with error "initialization of
> flexible array member is not allowed" in both C/C++, no crash even without my
> patch
> }
>
> union A {char x[]; };
> A a = {0}; // crashes even with my patch but with different assertion when
> trying -emit-llvm
> void foo() {
> A a = {0}; // fails with error "initialization of flexible array member is
> not allowed" in both C and C++, no crash even without my patch
> }
> ```
>
> It is not entirely clear to me why the behavior different for function and TU
> scope. gcc always gives an error about flexible array in union, no matter how
> it is used. Also, it is strange that I'm not seeing the same "initialization
> of flexible array member is not allowed" error message for structs, just for
> unions. I'm thinking that we don't really have proper codegen for the code
> that I'm trying to fix and we should reject the code like gcc does. MSVC is
> fine with all though - https://godbolt.org/z/aoarEzd56 .
>
> WDYT?
Ping?
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
