[PATCH] D123649: Allow flexible array initialization in C++.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D123649#4362756 , @efriedma wrote:

> In D123649#4362418 , @aaron.ballman 
> wrote:
>
>> In D123649#4362402 , @efriedma 
>> wrote:
>>
>>> The assertion is correct; without it, your code is getting miscompiled.
>>
>> The assertion may be correct in its intent, but we still should not be 
>> asserting in practice: https://godbolt.org/z/cs5bhvPGr
>
> I wasn't trying to suggest our behavior is correct here, just that the 
> assertion is correctly indicating an underlying issue with the generated IR.

Ah, sorry! I misunderstood. :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D123649#4362418 , @aaron.ballman 
wrote:

> In D123649#4362402 , @efriedma 
> wrote:
>
>> The assertion is correct; without it, your code is getting miscompiled.
>
> The assertion may be correct in its intent, but we still should not be 
> asserting in practice: https://godbolt.org/z/cs5bhvPGr

I wasn't trying to suggest our behavior is correct here, just that focusing 
assertion is correctly indicating an underlying issue with the generated IR.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Looks like `Init`'s type is wrong. It should be `{ i32, [1 x i32] }` instead of 
`{ i32, [0 x i32] }` in the case where `S` has two members.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D123649#4362402 , @efriedma wrote:

> The assertion is correct; without it, your code is getting miscompiled.

The assertion may be correct in its intent, but we still should not be 
asserting in practice: https://godbolt.org/z/cs5bhvPGr


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The assertion is correct; without it, your code is getting miscompiled.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

The assertion also fails when the following code is compiled:

  struct S { int i[]; };
  S value{0};

According to C99, flexible array members have to belong to structs that have 
more than one named member. But clang allows that as a GNU extension although 
gcc seems to reject it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

We found another case where the assertion fails.

$ cat test.cpp

  struct S {
int len;
int i[];
  };
  
  S value{0, 0};

$ clang++ -std=c++11 -c test.cpp

`CstSize` is only 4 while `VarSize` is 8.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4639
+  getDataLayout().getTypeAllocSize(Init->getType()));
+  assert(VarSize == CstSize && "Emitted constant has unexpected size");
+#endif

ahatanak wrote:
> efriedma wrote:
> > ahatanak wrote:
> > > This assertion fails when the following code is compiled:
> > > 
> > > 
> > > ```
> > > typedef unsigned char uint8_t;
> > > typedef uint8_t uint8_a16 __attribute__((aligned(16)));
> > > 
> > > void foo1() {
> > >   static const uint8_a16 array1[] = { 1 };
> > > }
> > > ```
> > `sizeof(uint8_a16[1])` is 16, but we currently emit a one-byte global.  So 
> > it seems like there's an underlying bug exposed by the assertion.
> > 
> > gcc thinks this is nonsense, and just prints an error.
> It seems to me that we should disallow arrays that have an element whose 
> alignment is larger than its size, just as gcc does.
> 
> But I see arrays like that declared in a couple of clang's regression tests 
> (e.g., `Sema/attr-aligned.c`). I'm not sure whether there was a reason for 
> not rejecting it or it was just an oversight.
I'd guess oversight; it's originally a gcc extension, and using it to insert 
padding into arrays doesn't really make sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4639
+  getDataLayout().getTypeAllocSize(Init->getType()));
+  assert(VarSize == CstSize && "Emitted constant has unexpected size");
+#endif

efriedma wrote:
> ahatanak wrote:
> > This assertion fails when the following code is compiled:
> > 
> > 
> > ```
> > typedef unsigned char uint8_t;
> > typedef uint8_t uint8_a16 __attribute__((aligned(16)));
> > 
> > void foo1() {
> >   static const uint8_a16 array1[] = { 1 };
> > }
> > ```
> `sizeof(uint8_a16[1])` is 16, but we currently emit a one-byte global.  So it 
> seems like there's an underlying bug exposed by the assertion.
> 
> gcc thinks this is nonsense, and just prints an error.
It seems to me that we should disallow arrays that have an element whose 
alignment is larger than its size, just as gcc does.

But I see arrays like that declared in a couple of clang's regression tests 
(e.g., `Sema/attr-aligned.c`). I'm not sure whether there was a reason for not 
rejecting it or it was just an oversight.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4639
+  getDataLayout().getTypeAllocSize(Init->getType()));
+  assert(VarSize == CstSize && "Emitted constant has unexpected size");
+#endif

ahatanak wrote:
> This assertion fails when the following code is compiled:
> 
> 
> ```
> typedef unsigned char uint8_t;
> typedef uint8_t uint8_a16 __attribute__((aligned(16)));
> 
> void foo1() {
>   static const uint8_a16 array1[] = { 1 };
> }
> ```
`sizeof(uint8_a16[1])` is 16, but we currently emit a one-byte global.  So it 
seems like there's an underlying bug exposed by the assertion.

