[analyzer] Handle [[assume(cond)]] as __builtin_assume(cond) #116462
Merged
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
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
Can you take a look at this PR when you have a chance? |
vinay-deshmukh
force-pushed
the
vinay-issue-100762
branch
2 times, most recently
from
d4ba202 to
336eb74
Compare
vinay-deshmukh
force-pushed
the
vinay-issue-100762
branch
from
336eb74 to
daddb9e
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Vinay Deshmukh (vinay-deshmukh) ChangesResolves #100762 Gist of the change:
Full diff: https://github.com/llvm/llvm-project/pull/116462.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index 8c7493e27fcaa6..078a1d840d0516 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -498,6 +498,10 @@ class ExprEngine {
void VisitInitListExpr(const InitListExpr *E, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
+ /// VisitAttributedStmt - Transfer function logic for AttributedStmt
+ void VisitAttributedStmt(const AttributedStmt *A, ExplodedNode *Pred,
+ ExplodedNodeSet &Dst);
+
/// VisitLogicalExpr - Transfer function logic for '&&', '||'
void VisitLogicalExpr(const BinaryOperator* B, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index f678ac6f2ff36a..fab10f51cf5cfc 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -456,6 +456,36 @@ reverse_children::reverse_children(Stmt *S) {
IE->getNumInits());
return;
}
+ case Stmt::AttributedStmtClass: {
+ AttributedStmt *attrStmt = cast<AttributedStmt>(S);
+ assert(attrStmt);
+
+ {
+ // for an attributed stmt, the "children()" returns only the NullStmt
+ // (;) but semantically the "children" are supposed to be the
+ // expressions _within_ i.e. the two square brackets i.e. [[ HERE ]]
+ // so we add the subexpressions first, _then_ add the "children"
+
+ for (const Attr *attr : attrStmt->getAttrs()) {
+
+ // i.e. one `assume()`
+ CXXAssumeAttr const *assumeAttr = llvm::dyn_cast<CXXAssumeAttr>(attr);
+ if (!assumeAttr) {
+ continue;
+ }
+ // Only handles [[ assume(<assumption>) ]] right now
+ Expr *assumption = assumeAttr->getAssumption();
+ childrenBuf.push_back(assumption);
+ }
+
+ // children() for an AttributedStmt is NullStmt(;)
+ llvm::append_range(childrenBuf, attrStmt->children());
+
+ // This needs to be done *after* childrenBuf has been populated.
+ children = childrenBuf;
+ }
+ return;
+ }
default:
break;
}
@@ -2475,6 +2505,14 @@ static bool isFallthroughStatement(const AttributedStmt *A) {
return isFallthrough;
}
+static bool isCXXAssumeAttr(const AttributedStmt *A) {
+ bool hasAssumeAttr = hasSpecificAttr<CXXAssumeAttr>(A->getAttrs());
+
+ assert((!hasAssumeAttr || isa<NullStmt>(A->getSubStmt())) &&
+ "expected [[assume]] not to have children");
+ return hasAssumeAttr;
+}
+
CFGBlock *CFGBuilder::VisitAttributedStmt(AttributedStmt *A,
AddStmtChoice asc) {
// AttributedStmts for [[likely]] can have arbitrary statements as children,
@@ -2490,6 +2528,11 @@ CFGBlock *CFGBuilder::VisitAttributedStmt(AttributedStmt *A,
appendStmt(Block, A);
}
+ if (isCXXAssumeAttr(A) && asc.alwaysAdd(*this, A)) {
+ autoCreateBlock();
+ appendStmt(Block, A);
+ }
+
return VisitChildren(A);
}
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 22eab9f66418d4..cbc83f1dbda145 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1946,7 +1946,6 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
// to be explicitly evaluated.
case Stmt::PredefinedExprClass:
case Stmt::AddrLabelExprClass:
- case Stmt::AttributedStmtClass:
case Stmt::IntegerLiteralClass:
case Stmt::FixedPointLiteralClass:
case Stmt::CharacterLiteralClass:
@@ -1977,6 +1976,13 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
break;
}
+ case Stmt::AttributedStmtClass: {
+ Bldr.takeNodes(Pred);
+ VisitAttributedStmt(cast<AttributedStmt>(S), Pred, Dst);
+ Bldr.addNodes(Dst);
+ break;
+ }
+
case Stmt::CXXDefaultArgExprClass:
case Stmt::CXXDefaultInitExprClass: {
Bldr.takeNodes(Pred);
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index f7020da2e6da20..1a211d1adc44f7 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -1200,3 +1200,30 @@ void ExprEngine::VisitLambdaExpr(const LambdaExpr *LE, ExplodedNode *Pred,
// FIXME: Move all post/pre visits to ::Visit().
getCheckerManager().runCheckersForPostStmt(Dst, Tmp, LE, *this);
}
+
+void ExprEngine::VisitAttributedStmt(const AttributedStmt *A,
+ ExplodedNode *Pred, ExplodedNodeSet &Dst) {
+
+ ExplodedNodeSet CheckerPreStmt;
+ getCheckerManager().runCheckersForPreStmt(CheckerPreStmt, Pred, A, *this);
+
+ ExplodedNodeSet EvalSet;
+ StmtNodeBuilder Bldr(CheckerPreStmt, EvalSet, *currBldrCtx);
+
+ {
+ for (const auto *attr : A->getAttrs()) {
+
+ CXXAssumeAttr const *assumeAttr = llvm::dyn_cast<CXXAssumeAttr>(attr);
+ if (!assumeAttr) {
+ continue;
+ }
+ Expr *AssumeExpr = assumeAttr->getAssumption();
+
+ for (auto *node : CheckerPreStmt) {
+ Visit(AssumeExpr, node, EvalSet);
+ }
+ }
+ }
+
+ getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, A, *this);
+}
diff --git a/clang/test/Analysis/out-of-bounds-new.cpp b/clang/test/Analysis/out-of-bounds-new.cpp
index f541bdf810d79c..4db351b10055b1 100644
--- a/clang/test/Analysis/out-of-bounds-new.cpp
+++ b/clang/test/Analysis/out-of-bounds-new.cpp
@@ -180,3 +180,19 @@ int test_reference_that_might_be_after_the_end(int idx) {
return ref;
}
+// From: https://github.com/llvm/llvm-project/issues/100762
+extern int arr[10];
+void using_builtin(int x) {
+ __builtin_assume(x > 101); // CallExpr
+ arr[x] = 404; // expected-warning{{Out of bound access to memory}}
+}
+
+void using_assume_attr(int ax) {
+ [[assume(ax > 100)]]; // NullStmt with an attribute
+ arr[ax] = 405; // expected-warning{{Out of bound access to memory}}
+}
+
+void using_many_assume_attr(int yx) {
+ [[assume(yx > 104), assume(yx > 200), assume(yx < 300)]]; // NullStmt with an attribute
+ arr[yx] = 406; // expected-warning{{Out of bound access to memory}}
+}
|
|
@llvm/pr-subscribers-clang-analysis Author: Vinay Deshmukh (vinay-deshmukh) ChangesResolves #100762 Gist of the change:
Full diff: https://github.com/llvm/llvm-project/pull/116462.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index 8c7493e27fcaa6..078a1d840d0516 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -498,6 +498,10 @@ class ExprEngine {
void VisitInitListExpr(const InitListExpr *E, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
+ /// VisitAttributedStmt - Transfer function logic for AttributedStmt
+ void VisitAttributedStmt(const AttributedStmt *A, ExplodedNode *Pred,
+ ExplodedNodeSet &Dst);
+
/// VisitLogicalExpr - Transfer function logic for '&&', '||'
void VisitLogicalExpr(const BinaryOperator* B, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index f678ac6f2ff36a..fab10f51cf5cfc 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -456,6 +456,36 @@ reverse_children::reverse_children(Stmt *S) {
IE->getNumInits());
return;
}
+ case Stmt::AttributedStmtClass: {
+ AttributedStmt *attrStmt = cast<AttributedStmt>(S);
+ assert(attrStmt);
+
+ {
+ // for an attributed stmt, the "children()" returns only the NullStmt
+ // (;) but semantically the "children" are supposed to be the
+ // expressions _within_ i.e. the two square brackets i.e. [[ HERE ]]
+ // so we add the subexpressions first, _then_ add the "children"
+
+ for (const Attr *attr : attrStmt->getAttrs()) {
+
+ // i.e. one `assume()`
+ CXXAssumeAttr const *assumeAttr = llvm::dyn_cast<CXXAssumeAttr>(attr);
+ if (!assumeAttr) {
+ continue;
+ }
+ // Only handles [[ assume(<assumption>) ]] right now
+ Expr *assumption = assumeAttr->getAssumption();
+ childrenBuf.push_back(assumption);
+ }
+
+ // children() for an AttributedStmt is NullStmt(;)
+ llvm::append_range(childrenBuf, attrStmt->children());
+
+ // This needs to be done *after* childrenBuf has been populated.
+ children = childrenBuf;
+ }
+ return;
+ }
default:
break;
}
@@ -2475,6 +2505,14 @@ static bool isFallthroughStatement(const AttributedStmt *A) {
return isFallthrough;
}
+static bool isCXXAssumeAttr(const AttributedStmt *A) {
+ bool hasAssumeAttr = hasSpecificAttr<CXXAssumeAttr>(A->getAttrs());
+
+ assert((!hasAssumeAttr || isa<NullStmt>(A->getSubStmt())) &&
+ "expected [[assume]] not to have children");
+ return hasAssumeAttr;
+}
+
CFGBlock *CFGBuilder::VisitAttributedStmt(AttributedStmt *A,
AddStmtChoice asc) {
// AttributedStmts for [[likely]] can have arbitrary statements as children,
@@ -2490,6 +2528,11 @@ CFGBlock *CFGBuilder::VisitAttributedStmt(AttributedStmt *A,
appendStmt(Block, A);
}
+ if (isCXXAssumeAttr(A) && asc.alwaysAdd(*this, A)) {
+ autoCreateBlock();
+ appendStmt(Block, A);
+ }
+
return VisitChildren(A);
}
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 22eab9f66418d4..cbc83f1dbda145 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1946,7 +1946,6 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
// to be explicitly evaluated.
case Stmt::PredefinedExprClass:
case Stmt::AddrLabelExprClass:
- case Stmt::AttributedStmtClass:
case Stmt::IntegerLiteralClass:
case Stmt::FixedPointLiteralClass:
case Stmt::CharacterLiteralClass:
@@ -1977,6 +1976,13 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
break;
}
+ case Stmt::AttributedStmtClass: {
+ Bldr.takeNodes(Pred);
+ VisitAttributedStmt(cast<AttributedStmt>(S), Pred, Dst);
+ Bldr.addNodes(Dst);
+ break;
+ }
+
case Stmt::CXXDefaultArgExprClass:
case Stmt::CXXDefaultInitExprClass: {
Bldr.takeNodes(Pred);
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index f7020da2e6da20..1a211d1adc44f7 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -1200,3 +1200,30 @@ void ExprEngine::VisitLambdaExpr(const LambdaExpr *LE, ExplodedNode *Pred,
// FIXME: Move all post/pre visits to ::Visit().
getCheckerManager().runCheckersForPostStmt(Dst, Tmp, LE, *this);
}
+
+void ExprEngine::VisitAttributedStmt(const AttributedStmt *A,
+ ExplodedNode *Pred, ExplodedNodeSet &Dst) {
+
+ ExplodedNodeSet CheckerPreStmt;
+ getCheckerManager().runCheckersForPreStmt(CheckerPreStmt, Pred, A, *this);
+
+ ExplodedNodeSet EvalSet;
+ StmtNodeBuilder Bldr(CheckerPreStmt, EvalSet, *currBldrCtx);
+
+ {
+ for (const auto *attr : A->getAttrs()) {
+
+ CXXAssumeAttr const *assumeAttr = llvm::dyn_cast<CXXAssumeAttr>(attr);
+ if (!assumeAttr) {
+ continue;
+ }
+ Expr *AssumeExpr = assumeAttr->getAssumption();
+
+ for (auto *node : CheckerPreStmt) {
+ Visit(AssumeExpr, node, EvalSet);
+ }
+ }
+ }
+
+ getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, A, *this);
+}
diff --git a/clang/test/Analysis/out-of-bounds-new.cpp b/clang/test/Analysis/out-of-bounds-new.cpp
index f541bdf810d79c..4db351b10055b1 100644
--- a/clang/test/Analysis/out-of-bounds-new.cpp
+++ b/clang/test/Analysis/out-of-bounds-new.cpp
@@ -180,3 +180,19 @@ int test_reference_that_might_be_after_the_end(int idx) {
return ref;
}
+// From: https://github.com/llvm/llvm-project/issues/100762
+extern int arr[10];
+void using_builtin(int x) {
+ __builtin_assume(x > 101); // CallExpr
+ arr[x] = 404; // expected-warning{{Out of bound access to memory}}
+}
+
+void using_assume_attr(int ax) {
+ [[assume(ax > 100)]]; // NullStmt with an attribute
+ arr[ax] = 405; // expected-warning{{Out of bound access to memory}}
+}
+
+void using_many_assume_attr(int yx) {
+ [[assume(yx > 104), assume(yx > 200), assume(yx < 300)]]; // NullStmt with an attribute
+ arr[yx] = 406; // expected-warning{{Out of bound access to memory}}
+}
|
|
This looks really good. I'll come back and push some changes to your branch to make it more similar with our coding style. Thank you for the PR. |
I'll upstream this commit separately to make getSpecificAttr work with const attributes.
llvmbot
added
clang:frontend
|
I simplified your implementation, and also identified a shortcoming of the The implementation looks pretty but there is a catch. And this is unfortunately a blocker issue. int divide_by_32(int x) {
[[assume(x >= 0)]];
return x/32; // The instructions produced for the division
// may omit handling of negative values.
}
int f(int y) {
[[assume(++y == 43)]]; // y is not incremented
return y; // statement may be replaced with return 42;
}Because we eval the subexpressions of the assume arguments, their sideeffects sneaks in to the ExplodedGraph. We should have a look at what the codegen does with this attribute, or even with the builtin assume call - because we should do something very similar. We should gather the constraints and apply them, just like codegen does (I assume). Basically, my idea now is more like: don't eval the expressions of the assume in the CFG, but only visit the AttributedStmt in the ExprEngine. When we would reach that, "do the same as codegen to get the constraints" and apply these constraints. Alternatively, we could take a snapshot of the Store and Environment before we would evaluate any expressions of the assume attribute and "restore" those after we finished with the AttributedStmt. If we would do this, we would be careful of sinking/"merging" all the paths we ended up creating while evaluating this Stmt. This is a lot more complicated, so I'd definitely try the previous approach before even considering this. |
vinay-deshmukh
commented
Nov 19, 2024
CodeGen seems to be doing a NOOP operation when encountering an attribute with side-effects in I've checked the Furthermore, it appears to me that Once the other comments regarding Comparison of GCC vs clang for compiler for pre-C++23 ASSUME macro and `[[assume()]]`Noting the current "real" behavior of This godbolt link shows the output of with the following scenarios, where the assumption argument is an expression with side-effects (
// From: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1774r8.pdf
#if defined(__clang__)
#define ASSUME(expr) __builtin_assume(expr)
#elif defined(__GNUC__) && !defined(__ICC)
#define ASSUME(expr) if (expr) {} else { __builtin_unreachable(); }
#endifa. U.B. (as per standard) i.e.
from Note:
Note: from b. Defined Behavior i.e.
a. U.B. (as per standard) i.e.
b. Defined Behavior i.e. Clang: |
vinay-deshmukh
force-pushed
the
vinay-issue-100762
branch
from
a1a85f0 to
c288bdc
Compare
|
I wanted to come back with another example: int ternary_in_assume(int a, int b) {
[[assume(a > 10 ? b == 4 : b == 10)]];
if (a > 20)
return b + 100; // expecting 104
if (a > 10)
return b + 200; // expecting 204
return b + 300; // expecting 310
}Actually, neither gcc, clang, msvc, or icc optimizes the return paths to constants - even though I think they would be allowed to. https://compiler-explorer.com/z/1ajvcvG9h I'd need to think about of the implications for us though. I think it should be safe to eval these subexpressions and do state splits if and only if none of those expressions have sideeffects. In that case, they wouldn't be emitted into the CFG, so they shouldn't be a problem. |
steakhal
removed
clang:frontend
steakhal
reviewed
Dec 18, 2024
There was a problem hiding this comment.
As far as I know, to eliminate the warning emitted corresponding to the FIXME comment, we need to implement a new Checker for attributes; we can do this in a second PR to avoid scope creep for this issue/PR.
Agreed. I had a look at the exploded graph and look correct to me.
I think I'll come back to this PR, apply some really minor recommendations and I think we can land it.
Thank you so much for pushing for this so hard. I really appreciate it.
Expect me back on this in a couple of days.
|
@vinay-deshmukh WDYT of the current shape? Can we merge it? |
|
Looks good to me @steakhal ! Thanks for the review! |
|
@vinay-deshmukh Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
I made the followup for this one in #120572 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/4556 Here is the relevant piece of the build log for the reference |
Please revert and reland with followup. Followup is good way to go for trivial cases which can be landed with quick or no review |
thurstond
added a commit
that referenced
this pull request
Dec 19, 2024
…116462)" This reverts commit 89da344. Reason: buildbot breakages e.g., https://lab.llvm.org/buildbot/#/builders/55/builds/4556 (for which the reverted patch is the only code change)
|
I've reverted it in 2b9abf0 to clear up some buildbots |
Thanks! Ill have a look later. |
|
Thanks @thurstond for the revert! Let me know if there's anything I can do here to help fix the issue! |
|
So we had the following assertion failure: On I wish I could come back to this to see why that assert fails. I don't have the bandwidth. Maybe at some point in January. |
steakhal
added a commit
to steakhal/llvm-project
that referenced
this pull request
Feb 28, 2025
…llvm#116462)" This reverts commit 2b9abf0.
steakhal
added a commit
that referenced
this pull request
Mar 6, 2025
#129234) This is the second attempt to bring initial support for [[assume()]] in the Clang Static Analyzer. The first attempt (#116462) was reverted in 2b9abf0 due to some weird failure in a libcxx test involving `#pragma clang loop vectorize(enable) interleave(enable)`. The failure could be reduced into: ```c++ template <class ExecutionPolicy> void transform(ExecutionPolicy) { #pragma clang loop vectorize(enable) interleave(enable) for (int i = 0; 0;) { // The DeclStmt of "i" would be added twice in the ThreadSafety analysis. // empty } } void entrypoint() { transform(1); } ``` As it turns out, the problem with the initial patch was this: ```c++ for (const auto *Attr : AS->getAttrs()) { if (const auto *AssumeAttr = dyn_cast<CXXAssumeAttr>(Attr)) { Expr *AssumeExpr = AssumeAttr->getAssumption(); if (!AssumeExpr->HasSideEffects(Ctx)) { childrenBuf.push_back(AssumeExpr); } } // Visit the actual children AST nodes. // For CXXAssumeAttrs, this is always a NullStmt. llvm::append_range(childrenBuf, AS->children()); // <--- This was not meant to be part of the "for" loop. children = childrenBuf; } return; ``` The solution was simple. Just hoist it from the loop. I also had a closer look at `CFGBuilder::VisitAttributedStmt`, where I also spotted another bug. We would have added the CFG blocks twice if the AttributedStmt would have both the `[[fallthrough]]` and the `[[assume()]]` attributes. With my fix, it will only once add the blocks. Added a regression test for this. Co-authored-by: Vinay Deshmukh <vinay_deshmukh AT outlook DOT com>
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
llvm#129234) This is the second attempt to bring initial support for [[assume()]] in the Clang Static Analyzer. The first attempt (llvm#116462) was reverted in 2b9abf0 due to some weird failure in a libcxx test involving `#pragma clang loop vectorize(enable) interleave(enable)`. The failure could be reduced into: ```c++ template <class ExecutionPolicy> void transform(ExecutionPolicy) { #pragma clang loop vectorize(enable) interleave(enable) for (int i = 0; 0;) { // The DeclStmt of "i" would be added twice in the ThreadSafety analysis. // empty } } void entrypoint() { transform(1); } ``` As it turns out, the problem with the initial patch was this: ```c++ for (const auto *Attr : AS->getAttrs()) { if (const auto *AssumeAttr = dyn_cast<CXXAssumeAttr>(Attr)) { Expr *AssumeExpr = AssumeAttr->getAssumption(); if (!AssumeExpr->HasSideEffects(Ctx)) { childrenBuf.push_back(AssumeExpr); } } // Visit the actual children AST nodes. // For CXXAssumeAttrs, this is always a NullStmt. llvm::append_range(childrenBuf, AS->children()); // <--- This was not meant to be part of the "for" loop. children = childrenBuf; } return; ``` The solution was simple. Just hoist it from the loop. I also had a closer look at `CFGBuilder::VisitAttributedStmt`, where I also spotted another bug. We would have added the CFG blocks twice if the AttributedStmt would have both the `[[fallthrough]]` and the `[[assume()]]` attributes. With my fix, it will only once add the blocks. Added a regression test for this. Co-authored-by: Vinay Deshmukh <vinay_deshmukh AT outlook DOT com>
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.
Resolves #100762
Gist of the change:
assume()by parsing those expressions, all of the code "just works" by evaluating the SVals, and hence leaning on the same logic that makes the code with__builtin_assumeworkcontinue-ing if theProgramPointis aStmtPoint