[clang] 5e47ef4 - [LifetimeSafety] Reapply liveness propagation and fix loop liveness leakage
via cfe-commits
cfe-commits at lists.llvm.org
Sat Jun 27 05:32:16 PDT 2026
Author: Utkarsh Saxena
Date: 2026-06-27T12:32:11Z
New Revision: 5e47ef456a7860e1fa12428f96ff072ff6c4f905
URL: https://github.com/llvm/llvm-project/commit/5e47ef456a7860e1fa12428f96ff072ff6c4f905
DIFF: https://github.com/llvm/llvm-project/commit/5e47ef456a7860e1fa12428f96ff072ff6c4f905.diff
LOG: [LifetimeSafety] Reapply liveness propagation and fix loop liveness leakage
(#205740)
Reapplies the liveness propagation fix (originally #205323, reverted in
#205687) and fixes a false positive involving conditional operators in
loops.
### Key Changes
* **Reapply**: Corrects liveness propagation through origin flows and
adds support for GNU statement expressions (`({ ... })`).
* **Loop Liveness Fix**: Resolves a false positive where temporary
origins leaked liveness across loop backedges via the conditional
operator's merge block. We now path-isolate these flows by generating
the `OriginFlowFact`s in their respective predecessor blocks (branches)
instead of the merge block.
Details about the old liveness leak. Consider this example
```cpp
for (int i = 0; i < 2; i++) {
int x, y;
consume(cond ? &x : &y); // no-warning
}
```
CFG:
```
[Loop Header / Cond]
/ \
[True Branch] [False Branch] <-- Arms of the ? :
(yields &x) (yields &y)
\ /
[Merge Block] <-- consume(CO) called here
|
[Loop Backedge / End] <-- x and y destroyed here
```
The thing that goes wrong is that origin of `&x` and `&y` ends up being
alive throughout the loop.
In the **old implementation**, the flow facts (`CO <- &x` and `CO <-
&y`) were generated in the **Merge Block** (where `consume(CO)` is
called). Because these flows occurred in the same block, they had to be
applied sequentially: one with `Kill=true` and the subsequent one with
`Merge=true` (no kill).
Going backward (as liveness analysis runs):
1. The use of `CO` in `consume(CO)` makes `CO` **live**.
2. This makes `&x` and `&y` alive at the beginning of "Merge Block".
3. This makes `&x` and `&y` alive in "True branch". In "True branch",
`&x` gets in a flow from `x` and is therefore killed. But that does not
happen to `&y`.
4. Same happens in "False branch", `&x` stays alive and only `&y` is
only killed.
5. This makes both `&x` and `&y` live at the "[Loop Header / Cond]" and
that spreads throughout the loop body.
6. This makes `&x` live at the end of loop body.
Pushing the flow back into the individual branches stops `&y` from being
live in "True branch" and vice versa.
AI usage: Antigravity for all parts of the PR
Added:
Modified:
clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp
clang/test/Sema/LifetimeSafety/invalidations.cpp
clang/test/Sema/LifetimeSafety/safety.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
index 88b509e1b94df..5c671a93b149c 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
@@ -351,6 +351,10 @@ class FactManager {
BlockToFacts[B->getBlockID()].assign(NewFacts.begin(), NewFacts.end());
}
+ void appendBlockFact(const CFGBlock *B, const Fact *F) {
+ BlockToFacts[B->getBlockID()].push_back(F);
+ }
+
template <typename FactType, typename... Args>
FactType *createFact(Args &&...args) {
void *Mem = FactAllocator.Allocate<FactType>();
diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
index 5ac67263681ac..9821078ec1d1e 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
@@ -57,6 +57,7 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> {
void VisitArraySubscriptExpr(const ArraySubscriptExpr *ASE);
void VisitCXXNewExpr(const CXXNewExpr *NE);
void VisitCXXDeleteExpr(const CXXDeleteExpr *DE);
+ void VisitStmtExpr(const StmtExpr *SE);
private:
OriginList *getOriginsList(const ValueDecl &D);
@@ -65,7 +66,8 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> {
bool hasOrigins(QualType QT) const;
bool hasOrigins(const Expr *E) const;
- void flow(OriginList *Dst, OriginList *Src, bool Kill);
+ void flow(OriginList *Dst, OriginList *Src, bool Kill,
+ const CFGBlock *Block = nullptr);
/// Handles assignment for both BinaryOperator and CXXOperatorCallExpr.
///
diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
index 50bf79d4c1a38..8358c69a5165a 100644
--- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
@@ -58,7 +58,16 @@ bool FactsGenerator::hasOrigins(const Expr *E) const {
/// * Level 1: pp <- p's address
/// * Level 2: (*pp) <- what p points to (i.e., &x)
/// - `View v = obj;` flows origins from `obj` (depth 1) to `v` (depth 1)
-void FactsGenerator::flow(OriginList *Dst, OriginList *Src, bool Kill) {
+///
+/// \param Dst The destination origin list.
+/// \param Src The source origin list.
+/// \param Kill If true, the destination's existing loans are killed before
+/// flowing.
+/// \param Block Optional. If provided, the generated flow facts are appended to
+/// this specific CFG block. Otherwise, they are appended to the
+/// current block being visited.
+void FactsGenerator::flow(OriginList *Dst, OriginList *Src, bool Kill,
+ const CFGBlock *Block) {
if (!Dst)
return;
assert(Src &&
@@ -67,8 +76,12 @@ void FactsGenerator::flow(OriginList *Dst, OriginList *Src, bool Kill) {
"Lists must have the same length");
while (Dst && Src) {
- CurrentBlockFacts.push_back(FactMgr.createFact<OriginFlowFact>(
- Dst->getOuterOriginID(), Src->getOuterOriginID(), Kill));
+ Fact *F = FactMgr.createFact<OriginFlowFact>(Dst->getOuterOriginID(),
+ Src->getOuterOriginID(), Kill);
+ if (Block)
+ FactMgr.appendBlockFact(Block, F);
+ else
+ CurrentBlockFacts.push_back(F);
Dst = Dst->peelOuterOrigin();
Src = Src->peelOuterOrigin();
}
@@ -541,66 +554,47 @@ void FactsGenerator::VisitBinaryOperator(const BinaryOperator *BO) {
// TODO: Handle assignments involving dereference like `*p = q`.
}
+static const CFGBlock *findPredBlockForExpr(const CFGBlock *MergeBlock,
+ const Expr *ArmExpr) {
+ if (!ArmExpr)
+ return nullptr;
+ const Expr *Target = ArmExpr->IgnoreParenImpCasts();
+ if (const auto *OVE = dyn_cast<OpaqueValueExpr>(Target))
+ if (const Expr *Src = OVE->getSourceExpr())
+ Target = Src->IgnoreParenImpCasts();
+
+ for (const CFGBlock *Pred : MergeBlock->preds()) {
+ if (!Pred)
+ continue;
+ for (const CFGElement &Elt : *Pred)
+ if (auto CS = Elt.getAs<CFGStmt>())
+ if (const auto *E = dyn_cast<Expr>(CS->getStmt()))
+ if (E->IgnoreParenImpCasts() == Target)
+ return Pred;
+ }
+ return nullptr;
+}
+
+/// Visits conditional operators (e.g., `cond ? a : b`).
+///
+/// To prevent liveness leakage across loop backedges (which causes false
+/// positives like in `while (...) { int x; consume(cond ? &x : nullptr); }`),
+/// we generate the flow facts in the respective predecessor blocks of the arms
+/// rather than in the merge block. This ensures that the liveness of the
+/// temporary origin from one arm does not propagate into the other arm's path.
void FactsGenerator::VisitAbstractConditionalOperator(
const AbstractConditionalOperator *CO) {
if (!hasOrigins(CO))
return;
- // For the GNU binary conditional `a ?: b`, getTrueExpr() is the
- // OpaqueValueExpr wrapping the common subexpression.
const Expr *TrueExpr = CO->getTrueExpr();
const Expr *FalseExpr = CO->getFalseExpr();
- const auto Preds = CurrentBlock->preds();
-
- // Skip origin flow from conditional operator arms that cannot produce the
- // result value: throw arms and calls to noreturn functions.
- bool TBHasEdge = true;
- bool FBHasEdge = true;
-
- switch (CurrentBlock->pred_size()) {
- case 0:
- return;
- case 1: {
- // For `a ?: b`, getTrueExpr() is the OpaqueValueExpr; the common
- // subexpression it wraps is what appears in the predecessor block.
- const Expr *TrueArm = TrueExpr->IgnoreParenImpCasts();
- if (const auto *OVE = dyn_cast<OpaqueValueExpr>(TrueArm))
- if (const Expr *Src = OVE->getSourceExpr())
- TrueArm = Src->IgnoreParenImpCasts();
- TBHasEdge = llvm::any_of(**Preds.begin(), [TrueArm](const CFGElement &Elt) {
- if (auto CS = Elt.getAs<CFGStmt>())
- return CS->getStmt() == TrueArm;
- return false;
- });
- FBHasEdge = !TBHasEdge;
- break;
- }
- case 2: {
- const auto *It = Preds.begin();
- TBHasEdge = It->isReachable();
- FBHasEdge = (++It)->isReachable();
- break;
- }
- default:
- llvm_unreachable("expected at most 2 predecessors");
- return;
- }
-
- bool FirstFlow = true;
- auto HandleFlow = [&](const Expr *E) {
- if (FirstFlow) {
- killAndFlowOrigin(*CO, *E);
- FirstFlow = false;
- } else {
- flowOrigin(*CO, *E);
- }
- };
-
- if (TBHasEdge)
- HandleFlow(TrueExpr);
- if (FBHasEdge)
- HandleFlow(FalseExpr);
+ if (const CFGBlock *TBPred = findPredBlockForExpr(CurrentBlock, TrueExpr))
+ flow(getOriginsList(*CO), getOriginsList(*TrueExpr), /*Kill=*/true, TBPred);
+ if (const CFGBlock *FBPred = findPredBlockForExpr(CurrentBlock, FalseExpr))
+ flow(getOriginsList(*CO), getOriginsList(*FalseExpr), /*Kill=*/true,
+ FBPred);
}
void FactsGenerator::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) {
@@ -801,6 +795,21 @@ void FactsGenerator::VisitCXXDeleteExpr(const CXXDeleteExpr *DE) {
FactMgr.createFact<InvalidateOriginFact>(List->getOuterOriginID(), DE));
}
+void FactsGenerator::VisitStmtExpr(const StmtExpr *SE) {
+ // A statement expression (`({ ...; e; })`) yields the value of its final
+ // expression `e`. Flow `e`'s origins into the statement expression's origin
+ // so a borrow `e` carries reaches the value's users.
+ const auto *CS = SE->getSubStmt();
+ if (!CS || CS->body_empty())
+ return;
+ const auto *Last = dyn_cast<Expr>(CS->body_back());
+ if (!Last)
+ return;
+ if (OriginList *Dst = getOriginsList(*SE))
+ if (OriginList *Src = getRValueOrigins(Last, getOriginsList(*Last)))
+ flow(Dst, Src, /*Kill=*/true);
+}
+
bool FactsGenerator::escapesViaReturn(OriginID OID) const {
return llvm::any_of(EscapesInCurrentBlock, [OID](const Fact *F) {
if (const auto *EF = F->getAs<ReturnEscapeFact>())
@@ -908,6 +917,12 @@ void FactsGenerator::handleMovedArgsInCall(const FunctionDecl *FD,
const ParmVarDecl *PVD = FD->getParamDecl(I - IsInstance);
if (!PVD->getType()->isRValueReferenceType())
continue;
+ // Skip lifetime annotated r-value reference parameters. Lifetime annotation
+ // indicates that the parameter is borrowed (not consumed), so it should not
+ // be marked as moved even though it's an r-value reference.
+ if (PVD->hasAttr<LifetimeBoundAttr>() ||
+ PVD->hasAttr<LifetimeCaptureByAttr>())
+ continue;
const Expr *Arg = Args[I];
OriginList *MovedOrigins = getOriginsList(*Arg);
assert(MovedOrigins->getLength() >= 1 &&
diff --git a/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp b/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp
index cfbcacf04b1b0..69b903c813555 100644
--- a/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp
@@ -161,9 +161,20 @@ class AnalysisImpl
/// An OriginFlow kills the liveness of the destination origin if `KillDest`
/// is true. Otherwise, it propagates liveness from destination to source.
Lattice transfer(Lattice In, const OriginFlowFact &OF) {
- if (!OF.getKillDest())
- return In;
- return Lattice(Factory.remove(In.LiveOrigins, OF.getDestOriginID()));
+ Lattice Out = In;
+ OriginID Dest = OF.getDestOriginID();
+ OriginID Src = OF.getSrcOriginID();
+ // If the destination of the flow is live, the source of the flow must also
+ // be marked live before this point as its value will flow into the
+ // destination.
+ if (In.LiveOrigins.contains(Dest)) {
+ const LivenessInfo *DestInfo = In.LiveOrigins.lookup(Dest);
+ assert(DestInfo);
+ Out = Lattice(Factory.add(Out.LiveOrigins, Src, *DestInfo));
+ }
+ if (OF.getKillDest())
+ Out = Lattice(Factory.remove(Out.LiveOrigins, Dest));
+ return Out;
}
Lattice transfer(Lattice In, const KillOriginFact &F) {
diff --git a/clang/test/Sema/LifetimeSafety/invalidations.cpp b/clang/test/Sema/LifetimeSafety/invalidations.cpp
index c2ac105855d07..be1acc6bc7fbc 100644
--- a/clang/test/Sema/LifetimeSafety/invalidations.cpp
+++ b/clang/test/Sema/LifetimeSafety/invalidations.cpp
@@ -402,10 +402,20 @@ void SelfInvalidatingMap() {
// Therefore the following is safe in practice.
// On the other hand, std::flat_map (since C++23) does not provide pointer stability on
// insertion and following is unsafe for this container.
- mp[1] = "42";
- mp[2] // expected-note {{local variable 'mp' is invalidated here}}
- =
- mp[1]; // expected-warning {{local variable 'mp' is later invalidated}} expected-note {{later used here}}
+ // FIXME: The warnings below are false positives (self-invalidation of the Owner).
+ // Modifying a container should not invalidate the container object itself.
+ // To resolve this, we need to:
+ // 1. Distinguish owner-borrow (borrowing the container object) from content-borrow (borrowing elements inside the container).
+ // 2. Make AccessPaths more precise to reason at element/field granularity rather than treating the whole container as a single storage location.
+ mp[1] = "42"; // expected-warning {{local variable 'mp' is later invalidated}} \
+ // expected-note {{local variable 'mp' is invalidated here}} \
+ // expected-note {{later used here}}
+ mp[2] = mp[1]; // expected-warning {{local variable 'mp' is later invalidated}} \
+ // expected-warning {{local variable 'mp' is later invalidated}} \
+ // expected-note {{local variable 'mp' is invalidated here}} \
+ // expected-note {{later used here}} \
+ // expected-note {{local variable 'mp' is invalidated here}} \
+ // expected-note {{later used here}}
}
void InvalidateErase() {
@@ -740,9 +750,16 @@ void MapSubscriptMultipleCallsDoesNotInvalidate(std::map<int, int> mp, int a, in
}
void FlatMapSubscriptMultipleCallsInvalidate(std::flat_map<int, int> mp, int a, int b) {
+ // FIXME: The duplicate warning below is a false positive caused by self-invalidation of the Owner 'mp'.
+ // While the warning on the temporary reference returned by mp[a] is a true positive (it dangles),
+ // the second warning on 'mp' itself is redundant and incorrect.
+ // Resolving this requires distinguishing owner-borrow from content-borrow.
PrintMax(mp[a], mp[b]); // expected-warning {{parameter 'mp' is later invalidated}} \
- // expected-note {{parameter 'mp' is invalidated here}} \
- // expected-note {{later used here}}
+ // expected-warning {{parameter 'mp' is later invalidated}} \
+ // expected-note {{parameter 'mp' is invalidated here}} \
+ // expected-note {{later used here}} \
+ // expected-note {{parameter 'mp' is invalidated here}} \
+ // expected-note {{later used here}}
}
} // namespace AssociativeContainers
diff --git a/clang/test/Sema/LifetimeSafety/safety.cpp b/clang/test/Sema/LifetimeSafety/safety.cpp
index 2c2d62b25b6c8..2cbf651eb46b5 100644
--- a/clang/test/Sema/LifetimeSafety/safety.cpp
+++ b/clang/test/Sema/LifetimeSafety/safety.cpp
@@ -3956,8 +3956,94 @@ PtrWithInt f() {
return PtrWithInt{10};
}
+// A GNU statement expression (`({ ...; e; })`) yields the value of its final
+// expression `e`. `e`'s origins flow into the statement expression's value, so
+// a borrow `e` carries is tracked: a borrow of a body-local dangles, and a
+// borrow forwarded from an outer object propagates to the value's users.
+namespace statement_expression {
+void use(int *p);
+
+// A borrow of a statement-expression-local escaping via the value.
+void borrow_of_local() {
+ int *p = ({ int x = 7; &x; }); // expected-warning {{local variable 'x' does not live long enough}} expected-note {{local variable 'x' is destroyed here}}
+ use(p); // expected-note {{later used here}}
+}
+
+// An outer borrow forwarded through a statement expression and returned:
+// use-after-return.
+int *return_borrow_of_local() {
+ int local = 0;
+ return ({ (void)0; &local; }); // expected-warning {{stack memory associated with local variable 'local' is returned}} expected-note {{returned here}}
+}
+
+// A view bound to a temporary produced by the statement expression dangles.
+void borrow_temporary() {
+ std::string_view view = ({ std::string x = "long enough heap string!!!!!!"; x; }); // expected-warning {{temporary object does not live long enough}} expected-note {{temporary object is destroyed here}}
+ (void)view; // expected-note {{later used here}}
+}
+
+// Forwarding an outer borrow that dangles.
+void forward_outer_borrow() {
+ int *p;
+ {
+ int local = 0;
+ p = ({ (void)0; &local; }); // expected-warning {{local variable 'local' does not live long enough}}
+ } // expected-note {{local variable 'local' is destroyed here}}
+ use(p); // expected-note {{later used here}}
+}
+
+// The statement-expression result carries the borrow, so a `?:` sibling
+// supplying a valid loan no longer hides it via the merge.
+void masked(bool c) {
+ static int valid;
+ int *keep = &valid;
+ int *r;
+ {
+ int local = 0;
+ r = c ? keep : ({ &local; }); // expected-warning {{local variable 'local' does not live long enough}}
+ } // expected-note {{local variable 'local' is destroyed here}}
+ use(r); // expected-note {{later used here}}
+}
+
+// Both conditional arms are statement expressions returning a borrow of a
+// body-local; each is caught as a returned stack address.
+int *conditional_arms(bool c) {
+ return c ? ({ int x = 7; &x; }) // expected-warning {{stack memory associated with local variable 'x' is returned}} expected-note 2 {{returned here}}
+ : ({ int y = 7; &y; }); // expected-warning {{stack memory associated with local variable 'y' is returned}}
+}
+
+// Negative: a statement expression yielding a long-lived borrow stays silent.
+void ok() {
+ static int s;
+ int *p = ({ int unused = 0; (void)unused; &s; });
+ use(p); // no-warning
+}
+
+// A discarded statement expression's value is not consumed, so a borrow of a
+// body-local in it does not reach any user and is correctly not flagged.
+void discarded_body_local() {
+ (void)({ int x = 7; &x; }); // no-warning
+}
+} // namespace statement_expression
+
// This would normally trigger a suggestion warning if -Wlifetime-safety-suggestions was on.
// Since it is off, we expect NO warnings or notes here.
View suggestion_disabled_test(View a) {
return a;
}
+
+// Test case for false positive involving conditional operator in a loop.
+struct LoopCondBindS {
+ int* get() const [[clang::lifetimebound]];
+};
+void consume_loop_cond_bind(int*);
+void test_loop_cond_bind(bool cond) {
+ for (int i = 0; i < 2; i++) {
+ LoopCondBindS s;
+ consume_loop_cond_bind(cond ? s.get() : nullptr); // no-warning
+ }
+ for (int i = 0; i < 2; i++) {
+ int x, y;
+ consume_loop_cond_bind(cond ? &x : &y); // no-warning
+ }
+}
More information about the cfe-commits
mailing list