[clang] Check `expr` inside `InitListChecker::UpdateStructuredListElement()`

[ArcsinX]

Summary

• Prevent nullptr-deference at try to emit warning for invalid expr
• Simplify InitListChecker::UpdateStructuredListElement() usages. We do not need to check expr and increment StructuredIndex (for invalid expr) before the call anymore.

[aaron.ballman]

Should the StructuredIndex still be incremented even in the case of an error, as done around line 1589 and 1647? Do we need a similar change around line 2405?

I sort of wonder if the correct change is to make UpdateStructuredListElement() resilient to being passed a null Expr * and then removing the manual increments when the expression is an error.

[ArcsinX]

In D85193#2204685, @aaron.ballman wrote:

Should the StructuredIndex still be incremented even in the case of an error, as done around line 1589 and 1647?

Yes, it looks like we need to increment StructuredIndex.

Do we need a similar change around line 2405?

Think so. But I could not find a test case for this.

I sort of wonder if the correct change is to make UpdateStructuredListElement() resilient to being passed a null Expr * and then removing the manual increments when the expression is an error.

Should it be in this patch?

[aaron.ballman]

In D85193#2207052, @ArcsinX wrote:

In D85193#2204685, @aaron.ballman wrote:

Should the StructuredIndex still be incremented even in the case of an error, as done around line 1589 and 1647?

Yes, it looks like we need to increment StructuredIndex.

Do we need a similar change around line 2405?

Think so. But I could not find a test case for this.

I sort of wonder if the correct change is to make UpdateStructuredListElement() resilient to being passed a null Expr * and then removing the manual increments when the expression is an error.

Should it be in this patch?

Given that we have at least four uses of this pattern in the file, I'd say it should be this patch. WDYT?

[ArcsinX]

Check for expr == nullptr inside InitListChecker::UpdateStructuredListElement()

[ArcsinX]

In D85193#2207798, @aaron.ballman wrote:

In D85193#2207052, @ArcsinX wrote:

In D85193#2204685, @aaron.ballman wrote:

I sort of wonder if the correct change is to make UpdateStructuredListElement() resilient to being passed a null Expr * and then removing the manual increments when the expression is an error.

Should it be in this patch?

Given that we have at least four uses of this pattern in the file, I'd say it should be this patch. WDYT?

Agree, updated patch.

But I am not sure about correct place for expr check. Seems nullptr dereference possible only here: diagnoseInitOverride(PrevInit, expr->getSourceRange());.

[aaron.ballman]

clang/lib/Sema/SemaInit.cpp

3088

I would move the check up to here as the only time we should get a null expression is if something else has gone wrong (and update the comment as well).

[ArcsinX]

clang/lib/Sema/SemaInit.cpp

3088

Could we add expr check after return? As far as we need to increment StructuredIndex in that case.

[aaron.ballman]

LGTM!

clang/lib/Sema/SemaInit.cpp

3088

Ack, sorry about my think-o, you're absolutely correct!

From what I can tell, updateInit() is resilient to a null Expr*, so your code is correct as-is. It looked odd to my eyes because it made me wonder if we need to still issue the diagnostic given the comment. Leaving your code as-is and updating the adjacent comment would also solve my concern.

[ArcsinX]

Update comment

[aaron.ballman]

I have a suggestion for the added comment, but I also forgot to ask, do you need someone to commit on your behalf?

clang/lib/Sema/SemaInit.cpp

3094

I was hoping for something that describes the "why" more than the "what". How about: No need to diagnose when expr is null because a more relevant diagnostic has already been issued and this diagnostic is potentially noise.

[ArcsinX]

Fix comment

[ArcsinX]

In D85193#2212976, @aaron.ballman wrote:

I have a suggestion for the added comment,

Fixed comment according to your suggestion.

but I also forgot to ask, do you need someone to commit on your behalf?

I already have commit access.

Thanks for review!

[aaron.ballman]

In D85193#2213384, @ArcsinX wrote:

