[clang] Reapply "[analyzer] Handle [[assume(cond)]] as __builtin_assume(cond)" (PR #129234)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 5 08:42:38 PST 2025
https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/129234
>From 67d6420eb64949b9639385a8a2f647db0d05d078 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Sat, 1 Feb 2025 10:03:27 +0100
Subject: [PATCH 1/6] Reapply "[analyzer] Handle [[assume(cond)]] as
__builtin_assume(cond) (#116462)"
This reverts commit 2b9abf0db2d106c7208b4372e662ef5df869e6f1.
---
clang/include/clang/AST/AttrIterator.h | 12 ++++
.../Core/PathSensitive/ExprEngine.h | 4 ++
clang/lib/Analysis/CFG.cpp | 72 +++++++++++++------
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 ++-
clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp | 7 +-
.../lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 18 +++++
.../test/Analysis/cxx23-assume-attribute.cpp | 58 +++++++++++++++
clang/test/Analysis/out-of-bounds-new.cpp | 60 +++++++++++++++-
8 files changed, 214 insertions(+), 25 deletions(-)
create mode 100644 clang/test/Analysis/cxx23-assume-attribute.cpp
diff --git a/clang/include/clang/AST/AttrIterator.h b/clang/include/clang/AST/AttrIterator.h
index 7e2bb0381d4c8..2f39c144dc160 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 9fd07ce47175c..804fc74b009df 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 3e144395cffc6..13e86be496db8 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,28 +443,47 @@ 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;
- }
- default:
- break;
+
+ // 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: {
+ auto *AS = cast<AttributedStmt>(S);
- // Default case for all other statements.
- llvm::append_range(childrenBuf, S->children());
+ // 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"
- // This needs to be done *after* childrenBuf has been populated.
- children = childrenBuf;
+ 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());
+ children = childrenBuf;
+ }
+ return;
+ }
+ default:
+ // Default case for all other statements.
+ llvm::append_range(childrenBuf, S->children());
+ children = childrenBuf;
+ }
}
namespace {
@@ -2433,7 +2452,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))
@@ -2449,7 +2468,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;
@@ -2484,6 +2503,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,
@@ -2499,6 +2526,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 318fa3c1caf06..8036f03e289d4 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1950,7 +1950,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:
@@ -1981,6 +1980,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 1061dafbb2473..50176f8fdbc3d 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -796,9 +796,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
@@ -806,7 +807,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 f7020da2e6da2..5f9dbcb55e811 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 0000000000000..defcdeec282f6
--- /dev/null
+++ b/clang/test/Analysis/cxx23-assume-attribute.cpp
@@ -0,0 +1,58 @@
+// 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 at -1 {{[-2147483648, 10]}}
+ // expected-warning at -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 at -1 {{[-2147483648, 10]}}
+ // expected-warning at -2 {{[11, 2147483647]}}
+
+ clang_analyzer_dump(b); // expected-warning {{4}} expected-warning {{10}}
+ // expected-warning-re at -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 at -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 at -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 at -1 {{(reg_${{[0-9]+}}<int b>) + 300}} FIXME: We shouldn't have this dump.
+ return 0;
+}
diff --git a/clang/test/Analysis/out-of-bounds-new.cpp b/clang/test/Analysis/out-of-bounds-new.cpp
index 4e5442422bff4..9843a8c3a650e 100644
--- a/clang/test/Analysis/out-of-bounds-new.cpp
+++ b/clang/test/Analysis/out-of-bounds-new.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-checker=unix,core,security.ArrayBound -verify %s
+// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -verify %s \
+// RUN: -analyzer-checker=unix,core,security.ArrayBound,debug.ExprInspection
+
+void clang_analyzer_eval(bool);
// Tests doing an out-of-bounds access after the end of an array using:
// - constant integer index
@@ -180,3 +183,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;
+}
>From 2e7c4e7c1baf7a87b08a0bef3e2a313f7c03b959 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Sat, 1 Feb 2025 16:01:10 +0100
Subject: [PATCH 2/6] Fix reverse_children of AttributedStmt
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.
---
clang/lib/Analysis/CFG.cpp | 32 +++++++++----------
.../test/Analysis/cxx23-assume-attribute.cpp | 19 +++++++++++
2 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 13e86be496db8..3f1e0c2b233a6 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -457,14 +457,13 @@ reverse_children::reverse_children(Stmt *S, ASTContext &Ctx) {
IE->getNumInits());
return;
}
- case Stmt::AttributedStmtClass: {
- auto *AS = cast<AttributedStmt>(S);
+ 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();
@@ -472,18 +471,23 @@ reverse_children::reverse_children(Stmt *S, ASTContext &Ctx) {
childrenBuf.push_back(AssumeExpr);
}
}
- // Visit the actual children AST nodes.
- // For CXXAssumeAttrs, this is always a NullStmt.
- llvm::append_range(childrenBuf, AS->children());
- children = childrenBuf;
}
+
+ // Visit the actual children AST nodes.
+ // For CXXAssumeAttrs, this is always a NullStmt.
+ llvm::append_range(childrenBuf, AS->children());
+ children = childrenBuf;
return;
}
default:
- // Default case for all other statements.
- llvm::append_range(childrenBuf, S->children());
- children = childrenBuf;
+ break;
}
+
+ // Default case for all other statements.
+ llvm::append_range(childrenBuf, S->children());
+
+ // This needs to be done *after* childrenBuf has been populated.
+ children = childrenBuf;
}
namespace {
@@ -2521,12 +2525,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)) {
- autoCreateBlock();
- appendStmt(Block, A);
- }
-
- if (isCXXAssumeAttr(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/test/Analysis/cxx23-assume-attribute.cpp b/clang/test/Analysis/cxx23-assume-attribute.cpp
index defcdeec282f6..ee049af9f13aa 100644
--- a/clang/test/Analysis/cxx23-assume-attribute.cpp
+++ b/clang/test/Analysis/cxx23-assume-attribute.cpp
@@ -56,3 +56,22 @@ int ternary_in_assume(int a, int b) {
// expected-warning-re at -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 at -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 at -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;
+}
>From bed651193fc4cb655fc4afc06711a72460ecfe46 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Tue, 4 Mar 2025 20:11:19 +0100
Subject: [PATCH 3/6] Fix review comments
---
clang/lib/Analysis/CFG.cpp | 2 +-
clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 3f1e0c2b233a6..74dd50a345934 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -459,7 +459,7 @@ reverse_children::reverse_children(Stmt *S, ASTContext &Ctx) {
}
case Stmt::AttributedStmtClass: {
- // for an attributed stmt, the "children()" returns only the NullStmt
+ // 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"
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
index 50176f8fdbc3d..3d0a69a515ab8 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -796,7 +796,6 @@ void ExprEngine::VisitGuardedExpr(const Expr *Ex,
// Find the predecessor block.
ProgramStateRef SrcState = state;
-
for (const ExplodedNode *N = Pred ; N ; N = *N->pred_begin()) {
auto Edge = N->getLocationAs<BlockEdge>();
if (!Edge.has_value()) {
>From 54f3aea27d7f53f488d9998fb9561911dde17a9c Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Wed, 5 Mar 2025 12:05:48 +0100
Subject: [PATCH 4/6] Fix dyncast
---
clang/lib/Analysis/CFG.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 74dd50a345934..68034d1a1937c 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -444,12 +444,12 @@ class reverse_children {
} // namespace
reverse_children::reverse_children(Stmt *S, ASTContext &Ctx) {
- switch (S->getStmtClass()) {
- case Stmt::CallExprClass: {
- children = cast<CallExpr>(S)->getRawSubExprs();
+ if (CallExpr *CE = dyn_cast<CallExpr>(S)) {
+ children = CE->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);
>From e1625b8b96a3171ee50a4694e87047b57a4c534f Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Wed, 5 Mar 2025 17:35:00 +0100
Subject: [PATCH 5/6] Fix buildin_assume callexprs with sideeffects, pin and
clean tests
---
clang/lib/Analysis/CFG.cpp | 13 +++++++-
clang/test/Analysis/out-of-bounds-new.cpp | 39 ++++++++++-------------
2 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 68034d1a1937c..9af1e915482da 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -2732,6 +2732,16 @@ static bool CanThrow(Expr *E, ASTContext &Ctx) {
return true;
}
+static bool isBuiltinAssumeWithSideEffects(const ASTContext &Ctx,
+ const CallExpr *CE) {
+ unsigned BuiltinID = CE->getBuiltinCallee();
+ if (BuiltinID != Builtin::BI__assume &&
+ BuiltinID != Builtin::BI__builtin_assume)
+ return false;
+
+ return CE->getArg(0)->HasSideEffects(Ctx);
+}
+
CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) {
// Compute the callee type.
QualType calleeType = C->getCallee()->getType();
@@ -2770,7 +2780,8 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) {
NoReturn = true;
if (FD->hasAttr<NoThrowAttr>())
AddEHEdge = false;
- if (FD->getBuiltinID() == Builtin::BI__builtin_object_size ||
+ if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) ||
+ FD->getBuiltinID() == Builtin::BI__builtin_object_size ||
FD->getBuiltinID() == Builtin::BI__builtin_dynamic_object_size)
OmitArguments = true;
}
diff --git a/clang/test/Analysis/out-of-bounds-new.cpp b/clang/test/Analysis/out-of-bounds-new.cpp
index 9843a8c3a650e..cb838dafd9509 100644
--- a/clang/test/Analysis/out-of-bounds-new.cpp
+++ b/clang/test/Analysis/out-of-bounds-new.cpp
@@ -1,7 +1,9 @@
// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -verify %s \
+// RUN: -triple=x86_64-unknown-linux-gnu \
// RUN: -analyzer-checker=unix,core,security.ArrayBound,debug.ExprInspection
void clang_analyzer_eval(bool);
+void clang_analyzer_value(int);
// Tests doing an out-of-bounds access after the end of an array using:
// - constant integer index
@@ -200,41 +202,34 @@ void using_many_assume_attr(int yx) {
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) {
+ int orig_y = y;
+ clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
// 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.
+ clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_eval(y == orig_y); // expected-warning {{TRUE}} Good.
return y;
}
+int using_builtin_assume_has_no_sideeffects(int y) {
+ int orig_y = y;
+ clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
-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}}
+ __builtin_assume(++y == 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
+ clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_eval(y == orig_y); // expected-warning {{TRUE}} Good.
- return u;
+ return y;
}
>From d3790caf2d2aec0fe916fd12649de76631d55f62 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Wed, 5 Mar 2025 17:42:22 +0100
Subject: [PATCH 6/6] Further refine testing
---
clang/test/Analysis/out-of-bounds-new.cpp | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/clang/test/Analysis/out-of-bounds-new.cpp b/clang/test/Analysis/out-of-bounds-new.cpp
index cb838dafd9509..9ae371819714d 100644
--- a/clang/test/Analysis/out-of-bounds-new.cpp
+++ b/clang/test/Analysis/out-of-bounds-new.cpp
@@ -4,6 +4,7 @@
void clang_analyzer_eval(bool);
void clang_analyzer_value(int);
+void clang_analyzer_dump(int);
// Tests doing an out-of-bounds access after the end of an array using:
// - constant integer index
@@ -206,11 +207,15 @@ int using_assume_attr_has_no_sideeffects(int y) {
int orig_y = y;
clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_dump(y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
+ clang_analyzer_dump(orig_y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
// 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_dump(y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
+ clang_analyzer_dump(orig_y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
clang_analyzer_eval(y == orig_y); // expected-warning {{TRUE}} Good.
@@ -222,11 +227,15 @@ int using_builtin_assume_has_no_sideeffects(int y) {
int orig_y = y;
clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
+ clang_analyzer_dump(y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
+ clang_analyzer_dump(orig_y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
// We should not apply sideeffects of the argument of __builtin_assume(...)
// "u" should not get incremented;
__builtin_assume(++y == 43); // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+ clang_analyzer_dump(y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
+ clang_analyzer_dump(orig_y); // expected-warning-re {{{{^}}reg_${{[0-9]+}}<int y> [debug.ExprInspection]{{$}}}}
clang_analyzer_value(y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
clang_analyzer_value(orig_y); // expected-warning {{32s:{ [-2147483648, 2147483647] }}}
clang_analyzer_eval(y == orig_y); // expected-warning {{TRUE}} Good.
More information about the cfe-commits
mailing list