[Clang] Correctly handle allocations in the condition of a if constexpr
#146890
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
…xpr`
Deal with the following scenario
```cpp
struct S {
char* c = new char;
constexpr ~S() {
delete c;
}
};
if constexpr((S{}, true)){};
```
There were two issues
- We need to produce a full expressions _before_ evaluating the condition
(otherwise automatic variables are neber destroyed)
- We need to preserve the evaluation context of the condition while doing
the semantics analysis for it (lest it is evaluated in a non constant
evaluated context)
Fixes llvm#120197
Fixes llvm#134820
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Corentin Jabot (cor3ntin) ChangesDeal with the following scenario struct S {
char* c = new char;
constexpr ~S() {
delete c;
}
};
if constexpr((S{}, true)){};There were two issues
Fixes #120197 Full diff: https://github.com/llvm/llvm-project/pull/146890.diff 6 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3d893e0aa8e2c..9f9fc84b53104 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -896,6 +896,7 @@ Bug Fixes to C++ Support
- Fixed a crash when constant evaluating some explicit object member assignment operators. (#GH142835)
- Fixed an access checking bug when substituting into concepts (#GH115838)
- Fix a bug where private access specifier of overloaded function not respected. (#GH107629)
+- Correctly handle allocations in the condition of a ``if constexpr``.(#GH120197) (#GH134820)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 3fe26f950ad51..b281b1cfef96a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7723,12 +7723,12 @@ class Sema final : public SemaBase {
class ConditionResult {
Decl *ConditionVar;
- FullExprArg Condition;
+ ExprResult Condition;
bool Invalid;
std::optional<bool> KnownValue;
friend class Sema;
- ConditionResult(Sema &S, Decl *ConditionVar, FullExprArg Condition,
+ ConditionResult(Sema &S, Decl *ConditionVar, ExprResult Condition,
bool IsConstexpr)
: ConditionVar(ConditionVar), Condition(Condition), Invalid(false) {
if (IsConstexpr && Condition.get()) {
@@ -7739,7 +7739,7 @@ class Sema final : public SemaBase {
}
}
explicit ConditionResult(bool Invalid)
- : ConditionVar(nullptr), Condition(nullptr), Invalid(Invalid),
+ : ConditionVar(nullptr), Condition(Invalid), Invalid(Invalid),
KnownValue(std::nullopt) {}
public:
diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp
index 1ea0cf52933f6..9f36c3ade5e74 100644
--- a/clang/lib/Parse/ParseExprCXX.cpp
+++ b/clang/lib/Parse/ParseExprCXX.cpp
@@ -1931,15 +1931,13 @@ Parser::ParseCXXCondition(StmtResult *InitStmt, SourceLocation Loc,
return ParseCXXCondition(nullptr, Loc, CK, MissingOK);
}
- ExprResult Expr = [&] {
- EnterExpressionEvaluationContext Eval(
- Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
- /*LambdaContextDecl=*/nullptr,
- /*ExprContext=*/Sema::ExpressionEvaluationContextRecord::EK_Other,
- /*ShouldEnter=*/CK == Sema::ConditionKind::ConstexprIf);
- // Parse the expression.
- return ParseExpression(); // expression
- }();
+ EnterExpressionEvaluationContext Eval(
+ Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+ /*LambdaContextDecl=*/nullptr,
+ /*ExprContext=*/Sema::ExpressionEvaluationContextRecord::EK_Other,
+ /*ShouldEnter=*/CK == Sema::ConditionKind::ConstexprIf);
+
+ ExprResult Expr = ParseExpression();
if (Expr.isInvalid())
return Sema::ConditionError();
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 437df742d572b..7c65489cc6234 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -20629,6 +20629,9 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
case ConditionKind::ConstexprIf:
Cond = CheckBooleanCondition(Loc, SubExpr, true);
+ assert(isa<FullExpr>(Cond.get()) &&
+ "we should have converted this expression to a FullExpr before "
+ "evaluating it");
break;
case ConditionKind::Switch:
@@ -20641,12 +20644,10 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
if (!Cond.get())
return ConditionError();
}
- // FIXME: FullExprArg doesn't have an invalid bit, so check nullness instead.
- FullExprArg FullExpr = MakeFullExpr(Cond.get(), Loc);
- if (!FullExpr.get())
- return ConditionError();
+ if (!isa<FullExpr>(Cond.get()))
+ Cond = ActOnFinishFullExpr(Cond.get(), Loc, /*DiscardedValue*/ false);
- return ConditionResult(*this, nullptr, FullExpr,
+ return ConditionResult(*this, nullptr, Cond,
CK == ConditionKind::ConstexprIf);
}
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 4a86cbd0633b6..f17a338825423 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -4357,7 +4357,8 @@ Sema::ConditionResult Sema::ActOnConditionVariable(Decl *ConditionVar,
CheckConditionVariable(cast<VarDecl>(ConditionVar), StmtLoc, CK);
if (E.isInvalid())
return ConditionError();
- return ConditionResult(*this, ConditionVar, MakeFullExpr(E.get(), StmtLoc),
+ E = ActOnFinishFullExpr(E.get(), /*DiscardedValue*/ false);
+ return ConditionResult(*this, ConditionVar, E,
CK == ConditionKind::ConstexprIf);
}
@@ -4417,6 +4418,12 @@ ExprResult Sema::CheckCXXBooleanCondition(Expr *CondExpr, bool IsConstexpr) {
if (!IsConstexpr || E.isInvalid() || E.get()->isValueDependent())
return E;
+ E = ActOnFinishFullExpr(E.get(), E.get()->getExprLoc(),
+ /*DiscardedValue*/ false,
+ /*IsConstexpr*/ true);
+ if (E.isInvalid())
+ return E;
+
// FIXME: Return this value to the caller so they don't need to recompute it.
llvm::APSInt Cond;
E = VerifyIntegerConstantExpression(
diff --git a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
index ed8cbbbfe7067..edffe177e08c7 100644
--- a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
+++ b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
@@ -242,3 +242,36 @@ void f() {
}
}
+
+namespace GH134820 {
+struct S {
+ char* c = new char;
+ constexpr ~S() {
+ delete c;
+ }
+};
+
+int f() {
+ if constexpr((S{}, true)) {
+ return 1;
+ }
+ return 0;
+}
+}
+
+namespace GH120197{
+struct NonTrivialDtor {
+ NonTrivialDtor() = default;
+ NonTrivialDtor(const NonTrivialDtor&) = default;
+ NonTrivialDtor(NonTrivialDtor&&) = default;
+ NonTrivialDtor& operator=(const NonTrivialDtor&) = default;
+ NonTrivialDtor& operator=(NonTrivialDtor&&) = default;
+ constexpr ~NonTrivialDtor() noexcept {}
+};
+
+static_assert(((void)NonTrivialDtor{}, true)); // passes
+
+void f() {
+ if constexpr ((void)NonTrivialDtor{}, true) {}
+}
+}
|
erichkeane
reviewed
Jul 3, 2025
clang/lib/Sema/SemaExpr.cpp
Outdated
| @@ -20629,6 +20629,9 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc, | |||
|
|
|||
| case ConditionKind::ConstexprIf: | |||
| Cond = CheckBooleanCondition(Loc, SubExpr, true); | |||
| assert(isa<FullExpr>(Cond.get()) && | |||
There was a problem hiding this comment.
Cond could still be invalid here, right? so .get would be ill-formed? Should we be doing some sort of if (Cond.isUsable()) or something?
There was a problem hiding this comment.
Actually the assert was incorrect, I removed it
erichkeane
reviewed
Jul 3, 2025
clang/lib/Sema/SemaExpr.cpp
Outdated
| Cond = ActOnFinishFullExpr(Cond.get(), Loc, /*DiscardedValue*/ false); | ||
|
|
||
| if (Cond.isInvalid()) |
There was a problem hiding this comment.
Suggested change
| if (Cond.isInvalid()) | |
| if (!Cond.isUsable()) |
Unless there is a reason that ConditionResult would be ok with a 'null' expr?
erichkeane
approved these changes
Jul 3, 2025
| Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated, | ||
| /*LambdaContextDecl=*/nullptr, | ||
| /*ExprContext=*/Sema::ExpressionEvaluationContextRecord::EK_Other, | ||
| /*ShouldEnter=*/CK == Sema::ConditionKind::ConstexprIf); |
There was a problem hiding this comment.
Do you need to make an equivalent change to TreeTransform?
I came up with the following testcase:
struct S {
char* c = new char;
consteval S(){}
constexpr ~S() {
delete c;
}
};
template<typename T>
int f() {
if constexpr((T{}, true)) {
return 1;
}
return 0;
}
auto ff = f<S>;
There was a problem hiding this comment.
Indeed, thanks for thinking about that
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/206/builds/2781 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Deal with the following scenario
There were two issues
Fixes #120197
Fixes #134820