[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour 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 rGc9ef33e1d8a8: [Clang] Fix crash when emitting diagnostic for 
out of order designated… (authored by shafik).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154675

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note 
{{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,43 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct A  {
+  unsigned x;
+  unsigned y;
+  unsigned z;
+};
+
+struct B {
+  unsigned a;
+  unsigned b;
+};
+
+struct : public A, public B {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+  {.z=0,
+ // pedantic-note@-1 {{first non-designated initializer is here}}
+ // reorder-note@-2 {{previous initialization for field 'z' is here}}
+   .y=1, // reorder-error {{field 'z' will be initialized after field 'y'}}
+ // reorder-note@-1 {{previous initialization for field 'y' is here}}
+   .x=2}, // reorder-error {{field 'y' will be initialized after field 'x'}}
+  {.b=3,  // reorder-note {{previous initialization for field 'b' is here}}
+   .a=4}, // reorder-error {{field 'b' will be initialized after field 'a'}}
+.e = 1, // reorder-note {{previous initialization for field 'e' is here}}
+// pedantic-error@-1 {{mixture of designated and non-designated 
initializers in the same initializer list is a C99 extension}}
+.d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}}
+// reorder-note@-1 {{previous initialization for field 'd' is 
here}}
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} 
// reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} 
// reorder-note {{previous initialization for field 'b' is here}}
+.a = 1, // reorder-error {{field 'b' will be initialized after field 'a'}}
+};
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2847,7 +2847,7 @@
 SemaRef.Diag(DIE->getBeginLoc(), diag::ext_designated_init_reordered)
 << KnownField << PrevField << DIE->getSourceRange();
 
-unsigned OldIndex = NumBases + PrevField->getFieldIndex();
+unsigned OldIndex = StructuredIndex - 1;
 if (StructuredList && OldIndex <= StructuredList->getNumInits()) {
   if (Expr *PrevInit = StructuredList->getInit(OldIndex)) {
 SemaRef.Diag(PrevInit->getBeginLoc(),
@@ -2951,8 +2951,12 @@
 // If this the first designator, our caller will continue checking
 // the rest of this struct/class/union subobject.
 if (IsFirstDesignator) {
+  if (Field != RD->field_end() && Field->isUnnamedBitfield())
+++Field;
+
   if (NextField)
 *NextField = Field;
+
   StructuredIndex = FieldIndex;
   return false;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -695,6 +695,9 @@
 - Fix handling of using-declarations in the init statements of for
   loop declarations.
   (`#63627 `_)
+- Fix crash when emitting diagnostic for out of order designated initializers
+  in C++.
+  (`#63605 `_)
 
 Bug Fixes to AST Handling
 ^


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides 

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 540587.
shafik marked 2 inline comments as done.
shafik added a comment.

- Add release notes.


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

https://reviews.llvm.org/D154675

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note 
{{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,43 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct A  {
+  unsigned x;
+  unsigned y;
+  unsigned z;
+};
+
+struct B {
+  unsigned a;
+  unsigned b;
+};
+
+struct : public A, public B {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+  {.z=0,
+ // pedantic-note@-1 {{first non-designated initializer is here}}
+ // reorder-note@-2 {{previous initialization for field 'z' is here}}
+   .y=1, // reorder-error {{field 'z' will be initialized after field 'y'}}
+ // reorder-note@-1 {{previous initialization for field 'y' is here}}
+   .x=2}, // reorder-error {{field 'y' will be initialized after field 'x'}}
+  {.b=3,  // reorder-note {{previous initialization for field 'b' is here}}
+   .a=4}, // reorder-error {{field 'b' will be initialized after field 'a'}}
+.e = 1, // reorder-note {{previous initialization for field 'e' is here}}
+// pedantic-error@-1 {{mixture of designated and non-designated 
initializers in the same initializer list is a C99 extension}}
+.d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}}
+// reorder-note@-1 {{previous initialization for field 'd' is 
here}}
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} 
// reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} 
// reorder-note {{previous initialization for field 'b' is here}}
+.a = 1, // reorder-error {{field 'b' will be initialized after field 'a'}}
+};
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2847,7 +2847,7 @@
 SemaRef.Diag(DIE->getBeginLoc(), diag::ext_designated_init_reordered)
 << KnownField << PrevField << DIE->getSourceRange();
 
