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

[usx95]

Instead of identifying various forms of pointer usage (like dereferencing, member access, or function calls) individually, this new approach simplifies the logic by treating all DeclRefExprs as uses of their underlying origin

When a DeclRefExpr appears on the left-hand side of an assignment, the corresponding UseFact is marked as a "write" operation. These write operations are then exempted from use-after-free checks.

[usx95]

• [LifetimeSafety] Mark all DeclRefExpr as usages of the corresp. origin #154316 : 2 dependent PRs (#154009 , #156896 ) 👈 (View in Graphite)
• [LifetimeSafety] Fix duplicate loan generation for ImplicitCastExpr #153661 : 2 other dependent PRs (#153669 , #153951 )
• [LifetimeSafety] Implement a basic use-after-free diagnostic #149731
• [LifetimeSafety] Add loan expiry analysis #148712 : 1 other dependent PR (#148976 )
• [LifetimeSafety] Revamp test suite using unittests #149158
• [LifetimeSafety] Add per-program-point lattice tracking #149199
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-clang-temporal-safety
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Utkarsh Saxena (usx95)

Changes

Instead of identifying various forms of pointer usage (like dereferencing, member access, or function calls) individually, this new approach simplifies the logic by treating all DeclRefExprs as uses of their underlying origin

When a DeclRefExpr appears on the left-hand side of an assignment, the corresponding UseFact is marked as a "write" operation. These write operations are then exempted from use-after-free checks.

---

Full diff: https://github.com/llvm/llvm-project/pull/154316.diff

2 Files Affected:

• (modified) clang/lib/Analysis/LifetimeSafety.cpp (+55-20)
• (modified) clang/test/Sema/warn-lifetime-safety-dataflow.cpp (+26)

diff --git a/clang/lib/Analysis/LifetimeSafety.cpp b/clang/lib/Analysis/LifetimeSafety.cpp
index 9397c530a9af2..00efa4c5e548f 100644
--- a/clang/lib/Analysis/LifetimeSafety.cpp
+++ b/clang/lib/Analysis/LifetimeSafety.cpp
@@ -317,6 +317,7 @@ class ReturnOfOriginFact : public Fact {
 class UseFact : public Fact {
   OriginID UsedOrigin;
   const Expr *UseExpr;
+  bool IsWritten = false;
 
 public:
   static bool classof(const Fact *F) { return F->getKind() == Kind::Use; }
@@ -326,11 +327,13 @@ class UseFact : public Fact {
 
   OriginID getUsedOrigin() const { return UsedOrigin; }
   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";
+    OS << " " << (isWritten() ? "Write" : "Read") << ")\n";
   }
 };
 
@@ -428,6 +431,8 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
             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.
@@ -461,10 +466,6 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
           }
         }
       }
-    } 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));
     }
   }
 
@@ -479,20 +480,13 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
   }
 
   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) {
@@ -559,9 +553,49 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
     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()))
+        if (hasOrigin(VD_LHS->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) {
+    const auto *VD = dyn_cast<ValueDecl>(DRE->getDecl());
+    if (VD && hasOrigin(VD->getType())) {
+      OriginID OID = FactMgr.getOriginMgr().get(*VD);
+      UseFact *UF = FactMgr.createFact<UseFact>(OID, 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;
   const CFGBlock *CurrentBlock = nullptr;
   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;
 };
 
 class FactGenerator : public RecursiveASTVisitor<FactGenerator> {
@@ -1076,7 +1110,8 @@ 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) {
-
+    if (UF->isWritten())
+      return;
     OriginID O = UF->getUsedOrigin();
 
     // Get the set of loans that the origin might hold at this program point.
diff --git a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
index bcde9adf25ca5..5660a8ace4a09 100644
--- a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
+++ b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
@@ -293,3 +293,29 @@ void pointer_indirection() {
   int *q = *pp;
 // CHECK:   AssignOrigin (Dest: {{[0-9]+}} (Decl: q), Src: {{[0-9]+}} (Expr: ImplicitCastExpr))
 }
+
+// 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)
+}

[Xazax-hun]

It is checked for use-after-free unless it is being written to.

I wonder if this is important if we end up using liveness analysis. I.e., I believe the value we are overwriting is not live. So, in case we only warn for the live usages, we get this check for free. (In case we are moving in that direction).

[usx95]

I think the liveness analysis can be built on top of this. A UseFact with a write a = b kills value in a and gens value of b. All other facts essentially gens the origins involved. WDYT ?

[Xazax-hun]

I think the liveness analysis can be built on top of this.

I see! I was not sure what the layering would be. Makes sense to me.

[Xazax-hun]

Overall looks good, some small questions inline.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ymand]

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.

[usx95]

(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.

[usx95]

Merge activity

• Sep 5, 8:15 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Sep 5, 8:17 PM UTC: @usx95 merged this pull request with Graphite.