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

[shafik]

Summary

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

[shafik]

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

195

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.

[cor3ntin]

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.

clang/lib/Sema/SemaInit.cpp

2836–2837

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.

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

182

Should we add bases?

[shafik]

-Add fix for wrong field in diagnostic

[shafik]

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

[shafik]

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

182

Good catch b/c of course bases would cause problems 🤣

[shafik]

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

[shafik]

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

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

182

Problems due to bases now fixed as well.

195

This is now fixed

[shafik]

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

[cor3ntin]

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

[aaron.ballman]

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. :-)

clang/lib/Sema/SemaInit.cpp

2847

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.

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

66

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.

[shafik]

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

66

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.

[aaron.ballman]

LGTM but please land with a release note.

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

66

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.

[shafik]

• Add release notes.

clang/lib/Sema/SemaInit.cpp

if (IsFirstDesignator && !VerifyOnly && SemaRef.getLangOpts().CPlusPlus &&
         NextField &&
         (*NextField == RD->field_end() ||
          (*NextField)->getFieldIndex() > Field->getFieldIndex() + 1)) {
       // Find the field that we just initialized.
       FieldDecl *PrevField = nullptr;
       for (auto FI = RD->field_begin(); FI != RD->field_end(); ++FI) {
         if (FI->isUnnamedBitfield())
           continue;
         if (*NextField != RD->field_end() &&
             declaresSameEntity(*FI, **NextField))
           break;
         PrevField = *FI;
       }
 
       if (PrevField &&
           PrevField->getFieldIndex() > KnownField->getFieldIndex()) {
         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(),
                          diag::note_previous_field_init)
                 << PrevField << PrevInit->getSourceRange();
           }
         }
       }
if (D->isFieldDesignator()) {
     // Find the position of the next field to be initialized in this
     // subobject.
     ++Field;
     ++FieldIndex;
 
     // 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;
     }
 
     if (!FinishSubobjectInit)
       return false;
 
     // We've already initialized something in the union; we're done.

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

 B b3 = {.a = 1, 2}; // pedantic-error {{mixture of designated and non-designated}} pedantic-note {{first non-designated}} pedantic-error {{brace elision}}
 B b4 = {.a = 1, 2, 3}; // pedantic-error {{mixture of designated and non-designated}} pedantic-note {{first non-designated}} pedantic-error {{brace elision}} expected-error {{excess elements}}
 B b5 = {.a = nullptr}; // expected-error {{cannot initialize}}
 struct C { int :0, x, :0, y, :0; };
 C c = {
   .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}}
 };
 }
 
 namespace base_class {
   struct base {
     int x;
namespace no_unwrap {
   void h(Q q);
   void h(S s);
 
   // OK, does not instantiate X<void&> (!).
   void i() {
     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'}}
+};
+}