Skip to content
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.

Sign up for GitHub

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

[analyzer] Avoid out-of-order node traversal on void return #117863

Merged
merged 5 commits into from
Nov 27, 2024
Merged

[analyzer] Avoid out-of-order node traversal on void return #117863

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c725ed7
[analyzer] Avoid out-of-order node traversal on void return
necto Nov 25, 2024
eb1b47c
Test top-level function effect. Thanks, Balázs for this example
necto Nov 27, 2024
4555ae3
[NFC] Remove curly braces around a single-statement block
necto Nov 27, 2024
674befc
skipPurge unconditionally
necto Nov 27, 2024
525c6ab
[NFC] fix format
necto Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,9 @@ class ExplodedNode : public llvm::FoldingSetNode {
/// Useful for explaining control flow that follows the current node.
/// If the statement belongs to a body-farmed definition, retrieve the
/// call site for that definition.
const Stmt *getNextStmtForDiagnostics() const;
/// If skipPurge is true, skip the purge-dead-symbols nodes (that are often
/// inserted out-of-order by the endinge).
const Stmt *getNextStmtForDiagnostics(bool skipPurge) const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if we always skipped purge-dead-symbol nodes? I wonder if we in general want to tie diagnostics to constructs written by the users instead of artificial constructs that we inserted as implementation details of the analysis.

Copy link
Contributor

@steakhal steakhal Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our policy was so far to minimize the impact to maximize the chance of approval on upstream review.
I think what you say makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot speak generally, but it breaks on more test case: clang/test/Analysis/copy-elision.cpp.
In short for the following code, it moves the report further down the execution:

// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 \
// RUN:    -analyzer-config elide-constructors=false -DNO_ELIDE_FLAG              \
// RUN:    -analyzer-config eagerly-assume=false -verify=expected,no-elide %s

void clang_analyzer_eval(bool);
void clang_analyzer_dump(int);

template <typename T> struct AddressVector {
  T *buf[20];
  int len;

  AddressVector() : len(0) {}

  void push(T *t) {
    buf[len] = t;
    ++len;
  }
};

class ClassWithDestructor {
  AddressVector<ClassWithDestructor> &v;

public:
  ClassWithDestructor(AddressVector<ClassWithDestructor> &v) : v(v) {
    push();
  }

  ClassWithDestructor(ClassWithDestructor &&c) : v(c.v) { push(); }
  ClassWithDestructor(const ClassWithDestructor &c) : v(c.v) { push(); }

  ~ClassWithDestructor() { push(); }

  void push() { v.push(this); }
};

struct TestCtorInitializer {
  ClassWithDestructor c;
  TestCtorInitializer(AddressVector<ClassWithDestructor> &v)
    : c(ClassWithDestructor(v)) {} // currently: <- address of stack var leaks via 'v'
};

void testCtorInitializer() {
  AddressVector<ClassWithDestructor> v;
  {
    TestCtorInitializer t(v); // skipping-purge: <- address of stack var leaks via 'v'
  }
}

I.e., it ends up skipping one more stack frame.
I don't think it is a large difference, although I slightly prefer the current report to the one resulting if we skip purge always. Moreover, I wanted to reduce the test footprint, so I minimized the behavior change.
Do you think it is better simplify the logic and skip purge always?

Copy link
Collaborator

@Xazax-hun Xazax-hun Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it hard to justify why would we ever not skip those nodes, and if there is a fallout we need to deal with it independently. I would prefer us to do the right thing and always skip the purge nodes (unless we discover why is it not a good idea, in that case we can document our findings in the form of comments). Having logic in the code that we do not fully the purpose of is technical debt. So I'd prefer minimizing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you go: 674befc skipPurge unconditionally


/// Find the statement that was executed immediately before this node.
/// Useful when the node corresponds to a CFG block entrance.
Expand Down
40 changes: 28 additions & 12 deletions clang/lib/StaticAnalyzer/Core/BugReporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,8 @@ static void removePiecesWithInvalidLocations(PathPieces &Pieces) {

PathDiagnosticLocation PathDiagnosticBuilder::ExecutionContinues(
const PathDiagnosticConstruct &C) const {
if (const Stmt *S = C.getCurrentNode()->getNextStmtForDiagnostics())
if (const Stmt *S =
C.getCurrentNode()->getNextStmtForDiagnostics(/*skipPurge=*/true))
return PathDiagnosticLocation(S, getSourceManager(),
C.getCurrLocationContext());

Expand Down Expand Up @@ -882,7 +883,8 @@ void PathDiagnosticBuilder::generateMinimalDiagForBlockEdge(

case Stmt::GotoStmtClass:
case Stmt::IndirectGotoStmtClass: {
if (const Stmt *S = C.getCurrentNode()->getNextStmtForDiagnostics())
if (const Stmt *S = C.getCurrentNode()->getNextStmtForDiagnostics(
/*skipPurge=*/false))
C.getActivePath().push_front(generateDiagForGotoOP(C, S, Start));
break;
}
Expand Down Expand Up @@ -2416,6 +2418,28 @@ PathSensitiveBugReport::getRanges() const {
return Ranges;
}

static bool exitingDestructor(const ExplodedNode *N) {
// Need to loop here, as some times the Error node is already outside of the
// destructor context, and the previous node is an edge that is also outside.
while (N && !N->getLocation().getAs<StmtPoint>()) {
N = N->getFirstPred();
}
return N && isa<CXXDestructorDecl>(N->getLocationContext()->getDecl());
}

static const Stmt *
findReasonableStmtCloseToFunctionExit(const ExplodedNode *N) {
if (exitingDestructor(N)) {
// If we are exiting a destructor call, it is more useful to point to
// the next stmt which is usually the temporary declaration.
if (const Stmt *S = N->getNextStmtForDiagnostics(/*skipPurge=*/false))
return S;
// If next stmt is not found, it is likely the end of a top-level
// function analysis. find the last execution statement then.
}
return N->getPreviousStmtForDiagnostics();
}

PathDiagnosticLocation
PathSensitiveBugReport::getLocation() const {
assert(ErrorNode && "Cannot create a location with a null node.");
Expand All @@ -2433,18 +2457,10 @@ PathSensitiveBugReport::getLocation() const {
if (const ReturnStmt *RS = FE->getStmt())
return PathDiagnosticLocation::createBegin(RS, SM, LC);

// If we are exiting a destructor call, it is more useful to point to the
// next stmt which is usually the temporary declaration.
// For non-destructor and non-top-level calls, the next stmt will still
// refer to the last executed stmt of the body.
S = ErrorNode->getNextStmtForDiagnostics();
// If next stmt is not found, it is likely the end of a top-level function
// analysis. find the last execution statement then.
if (!S)
S = ErrorNode->getPreviousStmtForDiagnostics();
S = findReasonableStmtCloseToFunctionExit(ErrorNode);
}
if (!S)
S = ErrorNode->getNextStmtForDiagnostics();
S = ErrorNode->getNextStmtForDiagnostics(/*skipPurge=*/false);
}

if (S) {
Expand Down
5 changes: 4 additions & 1 deletion clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,11 @@ const Stmt *ExplodedNode::getStmtForDiagnostics() const {
return nullptr;
}

const Stmt *ExplodedNode::getNextStmtForDiagnostics() const {
const Stmt *ExplodedNode::getNextStmtForDiagnostics(bool skipPurge) const {
for (const ExplodedNode *N = getFirstSucc(); N; N = N->getFirstSucc()) {
if (skipPurge && N->getLocation().isPurgeKind()) {
Xazax-hun marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
if (const Stmt *S = N->getStmtForDiagnostics()) {
// Check if the statement is '?' or '&&'/'||'. These are "merges",
// not actual statement points.
Expand Down
9 changes: 7 additions & 2 deletions clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,14 +353,19 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
ExplodedNodeSet CleanedNodes;
if (LastSt && Blk && AMgr.options.AnalysisPurgeOpt != PurgeNone) {
static SimpleProgramPointTag retValBind("ExprEngine", "Bind Return Value");
PostStmt Loc(LastSt, calleeCtx, &retValBind);
auto Loc = isa<ReturnStmt>(LastSt)
? ProgramPoint{PostStmt(LastSt, calleeCtx, &retValBind)}
: ProgramPoint{EpsilonPoint(calleeCtx, /*Data1=*/nullptr,
/*Data2=*/nullptr, &retValBind)};
const CFGBlock *PrePurgeBlock =
isa<ReturnStmt>(LastSt) ? Blk : &CEBNode->getCFG().getExit();
bool isNew;
ExplodedNode *BindedRetNode = G.getNode(Loc, state, false, &isNew);
BindedRetNode->addPredecessor(CEBNode, G);
if (!isNew)
return;

NodeBuilderContext Ctx(getCoreEngine(), Blk, BindedRetNode);
NodeBuilderContext Ctx(getCoreEngine(), PrePurgeBlock, BindedRetNode);
currBldrCtx = &Ctx;
// Here, we call the Symbol Reaper with 0 statement and callee location
// context, telling it to clean up everything in the callee's context
Expand Down
8 changes: 4 additions & 4 deletions clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ class UnguardedFieldThroughMethodTest {
Volume = 0;
break;
case A:
Area = 0; // expected-warning {{1 uninitialized field}}
break;
Area = 0;
break; // expected-warning {{1 uninitialized field}}
}
}

Expand Down Expand Up @@ -201,8 +201,8 @@ class UnguardedPublicFieldsTest {
Volume = 0;
break;
case A:
Area = 0; // expected-warning {{1 uninitialized field}}
break;
Area = 0;
break; // expected-warning {{1 uninitialized field}}
}
}

Expand Down
26 changes: 26 additions & 0 deletions clang/test/Analysis/void-call-exit-modelling.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-output text -verify %s

typedef __typeof(sizeof(int)) size_t;
void *malloc(size_t size);

void inf_loop_break_callee() {
void* data = malloc(10); // expected-note{{Memory is allocated}}
while (1) { // expected-note{{Loop condition is true}}
(void)data;
break; // No note that we jump to the line above from this break
} // expected-note@-1{{Execution jumps to the end of the function}}
} // expected-warning{{Potential leak of memory pointed to by 'data'}}
// expected-note@-1 {{Potential leak of memory pointed to by 'data'}}

void inf_loop_break_caller() {
inf_loop_break_callee(); // expected-note{{Calling 'inf_loop_break_callee'}}
}

void inf_loop_break_top() {
void* data = malloc(10); // expected-note{{Memory is allocated}}
while (1) { // expected-note{{Loop condition is true}}
(void)data;
break; // No note that we jump to the line above from this break
} // expected-note@-1{{Execution jumps to the end of the function}}
} // expected-warning{{Potential leak of memory pointed to by 'data'}}
// expected-note@-1 {{Potential leak of memory pointed to by 'data'}}