[clang] 0f032f1 - [LifetimeSafety] Fix duplicate loan generation for ImplicitCastExpr (#153661)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 3 07:31:02 PDT 2025
Author: Utkarsh Saxena
Date: 2025-09-03T16:30:58+02:00
New Revision: 0f032f1925389394802a86318ebc51b5e83f3b97
URL: https://github.com/llvm/llvm-project/commit/0f032f1925389394802a86318ebc51b5e83f3b97
DIFF: https://github.com/llvm/llvm-project/commit/0f032f1925389394802a86318ebc51b5e83f3b97.diff
LOG: [LifetimeSafety] Fix duplicate loan generation for ImplicitCastExpr (#153661)
This PR fixes a bug in the lifetime safety analysis where `ImplicitCastExpr` nodes were causing duplicate loan generation. The changes:
1. Remove the recursive `Visit(ICE->getSubExpr())` call in `VisitImplicitCastExpr` to prevent duplicate processing of the same expression
2. Ensure the CFG build options are properly configured for lifetime safety analysis by moving the flag check earlier
3. Enhance the unit test infrastructure to properly handle multiple loans per variable
4. Add a test case that verifies implicit casts to const don't create duplicate loans
5. Add a test case for ternary operators with a FIXME note about origin propagation
These changes prevent the analysis from generating duplicate loans when expressions are wrapped in implicit casts, which improves the accuracy of the lifetime safety analysis.
Added:
Modified:
clang/lib/Analysis/LifetimeSafety.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/Sema/warn-lifetime-safety-dataflow.cpp
clang/unittests/Analysis/LifetimeSafetyTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/LifetimeSafety.cpp b/clang/lib/Analysis/LifetimeSafety.cpp
index c2e6dd74d0758..3c9ac9c48b2e5 100644
--- a/clang/lib/Analysis/LifetimeSafety.cpp
+++ b/clang/lib/Analysis/LifetimeSafety.cpp
@@ -445,7 +445,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
void VisitImplicitCastExpr(const ImplicitCastExpr *ICE) {
if (!hasOrigin(ICE->getType()))
return;
- Visit(ICE->getSubExpr());
// An ImplicitCastExpr node itself gets an origin, which flows from the
// origin of its sub-expression (after stripping its own parens/casts).
// TODO: Consider if this is actually useful in practice. Alternatively, we
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 0b94b1044f072..1b66d83df5171 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2905,6 +2905,8 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
AC.getCFGBuildOptions().AddCXXNewAllocator = false;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ bool EnableLifetimeSafetyAnalysis = S.getLangOpts().EnableLifetimeSafety;
+
// Force that certain expressions appear as CFGElements in the CFG. This
// is used to speed up various analyses.
// FIXME: This isn't the right factoring. This is here for initial
@@ -2912,11 +2914,10 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
// expect to always be CFGElements and then fill in the BuildOptions
// appropriately. This is essentially a layering violation.
if (P.enableCheckUnreachable || P.enableThreadSafetyAnalysis ||
- P.enableConsumedAnalysis) {
+ P.enableConsumedAnalysis || EnableLifetimeSafetyAnalysis) {
// Unreachable code analysis and thread safety require a linearized CFG.
AC.getCFGBuildOptions().setAllAlwaysAdd();
- }
- else {
+ } else {
AC.getCFGBuildOptions()
.setAlwaysAdd(Stmt::BinaryOperatorClass)
.setAlwaysAdd(Stmt::CompoundAssignOperatorClass)
@@ -2927,7 +2928,6 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
.setAlwaysAdd(Stmt::UnaryOperatorClass);
}
- bool EnableLifetimeSafetyAnalysis = S.getLangOpts().EnableLifetimeSafety;
// Install the logical handler.
std::optional<LogicalErrorHandler> LEH;
if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
diff --git a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
index bcde9adf25ca5..bc7e0127e5d4c 100644
--- a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
+++ b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
@@ -293,3 +293,22 @@ void pointer_indirection() {
int *q = *pp;
// CHECK: AssignOrigin (Dest: {{[0-9]+}} (Decl: q), Src: {{[0-9]+}} (Expr: ImplicitCastExpr))
}
+
+// CHECK-LABEL: Function: ternary_operator
+// FIXME: Propagate origins across ConditionalOperator.
+void ternary_operator() {
+ int a, b;
+ int *p;
+ p = (a > b) ? &a : &b;
+ // CHECK: Block B2:
+ // CHECK: Issue (LoanID: [[L_A:[0-9]+]], ToOrigin: [[O_ADDR_A:[0-9]+]] (Expr: UnaryOperator))
+ // CHECK: End of Block
+
+ // CHECK: Block B3:
+ // CHECK: Issue (LoanID: [[L_B:[0-9]+]], ToOrigin: [[O_ADDR_A:[0-9]+]] (Expr: UnaryOperator))
+ // CHECK: End of Block
+
+ // CHECK: Block B1:
+ // CHECK: AssignOrigin (Dest: [[O_P:[0-9]+]] (Decl: p), Src: {{[0-9]+}} (Expr: ConditionalOperator))
+ // CHECK: End of Block
+}
diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
index c8d88b4ea2277..13e5832d70050 100644
--- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp
+++ b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
@@ -11,6 +11,7 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Testing/TestAST.h"
#include "llvm/ADT/StringMap.h"
+#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <optional>
@@ -20,6 +21,7 @@ namespace clang::lifetimes::internal {
namespace {
using namespace ast_matchers;
+using ::testing::SizeIs;
using ::testing::UnorderedElementsAreArray;
// A helper class to run the full lifetime analysis on a piece of code
@@ -96,21 +98,18 @@ class LifetimeTestHelper {
return OID;
}
- std::optional<LoanID> getLoanForVar(llvm::StringRef VarName) {
+ std::vector<LoanID> getLoansForVar(llvm::StringRef VarName) {
auto *VD = findDecl<VarDecl>(VarName);
- if (!VD)
- return std::nullopt;
+ if (!VD) {
+ ADD_FAILURE() << "No VarDecl found for '" << VarName << "'";
+ return {};
+ }
std::vector<LoanID> LID = Analysis.getLoanIDForVar(VD);
if (LID.empty()) {
ADD_FAILURE() << "Loan for '" << VarName << "' not found.";
- return std::nullopt;
- }
- // TODO: Support retrieving more than one loans to a var.
- if (LID.size() > 1) {
- ADD_FAILURE() << "More than 1 loans found for '" << VarName;
- return std::nullopt;
+ return {};
}
- return LID[0];
+ return LID;
}
std::optional<LoanSet> getLoansAtPoint(OriginID OID,
@@ -121,13 +120,12 @@ class LifetimeTestHelper {
return Analysis.getLoansAtPoint(OID, PP);
}
- std::optional<llvm::DenseSet<LoanID>>
+ std::optional<std::vector<LoanID>>
getExpiredLoansAtPoint(llvm::StringRef Annotation) {
ProgramPoint PP = Runner.getProgramPoint(Annotation);
if (!PP)
return std::nullopt;
- auto Expired = Analysis.getExpiredLoansAtPoint(PP);
- return llvm::DenseSet<LoanID>{Expired.begin(), Expired.end()};
+ return Analysis.getExpiredLoansAtPoint(PP);
}
private:
@@ -197,12 +195,13 @@ MATCHER_P2(HasLoansToImpl, LoanVars, Annotation, "") {
std::vector<LoanID> ExpectedLoans;
for (const auto &LoanVar : LoanVars) {
- std::optional<LoanID> ExpectedLIDOpt = Info.Helper.getLoanForVar(LoanVar);
- if (!ExpectedLIDOpt) {
+ std::vector<LoanID> ExpectedLIDs = Info.Helper.getLoansForVar(LoanVar);
+ if (ExpectedLIDs.empty()) {
*result_listener << "could not find loan for var '" << LoanVar << "'";
return false;
}
- ExpectedLoans.push_back(*ExpectedLIDOpt);
+ ExpectedLoans.insert(ExpectedLoans.end(), ExpectedLIDs.begin(),
+ ExpectedLIDs.end());
}
return ExplainMatchResult(UnorderedElementsAreArray(ExpectedLoans),
@@ -221,17 +220,17 @@ MATCHER_P(AreExpiredAt, Annotation, "") {
<< Annotation << "'";
return false;
}
- std::vector<LoanID> ActualExpiredLoans(ActualExpiredSetOpt->begin(),
- ActualExpiredSetOpt->end());
+ std::vector<LoanID> ActualExpiredLoans = *ActualExpiredSetOpt;
std::vector<LoanID> ExpectedExpiredLoans;
for (const auto &VarName : Info.LoanVars) {
- auto LoanIDOpt = Helper.getLoanForVar(VarName);
- if (!LoanIDOpt) {
+ auto LoanIDs = Helper.getLoansForVar(VarName);
+ if (LoanIDs.empty()) {
*result_listener << "could not find a loan for variable '" << VarName
<< "'";
return false;
}
- ExpectedExpiredLoans.push_back(*LoanIDOpt);
+ ExpectedExpiredLoans.insert(ExpectedExpiredLoans.end(), LoanIDs.begin(),
+ LoanIDs.end());
}
return ExplainMatchResult(UnorderedElementsAreArray(ExpectedExpiredLoans),
ActualExpiredLoans, result_listener);
@@ -730,5 +729,17 @@ TEST_F(LifetimeAnalysisTest, ReassignedPointerThenOriginalExpires) {
EXPECT_THAT(LoansTo({"s1", "s2"}), AreExpiredAt("p_after_s1_expires"));
}
+TEST_F(LifetimeAnalysisTest, NoDuplicateLoansForImplicitCastToConst) {
+ SetupTest(R"(
+ void target() {
+ MyObj a;
+ const MyObj* p = &a;
+ const MyObj* q = &a;
+ POINT(at_end);
+ }
+ )");
+ EXPECT_THAT(Helper->getLoansForVar("a"), SizeIs(2));
+}
+
} // anonymous namespace
} // namespace clang::lifetimes::internal
More information about the cfe-commits
mailing list