-unsigned OldIndex = NumBases + PrevField->getFieldIndex();
+unsigned OldIndex = StructuredIndex - 1;
 if (StructuredList && OldIndex <= StructuredList->getNumInits()) {
   if (Expr *PrevInit = StructuredList->getInit(OldIndex)) {
 SemaRef.Diag(PrevInit->getBeginLoc(),
@@ -2951,8 +2951,12 @@
 // If this the first designator, our caller will continue checking
 // the rest of this struct/class/union subobject.
 if (IsFirstDesignator) {
+  if (Field != RD->field_end() && Field->isUnnamedBitfield())
+++Field;
+
   if (NextField)
 *NextField = Field;
+
   StructuredIndex = FieldIndex;
   return false;
 }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -695,6 +695,9 @@
 - Fix handling of using-declarations in the init statements of for
   loop declarations.
   (`#63627 `_)
+- Fix crash when emitting diagnostic for out of order designated initializers
+  in C++.
+  (`#63605 `_)
 
 Bug Fixes to AST Handling
 ^


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note {{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides 

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM but please land with a release note.




Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:66
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}

shafik wrote:
> aaron.ballman wrote:
> > A few questions: 1) what is this new note attached to? The only `reorder-*` 
> > diagnostic I see in the test is for the initialization of `x`. 2) is the 
> > note actually on the correct previous use? I would have expected this to be 
> > on the assignment to `y` one line above.
> It is attached to :
> 
> ```
> .x = 1, // reorder-error {{declaration order}}
> ```
> 
> We were not issuing it before b/c it was broken, in this case it was just not 
> crashing but it was still broken.
> 
> We could argue the note should go on the first `.y` although I could see both 
> sides since the override is a warning and not an error I think using the 
> second is defensible. 
> 
> 
Oh! I had a brain fart and thought this was about *re*initialization of `y`. I 
see what's going on now and yeah, this makes sense. I'm not worried about which 
`y` the note attaches to.


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:66
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}

aaron.ballman wrote:
> A few questions: 1) what is this new note attached to? The only `reorder-*` 
> diagnostic I see in the test is for the initialization of `x`. 2) is the note 
> actually on the correct previous use? I would have expected this to be on the 
> assignment to `y` one line above.
It is attached to :

```
.x = 1, // reorder-error {{declaration order}}
```

We were not issuing it before b/c it was broken, in this case it was just not 
crashing but it was still broken.

We could argue the note should go on the first `.y` although I could see both 
sides since the override is a warning and not an error I think using the second 
is defensible. 




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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D154675#4484194 , @cor3ntin wrote:

> Thanks, this looks good! (the libc++ test seems completely unrelated, it 
> happens before compilation starts).
> Can you add re release note though? Thanks!

+1 to both of these sentences. :-)




Comment at: clang/lib/Sema/SemaInit.cpp:2847
 
-unsigned OldIndex = NumBases + PrevField->getFieldIndex();
+unsigned OldIndex = StructuredIndex - 1;
 if (StructuredList && OldIndex <= StructuredList->getNumInits()) {

Should we be asserting that `StructuredIndex` is not 0? The logic in this 
function makes it tough to see, but the fact that we're in a block checking 
`IsFirstDesignator` makes me think this is dangerous.



Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:66
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}

A few questions: 1) what is this new note attached to? The only `reorder-*` 
diagnostic I see in the test is for the initialization of `x`. 2) is the note 
actually on the correct previous use? I would have expected this to be on the 
assignment to `y` one line above.


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Thanks, this looks good! (the libc++ test seems completely unrelated, it 
happens before compilation starts).
Can you add re release note though? Thanks!


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@aaron.ballman I can't figure out why the libcxx bot failed, can you understand 
why?


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked an inline comment as done.
shafik added a comment.