gcc thinks this is nonsense, and just prints an error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4639
+  getDataLayout().getTypeAllocSize(Init->getType()));
+  assert(VarSize == CstSize && "Emitted constant has unexpected size");
+#endif

This assertion fails when the following code is compiled:


```
typedef unsigned char uint8_t;
typedef uint8_t uint8_a16 __attribute__((aligned(16)));

void foo1() {
  static const uint8_a16 array1[] = { 1 };
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

6cf0b1b3  
was a temporary fix to stop the crash. D123826 
 is the full fix to make the assertion 
actually work correctly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

For posterity, posting the link to the failure this caused in Linaro's TCWG's 
CI of kernel builds w/ clang: 
https://lore.kernel.org/llvm/906914372.14298.1650022522881@jenkins.jenkins/.

I'm guessing D123826  fixes this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Apparently the assertions I added are triggering in the LLVM test-suite.  
Commented them out in 6cf0b1b3da3e8662baf030a2d741e3becaaab2b0 
; I'll 
come up with a proper fix soon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5955a0f9375a: Allow flexible array initialization in C++. 
(authored by efriedma).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/test/CodeGenCXX/flexible-array-init.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp

Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2383,9 +2383,11 @@
   static_assert(b[2].x == 3, "");
   static_assert(b[2].arr[0], ""); // expected-error {{constant expression}} expected-note {{array member without known bound}}
 
-  // If we ever start to accept this, we'll need to ensure we can
-  // constant-evaluate it properly.
-  constexpr A c = {1, 2, 3}; // expected-error {{initialization of flexible array member}}
+  // Flexible array initialization is currently not supported by constant
+  // evaluation. Make sure we emit an error message, for now.
+  constexpr A c = {1, 2, 3}; // expected-error {{constexpr variable 'c' must be initialized by a constant expression}}
+  // expected-note@-1 {{flexible array initialization is not yet supported}}
+  // expected-warning@-2 {{flexible array initialization is a GNU extension}}
 }
 
 void local_constexpr_var() {
Index: clang/test/CodeGenCXX/flexible-array-init.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/flexible-array-init.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL1 %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL2 %s
+
+struct A { int x; int y[]; };
+A a = { 1, 7, 11 };
+// CHECK: @a ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 7, i32 11] }
+
+A b = { 1, { 13, 15 } };
+// CHECK: @b ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 13, i32 15] }
+
+int f();
+#ifdef FAIL1
+A c = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}}
+#endif
+#ifdef FAIL2
+void g() {
+  static A d = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}}
+}
+#endif
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2004,10 +2004,6 @@
   cast(InitExpr)->getNumInits() == 0) {
 // Empty flexible array init always allowed as an extension
 FlexArrayDiag = diag::ext_flexible_array_init;
-  } else if (SemaRef.getLangOpts().CPlusPlus) {
-// Disallow flexible array init in C++; it is not required for gcc
-// compatibility, and it needs work to IRGen correctly in general.
-FlexArrayDiag = diag::err_flexible_array_init;
   } else if (!TopLevelObject) {
 // Disallow flexible array init on non-top-level object
 FlexArrayDiag = diag::err_flexible_array_init;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4615,6 +4615,8 @@
 T = D->getType();
 
   if (getLangOpts().CPlusPlus) {
+if (!InitDecl->getFlexibleArrayInitChars(getContext()).isZero())
+  ErrorUnsupported(D, "flexible array initializer");
 Init = EmitNullConstant(T);
 NeedsGlobalCtor = true;
   } else {
@@ -4628,6 +4630,14 @@
   // also don't need to register a destructor.
   if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
 DelayedCXXInitPosition.erase(D);
+
+#ifndef NDEBUG
+  CharUnits VarSize = getContext().getTypeSizeInChars(ASTTy) +
+  InitDecl->getFlexibleArrayInitChars(getContext());
+  CharUnits CstSize = CharUnits::fromQuantity(
+  getDataLayout().getTypeAllocSize(Init->getType()));
+  assert(VarSize == CstSize && "Emitted constant has unexpected size");
+#endif
 }
   }
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -342,6 +342,8 @@
   if (!Init) {
 if (!getLangOpts().CPlusPlus)
   CGM.ErrorUnsupported(D.getInit(), "constant l-value expression");
+else if (!D.getFlexibleArrayInitChars(getContext()).isZero())
+  CGM.ErrorUnsupported(D.getInit(), 

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 422671.
efriedma added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/test/CodeGenCXX/flexible-array-init.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp

Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2383,9 +2383,11 @@
   static_assert(b[2].x == 3, "");
   static_assert(b[2].arr[0], ""); // expected-error {{constant expression}} expected-note {{array member without known bound}}
 
-  // If we ever start to accept this, we'll need to ensure we can
-  // constant-evaluate it properly.
-  constexpr A c = {1, 2, 3}; // expected-error {{initialization of flexible array member}}
+  // Flexible array initialization is currently not supported by constant
+  // evaluation. Make sure we emit an error message, for now.
+  constexpr A c = {1, 2, 3}; // expected-error {{constexpr variable 'c' must be initialized by a constant expression}}
+  // expected-note@-1 {{flexible array initialization is not yet supported}}
+  // expected-warning@-2 {{flexible array initialization is a GNU extension}}
 }
 
 void local_constexpr_var() {
Index: clang/test/CodeGenCXX/flexible-array-init.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/flexible-array-init.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL1 %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL2 %s
+
+struct A { int x; int y[]; };
+A a = { 1, 7, 11 };
+// CHECK: @a ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 7, i32 11] }
+
+A b = { 1, { 13, 15 } };
+// CHECK: @b ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 13, i32 15] }
+
+int f();
+#ifdef FAIL1
+A c = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}}
+#endif
+#ifdef FAIL2
+void g() {
+  static A d = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}}
+}
+#endif
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2004,10 +2004,6 @@
   cast(InitExpr)->getNumInits() == 0) {
 // Empty flexible array init always allowed as an extension
 FlexArrayDiag = diag::ext_flexible_array_init;
-  } else if (SemaRef.getLangOpts().CPlusPlus) {
-// Disallow flexible array init in C++; it is not required for gcc
-// compatibility, and it needs work to IRGen correctly in general.
-FlexArrayDiag = diag::err_flexible_array_init;
   } else if (!TopLevelObject) {
 // Disallow flexible array init on non-top-level object
 FlexArrayDiag = diag::err_flexible_array_init;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4615,6 +4615,8 @@
 T = D->getType();
 
   if (getLangOpts().CPlusPlus) {
+if (!InitDecl->getFlexibleArrayInitChars(getContext()).isZero())
+  ErrorUnsupported(D, "flexible array initializer");
 Init = EmitNullConstant(T);
 NeedsGlobalCtor = true;
   } else {
@@ -4628,6 +4630,14 @@
   // also don't need to register a destructor.
   if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
 DelayedCXXInitPosition.erase(D);
+
+#ifndef NDEBUG
+  CharUnits VarSize = getContext().getTypeSizeInChars(ASTTy) +
+  InitDecl->getFlexibleArrayInitChars(getContext());
+  CharUnits CstSize = CharUnits::fromQuantity(
+  getDataLayout().getTypeAllocSize(Init->getType()));
+  assert(VarSize == CstSize && "Emitted constant has unexpected size");
+#endif
 }
   }
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -342,6 +342,8 @@
   if (!Init) {
 if (!getLangOpts().CPlusPlus)
   CGM.ErrorUnsupported(D.getInit(), "constant l-value expression");
+else if (!D.getFlexibleArrayInitChars(getContext()).isZero())
+  CGM.ErrorUnsupported(D.getInit(), "flexible array initializer");
 else if (HaveInsertPoint()) {
   // Since we have a static initializer, this global variable 

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2388
+  // evaluation. Make sure we emit a sane error message, for now.
+  constexpr A c = {1, 2, 3}; // expected-warning {{flexible array 
initialization is a GNU extension}}
+  static_assert(c.arr[0] == 1, ""); // expected-error {{constant expression}} 
expected-note {{array member without known bound}}

erichkeane wrote:
> I would expect this to be an error, not the static-assert.  The constexpr 
> variable means 'initializable as a constant expression'.  
> 
> I'm guessing the problem is ACTUALLY that we support constexpr init, but not 
> the operator[].  I think I'd like to have us have the initialization fail 
> here, since it isn't otherwise usable.
I think we end up computing the initializer "correctly", but have no way to 
actually access the elements in constant evaluation.  Seems to come out of 
CGExprConstant lowering okay, but I guess we don't really need that to work at 
the moment; we fall back to the old CGExprConstant direct lowering code.

I'll add a bailout to constant evaluation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:346
+else if (!D.getFlexibleArrayInitChars(getContext()).isZero())
+  CGM.ErrorUnsupported(D.getInit(), "flexible array init");
 else if (HaveInsertPoint()) {

Can you write a test for this with a 'fixme'?  I don't see a reason why we 
shouldn't support this eventually.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4617
   if (getLangOpts().CPlusPlus) {
+if (!InitDecl->getFlexibleArrayInitChars(getContext()).isZero())
+  ErrorUnsupported(D, "flexible array initializer");

Same comment here.



Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2388
+  // evaluation. Make sure we emit a sane error message, for now.
+  constexpr A c = {1, 2, 3}; // expected-warning {{flexible array 
initialization is a GNU extension}}
+  static_assert(c.arr[0] == 1, ""); // expected-error {{constant expression}} 
expected-note {{array member without known bound}}

I would expect this to be an error, not the static-assert.  The constexpr 
variable means 'initializable as a constant expression'.  

I'm guessing the problem is ACTUALLY that we support constexpr init, but not 
the operator[].  I think I'd like to have us have the initialization fail here, 
since it isn't otherwise usable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123649/new/

https://reviews.llvm.org/D123649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123649: Allow flexible array initialization in C++.

[Eli Friedman via Phabricator via cfe-commits]

efriedma created this revision.
efriedma added reviewers: aaron.ballman, rsmith, erichkeane.
Herald added a project: All.
efriedma requested review of this revision.
Herald added a project: clang.

Flexible array initialization is a C/C++ extension implemented in many 
compilers to allow initializing the flexible array tail of a struct type that 
contains a flexible array.  In clang, this is currently restricted to C. But 
this construct is used in the Microsoft SDK headers, so I'd like to extend it 
to C++.

For now, this doesn't handle dynamic initialization; probably not hard to 
implement, but it's extra code, and I don't think it's necessary for the 
expected uses.  And I'm not handling constant evaluation; it should fail 
gracefully.

I've added some additional code to assert that flexible array init works the 
way we expect it to in both C and C++.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123649

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/test/CodeGenCXX/flexible-array-init.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp

Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2383,9 +2383,10 @@
   static_assert(b[2].x == 3, "");
   static_assert(b[2].arr[0], ""); // expected-error {{constant expression}} expected-note {{array member without known bound}}
 
-  // If we ever start to accept this, we'll need to ensure we can
-  // constant-evaluate it properly.
-  constexpr A c = {1, 2, 3}; // expected-error {{initialization of flexible array member}}
+  // Flexible array initialization is currently not supported by constant
+  // evaluation. Make sure we emit a sane error message, for now.
+  constexpr A c = {1, 2, 3}; // expected-warning {{flexible array initialization is a GNU extension}}
+  static_assert(c.arr[0] == 1, ""); // expected-error {{constant expression}} expected-note {{array member without known bound}}
 }
 
 void local_constexpr_var() {
Index: clang/test/CodeGenCXX/flexible-array-init.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/flexible-array-init.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+struct A { int x; int y[]; };
+A a = { 1, 7, 11 };
+// CHECK: @a ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 7, i32 11] }
+
+struct B { int x; int y[]; };
+B b = { 1, { 13, 15 } };
+// CHECK: @b ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 13, i32 15] }
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2004,10 +2004,6 @@
   cast(InitExpr)->getNumInits() == 0) {
 // Empty flexible array init always allowed as an extension
 FlexArrayDiag = diag::ext_flexible_array_init;
-  } else if (SemaRef.getLangOpts().CPlusPlus) {
-// Disallow flexible array init in C++; it is not required for gcc
-// compatibility, and it needs work to IRGen correctly in general.
-FlexArrayDiag = diag::err_flexible_array_init;
   } else if (!TopLevelObject) {
 // Disallow flexible array init on non-top-level object
 FlexArrayDiag = diag::err_flexible_array_init;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4614,6 +4614,8 @@
 T = D->getType();
 
   if (getLangOpts().CPlusPlus) {
+if (!InitDecl->getFlexibleArrayInitChars(getContext()).isZero())
+  ErrorUnsupported(D, "flexible array initializer");
 Init = EmitNullConstant(T);
 NeedsGlobalCtor = true;
   } else {
@@ -4627,6 +4629,14 @@
   // also don't need to register a destructor.
   if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
 DelayedCXXInitPosition.erase(D);
+
+#ifndef NDEBUG
+  CharUnits VarSize = getContext().getTypeSizeInChars(ASTTy) +
+  InitDecl->getFlexibleArrayInitChars(getContext());
+  CharUnits CstSize = CharUnits::fromQuantity(
+  getDataLayout().getTypeAllocSize(Init->getType()));
+  assert(VarSize == CstSize && "Emitted constant has unexpected size");
+#endif
 }
   }
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -342,6 +342,8 @@
   if (!Init) {
 if (!getLangOpts().CPlusPlus)
   CGM.ErrorUnsupported(D.getInit(), "constant l-value expression");
+else if (!D.getFlexibleArrayInitChars(getContext()).isZero())
+