[clang] fix sema init crashing on initialization sequences #98102

yuxuanchen1997 · 2024-07-09T01:21:39Z

We ran into a FE crash and root caused to ER.get() on line 5584 here being nullptr. I think this is a result of not checking if ER here is invalid.

Example of crash-on-valid C++ https://gist.github.com/yuxuanchen1997/576dce964666f0f8713fccacf5847138

Note that this crash happens only with -std=c++20.

llvmbot · 2024-07-09T01:22:11Z

@llvm/pr-subscribers-clang

Author: Yuxuan Chen (yuxuanchen1997)

Changes

We ran into a FE crash and root caused to ER.get() on line 5584 here being nullptr. I think this is a result of not checking if ER here is invalid.

We have been using automated reduction tools (like CReduce) for a while and it is not performing well and would like to ask upstream opinions on whether this condition here is handled correctly. Preferably with help to write a small, well contained crash-on-valid test case.

I do have a crash-on-invalid test here, would really appreciate any pointers.

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

1 Files Affected:

(modified) clang/lib/Sema/SemaInit.cpp (+4)

diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 41753a1661ace..80286302e9b9d 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -5576,6 +5576,10 @@ static void TryOrBuildParenListInitialization(
       ExprResult ER;
       ER = IS.Perform(S, SubEntity, SubKind,
                       Arg ? MultiExprArg(Arg) : std::nullopt);
+
+      if (ER.IsInvalid())
+        return false;
+
       if (InitExpr)
         *InitExpr = ER.get();
       else

yuxuanchen1997 · 2024-07-09T01:23:14Z

clang/lib/Sema/SemaInit.cpp

+
+      if (ER.IsInvalid())
+        return false;
+
      if (InitExpr)
        *InitExpr = ER.get();


ER.get() is null if we don't apply this fix.

AaronBallman

Thank you!

You should also add test coverage (clang/test/SemaCXX) for the changes and a release note (clang/docs/ReleaseNotes.rst).

AaronBallman · 2024-07-09T12:25:54Z

clang/lib/Sema/SemaInit.cpp

@@ -5576,6 +5576,10 @@ static void TryOrBuildParenListInitialization(
      ExprResult ER;
      ER = IS.Perform(S, SubEntity, SubKind,
                      Arg ? MultiExprArg(Arg) : std::nullopt);
+
+      if (ER.isInvalid())


Under what circumstances does IS.Failed() return false above but when we perform the initialization we get a null expression back? That seems strange to me -- I would have naively expected Failed() to return true in such a case. WDYT?

If you look at InitializationSequence::Perform (this function is 1000 lines long), it first checks Failed() then proceed to build a whole bunch of things. There are a couple of places it returned ExprError. I think it's possible to still be invalid after !Failed().

Ah you are correct, sorry! I see now that the common code pattern is to check isInvalid() on the returned result.

cor3ntin · 2024-07-09T14:36:24Z

@Endilll you are able to reduce that further?

dcci · 2024-07-09T14:57:34Z

You should also add test coverage (clang/test/SemaCXX) for the changes and a release note (clang/docs/ReleaseNotes.rst).

@AaronBallman -- are you OK with a crash-on-invalid (the one in first post)? The original example we had crashed on valid code, but reducing a 35MB file with a lot of SFINAE is taking weeks with c-reduce.
Thanks for your review :)

AaronBallman · 2024-07-09T15:44:00Z

You should also add test coverage (clang/test/SemaCXX) for the changes and a release note (clang/docs/ReleaseNotes.rst).

@AaronBallman -- are you OK with a crash-on-invalid (the one in first post)? The original example we had crashed on valid code, but reducing a 35MB file with a lot of SFINAE is taking weeks with c-reduce. Thanks for your review :)

Ideally we'd have test coverage for both the crash-on-invalid and crash-on-valid code, if possible. If we can't get a reduction to a crash-on-valid case, then crash-on-invalid that also fixes the valid case would be okay. But we could perhaps help reduce the crash-on-valid code if you can share it.

dcci · 2024-07-09T15:53:50Z

You should also add test coverage (clang/test/SemaCXX) for the changes and a release note (clang/docs/ReleaseNotes.rst).

@AaronBallman -- are you OK with a crash-on-invalid (the one in first post)? The original example we had crashed on valid code, but reducing a 35MB file with a lot of SFINAE is taking weeks with c-reduce. Thanks for your review :)

Ideally we'd have test coverage for both the crash-on-invalid and crash-on-valid code, if possible. If we can't get a reduction to a crash-on-valid case, then crash-on-invalid that also fixes the valid case would be okay. But we could perhaps help reduce the crash-on-valid code if you can share it.

Thank you Aaron. Unfortunately this is from some Meta internal codebase, so it's hard to share. I'm trying to reduce myself.

yuxuanchen1997 · 2024-07-09T23:57:36Z

@AaronBallman I have good news. I was able to manually write a well-formed C++ file that crashes. https://gist.github.com/yuxuanchen1997/576dce964666f0f8713fccacf5847138

