llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-temporal-safety Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> Previously, the `transfer` function for `OriginFlowFact` only handled killing the destination origin. It did not propagate liveness backwards from destination to source, meaning that if an origin flowed into another, the source was not marked live even when the destination was. The `transfer` function now propagates liveness from destination to source: if the destination origin is live, the source origin is also marked live with the same `LivenessInfo`, before optionally killing the destination. Additionally, `handleMovedArgsInCall` now skips rvalue reference parameters annotated with `[[clang::lifetimebound]]`, since such parameters should not be treated as moved-from. This introduces some false positives for invalidations as container modifications are not considered self-invalidating. The updated test expectations reflect this, along with `FIXME` comments explaining that they are false positives stemming from the lack of distinction between owner-borrows and content-borrows, which will require more precise `AccessPath` reasoning to resolve. --- Full diff: https://github.com/llvm/llvm-project/pull/205323.diff 3 Files Affected: - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+5) - (modified) clang/lib/Analysis/LifetimeSafety/LiveOrigins.cpp (+14-3) - (modified) clang/test/Sema/LifetimeSafety/invalidations.cpp (+23-6) ``````````diff diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 3861117005752..238b25824a4ce 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -887,6 +887,11 @@ void FactsGenerator::handleMovedArgsInCall(const FunctionDecl *FD, const ParmVarDecl *PVD = FD->getParamDecl(I - IsInstance); if (!PVD->getType()->isRValueReferenceType()) continue; + // Skip lifetimebound r-value reference parameters. Lifetimebound 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>()) + 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 `````````` </details> https://github.com/llvm/llvm-project/pull/205323 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
