Revert "[Clang][analyzer] replace Stmt* with ConstCFGElementRef in SymbolConjured" #137304
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
…mbolConj…" This reverts commit ec936b3.
llvmbot
added
clang
steakhal
added
skip-precommit-approval
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balazs Benics (steakhal) ChangesReverts llvm/llvm-project#128251 ASAN bots reported some errors: https://lab.llvm.org/buildbot/#/builders/55/builds/10398 Patch is 131.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/137304.diff 41 Files Affected:
diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h
index e70c70335e597..a7ff38c786a8f 100644
--- a/clang/include/clang/Analysis/CFG.h
+++ b/clang/include/clang/Analysis/CFG.h
@@ -122,8 +122,7 @@ class CFGElement {
return (Kind) x;
}
- void dumpToStream(llvm::raw_ostream &OS,
- bool TerminateWithNewLine = true) const;
+ void dumpToStream(llvm::raw_ostream &OS) const;
void dump() const {
dumpToStream(llvm::errs());
@@ -696,11 +695,6 @@ class CFGBlock {
void dump() const {
dumpToStream(llvm::errs());
}
-
- void Profile(llvm::FoldingSetNodeID &ID) const {
- ID.AddPointer(Parent);
- ID.AddInteger(Index);
- }
};
template <bool IsReverse, bool IsConst> class ElementRefIterator {
@@ -1196,8 +1190,6 @@ class CFGBlock {
}
};
-using ConstCFGElementRef = CFGBlock::ConstCFGElementRef;
-
/// CFGCallback defines methods that should be called when a logical
/// operator error is found when building the CFG.
class CFGCallback {
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h b/clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
index 6c1025ecc7f4d..519d2d5b3676b 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
+++ b/clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
@@ -19,7 +19,6 @@
#include "clang/AST/DeclCXX.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h"
#include "llvm/ADT/StringExtras.h"
-#include "llvm/Support/raw_ostream.h"
namespace clang {
@@ -30,13 +29,6 @@ class SValExplainer : public FullSValVisitor<SValExplainer, std::string> {
ASTContext &ACtx;
ProgramStateRef State;
- std::string printCFGElementRef(ConstCFGElementRef Elem) {
- std::string Str;
- llvm::raw_string_ostream OS(Str);
- Elem->dumpToStream(OS, /*TerminateWithNewLine=*/false);
- return Str;
- }
-
std::string printStmt(const Stmt *S) {
std::string Str;
llvm::raw_string_ostream OS(Str);
@@ -122,8 +114,7 @@ class SValExplainer : public FullSValVisitor<SValExplainer, std::string> {
std::string VisitSymbolConjured(const SymbolConjured *S) {
return "symbol of type '" + S->getType().getAsString() +
- "' conjured at CFG element '" +
- printCFGElementRef(S->getCFGElementRef()) + "'";
+ "' conjured at statement '" + printStmt(S->getStmt()) + "'";
}
std::string VisitSymbolDerived(const SymbolDerived *S) {
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
index 63ca3efc6d228..bb33a6912bec7 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -151,8 +151,6 @@ class CheckerContext {
return Pred->getSVal(S);
}
- ConstCFGElementRef getCFGElementRef() const { return Eng.getCFGElementRef(); }
-
/// Returns true if the value of \p E is greater than or equal to \p
/// Val under unsigned comparison.
bool isGreaterOrEqual(const Expr *E, unsigned long long Val);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index 285194148d3d3..5f855251b3cde 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -226,7 +226,7 @@ class ExprEngine {
return (*G.roots_begin())->getLocation().getLocationContext();
}
- ConstCFGElementRef getCFGElementRef() const {
+ CFGBlock::ConstCFGElementRef getCFGElementRef() const {
const CFGBlock *blockPtr = currBldrCtx ? currBldrCtx->getBlock() : nullptr;
return {blockPtr, currStmtIdx};
}
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
index 50f2197b8a174..e75228f92a8e5 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
@@ -27,8 +27,7 @@ namespace ento {
/// by the loop body in any iteration.
ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
const LocationContext *LCtx,
- unsigned BlockCount,
- ConstCFGElementRef Elem);
+ unsigned BlockCount, const Stmt *LoopStmt);
} // end namespace ento
} // end namespace clang
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
index 52714535e7907..4d66e086a2c2c 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -313,7 +313,7 @@ class ProgramState : public llvm::FoldingSetNode {
/// be triggered by this event.
///
/// \param Regions the set of regions to be invalidated.
- /// \param Elem The CFG Element that caused the invalidation.
+ /// \param E the expression that caused the invalidation.
/// \param BlockCount The number of times the current basic block has been
/// visited.
/// \param CausesPointerEscape the flag is set to true when the invalidation
@@ -325,17 +325,16 @@ class ProgramState : public llvm::FoldingSetNode {
/// \param ITraits information about special handling for particular regions
/// or symbols.
[[nodiscard]] ProgramStateRef
- invalidateRegions(ArrayRef<const MemRegion *> Regions,
- ConstCFGElementRef Elem, unsigned BlockCount,
- const LocationContext *LCtx, bool CausesPointerEscape,
- InvalidatedSymbols *IS = nullptr,
+ invalidateRegions(ArrayRef<const MemRegion *> Regions, const Stmt *S,
+ unsigned BlockCount, const LocationContext *LCtx,
+ bool CausesPointerEscape, InvalidatedSymbols *IS = nullptr,
const CallEvent *Call = nullptr,
RegionAndSymbolInvalidationTraits *ITraits = nullptr) const;
[[nodiscard]] ProgramStateRef
- invalidateRegions(ArrayRef<SVal> Values, ConstCFGElementRef Elem,
- unsigned BlockCount, const LocationContext *LCtx,
- bool CausesPointerEscape, InvalidatedSymbols *IS = nullptr,
+ invalidateRegions(ArrayRef<SVal> Values, const Stmt *S, unsigned BlockCount,
+ const LocationContext *LCtx, bool CausesPointerEscape,
+ InvalidatedSymbols *IS = nullptr,
const CallEvent *Call = nullptr,
RegionAndSymbolInvalidationTraits *ITraits = nullptr) const;
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
index bd5d245645788..54430d426a82a 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -19,7 +19,6 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprObjC.h"
#include "clang/AST/Type.h"
-#include "clang/Analysis/CFG.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/LangOptions.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h"
@@ -172,11 +171,19 @@ class SValBuilder {
// Forwarding methods to SymbolManager.
- const SymbolConjured *conjureSymbol(ConstCFGElementRef Elem,
+ const SymbolConjured* conjureSymbol(const Stmt *stmt,
const LocationContext *LCtx,
- QualType type, unsigned visitCount,
+ QualType type,
+ unsigned visitCount,
const void *symbolTag = nullptr) {
- return SymMgr.conjureSymbol(Elem, LCtx, type, visitCount, symbolTag);
+ return SymMgr.conjureSymbol(stmt, LCtx, type, visitCount, symbolTag);
+ }
+
+ const SymbolConjured* conjureSymbol(const Expr *expr,
+ const LocationContext *LCtx,
+ unsigned visitCount,
+ const void *symbolTag = nullptr) {
+ return SymMgr.conjureSymbol(expr, LCtx, visitCount, symbolTag);
}
/// Construct an SVal representing '0' for the specified type.
@@ -192,19 +199,29 @@ class SValBuilder {
/// preserve the relation between related(or even equivalent) expressions, so
/// conjured symbols should be used sparingly.
DefinedOrUnknownSVal conjureSymbolVal(const void *symbolTag,
- ConstCFGElementRef elem,
+ const Expr *expr,
const LocationContext *LCtx,
unsigned count);
- DefinedOrUnknownSVal conjureSymbolVal(const void *symbolTag,
- ConstCFGElementRef elem,
+ DefinedOrUnknownSVal conjureSymbolVal(const void *symbolTag, const Stmt *S,
const LocationContext *LCtx,
QualType type, unsigned count);
- DefinedOrUnknownSVal conjureSymbolVal(ConstCFGElementRef elem,
+ DefinedOrUnknownSVal conjureSymbolVal(const Stmt *stmt,
const LocationContext *LCtx,
- QualType type, unsigned visitCount);
+ QualType type,
+ unsigned visitCount);
/// Conjure a symbol representing heap allocated memory region.
- DefinedSVal getConjuredHeapSymbolVal(ConstCFGElementRef elem,
+ ///
+ /// Note, the expression should represent a location.
+ DefinedSVal getConjuredHeapSymbolVal(const Expr *E,
+ const LocationContext *LCtx,
+ unsigned Count);
+
+ /// Conjure a symbol representing heap allocated memory region.
+ ///
+ /// Note, now, the expression *doesn't* need to represent a location.
+ /// But the type need to!
+ DefinedSVal getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
QualType type, unsigned Count);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
index 29a53fcc9e28d..cf7623c7be409 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
@@ -14,13 +14,13 @@
#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_STORE_H
#include "clang/AST/Type.h"
-#include "clang/Basic/LLVM.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/StoreRef.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
+#include "clang/Basic/LLVM.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/SmallVector.h"
@@ -223,7 +223,7 @@ class StoreManager {
///
/// \param[in] store The initial store.
/// \param[in] Values The values to invalidate.
- /// \param[in] Elem The current CFG Element being evaluated. Used to conjure
+ /// \param[in] S The current statement being evaluated. Used to conjure
/// symbols to mark the values of invalidated regions.
/// \param[in] Count The current block count. Used to conjure
/// symbols to mark the values of invalidated regions.
@@ -241,8 +241,8 @@ class StoreManager {
/// even if they do not currently have bindings. Pass \c NULL if this
/// information will not be used.
virtual StoreRef invalidateRegions(
- Store store, ArrayRef<SVal> Values, ConstCFGElementRef Elem,
- unsigned Count, const LocationContext *LCtx, const CallEvent *Call,
+ Store store, ArrayRef<SVal> Values, const Stmt *S, unsigned Count,
+ const LocationContext *LCtx, const CallEvent *Call,
InvalidatedSymbols &IS, RegionAndSymbolInvalidationTraits &ITraits,
InvalidatedRegions *TopLevelRegions, InvalidatedRegions *Invalidated) = 0;
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
index 9e7c98fdded17..cbbea1b56bb40 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
@@ -80,62 +80,29 @@ class SymbolRegionValue : public SymbolData {
/// A symbol representing the result of an expression in the case when we do
/// not know anything about what the expression is.
class SymbolConjured : public SymbolData {
- ConstCFGElementRef Elem;
+ const Stmt *S;
QualType T;
unsigned Count;
const LocationContext *LCtx;
const void *SymbolTag;
friend class SymExprAllocator;
- SymbolConjured(SymbolID sym, ConstCFGElementRef elem,
- const LocationContext *lctx, QualType t, unsigned count,
- const void *symbolTag)
- : SymbolData(SymbolConjuredKind, sym), Elem(elem), T(t), Count(count),
+ SymbolConjured(SymbolID sym, const Stmt *s, const LocationContext *lctx,
+ QualType t, unsigned count, const void *symbolTag)
+ : SymbolData(SymbolConjuredKind, sym), S(s), T(t), Count(count),
LCtx(lctx), SymbolTag(symbolTag) {
+ // FIXME: 's' might be a nullptr if we're conducting invalidation
+ // that was caused by a destructor call on a temporary object,
+ // which has no statement associated with it.
+ // Due to this, we might be creating the same invalidation symbol for
+ // two different invalidation passes (for two different temporaries).
assert(lctx);
assert(isValidTypeForSymbol(t));
}
public:
- ConstCFGElementRef getCFGElementRef() const { return Elem; }
-
- // It might return null.
- const Stmt *getStmt() const {
- switch (Elem->getKind()) {
- case CFGElement::Initializer:
- return Elem->castAs<CFGInitializer>().getInitializer()->getInit();
- case CFGElement::ScopeBegin:
- return Elem->castAs<CFGScopeBegin>().getTriggerStmt();
- case CFGElement::ScopeEnd:
- return Elem->castAs<CFGScopeEnd>().getTriggerStmt();
- case CFGElement::NewAllocator:
- return Elem->castAs<CFGNewAllocator>().getAllocatorExpr();
- case CFGElement::LifetimeEnds:
- return Elem->castAs<CFGLifetimeEnds>().getTriggerStmt();
- case CFGElement::LoopExit:
- return Elem->castAs<CFGLoopExit>().getLoopStmt();
- case CFGElement::Statement:
- return Elem->castAs<CFGStmt>().getStmt();
- case CFGElement::Constructor:
- return Elem->castAs<CFGConstructor>().getStmt();
- case CFGElement::CXXRecordTypedCall:
- return Elem->castAs<CFGCXXRecordTypedCall>().getStmt();
- case CFGElement::AutomaticObjectDtor:
- return Elem->castAs<CFGAutomaticObjDtor>().getTriggerStmt();
- case CFGElement::DeleteDtor:
- return Elem->castAs<CFGDeleteDtor>().getDeleteExpr();
- case CFGElement::BaseDtor:
- return nullptr;
- case CFGElement::MemberDtor:
- return nullptr;
- case CFGElement::TemporaryDtor:
- return Elem->castAs<CFGTemporaryDtor>().getBindTemporaryExpr();
- case CFGElement::CleanupFunction:
- return nullptr;
- }
- return nullptr;
- }
-
+ /// It might return null.
+ const Stmt *getStmt() const { return S; }
unsigned getCount() const { return Count; }
/// It might return null.
const void *getTag() const { return SymbolTag; }
@@ -146,11 +113,11 @@ class SymbolConjured : public SymbolData {
void dumpToStream(raw_ostream &os) const override;
- static void Profile(llvm::FoldingSetNodeID &profile, ConstCFGElementRef Elem,
+ static void Profile(llvm::FoldingSetNodeID &profile, const Stmt *S,
const LocationContext *LCtx, QualType T, unsigned Count,
const void *SymbolTag) {
profile.AddInteger((unsigned)SymbolConjuredKind);
- profile.Add(Elem);
+ profile.AddPointer(S);
profile.AddPointer(LCtx);
profile.Add(T);
profile.AddInteger(Count);
@@ -158,7 +125,7 @@ class SymbolConjured : public SymbolData {
}
void Profile(llvm::FoldingSetNodeID& profile) override {
- Profile(profile, Elem, LCtx, T, Count, SymbolTag);
+ Profile(profile, S, LCtx, T, Count, SymbolTag);
}
// Implement isa<T> support.
@@ -566,12 +533,18 @@ class SymbolManager {
template <typename SymExprT, typename... Args>
const SymExprT *acquire(Args &&...args);
- const SymbolConjured *conjureSymbol(ConstCFGElementRef Elem,
+ const SymbolConjured *conjureSymbol(const Stmt *E,
const LocationContext *LCtx, QualType T,
unsigned VisitCount,
const void *SymbolTag = nullptr) {
+ return acquire<SymbolConjured>(E, LCtx, T, VisitCount, SymbolTag);
+ }
- return acquire<SymbolConjured>(Elem, LCtx, T, VisitCount, SymbolTag);
+ const SymbolConjured* conjureSymbol(const Expr *E,
+ const LocationContext *LCtx,
+ unsigned VisitCount,
+ const void *SymbolTag = nullptr) {
+ return conjureSymbol(E, LCtx, E->getType(), VisitCount, SymbolTag);
}
QualType getType(const SymExpr *SE) const {
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 4b50b9248d21a..d03a0a544b016 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -5803,17 +5803,16 @@ static void print_construction_context(raw_ostream &OS,
}
static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
- const CFGElement &E, bool TerminateWithNewLine = true);
+ const CFGElement &E);
-void CFGElement::dumpToStream(llvm::raw_ostream &OS,
- bool TerminateWithNewLine) const {
+void CFGElement::dumpToStream(llvm::raw_ostream &OS) const {
LangOptions LangOpts;
StmtPrinterHelper Helper(nullptr, LangOpts);
- print_elem(OS, Helper, *this, TerminateWithNewLine);
+ print_elem(OS, Helper, *this);
}
static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
- const CFGElement &E, bool TerminateWithNewLine) {
+ const CFGElement &E) {
switch (E.getKind()) {
case CFGElement::Kind::Statement:
case CFGElement::Kind::CXXRecordTypedCall:
@@ -5830,9 +5829,7 @@ static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
if (Children.begin() != Children.end()) {
OS << "({ ... ; ";
Helper.handledStmt(*SE->getSubStmt()->body_rbegin(),OS);
- OS << " })";
- if (TerminateWithNewLine)
- OS << '\n';
+ OS << " })\n";
return;
}
}
@@ -5841,8 +5838,7 @@ static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
if (B->getOpcode() == BO_Comma) {
OS << "... , ";
Helper.handledStmt(B->getRHS(),OS);
- if (TerminateWithNewLine)
- OS << '\n';
+ OS << '\n';
return;
}
}
@@ -5870,14 +5866,15 @@ static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
}
// Expressions need a newline.
- if (isa<Expr>(S) && TerminateWithNewLine)
+ if (isa<Expr>(S))
OS << '\n';
- return;
+ break;
}
case CFGElement::Kind::Initializer:
print_initializer(OS, Helper, E.castAs<CFGInitializer>().getInitializer());
+ OS << '\n';
break;
case CFGElement::Kind::AutomaticObjectDtor: {
@@ -5891,44 +5888,43 @@ static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
OS << ".~";
T.getUnqualifiedType().print(OS, PrintingPolicy(Helper.getLangOpts())...
[truncated]
|
steakhal
deleted the
revert-128251-clang-analyzer-conjured-symbol-use-cfgelement-ref
branch
fangyi-zhou
added a commit
to fangyi-zhou/llvm-project
that referenced
this pull request
Apr 25, 2025
…ured Closes llvm#57270. This PR changes the `Stmt *` field in `SymbolConjured` with `CFGBlock::ConstCFGElementRef`. The motivation is that, when conjuring a symbol, there might not always be a statement available, causing information to be lost for conjured symbols, whereas the CFGElementRef can always be provided at the callsite. Following the idea, this PR changes callsites of functions to create conjured symbols, and replaces them with appropriate `CFGElementRef`s. There is a caveat at loop widening, where the correct location is the CFG terminator (which is not an element and does not have a ref). In this case, the first element in the block is passed as a location. Previous PR llvm#128251, Reverted at llvm#137304.
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.
Reverts #128251
ASAN bots reported some errors: https://lab.llvm.org/buildbot/#/builders/55/builds/10398
Reverting for investigation.