Skip to content

[LifetimeSafety] Mark all DeclRefExpr as usages of the corresp. origin #154316

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 1 commit into from
Sep 5, 2025
Merged

[LifetimeSafety] Mark all DeclRefExpr as usages of the corresp. origin #154316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
89 changes: 63 additions & 26 deletions clang/lib/Analysis/LifetimeSafety.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class OriginManager {
return AllOrigins.back();
}

// TODO: Mark this method as const once we remove the call to getOrCreate.
OriginID get(const Expr &E) {
// Origin of DeclRefExpr is that of the declaration it refers to.
if (const auto *DRE = dyn_cast<DeclRefExpr>(&E))
Expand Down Expand Up @@ -314,22 +315,28 @@ class ReturnOfOriginFact : public Fact {
};

class UseFact : public Fact {
OriginID UsedOrigin;
const Expr *UseExpr;
// True if this use is a write operation (e.g., left-hand side of assignment).
// Write operations are exempted from use-after-free checks.
bool IsWritten = false;

public:
static bool classof(const Fact *F) { return F->getKind() == Kind::Use; }

UseFact(OriginID UsedOrigin, const Expr *UseExpr)
: Fact(Kind::Use), UsedOrigin(UsedOrigin), UseExpr(UseExpr) {}
UseFact(const Expr *UseExpr) : Fact(Kind::Use), UseExpr(UseExpr) {}

OriginID getUsedOrigin() const { return UsedOrigin; }
OriginID getUsedOrigin(const OriginManager &OM) const {
// TODO: Remove const cast and make OriginManager::get as const.
return const_cast<OriginManager &>(OM).get(*UseExpr);
}
const Expr *getUseExpr() const { return UseExpr; }
void markAsWritten() { IsWritten = true; }
bool isWritten() const { return IsWritten; }

void dump(llvm::raw_ostream &OS, const OriginManager &OM) const override {
OS << "Use (";
OM.dump(getUsedOrigin(), OS);
OS << ")\n";
OM.dump(getUsedOrigin(OM), OS);
OS << " " << (isWritten() ? "Write" : "Read") << ")\n";
}
};

Expand Down Expand Up @@ -436,6 +443,8 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
addAssignOriginFact(*VD, *InitExpr);
}

void VisitDeclRefExpr(const DeclRefExpr *DRE) { handleUse(DRE); }

void VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *N) {
/// TODO: Handle nullptr expr as a special 'null' loan. Uninitialized
/// pointers can use the same type of loan.
Expand Down Expand Up @@ -469,10 +478,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
}
}
}
} else if (UO->getOpcode() == UO_Deref) {
// This is a pointer use, like '*p'.
OriginID OID = FactMgr.getOriginMgr().get(*UO->getSubExpr());
CurrentBlockFacts.push_back(FactMgr.createFact<UseFact>(OID, UO));
}
}

Expand All @@ -487,20 +492,13 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
}

void VisitBinaryOperator(const BinaryOperator *BO) {
if (BO->isAssignmentOp()) {
const Expr *LHSExpr = BO->getLHS();
const Expr *RHSExpr = BO->getRHS();

// We are interested in assignments like `ptr1 = ptr2` or `ptr = &var`
// LHS must be a pointer/reference type that can be an origin.
// RHS must also represent an origin (either another pointer/ref or an
// address-of).
if (const auto *DRE_LHS = dyn_cast<DeclRefExpr>(LHSExpr))
if (const auto *VD_LHS =
dyn_cast<ValueDecl>(DRE_LHS->getDecl()->getCanonicalDecl());
VD_LHS && hasOrigin(VD_LHS->getType()))
addAssignOriginFact(*VD_LHS, *RHSExpr);
}
if (BO->isAssignmentOp())
handleAssignment(BO->getLHS(), BO->getRHS());
}

void VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) {
if (OCE->isAssignmentOp() && OCE->getNumArgs() == 2)
handleAssignment(OCE->getArg(0), OCE->getArg(1));
}

void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *FCE) {
Expand Down Expand Up @@ -567,9 +565,47 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
return false;
}

