Skip to content

[NFC] [analyzer] Factor out SymbolManager::get<*> #121781

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

Merged
merged 15 commits into from
Jan 10, 2025
Merged

[NFC] [analyzer] Factor out SymbolManager::get<*> #121781

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
08b838a
[NFC] Replace getRegionValueSymbol with get<SymbolRegionValue>
necto Jan 6, 2025
5213d1b
[NFC] Redefine conjureSymbol via get<SymbolConjured>
necto Jan 6, 2025
cd94ced
[NFC] Replace getDerivedSymbol with get<SymbolDerived>
necto Jan 6, 2025
605ee80
[NFC] Replace getExtentSymbol with get<SymbolExtent>
necto Jan 6, 2025
883a6e3
[NFC] Replace getMetadataSymbol with get<SymbolMetadata>
necto Jan 6, 2025
dd41465
[NFC] Replace getCastSybol with get<SymbolCast>
necto Jan 6, 2025
71d05bb
[NFC] Replace getSymIntExpr with get<SymIntExpr>
necto Jan 6, 2025
8dc8cfd
[NFC] Replace getIntSymExpr with get<IntSymExpr>
necto Jan 6, 2025
207ff9b
[NFC] Replace getSymSymExpr with get<SymSymExpr>
necto Jan 6, 2025
8246491
[NFC] Replace getUnarySymExpr with get<UnarySymExpr>
necto Jan 6, 2025
3ed3a34
[NFC] fix formatting
necto Jan 6, 2025
83b9302
[NFC] Rename and document `acquire<SymExprT, ...>` member function
necto Jan 7, 2025
3a6b5d8
[NFC] fix formatting
necto Jan 9, 2025
fb5b3ad
[NFC] fix formatting
necto Jan 9, 2025
ecb06cb
Forward args only once!
steakhal Jan 10, 2025
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
70 changes: 25 additions & 45 deletions clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ class SymbolConjured : public SymbolData {

void dumpToStream(raw_ostream &os) const override;

static void Profile(llvm::FoldingSetNodeID& profile, const Stmt *S,
QualType T, unsigned Count, const LocationContext *LCtx,
static void Profile(llvm::FoldingSetNodeID &profile, const Stmt *S,
const LocationContext *LCtx, QualType T, unsigned Count,
const void *SymbolTag) {
profile.AddInteger((unsigned) SymbolConjuredKind);
profile.AddInteger((unsigned)SymbolConjuredKind);
profile.AddPointer(S);
profile.AddPointer(LCtx);
profile.Add(T);
Expand All @@ -125,7 +125,7 @@ class SymbolConjured : public SymbolData {
}

void Profile(llvm::FoldingSetNodeID& profile) override {
Profile(profile, S, T, Count, LCtx, SymbolTag);
Profile(profile, S, LCtx, T, Count, SymbolTag);
}

// Implement isa<T> support.
Expand Down Expand Up @@ -224,6 +224,8 @@ class SymbolMetadata : public SymbolData {
const Stmt *S;
QualType T;
const LocationContext *LCtx;
/// Count can be used to differentiate regions corresponding to
/// different loop iterations, thus, making the symbol path-dependent.
unsigned Count;
const void *Tag;

Expand Down Expand Up @@ -525,14 +527,14 @@ class SymbolManager {

static bool canSymbolicate(QualType T);

/// Make a unique symbol for MemRegion R according to its kind.
const SymbolRegionValue* getRegionValueSymbol(const TypedValueRegion* R);
template <typename T, typename... Args> const T *get(Args &&...args);
Copy link
Contributor

@NagyDonat NagyDonat Jan 6, 2025

Choose a reason for hiding this comment

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

If somebody reads this generic declaration without any context, they won't see any connection to symbols. Perhaps it would be good to either rename T to something like SymbolType or add a brief comment to describe the role of this method.

🤔 In fact, now that I write this, maybe it would be better to rename this method to "make" or something similar to highlight that it's a factory method that makes new symbol objects (as opposed to e.g. ProgramState::get<>() which queries an existing state trait object from the state).

Copy link
Contributor Author

@necto necto Jan 7, 2025

Choose a reason for hiding this comment

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

If somebody reads this generic declaration without any context, they won't see any connection to symbols. Perhaps it would be good to either rename T to something like SymbolType or add a brief comment to describe the role of this method.

That makes sense. I'll do both once we agree on the next point

🤔 In fact, now that I write this, maybe it would be better to rename this method to "make" or something similar to highlight that it's a factory method that makes new symbol objects (as opposed to e.g. ProgramState::get<>() which queries an existing state trait object from the state).

I was considering "make" briefly, but it is not entirely correct: this function doesn't always make a SymExpr, sometimes it retrieves an existing one matching the arguments. Besides, most of the replaced member functions were called get*. How do you like findOrCreate, createIfNone, ensureExists, obtain, acquire, procure?

Copy link
Contributor

@NagyDonat NagyDonat Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was considering "make" briefly, but it is not entirely correct: this function doesn't always make a SymExpr, sometimes it retrieves an existing one matching the arguments. Besides, most of the replaced member functions were called get*.

I would say that "make" is still reasonable in this case, because semantically it makes the symbol which is determined by the arguments passed to it -- and it's just an implementation detail that symbols are immutable objects and the make call returns already created symbols instead of recreating them. However, I can accept a different name if you're opposed to "make".

How do you like findOrCreate, createIfNone, ensureExists, obtain, acquire, procure?

I think acquire would be the best choice among these, followed by something like makeOrGet or findOrCreate. IMO obtain suggests that we're getting the symbol from an external source, procure has unfortunate connections to sexual services and I don't like createIfNone and ensureExists because they don't imply that the method returns the symbol in addition to just initializing it in some internal table.

I still slightly prefer make over acquire (it's a shorter, more typical name for a factory method), but acquire is close to it.

Copy link
Contributor Author

@necto necto Jan 7, 2025

Choose a reason for hiding this comment

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

Addressed in
83b9302 [NFC] Rename and document acquire<SymExprT, ...> member function


const SymbolConjured* conjureSymbol(const Stmt *E,
const LocationContext *LCtx,
QualType T,
const SymbolConjured *conjureSymbol(const Stmt *E,
const LocationContext *LCtx, QualType T,
unsigned VisitCount,
const void *SymbolTag = nullptr);
const void *SymbolTag = nullptr) {
return get<SymbolConjured>(E, LCtx, T, VisitCount, SymbolTag);
}

const SymbolConjured* conjureSymbol(const Expr *E,
const LocationContext *LCtx,
Expand All @@ -541,41 +543,6 @@ class SymbolManager {
return conjureSymbol(E, LCtx, E->getType(), VisitCount, SymbolTag);
}

const SymbolDerived *getDerivedSymbol(SymbolRef parentSymbol,
const TypedValueRegion *R);

const SymbolExtent *getExtentSymbol(const SubRegion *R);

/// Creates a metadata symbol associated with a specific region.
///
/// VisitCount can be used to differentiate regions corresponding to
/// different loop iterations, thus, making the symbol path-dependent.
const SymbolMetadata *getMetadataSymbol(const MemRegion *R, const Stmt *S,
QualType T,
const LocationContext *LCtx,
unsigned VisitCount,
const void *SymbolTag = nullptr);

const SymbolCast* getCastSymbol(const SymExpr *Operand,
QualType From, QualType To);

const SymIntExpr *getSymIntExpr(const SymExpr *lhs, BinaryOperator::Opcode op,
APSIntPtr rhs, QualType t);

const SymIntExpr *getSymIntExpr(const SymExpr &lhs, BinaryOperator::Opcode op,
APSIntPtr rhs, QualType t) {
return getSymIntExpr(&lhs, op, rhs, t);
}

const IntSymExpr *getIntSymExpr(APSIntPtr lhs, BinaryOperator::Opcode op,
const SymExpr *rhs, QualType t);

const SymSymExpr *getSymSymExpr(const SymExpr *lhs, BinaryOperator::Opcode op,
const SymExpr *rhs, QualType t);

const UnarySymExpr *getUnarySymExpr(const SymExpr *operand,
UnaryOperator::Opcode op, QualType t);

QualType getType(const SymExpr *SE) const {
return SE->getType();
}
Expand Down Expand Up @@ -707,6 +674,19 @@ class SymbolVisitor {
virtual bool VisitMemRegion(const MemRegion *) { return true; }
};

template <typename T, typename... Args>
const T *SymbolManager::get(Args &&...args) {
llvm::FoldingSetNodeID profile;
T::Profile(profile, std::forward<Args>(args)...);
void *InsertPos;
SymExpr *SD = DataSet.FindNodeOrInsertPos(profile, InsertPos);
if (!SD) {
SD = Alloc.make<T>(std::forward<Args>(args)...);
DataSet.InsertNode(SD, InsertPos);
}
return cast<T>(SD);
}

} // namespace ento

} // namespace clang
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/StaticAnalyzer/Core/MemRegion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ DefinedOrUnknownSVal MemRegionManager::getStaticSize(const MemRegion *MR,
switch (SR->getKind()) {
case MemRegion::AllocaRegionKind:
case MemRegion::SymbolicRegionKind:
return nonloc::SymbolVal(SymMgr.getExtentSymbol(SR));
return nonloc::SymbolVal(SymMgr.get<SymbolExtent>(SR));
case MemRegion::StringRegionKind:
return SVB.makeIntVal(
cast<StringRegion>(SR)->getStringLiteral()->getByteLength() + 1,
Expand All @@ -829,7 +829,7 @@ DefinedOrUnknownSVal MemRegionManager::getStaticSize(const MemRegion *MR,
case MemRegion::ObjCStringRegionKind: {
QualType Ty = cast<TypedValueRegion>(SR)->getDesugaredValueType(Ctx);
if (isa<VariableArrayType>(Ty))
return nonloc::SymbolVal(SymMgr.getExtentSymbol(SR));
return nonloc::SymbolVal(SymMgr.get<SymbolExtent>(SR));

if (Ty->isIncompleteType())
return UnknownVal();
Expand Down Expand Up @@ -897,7 +897,7 @@ DefinedOrUnknownSVal MemRegionManager::getStaticSize(const MemRegion *MR,
case MemRegion::BlockDataRegionKind:
case MemRegion::BlockCodeRegionKind:
case MemRegion::FunctionCodeRegionKind:
return nonloc::SymbolVal(SymMgr.getExtentSymbol(SR));
return nonloc::SymbolVal(SymMgr.get<SymbolExtent>(SR));
default:
llvm_unreachable("Unhandled region");
}
Expand Down
12 changes: 6 additions & 6 deletions clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,7 @@ class SymbolicRangeInferrer
return getRangeForNegatedExpr(
[SSE, State = this->State]() -> SymbolRef {
if (SSE->getOpcode() == BO_Sub)
return State->getSymbolManager().getSymSymExpr(
return State->getSymbolManager().get<SymSymExpr>(
SSE->getRHS(), BO_Sub, SSE->getLHS(), SSE->getType());
return nullptr;
},
Expand All @@ -1481,8 +1481,8 @@ class SymbolicRangeInferrer
std::optional<RangeSet> getRangeForNegatedSym(SymbolRef Sym) {
return getRangeForNegatedExpr(
[Sym, State = this->State]() {
return State->getSymbolManager().getUnarySymExpr(Sym, UO_Minus,
Sym->getType());
return State->getSymbolManager().get<UnarySymExpr>(Sym, UO_Minus,
Sym->getType());
},
Sym->getType());
}
Expand All @@ -1495,7 +1495,7 @@ class SymbolicRangeInferrer
if (!IsCommutative)
return std::nullopt;

SymbolRef Commuted = State->getSymbolManager().getSymSymExpr(
SymbolRef Commuted = State->getSymbolManager().get<SymSymExpr>(
SSE->getRHS(), Op, SSE->getLHS(), SSE->getType());
if (const RangeSet *Range = getConstraint(State, Commuted))
return *Range;
Expand Down Expand Up @@ -1540,15 +1540,15 @@ class SymbolicRangeInferrer

// Let's find an expression e.g. (x < y).
BinaryOperatorKind QueriedOP = OperatorRelationsTable::getOpFromIndex(i);
const SymSymExpr *SymSym = SymMgr.getSymSymExpr(LHS, QueriedOP, RHS, T);
const SymSymExpr *SymSym = SymMgr.get<SymSymExpr>(LHS, QueriedOP, RHS, T);
const RangeSet *QueriedRangeSet = getConstraint(State, SymSym);

// If ranges were not previously found,
// try to find a reversed expression (y > x).
if (!QueriedRangeSet) {
const BinaryOperatorKind ROP =
BinaryOperator::reverseComparisonOp(QueriedOP);
SymSym = SymMgr.getSymSymExpr(RHS, ROP, LHS, T);
SymSym = SymMgr.get<SymSymExpr>(RHS, ROP, LHS, T);
QueriedRangeSet = getConstraint(State, SymSym);
}

Expand Down
8 changes: 4 additions & 4 deletions clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ ProgramStateRef RangedConstraintManager::assumeSym(ProgramStateRef State,

SymbolManager &SymMgr = getSymbolManager();
QualType DiffTy = SymMgr.getContext().getPointerDiffType();
SymbolRef Subtraction =
SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
SymbolRef Subtraction = SymMgr.get<SymSymExpr>(SSE->getRHS(), BO_Sub,
SSE->getLHS(), DiffTy);

const llvm::APSInt &Zero = getBasicVals().getValue(0, DiffTy);
Op = BinaryOperator::reverseComparisonOp(Op);
Expand All @@ -76,8 +76,8 @@ ProgramStateRef RangedConstraintManager::assumeSym(ProgramStateRef State,
SymbolManager &SymMgr = getSymbolManager();

QualType ExprType = SSE->getType();
SymbolRef CanonicalEquality =
SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
SymbolRef CanonicalEquality = SymMgr.get<SymSymExpr>(
SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);

bool WasEqual = SSE->getOpcode() == BO_EQ;
bool IsExpectedEqual = WasEqual == Assumption;
Expand Down
18 changes: 9 additions & 9 deletions clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,30 +79,30 @@ nonloc::SymbolVal SValBuilder::makeNonLoc(const SymExpr *lhs,
APSIntPtr rhs, QualType type) {
assert(lhs);
assert(!Loc::isLocType(type));
return nonloc::SymbolVal(SymMgr.getSymIntExpr(lhs, op, rhs, type));
return nonloc::SymbolVal(SymMgr.get<SymIntExpr>(lhs, op, rhs, type));
}

nonloc::SymbolVal SValBuilder::makeNonLoc(APSIntPtr lhs,
BinaryOperator::Opcode op,
const SymExpr *rhs, QualType type) {
assert(rhs);
assert(!Loc::isLocType(type));
return nonloc::SymbolVal(SymMgr.getIntSymExpr(lhs, op, rhs, type));
return nonloc::SymbolVal(SymMgr.get<IntSymExpr>(lhs, op, rhs, type));
}

nonloc::SymbolVal SValBuilder::makeNonLoc(const SymExpr *lhs,
BinaryOperator::Opcode op,
const SymExpr *rhs, QualType type) {
assert(lhs && rhs);
assert(!Loc::isLocType(type));
return nonloc::SymbolVal(SymMgr.getSymSymExpr(lhs, op, rhs, type));
return nonloc::SymbolVal(SymMgr.get<SymSymExpr>(lhs, op, rhs, type));
}

NonLoc SValBuilder::makeNonLoc(const SymExpr *operand, UnaryOperator::Opcode op,
QualType type) {
assert(operand);
assert(!Loc::isLocType(type));
return nonloc::SymbolVal(SymMgr.getUnarySymExpr(operand, op, type));
return nonloc::SymbolVal(SymMgr.get<UnarySymExpr>(operand, op, type));
}

nonloc::SymbolVal SValBuilder::makeNonLoc(const SymExpr *operand,
Expand All @@ -111,7 +111,7 @@ nonloc::SymbolVal SValBuilder::makeNonLoc(const SymExpr *operand,
assert(!Loc::isLocType(toTy));
if (fromTy == toTy)
return nonloc::SymbolVal(operand);
return nonloc::SymbolVal(SymMgr.getCastSymbol(operand, fromTy, toTy));
return nonloc::SymbolVal(SymMgr.get<SymbolCast>(operand, fromTy, toTy));
}

SVal SValBuilder::convertToArrayIndex(SVal val) {
Expand Down Expand Up @@ -143,7 +143,7 @@ SValBuilder::getRegionValueSymbolVal(const TypedValueRegion *region) {
if (!SymbolManager::canSymbolicate(T))
return UnknownVal();

SymbolRef sym = SymMgr.getRegionValueSymbol(region);
SymbolRef sym = SymMgr.get<SymbolRegionValue>(region);

if (Loc::isLocType(T))
return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
Expand Down Expand Up @@ -245,7 +245,7 @@ DefinedSVal SValBuilder::getMetadataSymbolVal(const void *symbolTag,
assert(SymbolManager::canSymbolicate(type) && "Invalid metadata symbol type");

SymbolRef sym =
SymMgr.getMetadataSymbol(region, expr, type, LCtx, count, symbolTag);
SymMgr.get<SymbolMetadata>(region, expr, type, LCtx, count, symbolTag);

if (Loc::isLocType(type))
return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
Expand All @@ -264,7 +264,7 @@ SValBuilder::getDerivedRegionValueSymbolVal(SymbolRef parentSymbol,
if (!SymbolManager::canSymbolicate(T))
return UnknownVal();

SymbolRef sym = SymMgr.getDerivedSymbol(parentSymbol, region);
SymbolRef sym = SymMgr.get<SymbolDerived>(parentSymbol, region);

if (Loc::isLocType(T))
return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
Expand Down Expand Up @@ -724,7 +724,7 @@ class EvalCastVisitor : public SValVisitor<EvalCastVisitor, SVal> {
// because there are no generic region address metadata
// symbols to use, only content metadata.
return nonloc::SymbolVal(
VB.getSymbolManager().getExtentSymbol(FTR));
VB.getSymbolManager().get<SymbolExtent>(FTR));

if (const SymbolicRegion *SymR = R->getSymbolicBase()) {
SymbolRef Sym = SymR->getSymbol();
Expand Down
10 changes: 5 additions & 5 deletions clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,16 +328,16 @@ static NonLoc doRearrangeUnchecked(ProgramStateRef State,
// FIXME: Maybe it'd be better to have consistency in
// "$x - $y" vs. "$y - $x" because those are solver's keys.
if (LInt > RInt) {
ResultSym = SymMgr.getSymSymExpr(RSym, BO_Sub, LSym, SymTy);
ResultSym = SymMgr.get<SymSymExpr>(RSym, BO_Sub, LSym, SymTy);
ResultOp = BinaryOperator::reverseComparisonOp(Op);
ResultInt = LInt - RInt; // Opposite order!
} else {
ResultSym = SymMgr.getSymSymExpr(LSym, BO_Sub, RSym, SymTy);
ResultSym = SymMgr.get<SymSymExpr>(LSym, BO_Sub, RSym, SymTy);
ResultOp = Op;
ResultInt = RInt - LInt; // Opposite order!
}
} else {
ResultSym = SymMgr.getSymSymExpr(LSym, Op, RSym, SymTy);
ResultSym = SymMgr.get<SymSymExpr>(LSym, Op, RSym, SymTy);
ResultInt = (Op == BO_Add) ? (LInt + RInt) : (LInt - RInt);
ResultOp = BO_Add;
// Bring back the cosmetic difference.
Expand All @@ -350,8 +350,8 @@ static NonLoc doRearrangeUnchecked(ProgramStateRef State,
}
}
APSIntPtr PersistentResultInt = BV.getValue(ResultInt);
return nonloc::SymbolVal(
SymMgr.getSymIntExpr(ResultSym, ResultOp, PersistentResultInt, ResultTy));
return nonloc::SymbolVal(SymMgr.get<SymIntExpr>(
ResultSym, ResultOp, PersistentResultInt, ResultTy));
}

// Rearrange if symbol type matches the result type and if the operator is a
Expand Down
Loading
Loading