-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[coverage] fix crash in code coverage and if constexpr
with ExprWithCleanups
#80292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Hana Dusíková (hanickadot) ChangesFixes #80285 Full diff: https://github.com/llvm/llvm-project/pull/80292.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 8b5e6c4ad8272..6640fe6f41fd1 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1808,12 +1808,24 @@ struct CounterCoverageMappingBuilder
}
}
+private:
+ static bool evaluateConstantCondition(const Expr *Condition) {
+ if (const auto *Expr = dyn_cast<ConstantExpr>(Condition))
+ return Expr->getResultAsAPSInt().getExtValue();
+
+ if (const auto *Expr = dyn_cast<ExprWithCleanups>(Condition))
+ return evaluateConstantCondition(Expr->getSubExpr()); // recursion
+
+ assert(false && "Unexpected node in 'if constexpr' condition");
+ return false;
+ }
+
+public:
void coverIfConstexpr(const IfStmt *S) {
assert(S->isConstexpr());
// evaluate constant condition...
- const auto *E = cast<ConstantExpr>(S->getCond());
- const bool isTrue = E->getResultAsAPSInt().getExtValue();
+ const bool isTrue = evaluateConstantCondition(S->getCond());
extendRegion(S);
diff --git a/clang/test/CoverageMapping/if.cpp b/clang/test/CoverageMapping/if.cpp
index 3045ffe43948c..4de1467aa7ee3 100644
--- a/clang/test/CoverageMapping/if.cpp
+++ b/clang/test/CoverageMapping/if.cpp
@@ -234,6 +234,35 @@ constexpr int check_macro_consteval_if_skipped(int i) { // CHECK-NEXT: [[@LINE
return i;
}
+struct false_value {
+ constexpr operator bool() {
+ return false;
+ }
+};
+
+template <typename> struct dependable_false_value {
+ constexpr operator bool() {
+ return false;
+ }
+};
+
+// GH-80285
+void should_not_crash() {
+ if constexpr (false_value{}) { };
+}
+
+template <typename> void should_not_crash_dependable() {
+ if constexpr (dependable_false_value<int>{}) { };
+}
+
+void should_not_crash_with_template_instance() {
+ should_not_crash_dependable<int>();
+}
+
+void should_not_crash_with_requires_expr() {
+ if constexpr (requires {42;}) { };
+}
+
int instantiate_consteval(int i) {
i *= check_consteval_with_else_discarded_then(i);
i *= check_notconsteval_with_else_discarded_else(i);
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst
so users know about the fix.
Thank you for the fix!
Even if it's a fix for recent PR I did few days ago? Also, how do I get this into release branch (the previous change is there)? |
FYI this is a regression in 18 affecting users so we should backport |
Oh, I had missed that, sorry! Then no, a release note isn't needed. |
Btw, instructions on how to do that can be found here: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches |
…thCleanups` (llvm#80292) Fixes llvm#80285 (cherry picked from commit bfc6eaa)
…thCleanups` (llvm#80292) Fixes llvm#80285 (cherry picked from commit bfc6eaa)
…thCleanups` (llvm#80292) Fixes llvm#80285
…thCleanups` (llvm#80292) Fixes llvm#80285 (cherry picked from commit bfc6eaa)
…thCleanups` (llvm#80292) Fixes llvm#80285 (cherry picked from commit bfc6eaa)
…thCleanups` (llvm#80292) Fixes llvm#80285 (cherry picked from commit bfc6eaa)
…thCleanups` (llvm#80292) Fixes llvm#80285 (cherry picked from commit bfc6eaa)
Fixes #80285