Reapply "[analyzer] Handle [[assume(cond)]] as __builtin_assume(cond)" #125348
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
The `append_range` was accidentally executed for each CXXAssumeAttr.
This caused the the ThreadSafety analysis to misbehave on code like
this:
```c++
template <class ExecutionPolicy>
void transform(ExecutionPolicy) {
#pragma clang loop vectorize(enable) interleave(enable)
for (int __i = 0; 0;) {
// empty
}
}
void entrypoint() {
transform(1);
}
```
In the `transform()` somehow the ThreadSafety analysis would have a
malformed CFG for the function, breaking invariants inside their
algorithm, causing an assertion to fire and break some libcxx build bot.
I also noticed that `CFGBuilder::VisitAttributedStmt` would "build" the
blocks for an AttributedStmt iff that holds both the Fallthrough and the
assume attributes. So I fixed that too.
llvmbot
added
clang
|
@Xazax-hun Please have a thorough review of this one. |
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balazs Benics (steakhal) ChangesThis is the second attempt to bring initial support for [[assume()]] in the Clang Static Analyzer. The failure could be reduced into: 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: 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 Full diff: https://github.com/llvm/llvm-project/pull/125348.diff 8 Files Affected:
diff --git a/clang/include/clang/AST/AttrIterator.h b/clang/include/clang/AST/AttrIterator.h
index 7e2bb0381d4c8f0..2f39c144dc1608a 100644
--- a/clang/include/clang/AST/AttrIterator.h
+++ b/clang/include/clang/AST/AttrIterator.h
@@ -16,6 +16,7 @@
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/ADL.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/iterator_range.h"
#include "llvm/Support/Casting.h"
#include <cassert>
#include <cstddef>
@@ -124,6 +125,17 @@ inline auto *getSpecificAttr(const Container &container) {
return It != specific_attr_end<IterTy>(container) ? *It : nullptr;
}
+template <typename SpecificAttr, typename Container>
+inline auto getSpecificAttrs(const Container &container) {
+ using ValueTy = llvm::detail::ValueOfRange<Container>;
+ using ValuePointeeTy = std::remove_pointer_t<ValueTy>;
+ using IterTy = std::conditional_t<std::is_const_v<ValuePointeeTy>,
+ const SpecificAttr, SpecificAttr>;
+ auto Begin = specific_attr_begin<IterTy>(container);
+ auto End = specific_attr_end<IterTy>(container);
+ return llvm::make_range(Begin, End);
+}
+
} // namespace clang
#endif // LLVM_CLANG_AST_ATTRITERATOR_H
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index 20c446e33ef9a5a..df26f9abe22a188 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 304bbb2b422c61d..5b119f08e91dd8b 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -433,7 +433,7 @@ class reverse_children {
ArrayRef<Stmt *> children;
public:
- reverse_children(Stmt *S);
+ reverse_children(Stmt *S, ASTContext &Ctx);
using iterator = ArrayRef<Stmt *>::reverse_iterator;
@@ -443,21 +443,44 @@ class reverse_children {
} // namespace
-reverse_children::reverse_children(Stmt *S) {
- if (CallExpr *CE = dyn_cast<CallExpr>(S)) {
- children = CE->getRawSubExprs();
+reverse_children::reverse_children(Stmt *S, ASTContext &Ctx) {
+ switch (S->getStmtClass()) {
+ case Stmt::CallExprClass: {
+ children = cast<CallExpr>(S)->getRawSubExprs();
return;
}
- switch (S->getStmtClass()) {
- // Note: Fill in this switch with more cases we want to optimize.
- case Stmt::InitListExprClass: {
- InitListExpr *IE = cast<InitListExpr>(S);
- children = llvm::ArrayRef(reinterpret_cast<Stmt **>(IE->getInits()),
- IE->getNumInits());
- return;
+
+ // Note: Fill in this switch with more cases we want to optimize.
+ case Stmt::InitListExprClass: {
+ InitListExpr *IE = cast<InitListExpr>(S);
+ children = llvm::ArrayRef(reinterpret_cast<Stmt **>(IE->getInits()),
+ IE->getNumInits());
+ return;
+ }
+
+ case Stmt::AttributedStmtClass: {
+ // 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"
+ auto *AS = cast<AttributedStmt>(S);
+ 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);
+ }
+ }
}
- default:
- break;
+
+ // Visit the actual children AST nodes.
+ // For CXXAssumeAttrs, this is always a NullStmt.
+ llvm::append_range(childrenBuf, AS->children());
+ children = childrenBuf;
+ return;
+ }
+ default:
+ break;
}
// Default case for all other statements.
@@ -2431,7 +2454,7 @@ CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
// Visit the children in their reverse order so that they appear in
// left-to-right (natural) order in the CFG.
- reverse_children RChildren(S);
+ reverse_children RChildren(S, *Context);
for (Stmt *Child : RChildren) {
if (Child)
if (CFGBlock *R = Visit(Child))
@@ -2447,7 +2470,7 @@ CFGBlock *CFGBuilder::VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc) {
}
CFGBlock *B = Block;
- reverse_children RChildren(ILE);
+ reverse_children RChildren(ILE, *Context);
for (Stmt *Child : RChildren) {
if (!Child)
continue;
@@ -2482,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,
@@ -2492,7 +2523,8 @@ CFGBlock *CFGBuilder::VisitAttributedStmt(AttributedStmt *A,
// So only add the AttributedStmt for FallThrough, which has CFG effects and
// also no children, and omit the others. None of the other current StmtAttrs
// have semantic meaning for the CFG.
- if (isFallthroughStatement(A) && asc.alwaysAdd(*this, A)) {
+ bool isInterestingAttribute = isFallthroughStatement(A) || isCXXAssumeAttr(A);
+ if (isInterestingAttribute && asc.alwaysAdd(*this, A)) {
autoCreateBlock();
appendStmt(Block, A);
}
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 2b1872f8386aad1..fd416b8eb1e57ff 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1947,7 +1947,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:
@@ -1978,6 +1977,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/ExprEngineC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
index 7a900780384a91d..1315bd10496f5c7 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -794,9 +794,10 @@ void ExprEngine::VisitGuardedExpr(const Expr *Ex,
// Find the predecessor block.
ProgramStateRef SrcState = state;
+
for (const ExplodedNode *N = Pred ; N ; N = *N->pred_begin()) {
- ProgramPoint PP = N->getLocation();
- if (PP.getAs<PreStmtPurgeDeadSymbols>() || PP.getAs<BlockEntrance>()) {
+ auto Edge = N->getLocationAs<BlockEdge>();
+ if (!Edge.has_value()) {
// If the state N has multiple predecessors P, it means that successors
// of P are all equivalent.
// In turn, that means that all nodes at P are equivalent in terms
@@ -804,7 +805,7 @@ void ExprEngine::VisitGuardedExpr(const Expr *Ex,
// FIXME: a more robust solution which does not walk up the tree.
continue;
}
- SrcBlock = PP.castAs<BlockEdge>().getSrc();
+ SrcBlock = Edge->getSrc();
SrcState = N->getState();
break;
}
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index f7020da2e6da200..5f9dbcb55e811ca 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/AST/AttrIterator.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/ParentMap.h"
#include "clang/AST/StmtCXX.h"
@@ -1200,3 +1201,20 @@ 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 : getSpecificAttrs<CXXAssumeAttr>(A->getAttrs())) {
+ for (ExplodedNode *N : CheckerPreStmt) {
+ Visit(Attr->getAssumption(), N, EvalSet);
+ }
+ }
+
+ getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, A, *this);
+}
diff --git a/clang/test/Analysis/cxx23-assume-attribute.cpp b/clang/test/Analysis/cxx23-assume-attribute.cpp
new file mode 100644
index 000000000000000..ee049af9f13aae3
--- /dev/null
+++ b/clang/test/Analysis/cxx23-assume-attribute.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_analyze_cc1 -std=c++23 -triple x86_64-pc-linux-gnu \
+// RUN: -analyzer-checker=core,debug.ExprInspection -verify %s
+
+template <typename T> void clang_analyzer_dump(T);
+template <typename T> void clang_analyzer_value(T);
+
+int ternary_in_builtin_assume(int a, int b) {
+ __builtin_assume(a > 10 ? b == 4 : b == 10);
+
+ clang_analyzer_value(a);
+ // expected-warning@-1 {{[-2147483648, 10]}}
+ // expected-warning@-2 {{[11, 2147483647]}}
+
+ clang_analyzer_dump(b); // expected-warning{{4}} expected-warning{{10}}
+
+ if (a > 20) {
+ clang_analyzer_dump(b + 100); // expected-warning {{104}}
+ return 2;
+ }
+ if (a > 10) {
+ clang_analyzer_dump(b + 200); // expected-warning {{204}}
+ return 1;
+ }
+ clang_analyzer_dump(b + 300); // expected-warning {{310}}
+ return 0;
+}
+
+// From: https://github.com/llvm/llvm-project/pull/116462#issuecomment-2517853226
+int ternary_in_assume(int a, int b) {
+ // FIXME notes
+ // Currently, if this test is run without the core.builtin.Builtin checker, the above function with the __builtin_assume behaves identically to the following test
+ // i.e. calls to `clang_analyzer_dump` result in "extraneous" prints of the SVal(s) `reg<int b> ...`
+ // as opposed to 4 or 10
+ // which likely implies the Program State(s) did not get narrowed.
+ // A new checker is likely needed to be implemented to properly handle the expressions within `[[assume]]` to eliminate the states where `b` is not narrowed.
+
+ [[assume(a > 10 ? b == 4 : b == 10)]];
+ clang_analyzer_value(a);
+ // expected-warning@-1 {{[-2147483648, 10]}}
+ // expected-warning@-2 {{[11, 2147483647]}}
+
+ clang_analyzer_dump(b); // expected-warning {{4}} expected-warning {{10}}
+ // expected-warning-re@-1 {{reg_${{[0-9]+}}<int b>}} FIXME: We shouldn't have this dump.
+
+ if (a > 20) {
+ clang_analyzer_dump(b + 100); // expected-warning {{104}}
+ // expected-warning-re@-1 {{(reg_${{[0-9]+}}<int b>) + 100}} FIXME: We shouldn't have this dump.
+ return 2;
+ }
+ if (a > 10) {
+ clang_analyzer_dump(b + 200); // expected-warning {{204}}
+ // expected-warning-re@-1 {{(reg_${{[0-9]+}}<int b>) + 200}} FIXME: We shouldn't have this dump.
+ return 1;
+ }
+ clang_analyzer_dump(b + 300); // expected-warning {{310}}
+ // expected-warning-re@-1 {{(reg_${{[0-9]+}}<int b>) + 300}} FIXME: We shouldn't have this dump.
+ return 0;
+}
+
+int assume_and_fallthrough_at_the_same_attrstmt(int a, int b) {
+ [[assume(a == 2)]];
+ clang_analyzer_dump(a); // expected-warning {{2 S32b}}
+ // expected-warning-re@-1 {{reg_${{[0-9]+}}<int a>}} FIXME: We shouldn't have this dump.
+ switch (a) {
+ case 2:
+ [[fallthrough, assume(b == 30)]];
+ case 4: {
+ clang_analyzer_dump(b); // expected-warning {{30 S32b}}
+ // expected-warning-re@-1 {{reg_${{[0-9]+}}<int b>}} FIXME: We shouldn't have this dump.
+ return b;
+ }
+ }
+ // This code should be unreachable.
+ [[assume(false)]]; // This should definitely make it so.
+ clang_analyzer_dump(33); // expected-warning {{33 S32b}}
+ return 0;
+}
diff --git a/clang/test/Analysis/out-of-bounds-new.cpp b/clang/test/Analysis/out-of-bounds-new.cpp
index f541bdf810d79ce..39a40eb10bea7d5 100644
--- a/clang/test/Analysis/out-of-bounds-new.cpp
+++ b/clang/test/Analysis/out-of-bounds-new.cpp
@@ -1,4 +1,11 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-checker=unix,core,alpha.security.ArrayBoundV2 -verify %s
+// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -verify %s \
+// RUN: -analyzer-checker=unix,core,alpha.security.ArrayBoundV2,debug.ExprInspection
+
+template <typename T> void clang_analyzer_dump(T);
+template <typename T> void clang_analyzer_value(T);
+void clang_analyzer_eval(bool);
+template <typename T>
+void clang_analyzer_explain(T);
// Tests doing an out-of-bounds access after the end of an array using:
// - constant integer index
@@ -180,3 +187,58 @@ int test_reference_that_might_be_after_the_end(int idx) {
return ref;
}
+// From: https://github.com/llvm/llvm-project/issues/100762
+extern int arrOf10[10];
+void using_builtin(int x) {
+ __builtin_assume(x > 101); // CallExpr
+ arrOf10[x] = 404; // expected-warning {{Out of bound access to memory}}
+}
+
+void using_assume_attr(int ax) {
+ [[assume(ax > 100)]]; // NullStmt with an "assume" attribute.
+ arrOf10[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
+ arrOf10[yx] = 406; // expected-warning{{Out of bound access to memory}}
+}
+
+
+int using_builtin_assume_has_no_sideeffects(int y) {
+ // We should not apply sideeffects of the argument of [[assume(...)]].
+ // "y" should not get incremented;
+ __builtin_assume(++y == 43); // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+ clang_analyzer_eval(y == 42); // expected-warning {{FALSE}}
+ return y;
+}
+
+
+
+int using_assume_attr_has_no_sideeffects(int y) {
+
+ // We should not apply sideeffects of the argument of [[assume(...)]].
+ // "y" should not get incremented;
+ [[assume(++y == 43)]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+
+ clang_analyzer_eval(y == 42); // expected-warning {{TRUE}} expected-warning {{FALSE}} FIXME: This should be only TRUE.
+
+ clang_analyzer_eval(y == 43); // expected-warning {{FALSE}} expected-warning {{TRUE}} FIXME: This should be only FALSE.
+
+ return y;
+}
+
+
+int using_builtinassume_has_no_sideeffects(int u) {
+ // We should not apply sideeffects of the argument of __builtin_assume(...)
+ // "u" should not get incremented;
+ __builtin_assume(++u == 43); // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+
+ // FIXME: evaluate this to true
+ clang_analyzer_eval(u == 42); // expected-warning {{FALSE}} current behavior
+
+ // FIXME: evaluate this to false
+ clang_analyzer_eval(u == 43); // expected-warning {{TRUE}} current behavior
+
+ return u;
+}
|
|
@vinay-deshmukh What email should I use for attribution in the "Co-authored-by"? |
|
You can use:
Thank you for looking into the issue again! |
|
Ping @Xazax-hun for a review. |
|
Reopening this PR as #129234 as I accidentally closed this permanently. |
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.
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:
As it turns out, the problem with the initial patch was this:
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>