Found a new issue https://github.com/llvm/llvm-project/issues/63759 but this 
feels different enough that I will deal with it separately.




Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:182
+namespace GH63605 {
+struct {
+  unsigned : 2;

shafik wrote:
> cor3ntin wrote:
> > Should we add bases?
> Good catch b/c of course bases would cause problems 藍
Problems due to bases now fixed as well.



Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:195
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} 
// reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} 
// reorder-note {{previous initialization for field 'e' is here}}
+.a = 1, // reorder-error {{field 'e' will be initialized after field 'a'}}

shafik wrote:
> Note the `e` in the diagnostic in this line and the next line should be `b`. 
> I have not managed to figure out how to fix this yet but this feels 
> relatively minor compared to the crash.
This is now fixed


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 538456.
shafik added a comment.

- Stopped using `NumBases` when calculating `OldIndex`
- Added bases classes to the test and added more test cases


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

https://reviews.llvm.org/D154675

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note 
{{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,43 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct A  {
+  unsigned x;
+  unsigned y;
+  unsigned z;
+};
+
+struct B {
+  unsigned a;
+  unsigned b;
+};
+
+struct : public A, public B {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+  {.z=0,
+ // pedantic-note@-1 {{first non-designated initializer is here}}
+ // reorder-note@-2 {{previous initialization for field 'z' is here}}
+   .y=1, // reorder-error {{field 'z' will be initialized after field 'y'}}
+ // reorder-note@-1 {{previous initialization for field 'y' is here}}
+   .x=2}, // reorder-error {{field 'y' will be initialized after field 'x'}}
+  {.b=3,  // reorder-note {{previous initialization for field 'b' is here}}
+   .a=4}, // reorder-error {{field 'b' will be initialized after field 'a'}}
+.e = 1, // reorder-note {{previous initialization for field 'e' is here}}
+// pedantic-error@-1 {{mixture of designated and non-designated 
initializers in the same initializer list is a C99 extension}}
+.d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}}
+// reorder-note@-1 {{previous initialization for field 'd' is 
here}}
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} 
// reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} 
// reorder-note {{previous initialization for field 'b' is here}}
+.a = 1, // reorder-error {{field 'b' will be initialized after field 'a'}}
+};
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2844,7 +2844,7 @@
 SemaRef.Diag(DIE->getBeginLoc(), diag::ext_designated_init_reordered)
 << KnownField << PrevField << DIE->getSourceRange();
 
