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:
> 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?
Ouch, that's a rather confused situation, isn't it? I don't have the impression
we actually support this extension and we do not document support for it in the
language extensions page, so due to the crashing I'm leaning towards diagnosing
this code as invalid per the standards. That doesn't mean someone can't come
along and support this extension if they need it to work, but the current state
of things is not production-ready. Any disagreement with that?
(If we go that route, we should definitely follow the potentially breaking
changes policy
(https://llvm.org/docs/DeveloperPolicy.html#making-potentially-breaking-changes)
because this could be disruptive depending on how much this is used in the
wild.)
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