And I am able to validate that the fix I proposed here does fix the problem. I am going to add this test to the patch and figure out why we were crashing in the first place.

Endilll · 2024-07-10T09:51:18Z

@Endilll you are able to reduce that further?

I don't see the original reproducer anywhere. Snippet in the description is already reduced (and got quite damaged in the process). I can reduce it myself if someone points me out to the original reproducer.

AaronBallman

The changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.

AaronBallman · 2024-07-10T12:51:47Z

clang/lib/Sema/SemaInit.cpp

@@ -5576,6 +5576,10 @@ static void TryOrBuildParenListInitialization(
      ExprResult ER;
      ER = IS.Perform(S, SubEntity, SubKind,
                      Arg ? MultiExprArg(Arg) : std::nullopt);
+
+      if (ER.isInvalid())


Ah you are correct, sorry! I see now that the common code pattern is to check isInvalid() on the returned result.

yuxuanchen1997 · 2024-07-10T16:50:28Z

The changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.

Added. Thanks.

AaronBallman

LGTM, thank you for the fix!

We ran into a FE crash and root caused to `ER.get()` on line 5584 here being nullptr. I think this is a result of not checking if ER here is invalid. Example of crash-on-valid C++ https://gist.github.com/yuxuanchen1997/576dce964666f0f8713fccacf5847138 Note that this crash happens only with `-std=c++20`.

yuxuanchen1997 requested review from cor3ntin and AaronBallman July 9, 2024 01:21

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

yuxuanchen1997 commented Jul 9, 2024

View reviewed changes

dcci requested review from shafik and erichkeane July 9, 2024 01:46

yuxuanchen1997 force-pushed the fix-sema-init-crash branch from 82e3cb0 to 01a5dd6 Compare July 9, 2024 06:00

AaronBallman reviewed Jul 9, 2024

View reviewed changes

yuxuanchen1997 force-pushed the fix-sema-init-crash branch 5 times, most recently from f12bfc7 to 3c5b72d Compare July 10, 2024 04:07

AaronBallman reviewed Jul 10, 2024

View reviewed changes

yuxuanchen1997 changed the title ~~[clang] fix sema init crash for not checking a ExprResult~~ [clang] fix sema init crashing on initialization sequences Jul 10, 2024

yuxuanchen1997 and others added 2 commits July 10, 2024 09:49

[clang] fix sema init crash for not checking a ExprResult

097881d

Add a crash-on-valid unit test

4c2b881

yuxuanchen1997 force-pushed the fix-sema-init-crash branch from 3c5b72d to 4c2b881 Compare July 10, 2024 16:50

AaronBallman approved these changes Jul 10, 2024

View reviewed changes

yuxuanchen1997 merged commit 6f13c71 into llvm:main Jul 10, 2024
6 of 7 checks passed

yuxuanchen1997 deleted the fix-sema-init-crash branch July 10, 2024 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] fix sema init crashing on initialization sequences #98102

[clang] fix sema init crashing on initialization sequences #98102

yuxuanchen1997 commented Jul 9, 2024 •

edited

Loading

llvmbot commented Jul 9, 2024

yuxuanchen1997 Jul 9, 2024

AaronBallman left a comment

AaronBallman Jul 9, 2024

yuxuanchen1997 Jul 9, 2024

AaronBallman Jul 10, 2024

cor3ntin commented Jul 9, 2024

dcci commented Jul 9, 2024

AaronBallman commented Jul 9, 2024

dcci commented Jul 9, 2024

yuxuanchen1997 commented Jul 9, 2024 •

edited

Loading

Endilll commented Jul 10, 2024 •

edited

Loading

AaronBallman left a comment

AaronBallman Jul 10, 2024

yuxuanchen1997 commented Jul 10, 2024

AaronBallman left a comment

[clang] fix sema init crashing on initialization sequences #98102

[clang] fix sema init crashing on initialization sequences #98102

Conversation

yuxuanchen1997 commented Jul 9, 2024 • edited Loading

llvmbot commented Jul 9, 2024

yuxuanchen1997 Jul 9, 2024

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Jul 9, 2024

Choose a reason for hiding this comment

yuxuanchen1997 Jul 9, 2024

Choose a reason for hiding this comment

AaronBallman Jul 10, 2024

Choose a reason for hiding this comment

cor3ntin commented Jul 9, 2024

dcci commented Jul 9, 2024

AaronBallman commented Jul 9, 2024

dcci commented Jul 9, 2024

yuxuanchen1997 commented Jul 9, 2024 • edited Loading

Endilll commented Jul 10, 2024 • edited Loading

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Jul 10, 2024

Choose a reason for hiding this comment

yuxuanchen1997 commented Jul 10, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

yuxuanchen1997 commented Jul 9, 2024 •

edited

Loading

yuxuanchen1997 commented Jul 9, 2024 •

edited

Loading

Endilll commented Jul 10, 2024 •

edited

Loading