[Clang] fix range calculation for conditionals with throw expressions #112081
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Oleksandr T. (a-tarasyuk) ChangesFixes #111854 Full diff: https://github.com/llvm/llvm-project/pull/112081.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 337e3fc10bf49d..2ab13640bfa53c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -512,6 +512,7 @@ Bug Fixes to C++ Support
and undeclared templates. (#GH107047, #GH49093)
- Clang no longer crashes when a lambda contains an invalid block declaration that contains an unexpanded
parameter pack. (#GH109148)
+- Fixed assertion failure in range calculations for conditional throw expressions (#GH111854)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 2bcb930acdcb57..b3d88f053872c1 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -9827,6 +9827,9 @@ static IntRange GetExprRange(ASTContext &C, const Expr *E, unsigned MaxWidth,
return IntRange(BitField->getBitWidthValue(C),
BitField->getType()->isUnsignedIntegerOrEnumerationType());
+ if (GetExprType(E)->isVoidType())
+ return IntRange{0, true};
+
return IntRange::forValueOfType(C, GetExprType(E));
}
diff --git a/clang/test/SemaCXX/conditional-expr.cpp b/clang/test/SemaCXX/conditional-expr.cpp
index 01effaa189322b..8f17555fd806ff 100644
--- a/clang/test/SemaCXX/conditional-expr.cpp
+++ b/clang/test/SemaCXX/conditional-expr.cpp
@@ -429,3 +429,10 @@ void g() {
long e = a = b ? throw 0 : throw 1;
}
} // namespace PR46484
+
+namespace GH111854 {
+void f() {
+ (true ? throw 0 : 0) <= 0; // expected-warning {{relational comparison result unused}}
+ (false ? 0 : throw 0) <= 0; // expected-warning {{relational comparison result unused}}
+}
+}
|
a-tarasyuk
commented
Oct 17, 2024
shafik
reviewed
Oct 18, 2024
Collaborator
shafik
left a comment
shafik
left a comment
There was a problem hiding this comment.
After answering my questions this makes sense to me but I want a second set of eyes.
clang/lib/Sema/SemaChecking.cpp
Outdated
| BitField->getType()->isUnsignedIntegerOrEnumerationType()); | ||
|
|
||
| if (GetExprType(E)->isVoidType()) | ||
| return IntRange{0, true}; |
Collaborator
There was a problem hiding this comment.
I think these changes are fine as-is, but I do wonder if a cleaner model for us to aim for is to make the return value std::optional and return no valid object in this case; a void expression has no values it can take, and that's different than a value range of [0,0].
Member
Author
There was a problem hiding this comment.
@AaronBallman thanks for the feedback. Do you mean the following changes?
Member
Author
There was a problem hiding this comment.
@erichkeane @shafik @AaronBallman when you have a moment, could you review these changes? thanks
Member
Author
There was a problem hiding this comment.
@AaronBallman @shafik if these changes look good to you, would you mind proceeding with the merge? thanks
… expression evaluation
… into fix/111854
a-tarasyuk
force-pushed
the
fix/111854
branch
from
f3b11c8 to
3d1ed95
Compare
erichkeane
approved these changes
Oct 25, 2024
Collaborator
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Update comment, else LGTM.
erichkeane
approved these changes
Oct 25, 2024
Contributor
|
@a-tarasyuk do you need us to merge that for you? |
Member
Author
|
@cor3ntin yes, I do. I don't have permission to merge. thanks |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/41/builds/3058 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/4/builds/3185 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/3165 Here is the relevant piece of the build log for the reference |
Collaborator
|
The sanitizer failures are likely unrelated to this, but the self-build of the Flang sources might be? Do we have evidence of those failing on other commits? |
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…llvm#112081) Fixes llvm#111854 --- The issue arises when `GetExprRange` encounters a `ConditionalOperator` with a `CXXThrowExpr` ```md ConditionalOperator 0x1108658e0 'int' |-CXXBoolLiteralExpr 0x110865878 '_Bool' true |-CXXThrowExpr 0x1108658a8 'void' | `-IntegerLiteral 0x110865888 'int' 0 `-IntegerLiteral 0x1108658c0 'int' 0 ``` https://github.com/llvm/llvm-project/blob/ed3d05178274890fb804f43ae1bcdfd33b5fd8f0/clang/lib/Sema/SemaChecking.cpp#L9628-L9631 The current behavior causes the `GetExprRange` to proceed with the throw expression (`CO->getTrueExpr()`/`void` type)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #111854
The issue arises when
GetExprRangeencounters aConditionalOperatorwith aCXXThrowExprConditionalOperator 0x1108658e0 'int' |-CXXBoolLiteralExpr 0x110865878 '_Bool' true |-CXXThrowExpr 0x1108658a8 'void' | `-IntegerLiteral 0x110865888 'int' 0 `-IntegerLiteral 0x1108658c0 'int' 0llvm-project/clang/lib/Sema/SemaChecking.cpp
Lines 9628 to 9631 in ed3d051
The current behavior causes the
GetExprRangeto proceed with the throw expression (CO->getTrueExpr()/voidtype)