llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: kmehltretter82
<details>
<summary>Changes</summary>
The analyzer ends a path at a block whose terminator is an asm goto
("TODO: Handle jumping to labels" in CoreEngine::HandleBlockExit()).
Linux kernel static keys compile to an asm goto with CONFIG_JUMP_LABEL.
Every tracepoint has one, and with allocation profiling every kmalloc()
has one too, so kernel paths end at the first of them.
An asm goto is a block terminator and never reaches VisitGCCAsmStmt().
Invalidate its operands the same way, through a shared helper, and
continue at every successor.
The three FIXMEs in egraph-asm-goto-no-crash.cpp become expected
warnings. asm-goto-paths.c is new.
370 files of an x86_64 allmodconfig kernel (drivers/leds, sound/isa,
fs/ext4, net/ipv4), core checkers:
before after
analysis time, summed over files 858 s 973 s
wall clock, 4 jobs 236 s 265 s
reports 54 77
step limit reached 320 396
25 reports in 10 files are new, all in code that was not reached before.
24 are existing false positives, one is a harmless uninitialized read
(fs/ext4/extents.c:3158). None comes from the asm goto handling itself.
2 reports are gone. One function is now inlined into its caller and no
longer analyzed alone, and one path no longer fits the node budget.
Assisted-by: LLM
Fixes #<!-- -->229302
---
Full diff: https://github.com/llvm/llvm-project/pull/229316.diff
5 Files Affected:
- (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
(+15)
- (modified) clang/lib/StaticAnalyzer/Core/CoreEngine.cpp (+8-3)
- (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+49-15)
- (added) clang/test/Analysis/asm-goto-paths.c (+110)
- (modified) clang/test/Analysis/egraph-asm-goto-no-crash.cpp (+3-8)
``````````diff
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index af813714e6831..ce98616549c56 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -387,6 +387,12 @@ class ExprEngine {
void processIndirectGoto(ExplodedNodeSet &Dst, const Expr *Tgt,
const CFGBlock *Dispatch, ExplodedNode *Pred);
+ /// processAsmGoto - Called by CoreEngine. Used to generate successor
+ /// nodes for an asm goto, which may fall through or jump to any of its
+ /// labels. The operands are invalidated as for a plain asm statement.
+ void processAsmGoto(const GCCAsmStmt *A, const CFGBlock *B,
+ ExplodedNode *Pred, ExplodedNodeSet &Dst);
+
/// ProcessSwitch - Called by CoreEngine. Used to generate successor
/// nodes by processing the 'effects' of a switch statement.
void processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred,
@@ -489,6 +495,15 @@ class ExprEngine {
void VisitGCCAsmStmt(const GCCAsmStmt *A, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
+ /// Invalidate the regions that the outputs and inputs of an inline asm
+ /// statement refer to. Nothing is known about what the assembly does with
+ /// them.
+ ProgramStateRef invalidateAsmOperands(const GCCAsmStmt *A,
+ ProgramStateRef State,
+ ConstCFGElementRef Elem,
+ unsigned BlockCount,
+ const StackFrame *SF);
+
/// VisitMSAsmStmt - Transfer function logic for MS inline asm.
void VisitMSAsmStmt(const MSAsmStmt *A, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
index 04b700726fbc6..9ad83b76f1554 100644
--- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -460,10 +460,15 @@ void CoreEngine::HandleBlockExit(const CFGBlock * B,
ExplodedNode *Pred) {
HandleBranch(cast<WhileStmt>(Term)->getCond(), Term, B, Pred);
return;
- case Stmt::GCCAsmStmtClass:
- assert(cast<GCCAsmStmt>(Term)->isAsmGoto() && "Encountered GCCAsmStmt
without labels");
- // TODO: Handle jumping to labels
+ case Stmt::GCCAsmStmtClass: {
+ // An asm goto may fall through or jump to any of its labels.
+ assert(cast<GCCAsmStmt>(Term)->isAsmGoto() &&
+ "Encountered GCCAsmStmt without labels");
+ ExplodedNodeSet Dst;
+ ExprEng.processAsmGoto(cast<GCCAsmStmt>(Term), B, Pred, Dst);
+ enqueue(Dst);
return;
+ }
}
}
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 059bc4770da91..6d010291e8406 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -66,6 +66,7 @@
#include "llvm/ADT/ImmutableMap.h"
#include "llvm/ADT/ImmutableSet.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/Compiler.h"
@@ -3926,40 +3927,73 @@ bool
ExprEngine::didEagerlyAssumeBifurcateAt(ProgramStateRef State,
return Ex && State->get<LastEagerlyAssumeExprIfSuccessful>() == Ex;
}
-void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A, ExplodedNode *Pred,
- ExplodedNodeSet &Dst) {
- // We have processed both the inputs and the outputs. All of the outputs
- // should evaluate to Locs. Nuke all of their values.
+ProgramStateRef ExprEngine::invalidateAsmOperands(const GCCAsmStmt *A,
+ ProgramStateRef State,
+ ConstCFGElementRef Elem,
+ unsigned BlockCount,
+ const StackFrame *SF) {
+ // All of the outputs should evaluate to Locs. Nuke all of their values.
// FIXME: Some day in the future it would be nice to allow a "plug-in"
// which interprets the inline asm and stores proper results in the
// outputs.
- ProgramStateRef state = Pred->getState();
-
for (const Expr *O : A->outputs()) {
- SVal X = state->getSVal(O, Pred->getStackFrame());
+ SVal X = State->getSVal(O, SF);
assert(!isa<NonLoc>(X)); // Should be an Lval, or unknown, undef.
if (std::optional<Loc> LV = X.getAs<Loc>())
- state = state->invalidateRegions(*LV, getCFGElementRef(),
- getNumVisitedCurrent(),
- Pred->getStackFrame(),
+ State = State->invalidateRegions(*LV, Elem, BlockCount, SF,
/*CausedByPointerEscape=*/true);
}
// Do not reason about locations passed inside inline assembly.
for (const Expr *I : A->inputs()) {
- SVal X = state->getSVal(I, Pred->getStackFrame());
+ SVal X = State->getSVal(I, SF);
if (std::optional<Loc> LV = X.getAs<Loc>())
- state = state->invalidateRegions(*LV, getCFGElementRef(),
- getNumVisitedCurrent(),
- Pred->getStackFrame(),
+ State = State->invalidateRegions(*LV, Elem, BlockCount, SF,
/*CausedByPointerEscape=*/true);
}
- Dst.insert(Engine.makePostStmtNode(A, state, Pred));
+ return State;
+}
+
+void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A, ExplodedNode *Pred,
+ ExplodedNodeSet &Dst) {
+ // We have processed both the inputs and the outputs.
+ ProgramStateRef State =
+ invalidateAsmOperands(A, Pred->getState(), getCFGElementRef(),
+ getNumVisitedCurrent(), Pred->getStackFrame());
+
+ Dst.insert(Engine.makePostStmtNode(A, State, Pred));
+}
+
+void ExprEngine::processAsmGoto(const GCCAsmStmt *A, const CFGBlock *B,
+ ExplodedNode *Pred, ExplodedNodeSet &Dst) {
+ ProgramStateRef State = Pred->getState();
+ const StackFrame *SF = Pred->getStackFrame();
+
+ // An asm goto is the terminator of its block, not one of its elements, so
+ // its operands do not pass through VisitGCCAsmStmt. Invalidate them here.
+ // A terminator has no CFG element of its own, so the new values are
+ // conjured at the last element of its block. Parts of an operand, such as
+ // the arms of a conditional, may have been evaluated in earlier blocks.
+ // Without operands the block can be empty, and nothing is invalidated.
+ if (!B->empty())
+ State =
+ invalidateAsmOperands(A, State, ConstCFGElementRef(B, B->size() - 1),
+ getNumVisited(SF, B), SF);
+
+ // Nothing is known about what the assembly does, so it may fall through or
+ // jump to any of its labels. Follow every successor once.
+ llvm::SmallPtrSet<const CFGBlock *, 4> Followed;
+ for (const CFGBlock::AdjacentBlock &Succ : B->succs()) {
+ const CFGBlock *Next = Succ.getReachableBlock();
+ if (!Next || !Followed.insert(Next).second)
+ continue;
+ Dst.insert(Engine.makeNode(BlockEdge(B, Next, SF), State, Pred));
+ }
}
void ExprEngine::VisitMSAsmStmt(const MSAsmStmt *A, ExplodedNode *Pred,
diff --git a/clang/test/Analysis/asm-goto-paths.c
b/clang/test/Analysis/asm-goto-paths.c
new file mode 100644
index 0000000000000..e863d5f115724
--- /dev/null
+++ b/clang/test/Analysis/asm-goto-paths.c
@@ -0,0 +1,110 @@
+// RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu \
+// RUN: -analyzer-checker=core,debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false -verify %s
+
+void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached(void);
+
+// The shape of a Linux kernel static branch.
+static inline int static_branch(const int *key) {
+ asm goto("jmp %l[l_yes]" : : "i"(key) : : l_yes);
+ return 0;
+l_yes:
+ return 1;
+}
+
+int key;
+
+void both_ways(void) {
+ if (static_branch(&key))
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+ else
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+int null_after_asm_goto(int *p) {
+ if (static_branch(&key))
+ p = 0;
+ return *p; // expected-warning {{Dereference of null pointer (loaded from
variable 'p')}}
+}
+
+void outputs_are_forgotten(void) {
+ int x = 1;
+
+ asm goto("" : "=r"(x) : : : out);
+ clang_analyzer_eval(x == 1); // expected-warning {{UNKNOWN}}
+ return;
+out:
+ clang_analyzer_eval(x == 1); // expected-warning {{UNKNOWN}}
+}
+
+struct S { int a; };
+
+void struct_outputs_are_forgotten(void) {
+ struct S s = {1};
+
+ asm goto("" : "=m"(s) : : : out);
+ clang_analyzer_eval(s.a == 1); // expected-warning {{UNKNOWN}}
+ return;
+out:
+ clang_analyzer_eval(s.a == 1); // expected-warning {{UNKNOWN}}
+}
+
+// Memory that an input points to may have been written, as for a plain asm.
+void input_pointees_are_forgotten(int *p) {
+ *p = 1;
+
+ asm goto("" : : "r"(p) : "memory" : out);
+ clang_analyzer_eval(*p == 1); // expected-warning {{UNKNOWN}}
+ return;
+out:
+ clang_analyzer_eval(*p == 1); // expected-warning {{UNKNOWN}}
+}
+
+// The arms of a conditional operand are evaluated in earlier blocks. Only
+// the memory behind the arm that was taken is forgotten.
+void conditional_operand(int c, int *p, int *q) {
+ *p = 1;
+ *q = 1;
+
+ asm goto("" : : "r"(c ? p : q) : "memory" : out);
+ clang_analyzer_eval(*p == 1); // expected-warning {{TRUE}}
+ // expected-warning@-1 {{UNKNOWN}}
+ clang_analyzer_eval(*q == 1); // expected-warning {{TRUE}}
+ // expected-warning@-1 {{UNKNOWN}}
+ return;
+out:
+ clang_analyzer_eval(*p == 1); // expected-warning {{TRUE}}
+ // expected-warning@-1 {{UNKNOWN}}
+ clang_analyzer_eval(*q == 1); // expected-warning {{TRUE}}
+ // expected-warning@-1 {{UNKNOWN}}
+}
+
+// A label before the asm goto makes a loop. The checks only run on the
+// second time round: the backward edge is followed, the output gets a new
+// value each time, and the path goes on behind the loop.
+void backward_jump(void) {
+ int x = 0;
+ int prev;
+ int count = 0;
+
+again:
+ prev = x;
+ count++;
+ asm goto("" : "=r"(x) : : : again);
+ if (count == 2) {
+ clang_analyzer_eval(prev == x); // expected-warning {{UNKNOWN}}
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+ }
+}
+
+// No operands, so the block has no elements and there is nothing to
+// invalidate. Both ways are still reachable.
+void no_operands(void) {
+ asm goto("jmp %l[out]" : : : : out);
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+ return;
+out:
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
diff --git a/clang/test/Analysis/egraph-asm-goto-no-crash.cpp
b/clang/test/Analysis/egraph-asm-goto-no-crash.cpp
index 37f8fc533abe3..a5b5729c7c83f 100644
--- a/clang/test/Analysis/egraph-asm-goto-no-crash.cpp
+++ b/clang/test/Analysis/egraph-asm-goto-no-crash.cpp
@@ -1,7 +1,5 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify
%s
-// expected-no-diagnostics
-
void clang_analyzer_warnIfReached();
void testAsmGoto() {
@@ -11,16 +9,13 @@ void testAsmGoto() {
: /* clobbers */
: label1, label2 /* any labels used */);
- // FIXME: Should be reachable.
- clang_analyzer_warnIfReached();
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
label1:
- // FIXME: Should be reachable.
- clang_analyzer_warnIfReached();
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
return;
label2:
- // FIXME: Should be reachable.
- clang_analyzer_warnIfReached();
+ clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
return;
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/229316
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
