[LifetimeSafety] Add support for tracking non-trivially destructed temporary objects #172007
+143
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang-temporal-safety Author: Abhinav Pradeep (AbhinavPradeep) ChangesAdd support for tracking loans to temporary materializations that require non-trivial destructors This small PR introduces the following changes:
Addresses: #152514 Full diff: https://github.com/llvm/llvm-project/pull/172007.diff 6 Files Affected:
diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
index a1acd8615afdd..cccb25bd41037 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
@@ -52,6 +52,8 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> {
private:
void handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds);
+ void handleTemporaryDtor(const CFGTemporaryDtor &TemporaryDtor);
+
void handleGSLPointerConstruction(const CXXConstructExpr *CCE);
/// Checks if a call-like expression creates a borrow by passing a value to a
diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h
index e9bccd4773622..96b34e52529e3 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h
@@ -15,6 +15,7 @@
#define LLVM_CLANG_ANALYSIS_ANALYSES_LIFETIMESAFETY_LOANS_H
#include "clang/AST/Decl.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/Analysis/Analyses/LifetimeSafety/Utils.h"
#include "llvm/Support/raw_ostream.h"
@@ -29,9 +30,25 @@ inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, LoanID ID) {
/// variable.
/// TODO: Model access paths of other types, e.g., s.field, heap and globals.
struct AccessPath {
- const clang::ValueDecl *D;
+ // Currently, an access path can be:
+ // - ValueDecl * , to represent the storage location corresponding to the
+ // variable declared in ValueDecl.
+ // - CXXBindTemporaryExpr * , to represent the storage location of the
+ // temporary object used for the binding in CXXBindTemporaryExpr.
+ const llvm::PointerUnion<const clang::ValueDecl *,
+ const clang::CXXBindTemporaryExpr *>
+ P;
+
+ AccessPath(const clang::ValueDecl *D) : P(D) {}
+ AccessPath(const clang::CXXBindTemporaryExpr *BTE) : P(BTE) {}
+
+ const clang::ValueDecl *getAsValueDecl() const {
+ return P.dyn_cast<const clang::ValueDecl *>();
+ }
- AccessPath(const clang::ValueDecl *D) : D(D) {}
+ const clang::CXXBindTemporaryExpr *getAsCXXBindTemporaryExpr() const {
+ return P.dyn_cast<const clang::CXXBindTemporaryExpr *>();
+ }
};
/// An abstract base class for a single "Loan" which represents lending a
diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
index 2f270b03996f2..ba75bdec3e6f9 100644
--- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
@@ -41,6 +41,12 @@ static const PathLoan *createLoan(FactManager &FactMgr,
return nullptr;
}
+static const PathLoan *createLoan(FactManager &FactMgr,
+ const CXXBindTemporaryExpr *BTE) {
+ AccessPath Path(BTE);
+ return FactMgr.getLoanMgr().createLoan<PathLoan>(Path, BTE);
+}
+
void FactsGenerator::run() {
llvm::TimeTraceScope TimeProfile("FactGenerator");
const CFG &Cfg = *AC.getCFG();
@@ -60,6 +66,9 @@ void FactsGenerator::run() {
else if (std::optional<CFGLifetimeEnds> LifetimeEnds =
Element.getAs<CFGLifetimeEnds>())
handleLifetimeEnds(*LifetimeEnds);
+ else if (std::optional<CFGTemporaryDtor> TemporaryDtor =
+ Element.getAs<CFGTemporaryDtor>())
+ handleTemporaryDtor(*TemporaryDtor);
}
CurrentBlockFacts.append(EscapesInCurrentBlock.begin(),
EscapesInCurrentBlock.end());
@@ -218,9 +227,18 @@ void FactsGenerator::VisitMaterializeTemporaryExpr(
const MaterializeTemporaryExpr *MTE) {
if (!hasOrigin(MTE))
return;
- // A temporary object's origin is the same as the origin of the
- // expression that initializes it.
- killAndFlowOrigin(*MTE, *MTE->getSubExpr());
+ const Expr *Child = MTE->getSubExpr()->IgnoreImpCasts();
+ if (const CXXBindTemporaryExpr *BTE =
+ dyn_cast<const CXXBindTemporaryExpr>(Child)) {
+ // Issue a loan to MTE for the storage location represented by BTE
+ const Loan *L = createLoan(FactMgr, BTE);
+ OriginID OID = FactMgr.getOriginMgr().getOrCreate(*MTE);
+ CurrentBlockFacts.push_back(FactMgr.createFact<IssueFact>(L->getID(), OID));
+ } else {
+ // A temporary object's origin is the same as the origin of the
+ // expression that initializes it.
+ killAndFlowOrigin(*MTE, *MTE->getSubExpr());
+ }
}
void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) {
@@ -233,13 +251,35 @@ void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) {
if (const auto *BL = dyn_cast<PathLoan>(Loan)) {
// Check if the loan is for a stack variable and if that variable
// is the one being destructed.
- if (BL->getAccessPath().D == LifetimeEndsVD)
+ const AccessPath AP = BL->getAccessPath();
+ const ValueDecl *Path = AP.getAsValueDecl();
+ if (Path == LifetimeEndsVD)
CurrentBlockFacts.push_back(FactMgr.createFact<ExpireFact>(
BL->getID(), LifetimeEnds.getTriggerStmt()->getEndLoc()));
}
}
}
+void FactsGenerator::handleTemporaryDtor(
+ const CFGTemporaryDtor &TemporaryDtor) {
+ const CXXBindTemporaryExpr *BTE = TemporaryDtor.getBindTemporaryExpr();
+ if (!BTE) {
+ return;
+ }
+ // Iterate through all loans to see if any expire.
+ for (const auto *Loan : FactMgr.getLoanMgr().getLoans()) {
+ if (const auto *BL = dyn_cast<PathLoan>(Loan)) {
+ // Check if the loan is for a temporary materialization and if that storage
+ // location is the one being destructed.
+ const AccessPath AP = BL->getAccessPath();
+ const CXXBindTemporaryExpr *Path = AP.getAsCXXBindTemporaryExpr();
+ if (Path == BTE)
+ CurrentBlockFacts.push_back(FactMgr.createFact<ExpireFact>(
+ BL->getID(), TemporaryDtor.getBindTemporaryExpr()->getEndLoc()));
+ }
+ }
+}
+
void FactsGenerator::handleGSLPointerConstruction(const CXXConstructExpr *CCE) {
assert(isGslPointerType(CCE->getType()));
if (CCE->getNumArgs() != 1)
diff --git a/clang/lib/Analysis/LifetimeSafety/Loans.cpp b/clang/lib/Analysis/LifetimeSafety/Loans.cpp
index fdfdbb40a2a46..5f6ba6809d2fb 100644
--- a/clang/lib/Analysis/LifetimeSafety/Loans.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/Loans.cpp
@@ -12,7 +12,16 @@ namespace clang::lifetimes::internal {
void PathLoan::dump(llvm::raw_ostream &OS) const {
OS << getID() << " (Path: ";
- OS << Path.D->getNameAsString() << ")";
+ if (const clang::ValueDecl *VD = Path.getAsValueDecl()) {
+ OS << VD->getNameAsString();
+ } else if (const clang::CXXBindTemporaryExpr *BTE =
+ Path.getAsCXXBindTemporaryExpr()) {
+ // No nice "name" for the temporary, so deferring to LLVM default
+ OS << "CXXBindTemporaryExpr at " << BTE;
+ } else {
+ llvm_unreachable("access path is not one of any supported types");
+ }
+ OS << ")";
}
void PlaceholderLoan::dump(llvm::raw_ostream &OS) const {
diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp
index 1191469e23df1..635489581e236 100644
--- a/clang/test/Sema/warn-lifetime-safety.cpp
+++ b/clang/test/Sema/warn-lifetime-safety.cpp
@@ -16,6 +16,9 @@ class TriviallyDestructedClass {
View a, b;
};
+MyObj temporary();
+void use(View);
+
//===----------------------------------------------------------------------===//
// Basic Definite Use-After-Free (-W...permissive)
// These are cases where the pointer is guaranteed to be dangling at the use site.
@@ -943,3 +946,10 @@ void parentheses(bool cond) {
} // expected-note 4 {{destroyed here}}
(void)*p; // expected-note 4 {{later used here}}
}
+
+void use_temporary_after_destruction() {
+ View a;
+ a = temporary(); // expected-warning {{object whose reference is captured does not live long enough}} \
+ expected-note {{destroyed here}}
+ use(a); // expected-note {{later used here}}
+}
\ No newline at end of file
diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
index fee4e79e27d03..86e5d662880b6 100644
--- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp
+++ b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
@@ -9,6 +9,7 @@
#include "clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/Analyses/LifetimeSafety/Loans.h"
#include "clang/Testing/TestAST.h"
#include "llvm/ADT/StringMap.h"
#include "gmock/gmock.h"
@@ -116,7 +117,7 @@ class LifetimeTestHelper {
std::vector<LoanID> LID;
for (const Loan *L : Analysis.getFactManager().getLoanMgr().getLoans())
if (const auto *BL = dyn_cast<PathLoan>(L))
- if (BL->getAccessPath().D == VD)
+ if (BL->getAccessPath().getAsValueDecl() == VD)
LID.push_back(L->getID());
if (LID.empty()) {
ADD_FAILURE() << "Loan for '" << VarName << "' not found.";
@@ -125,6 +126,14 @@ class LifetimeTestHelper {
return LID;
}
+ bool isLoanToATemporary(LoanID LID) {
+ const Loan *L = Analysis.getFactManager().getLoanMgr().getLoan(LID);
+ if (const auto *BL = dyn_cast<PathLoan>(L)) {
+ return BL->getAccessPath().getAsCXXBindTemporaryExpr() != nullptr;
+ }
+ return false;
+ }
+
// Gets the set of loans that are live at the given program point. A loan is
// considered live at point P if there is a live origin which contains this
// loan.
@@ -402,6 +411,35 @@ MATCHER_P(AreLiveAt, Annotation, "") {
arg, result_listener);
}
+MATCHER_P(HasLoanToATemporary, Annotation, "") {
+ const OriginInfo &Info = arg;
+ auto &Helper = Info.Helper;
+ std::optional<OriginID> OIDOpt = Helper.getOriginForDecl(Info.OriginVar);
+ if (!OIDOpt) {
+ *result_listener << "could not find origin for '" << Info.OriginVar.str()
+ << "'";
+ return false;
+ }
+
+ std::optional<LoanSet> LoansSetOpt =
+ Helper.getLoansAtPoint(*OIDOpt, Annotation);
+ if (!LoansSetOpt) {
+ *result_listener << "could not get a valid loan set at point '"
+ << Annotation << "'";
+ return false;
+ }
+
+ std::vector<LoanID> Loans(LoansSetOpt->begin(), LoansSetOpt->end());
+
+ for (LoanID LID : Loans) {
+ if (Helper.isLoanToATemporary(LID))
+ return true;
+ }
+ *result_listener << "could not find loan to a temporary for '"
+ << Info.OriginVar.str() << "'";
+ return false;
+}
+
// Base test fixture to manage the runner and helper.
class LifetimeAnalysisTest : public ::testing::Test {
protected:
@@ -807,12 +845,15 @@ TEST_F(LifetimeAnalysisTest, ExtraParenthesis) {
TEST_F(LifetimeAnalysisTest, ViewFromTemporary) {
SetupTest(R"(
MyObj temporary();
+ void use(View);
void target() {
- View v = temporary();
- POINT(p1);
+ View a;
+ a = temporary();
+ POINT(p1);
+ use(a);
}
)");
- EXPECT_THAT(Origin("v"), HasLoansTo({}, "p1"));
+ EXPECT_THAT(Origin("a"), HasLoanToATemporary("p1"));
}
TEST_F(LifetimeAnalysisTest, GslPointerWithConstAndAuto) {
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp -- clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp clang/lib/Analysis/LifetimeSafety/Loans.cpp clang/test/Sema/warn-lifetime-safety.cpp clang/unittests/Analysis/LifetimeSafetyTest.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
index ba75bdec3..71910c962 100644
--- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
@@ -269,8 +269,8 @@ void FactsGenerator::handleTemporaryDtor(
// Iterate through all loans to see if any expire.
for (const auto *Loan : FactMgr.getLoanMgr().getLoans()) {
if (const auto *BL = dyn_cast<PathLoan>(Loan)) {
- // Check if the loan is for a temporary materialization and if that storage
- // location is the one being destructed.
+ // Check if the loan is for a temporary materialization and if that
+ // storage location is the one being destructed.
const AccessPath AP = BL->getAccessPath();
const CXXBindTemporaryExpr *Path = AP.getAsCXXBindTemporaryExpr();
if (Path == BTE)
|
usx95
changed the title
usx95
changed the title
Xazax-hun
reviewed
Dec 12, 2025
Collaborator
Xazax-hun
left a comment
Xazax-hun
left a comment
There was a problem hiding this comment.
Overall looks great to me, thanks! I have one question inline.
| // Currently, an access path can be: | ||
| // - ValueDecl * , to represent the storage location corresponding to the | ||
| // variable declared in ValueDecl. | ||
| // - CXXBindTemporaryExpr * , to represent the storage location of the |
Collaborator
There was a problem hiding this comment.
I wonder if it is better to store the MaterializeTemporaryExpr as we do not have CXXBindTemporaryExprs for trivial types and MaterializeTemporaryExpr is the point when we actually start to have a memory address for the value.
| a = temporary(); // expected-warning {{object whose reference is captured does not live long enough}} \ | ||
| expected-note {{destroyed here}} | ||
| use(a); // expected-note {{later used here}} | ||
| } |
Collaborator
There was a problem hiding this comment.
Nit: missing new line at the end of file.
Contributor
You could use this syntax. |
usx95
reviewed
Dec 14, 2025
AbhinavPradeep
changed the title
Xazax-hun
reviewed
Dec 15, 2025
Collaborator
Xazax-hun
left a comment
Xazax-hun
left a comment
There was a problem hiding this comment.
We talked about the future directions over discord, I am happy with this as an intermediate step in that direction.
| // A temporary object's origin is the same as the origin of the | ||
| // expression that initializes it. | ||
| killAndFlowOrigin(*MTE, *MTE->getSubExpr()); | ||
| if (getChildBinding(MTE)) { |
Collaborator
There was a problem hiding this comment.
You mentioned lifetime extended temporaries offline. Is that reflected in the current iteration of the patch or do you need to update this?
Author
There was a problem hiding this comment.
That is not reflected here, it will be in future iterations. For the future iterations, I believe my next PR would be in handling the case of non lifetime-extended general temporaries. I believe @usx95 suggested that I should choose to not track the MTE if I detect it is lifetime extended?
Collaborator
There was a problem hiding this comment.
In case this is not too hard to do, I'd prefer that bit to be included in this PR.
Author
There was a problem hiding this comment.
Makes sense will include it!
usx95
reviewed
Dec 15, 2025
Contributor
usx95
left a comment
usx95
left a comment
There was a problem hiding this comment.
Overall LGTM.
One more followup I would request is that we have a separate warning for dangling references to temporaries. We do not require "destroyed here". More helpful wording could be "temporary whose reference is captured is destroyed at the end of full expression". This can be done in a separate PR.
| } | ||
|
|
||
| /// Creates a loan for the storage location of a temporary object. | ||
| /// \param MTW The MaterializeTemporaryExpr that represents the temporary |
| // A temporary object's origin is the same as the origin of the | ||
| // expression that initializes it. | ||
| killAndFlowOrigin(*MTE, *MTE->getSubExpr()); | ||
| if (getChildBinding(MTE)) { |
Contributor
There was a problem hiding this comment.
I would use !isPointerType(MTE->getType()) instead of this to correctly reflect the issue with l-valued pointer type expr. For other MTE, we would have a loan issued but no corresponding expire because of no BTE child.
Comment on lines
+276
to
+278
| if (!BTE) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
nit: remove the curly braces in a single-statement if body
| } | ||
| // Iterate through all loans to see if any expire. | ||
| for (const auto *Loan : FactMgr.getLoanMgr().getLoans()) { | ||
| if (const auto *BL = dyn_cast<PathLoan>(Loan)) { |
Contributor
There was a problem hiding this comment.
nit: rename to PL to denote PathLoan
|
|
||
| void FactsGenerator::handleTemporaryDtor( | ||
| const CFGTemporaryDtor &TemporaryDtor) { | ||
| const CXXBindTemporaryExpr *BTE = TemporaryDtor.getBindTemporaryExpr(); |
Comment on lines
+15
to
+24
| if (const clang::ValueDecl *VD = Path.getAsValueDecl()) { | ||
| OS << VD->getNameAsString(); | ||
| } else if (const clang::MaterializeTemporaryExpr *MTE = | ||
| Path.getAsMaterializeTemporaryExpr()) { | ||
| // No nice "name" for the temporary, so deferring to LLVM default | ||
| OS << "MaterializeTemporaryExpr at " << MTE; | ||
| } else { | ||
| llvm_unreachable("access path is not one of any supported types"); | ||
| } | ||
| OS << ")"; |
Contributor
There was a problem hiding this comment.
nit: remove the {} for single-stmt bodies
usx95
reviewed
Dec 15, 2025
Comment on lines
+950
to
+951
| void use_temporary_after_destruction() { | ||
| View a; |
Contributor
There was a problem hiding this comment.
Could you try adding more tests. Like passing a temporary to a function with lifetimebound arg. Please also add tests which do not work at the moment with a FIXME (trivial temporaries, nested BTE).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for tracking loans to temporary materializations that require non-trivial destructors. We only support non-trivially destructed temporaries as they have a nice end-of-life marker via the
CFGTemporaryDtor.This small PR introduces the following changes:
MaterializeTemporaryExpr *viallvm::PointerUnionFactsGenerator::VisitMaterializeTemporaryExprnow checks to see if the temporary materialization is such that it requires a non-trivial destructor (by checking for a childCXXBindTemporaryExprnode when all implicit casts are stripped away), and if so: creates a Loan whose AccessPath is a pointer to thatMaterializeTemporaryExpr, and issues it to the origin represented by theMaterializeTemporaryExprnode we were called on. When we cannot find a childCXXBindTemporaryExpr, we fall-back to anOriginFlowas before.FactsGenerator::handleTemporaryDtoris called fromFactsGenerator::runwhen it encounters aCFGTemporaryDtor. It then issues anExpireFactfor loan to the corresponding destructed temporary by matching onCXXBindTemporaryExpr *HasLoanToATemporarywhich can check if an origin has a loan to at least 1 temporary. This is done as we are not nicely and reproducibly able to uniquely identify a loan withAccessPathMaterializeTemporaryExpr *, so we settle for checking the presence of at least a loan to a temporary.Addresses: #152514