In D85193#2212976, @aaron.ballman wrote:

I have a suggestion for the added comment,

Fixed comment according to your suggestion.

but I also forgot to ask, do you need someone to commit on your behalf?

I already have commit access.

Thanks for review!

Excellent, this LGTM, thank you for the fix!

clang/lib/Sema/SemaInit.cpp

else {
     ResultExpr = Result.getAs<Expr>();
 
     if (ResultExpr != expr && !VerifyOnly) {
       // The type was promoted, update initializer list.
       // FIXME: Why are we updating the syntactic init list?
       IList->setInit(Index, ResultExpr);
     }
   }
-  if (hadError)
-    ++StructuredIndex;
-  else
-    UpdateStructuredListElement(StructuredList, StructuredIndex, ResultExpr);
   ++Index;
 }
 
 void InitListChecker::CheckReferenceType(const InitializedEntity &Entity,
                                          InitListExpr *IList, QualType DeclType,
                                          unsigned &Index,
                                          InitListExpr *StructuredList,
                                          unsigned &StructuredIndex) {
void InitListChecker::CheckReferenceType(const InitializedEntity &Entity,
   if (Result.isInvalid())
     hadError = true;
 
   expr = Result.getAs<Expr>();
   // FIXME: Why are we updating the syntactic init list?
   if (!VerifyOnly && expr)
     IList->setInit(Index, expr);
 
-  if (hadError)
-    ++StructuredIndex;
-  else
-    UpdateStructuredListElement(StructuredList, StructuredIndex, expr);
   ++Index;
 }
 
 void InitListChecker::CheckVectorType(const InitializedEntity &Entity,
                                       InitListExpr *IList, QualType DeclType,
                                       unsigned &Index,
                                       InitListExpr *StructuredList,
                                       unsigned &StructuredIndex) {
if (!isa<InitListExpr>(Init) && Init->getType()->isVectorType()) {
         ResultExpr = Result.getAs<Expr>();
 
         if (ResultExpr != Init && !VerifyOnly) {
           // The type was promoted, update initializer list.
           // FIXME: Why are we updating the syntactic init list?
           IList->setInit(Index, ResultExpr);
         }
       }
-      if (hadError)
-        ++StructuredIndex;
-      else
-        UpdateStructuredListElement(StructuredList, StructuredIndex,
-                                    ResultExpr);
+      UpdateStructuredListElement(StructuredList, StructuredIndex, ResultExpr);
       ++Index;
       return;
     }
 
     InitializedEntity ElementEntity =
       InitializedEntity::InitializeElement(SemaRef.Context, 0, Entity);
 
     for (unsigned i = 0; i < maxElements; ++i, ++numEltsInit) {

 }
 
 /// Update the initializer at index @p StructuredIndex within the
 /// structured initializer list to the value @p expr.
 void InitListChecker::UpdateStructuredListElement(InitListExpr *StructuredList,
                                                   unsigned &StructuredIndex,
                                                   Expr *expr) {
   // No structured initializer list to update
   if (!StructuredList)
     return;
 
   if (Expr *PrevInit = StructuredList->updateInit(SemaRef.Context,
                                                   StructuredIndex, expr)) {
-    // This initializer overwrites a previous initializer. Warn.
+    // This initializer overwrites a previous initializer.
+    // No need to diagnose when `expr` is nullptr because a more relevant
+    // diagnostic has already been issued and this diagnostic is potentially
+    // noise.
+    if (expr)
       diagnoseInitOverride(PrevInit, expr->getSourceRange());
   }
 
   ++StructuredIndex;
 }
 
 /// Determine whether we can perform aggregate initialization for the purposes
 /// of overload resolution.
 bool Sema::CanPerformAggregateInitializationForOverloadResolution(

clang/test/Sema/init-invalid-struct-array.c

+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+struct S {
+  Unknown u; // expected-error {{unknown type name 'Unknown'}}
+  int i;
+};
+// Should not crash
+struct S s[] = {[0].i = 0, [1].i = 1, {}};