void handleAssignment(const Expr *LHSExpr, const Expr *RHSExpr) {
// Find the underlying variable declaration for the left-hand side.
if (const auto *DRE_LHS =
dyn_cast<DeclRefExpr>(LHSExpr->IgnoreParenImpCasts())) {
markUseAsWrite(DRE_LHS);
if (const auto *VD_LHS = dyn_cast<ValueDecl>(DRE_LHS->getDecl()))
Comment on lines +570 to +573
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not loving this pattern. We're using a bottom-up visitor, but then here we do a top-down search and rewrite results that bubbled up. This non-local behavior makes it hard to reason about the computation being performed by the visitor.

For starters, consider commenting here instead of (only) below at the field declaration. But, I would encourage you to consider looking for a compositional algorithm (which can be done in a future PR). It might require a different visit order (does the RAV support visit before and after?). But, it's possible this can all be done bottom up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(We are no more using an RAV to visit bottom-up but using AC.getCFGBuildOptions().setAllAlwaysAdd(); to get a linearised AST stmts CFG essentially achieving the bottom up traversal order).

I am not a big fan either of the current top-down approach.
RAV only has a Traverse* which can be used as before but not after. Also using RAV does not guarantee that CFG does not show the DRE earlier than required.

What we can instead do is generate UseFacts separately in another pass on AST (not on CFG, we don't need the CFG for this as we are only interested in all DRE expressions) which can be context aware.

if (hasOrigin(LHSExpr->getType()))
// We are interested in assignments like `ptr1 = ptr2` or `ptr = &var`
// LHS must be a pointer/reference type that can be an origin.
// RHS must also represent an origin (either another pointer/ref or an
// address-of).
addAssignOriginFact(*VD_LHS, *RHSExpr);
}
}

// A DeclRefExpr is a use of the referenced decl. It is checked for
// use-after-free unless it is being written to (e.g. on the left-hand side
// of an assignment).
void handleUse(const DeclRefExpr *DRE) {
if (hasOrigin(DRE->getType())) {
UseFact *UF = FactMgr.createFact<UseFact>(DRE);
CurrentBlockFacts.push_back(UF);
assert(!UseFacts.contains(DRE));
UseFacts[DRE] = UF;
}
}

void markUseAsWrite(const DeclRefExpr *DRE) {
assert(UseFacts.contains(DRE));
UseFacts[DRE]->markAsWritten();
}

FactManager &FactMgr;
AnalysisDeclContext &AC;
llvm::SmallVector<Fact *> CurrentBlockFacts;
// To distinguish between reads and writes for use-after-free checks, this map
// stores the `UseFact` for each `DeclRefExpr`. We initially identify all
// `DeclRefExpr`s as "read" uses. When an assignment is processed, the use
// corresponding to the left-hand side is updated to be a "write", thereby
// exempting it from the check.
llvm::DenseMap<const DeclRefExpr *, UseFact *> UseFacts;
};

// ========================================================================= //
Expand Down Expand Up @@ -1032,8 +1068,9 @@ class LifetimeChecker {
/// graph. It determines if the loans held by the used origin have expired
/// at the point of use.
void checkUse(const UseFact *UF) {

OriginID O = UF->getUsedOrigin();
if (UF->isWritten())
return;
OriginID O = UF->getUsedOrigin(FactMgr.getOriginMgr());

// Get the set of loans that the origin might hold at this program point.
LoanSet HeldLoans = LoanPropagation.getLoans(O, UF);
Expand Down
26 changes: 26 additions & 0 deletions clang/test/Sema/warn-lifetime-safety-dataflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,3 +312,29 @@ void ternary_operator() {
// CHECK: AssignOrigin (Dest: [[O_P:[0-9]+]] (Decl: p), Src: {{[0-9]+}} (Expr: ConditionalOperator))
// CHECK: End of Block
}

// CHECK-LABEL: Function: test_use_facts
void usePointer(MyObj*);
void test_use_facts() {
// CHECK: Block B{{[0-9]+}}:
MyObj x;
MyObj *p;
p = &x;
// CHECK: Use ([[O_P:[0-9]+]] (Decl: p) Write)
(void)*p;
// CHECK: Use ([[O_P]] (Decl: p) Read)
usePointer(p);
// CHECK: Use ([[O_P]] (Decl: p) Read)
p->id = 1;
// CHECK: Use ([[O_P]] (Decl: p) Read)


MyObj* q;
q = p;
// CHECK: Use ([[O_P]] (Decl: p) Read)
// CHECK: Use ([[O_Q:[0-9]+]] (Decl: q) Write)
usePointer(q);
// CHECK: Use ([[O_Q]] (Decl: q) Read)
q->id = 2;
// CHECK: Use ([[O_Q]] (Decl: q) Read)
}