[clang] Fix a crash issue that caused by handling of fields with initializers in nested anonymous unions #113049

yronglin · 2024-10-19T16:17:24Z

This PR create an RecoveryExpr for invalid in-class-initializer.

llvmbot · 2024-10-19T16:17:56Z

@llvm/pr-subscribers-clang

Author: None (yronglin)

Changes

Fix a crash issue that caused by handling of fields with initializers in nested anonymous unions.
If my thinking is wrong, I apologize, and IIUC seems it's caused by 5ae5774.

Full diff: https://github.com/llvm/llvm-project/pull/113049.diff

2 Files Affected:

(modified) clang/lib/Sema/SemaInit.cpp (+10-5)
(modified) clang/test/SemaCXX/cxx1y-initializer-aggregates.cpp (+11)

diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index f560865681fa5a..4878dcb5d2c8e0 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -868,13 +868,19 @@ InitListChecker::FillInEmptyInitializations(const InitializedEntity &Entity,
 
   if (const RecordType *RType = ILE->getType()->getAs<RecordType>()) {
     const RecordDecl *RDecl = RType->getDecl();
-    if (RDecl->isUnion() && ILE->getInitializedFieldInUnion()) {
+    if (RDecl->isUnion() && ILE->getInitializedFieldInUnion())
       FillInEmptyInitForField(0, ILE->getInitializedFieldInUnion(), Entity, ILE,
                               RequiresSecondPass, FillWithNoInit);
+    else if (RDecl->isUnion() && isa<CXXRecordDecl>(RDecl) &&
+             cast<CXXRecordDecl>(RDecl)->hasInClassInitializer()) {
+      for (auto *Field : RDecl->fields()) {
+        if (Field->hasInClassInitializer()) {
+          FillInEmptyInitForField(0, Field, Entity, ILE, RequiresSecondPass,
+                                  FillWithNoInit);
+          break;
+        }
+      }
     } else {
-      assert((!RDecl->isUnion() || !isa<CXXRecordDecl>(RDecl) ||
-              !cast<CXXRecordDecl>(RDecl)->hasInClassInitializer()) &&
-             "We should have computed initialized fields already");
       // The fields beyond ILE->getNumInits() are default initialized, so in
       // order to leave them uninitialized, the ILE is expanded and the extra
       // fields are then filled with NoInitExpr.
@@ -2296,7 +2302,6 @@ void InitListChecker::CheckStructUnionTypes(
           return;
         }
       }
-      llvm_unreachable("Couldn't find in-class initializer");
     }
 
     // Value-initialize the first member of the union that isn't an unnamed
diff --git a/clang/test/SemaCXX/cxx1y-initializer-aggregates.cpp b/clang/test/SemaCXX/cxx1y-initializer-aggregates.cpp
index 03a6800898d18f..8360b8fd7d8ee2 100644
--- a/clang/test/SemaCXX/cxx1y-initializer-aggregates.cpp
+++ b/clang/test/SemaCXX/cxx1y-initializer-aggregates.cpp
@@ -115,3 +115,14 @@ namespace nested_union {
   // of Test3, or we should exclude f(Test3) as a candidate.
   static_assert(f({1}) == 2, ""); // expected-error {{call to 'f' is ambiguous}}
 }
+
+// Fix crash issue https://github.com/llvm/llvm-project/issues/112560.
+// Make sure clang compiles the following code without crashing:
+namespace GH112560 {
+union U {
+  int f = ; // expected-error {{expected expression}}
+};
+void foo() {
+  U g{};
+}
+} // namespace GH112560

efriedma-quic · 2024-10-21T06:05:42Z

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

+// Make sure clang compiles the following code without crashing:
+namespace GH112560 {
+union U {
+  int f = ; // expected-error {{expected expression}}


As far as I can tell, the reason the code in SemaInit is getting confused is that the union is marked "hasInClassInitializer" because of the initializer... but then there isn't actually an initializer anywhere because we failed to parse it.

I think hasInClassInitializer() on RecordDecl needs to be consistent with hasInClassInitializer() on FieldDecl. If hasInClassInitializer is true, there should be an initializer somewhere. Either we should pretend the user didn't write the "=", or we should construct a RecoveryExpr to represent the missing initializer. Adding extra handling to every bit of code that checks hasInClassInitializer spreads out error handling to places it shouldn't exist.

Thanks for the review! Make sense, there should have a RecoveryExpr as the initializer. I'd like to give a try.

shafik · 2024-11-16T02:09:07Z

Just wanted to ping on this and see how it was going.

efriedma-quic · 2024-11-19T00:08:45Z

On my end, still waiting for review comments to be addressed.

…ializers in nested anonymous unions Signed-off-by: yronglin <[email protected]>

yronglin · 2024-11-20T16:01:31Z

Sorry for the very late update, I had some troubles in the past two months.

This update check the number of C++11 in-class-initializer in the class and update CXXRecordDecl::DefinitionData::HasInClassInitializer when calling FieldDecl::hasInClassInitializer().

I think we might go futhur to create RecoveryExpr for invalid in-class-initializer in FieldDecl, but this will interrupt the processing of the rest class members. e.g. The "default member initializer for 'r' needed" will missing because FibTree *l is invalid in the following:

namespace use_self {
  struct FibTree {
    int n;
    FibTree *l = // expected-note {{declared here}}
      n > 1 ? new FibTree{n-1} : &fib0; // expected-error {{default member initializer for 'l' needed}}
    FibTree *r = // expected-note {{declared here}}
      n > 2 ? new FibTree{n-2} : &fib0; // expected-error {{default member initializer for 'r' needed}}
    int v = l->v + r->v;

    static FibTree fib0;
  };
  FibTree FibTree::fib0{0, nullptr, nullptr, 1};

  int fib(int n) { return FibTree{n}.v; }
}

The hadError flag will be true after filled in FibTree *l.

llvm-project/clang/lib/Sema/SemaInit.cpp

Lines 895 to 912 in fce917d

    
           for (auto *Field : RDecl->fields()) { 
        
             if (Field->isUnnamedBitField()) 
        
               continue; 
        
             if (hadError) 
        
               return; 
        
             FillInEmptyInitForField(Init, Field, Entity, ILE, RequiresSecondPass, 
        
                                     FillWithNoInit); 
        
             if (hadError) 
        
               return; 
        
             ++Init; 
        
             // Only look at the first initialization of a union. 
        
             if (RDecl->isUnion()) 
        
               break; 
        
           }

Sould we create an RecoveryExpr for invalid in-class-initializer? WDYT?

efriedma-quic · 2024-11-20T23:16:58Z

We could maybe look at setting hadError less aggressively in InitListChecker, for cases where the error is unlikely to impact the overall parse. If a member has a RecoveryExpr as its initializer, it's probably reasonable to continue producing error messages.

That said, I don't really care if we suppress the second error message in your FibTree example. It's not a situation I'd expect people to trip over regularly.

When I first looked at the patch, I didn't realize the error messages were coming out of delayed C++ field parsing. I don't think the current version of the patch is viable: modifying RecordDecl::hasInClassInitializer() after it's already been queried is likely to lead to strange results.

…ith initializers in nested anonymous unions" This reverts commit 804b103.

Signed-off-by: yronglin <[email protected]>

yronglin · 2024-11-24T16:15:57Z

Thanks for your review! The latest update add a RecoveryExpr for invalid in-class-initializer. But I have some concerns about the inconsistency between RecordDecl::hasInClassInitializer() and FieldDecl::hasInClassInitializer(). FieldDecl has a method removeInClassInitializer to modifiy the AST Node.

hokein

Attaching an empty RecoveryExpr as an initializer to indicate that initialization was attempted but failed seems reasonable to me. I'll leave the approval to @efriedma-quic.

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

Signed-off-by: yronglin <[email protected]>

yronglin · 2024-11-29T11:13:03Z

friendly ping～

hokein

Is the PR description still up-to-date?

clang/lib/Sema/SemaInit.cpp

yronglin · 2024-12-04T14:00:41Z

Is the PR description still up-to-date?

Yeah! The description out-of-date.

Signed-off-by: yronglin <[email protected]>

efriedma-quic · 2024-12-04T20:28:21Z

clang/lib/Sema/SemaInit.cpp

+
+      // We do not want to aggressively set the hadError flag and cutoff
+      // parsing. Try to recover when in-class-initializer had errors.
+      if (Field->getInClassInitializer() &&


I'm not sure I understand the purpose of this code. Why are we not handling this in BuildCXXDefaultInitExpr()?

I've move the code into BuildCXXDefaultInitExpr(), the original purpose of putting it here is to return quickly and avoid entering any context again.

ExprResult DIE; { // Enter a default initializer rebuild context, then we can support // lifetime extension of temporary created by aggregate initialization // using a default member initializer. // CWG1815 (https://wg21.link/CWG1815). EnterExpressionEvaluationContext RebuildDefaultInit( SemaRef, Sema::ExpressionEvaluationContext::PotentiallyEvaluated); SemaRef.currentEvaluationContext().RebuildDefaultArgOrDefaultInit = true; SemaRef.currentEvaluationContext().DelayedDefaultInitializationContext = SemaRef.parentEvaluationContext() .DelayedDefaultInitializationContext; SemaRef.currentEvaluationContext().InLifetimeExtendingContext = SemaRef.parentEvaluationContext().InLifetimeExtendingContext; DIE = SemaRef.BuildCXXDefaultInitExpr(Loc, Field); }

Signed-off-by: yronglin <[email protected]>

efriedma-quic · 2024-12-05T20:15:17Z

clang/lib/Sema/SemaExpr.cpp

+  // in-class-initializer had errors.
+  if (Field->getInClassInitializer() &&
+      Field->getInClassInitializer()->containsErrors())
+    return Field->getInClassInitializer();


I guess the idea here is that we're throwing away too much if we just return an ExprError? I assume everything would work without crashing, we'd just produce slightly different diagnostics. So... I guess my first question is, do we really need to do this in this patch, as opposed to a followup?

BuildCXXDefaultInitExpr is supposed to, as the name suggests, return a CXXDefaultInitExpr. As-is, this is going to produce weird results with erroneous expressions that aren't just a RecoveryExpr, like typo corrections. Maybe we can explicitly create a RecoveryExpr here, though, nested inside the CXXDefaultInitExpr, or something like that.

I guess the idea here is that we're throwing away too much if we just return an ExprError?

Yes.

BuildCXXDefaultInitExpr is supposed to, as the name suggests, return a CXXDefaultInitExpr.

Agree.

Should we remove this code from BuildCXXDefaultInitExpr? Because we createdRecoveryExpr in Sema::ActOnFinishCXXInClassMemberInitializer.

// If we might have already tried and failed to instantiate, don't try again. if (Field->isInvalidDecl()) return ExprError();

and the AST will be:

-CompoundStmt 0x12480cb70 <col:12, line:494:1> 537: `-DeclStmt 0x12480cb58 <line:493:3, col:8> 538: `-VarDecl 0x12480ca40 <col:3, col:7> col:5 g 'U':'GH112560::U' listinit 539: `-InitListExpr 0x12480cae8 <col:6, col:7> 'U':'GH112560::U' contains-errors field Field 0x12480c870 'f' 'int' 540: `-CXXDefaultInitExpr 0x12480cb30 <col:7> 'int' contains-errors has rewritten init 541: `-RecoveryExpr 0x12480c910 <line:482:9> 'int' contains-errors

Signed-off-by: yronglin <[email protected]>

efriedma-quic

LGTM

I'm a little concerned this is going to lead to other crashes due to exposing more codepaths to "invalid" decls. But it seems like it's doing the right thing in the given cases.

yronglin · 2024-12-10T09:33:08Z

Thanks for the review!

yronglin · 2024-12-10T14:39:17Z

LGTM

I'm a little concerned this is going to lead to other crashes due to exposing more codepaths to "invalid" decls. But it seems like it's doing the right thing in the given cases.

Yeah, if the PR breaks clang, I'll revert it quickly.

yronglin requested a review from efriedma-quic October 19, 2024 16:17

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 19, 2024

yronglin mentioned this pull request Oct 19, 2024

[clang++] Couldn't find in-class initializer UNREACHABLE executed at /root/llvm-project/clang/lib/Sema/SemaInit.cpp:2299! #112560

Closed

yronglin requested review from cor3ntin, shafik and zygoloid October 19, 2024 16:48

efriedma-quic reviewed Oct 21, 2024

View reviewed changes

[clang] Fix a crash issue that caused by handling of fields with init…

804b103

…ializers in nested anonymous unions Signed-off-by: yronglin <[email protected]>

yronglin force-pushed the fix_gh_112560 branch from 5729311 to 804b103 Compare November 20, 2024 15:46

yronglin added 2 commits November 23, 2024 23:26

Revert "[clang] Fix a crash issue that caused by handling of fields w…

de4a9d5

…ith initializers in nested anonymous unions" This reverts commit 804b103.

Recovery from the invalid in-class-initializer

882fd4e

Signed-off-by: yronglin <[email protected]>

yronglin requested a review from Endilll as a code owner November 24, 2024 16:10

yronglin requested a review from hokein November 24, 2024 16:16

hokein reviewed Nov 25, 2024

View reviewed changes

clang/test/SemaCXX/cxx1y-initializer-aggregates.cpp Show resolved Hide resolved

Add test for ast-dump

cf5a038

Signed-off-by: yronglin <[email protected]>

hokein reviewed Dec 3, 2024

View reviewed changes

clang/lib/Sema/SemaInit.cpp Outdated Show resolved Hide resolved

Try to recover when in-class-initializer had errors

ac30f8a

Signed-off-by: yronglin <[email protected]>

efriedma-quic reviewed Dec 4, 2024

View reviewed changes

Move the error check into BuildCXXDefaultInit

c0be803

Signed-off-by: yronglin <[email protected]>

efriedma-quic reviewed Dec 5, 2024

View reviewed changes

Build invalid default-member-init in BuildCXXDefaultInitExpr

5be9331

Signed-off-by: yronglin <[email protected]>

efriedma-quic approved these changes Dec 6, 2024

View reviewed changes

yronglin merged commit 740861d into llvm:main Dec 10, 2024
8 checks passed

[clang] Fix a crash issue that caused by handling of fields with initializers in nested anonymous unions #113049

[clang] Fix a crash issue that caused by handling of fields with initializers in nested anonymous unions #113049

Uh oh!

Conversation

yronglin commented Oct 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 19, 2024

Uh oh!

efriedma-quic Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

yronglin Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

shafik commented Nov 16, 2024

Uh oh!

efriedma-quic commented Nov 19, 2024

Uh oh!

yronglin commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yronglin commented Nov 24, 2024

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yronglin commented Nov 29, 2024

Uh oh!

hokein left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yronglin commented Dec 4, 2024

Uh oh!

efriedma-quic Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

yronglin Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

yronglin Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

yronglin commented Dec 10, 2024

Uh oh!

Uh oh!

yronglin commented Dec 10, 2024

Uh oh!

Uh oh!

yronglin commented Oct 19, 2024 •

edited

Loading

yronglin commented Nov 20, 2024 •

edited

Loading

efriedma-quic commented Nov 20, 2024 •

edited

Loading

hokein left a comment •

edited

Loading

yronglin Dec 6, 2024 •

edited

Loading