-unsigned OldIndex = NumBases + PrevField->getFieldIndex();
+unsigned OldIndex = StructuredIndex - 1;
 if (StructuredList && OldIndex <= StructuredList->getNumInits()) {
   if (Expr *PrevInit = StructuredList->getInit(OldIndex)) {
 SemaRef.Diag(PrevInit->getBeginLoc(),
@@ -2948,8 +2948,12 @@
 // If this the first designator, our caller will continue checking
 // the rest of this struct/class/union subobject.
 if (IsFirstDesignator) {
+  if (Field != RD->field_end() && Field->isUnnamedBitfield())
+++Field;
+
   if (NextField)
 *NextField = Field;
+
   StructuredIndex = FieldIndex;
   return false;
 }


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note {{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note {{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,43 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct A  {
+  unsigned x;
+  unsigned y;
+  unsigned z;
+};
+
+struct B {
+  unsigned a;
+  unsigned b;
+};
+
+struct : public A, public B {
+  unsigned : 2;
+  unsigned a : 6;
+  

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:182
+namespace GH63605 {
+struct {
+  unsigned : 2;

cor3ntin wrote:
> Should we add bases?
Good catch b/c of course bases would cause problems 藍


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I can't really see any details on the libcxx failure :-(


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 538323.
shafik marked an inline comment as done.
shafik added a comment.

-Add fix for wrong field in diagnostic


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

https://reviews.llvm.org/D154675

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note 
{{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,22 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+.e = 1, // reorder-note {{previous initialization for field 'e' is here}}
+.d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}} 
// reorder-note {{previous initialization for field 'd' is here}}
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} 
// reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} 
// reorder-note {{previous initialization for field 'b' is here}}
+.a = 1, // reorder-error {{field 'b' will be initialized after field 'a'}}
+};
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2844,7 +2844,7 @@
 SemaRef.Diag(DIE->getBeginLoc(), diag::ext_designated_init_reordered)
 << KnownField << PrevField << DIE->getSourceRange();
 
-unsigned OldIndex = NumBases + PrevField->getFieldIndex();
+unsigned OldIndex = NumBases + StructuredIndex - 1;
 if (StructuredList && OldIndex <= StructuredList->getNumInits()) {
   if (Expr *PrevInit = StructuredList->getInit(OldIndex)) {
 SemaRef.Diag(PrevInit->getBeginLoc(),
@@ -2948,8 +2948,12 @@
 // If this the first designator, our caller will continue checking
 // the rest of this struct/class/union subobject.
 if (IsFirstDesignator) {
+  if (Field != RD->field_end() && Field->isUnnamedBitfield())
+++Field;
+
   if (NextField)
 *NextField = Field;
+
   StructuredIndex = FieldIndex;
   return false;
 }


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note {{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note {{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,22 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+.e = 1, // reorder-note {{previous initialization for field 'e' is here}}
+.d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}} // reorder-note {{previous initialization for field 'd' is here}}
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} // reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} // reorder-note {{previous initialization for field 'b' is here}}
+.a = 1, // reorder-error {{field 'b' will be initialized after field 'a'}}
+};
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2844,7 +2844,7 @@
 SemaRef.Diag(DIE->getBeginLoc(), diag::ext_designated_init_reordered)
 

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:182
+namespace GH63605 {
+struct {
+  unsigned : 2;

Should we add bases?


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Thanks Shafik.

I have some concern about the diagnostics being wonky because,  if the init and 
field don't match,  it's hard to be sure we won't run into simar issues 
(although i agree this fixes the crash.)

Also, I'm not saying you should do this but... maybe this thing could be 
improved by a sort of `field_iterator` that skips over unnamed bitfield.




Comment at: clang/lib/Sema/SemaInit.cpp:2836-2837
   continue;
 if (*NextField != RD->field_end() &&
 declaresSameEntity(*FI, **NextField))
   break;

I think this is probably where it fails produce something sensible to 
diagnostics.
in the `*NextField == RD->field_end()` case we just go straight to the end.

In that case, we probably want to break when the previous element is the one 
under test, that way you might get to `b`.



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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:195
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} 
// reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} 
// reorder-note {{previous initialization for field 'e' is here}}
+.a = 1, // reorder-error {{field 'e' will be initialized after field 'a'}}

Note the `e` in the diagnostic in this line and the next line should be `b`. I 
have not managed to figure out how to fix this yet but this feels relatively 
minor compared to the crash.


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

https://reviews.llvm.org/D154675

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

[PATCH] D154675: [Clang] Fix crash when emitting diagnostic for out of order designated initializers in C++

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: aaron.ballman, cor3ntin.
Herald added a project: All.
shafik requested review of this revision.

In C++ we are not allowed to use designated initializers to initialize fields 
out of order. In some cases when diagnosing this we are crashing because we are 
not indexing correctly and therefore going out of bounds.

This fixes: https://github.com/llvm/llvm-project/issues/63605


https://reviews.llvm.org/D154675

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note 
{{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note 
{{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides 
prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,22 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+.e = 1, // reorder-note {{previous initialization for field 'e' is here}}
+.d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}} 
// reorder-note {{previous initialization for field 'd' is here}}
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} 
// reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} 
// reorder-note {{previous initialization for field 'e' is here}}
+.a = 1, // reorder-error {{field 'e' will be initialized after field 'a'}}
+};
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2844,7 +2844,7 @@
 SemaRef.Diag(DIE->getBeginLoc(), diag::ext_designated_init_reordered)
 << KnownField << PrevField << DIE->getSourceRange();
 
-unsigned OldIndex = NumBases + PrevField->getFieldIndex();
+unsigned OldIndex = NumBases + StructuredIndex - 1;
 if (StructuredList && OldIndex <= StructuredList->getNumInits()) {
   if (Expr *PrevInit = StructuredList->getInit(OldIndex)) {
 SemaRef.Diag(PrevInit->getBeginLoc(),


Index: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
===
--- clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -63,7 +63,7 @@
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note {{previous}}
   .y = 1, // override-note {{previous}}
-  .y = 1, // override-error {{overrides prior initialization}}
+  .y = 1, // override-error {{overrides prior initialization}} // reorder-note {{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
@@ -177,3 +177,22 @@
 h({.a = 1});
   }
 }
+
+namespace GH63605 {
+struct {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+.e = 1, // reorder-note {{previous initialization for field 'e' is here}}
+.d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}} // reorder-note {{previous initialization for field 'd' is here}}
+.c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} // reorder-note {{previous initialization for field 'c' is here}}
+.b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} // reorder-note {{previous initialization for field 'e' is here}}
+.a = 1, // reorder-error {{field 'e' will be initialized after field 'a'}}
+};
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2844,7 +2844,7 @@
 SemaRef.Diag(DIE->getBeginLoc(), diag::ext_designated_init_reordered)
 << KnownField << PrevField << DIE->getSourceRange();
 
-unsigned OldIndex = NumBases + PrevField->getFieldIndex();
+unsigned OldIndex = NumBases +