llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Gábor Horváth (Xazax-hun) <details> <summary>Changes</summary> FactsGenerator only handled the ternary, so a borrow used through the GNU binary conditional `a ?: b` was silently dropped. Handle both via VisitAbstractConditionalOperator, flowing from getTrueExpr()/getFalseExpr(). For `a ?: b` getTrueExpr() is an OpaqueValueExpr, so make OpaqueValueExpr transparent in the origin manager and peel it in the arm-reachability check; guard against flowing a void (e.g. throw) arm. Assisted-by: Claude Opus 4.8 --- Full diff: https://github.com/llvm/llvm-project/pull/204439.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h (+1-1) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+18-8) - (modified) clang/lib/Analysis/LifetimeSafety/Origins.cpp (+6) - (added) clang/test/Sema/LifetimeSafety/binary-conditional-throw.cpp (+19) - (modified) clang/test/Sema/LifetimeSafety/safety.cpp (+82) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h index 2c961bd305fac..fed6531aa9097 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h @@ -45,7 +45,7 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> { void VisitUnaryOperator(const UnaryOperator *UO); void VisitReturnStmt(const ReturnStmt *RS); void VisitBinaryOperator(const BinaryOperator *BO); - void VisitConditionalOperator(const ConditionalOperator *CO); + void VisitAbstractConditionalOperator(const AbstractConditionalOperator *CO); void VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE); void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *FCE); void VisitInitListExpr(const InitListExpr *ILE); diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index be0577b0f3f8f..8fa80c729d111 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -460,10 +460,13 @@ void FactsGenerator::VisitBinaryOperator(const BinaryOperator *BO) { // TODO: Handle assignments involving dereference like `*p = q`. } -void FactsGenerator::VisitConditionalOperator(const ConditionalOperator *CO) { +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(); @@ -478,13 +481,17 @@ void FactsGenerator::VisitConditionalOperator(const ConditionalOperator *CO) { case 0: return; case 1: { - TBHasEdge = llvm::any_of(**Preds.begin(), - [ExpectedStmt = TrueExpr->IgnoreParenImpCasts()]( - const CFGElement &Elt) { - if (auto CS = Elt.getAs<CFGStmt>()) - return CS->getStmt() == ExpectedStmt; - return false; - }); + // 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; } @@ -501,6 +508,9 @@ void FactsGenerator::VisitConditionalOperator(const ConditionalOperator *CO) { bool FirstFlow = true; auto HandleFlow = [&](const Expr *E) { + // An arm with no origins (e.g. a `throw`/void arm) carries no loans. + if (!getOriginsList(*E)) + return; if (FirstFlow) { killAndFlowOrigin(*CO, *E); FirstFlow = false; diff --git a/clang/lib/Analysis/LifetimeSafety/Origins.cpp b/clang/lib/Analysis/LifetimeSafety/Origins.cpp index 55d3b36e3163a..460a04b3b1494 100644 --- a/clang/lib/Analysis/LifetimeSafety/Origins.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Origins.cpp @@ -222,6 +222,12 @@ OriginList *OriginManager::getOrCreateList(const Expr *E) { if (const ExprWithCleanups *EWC = dyn_cast<ExprWithCleanups>(E)) return getOrCreateList(EWC->getSubExpr()); + // An OpaqueValueExpr is a placeholder for an already-evaluated subexpression + // (e.g. the common operand of `a ?: b`); reuse its source's origins. + if (const auto *OVE = dyn_cast<OpaqueValueExpr>(E)) + if (const Expr *Src = OVE->getSourceExpr()) + return getOrCreateList(Src); + if (!hasOrigins(E)) return nullptr; diff --git a/clang/test/Sema/LifetimeSafety/binary-conditional-throw.cpp b/clang/test/Sema/LifetimeSafety/binary-conditional-throw.cpp new file mode 100644 index 0000000000000..4b15f13b3b0e3 --- /dev/null +++ b/clang/test/Sema/LifetimeSafety/binary-conditional-throw.cpp @@ -0,0 +1,19 @@ +// RUN: %clang_cc1 -fsyntax-only -Wlifetime-safety -Wno-dangling -fcxx-exceptions -verify %s + +// A `throw` arm of a GNU binary conditional `a ?: b` collapses the CFG to a +// single predecessor and carries no origins; flowing it must not crash, and the +// common value's loans must still flow. + +int *select_throw(int *in) { + return in ?: throw 1; // no-warning (in is caller storage) +} + +void void_context(int *in) { + in ?: throw 1; // no-warning +} + +int *dangling_common() { + int x; + int *p = &x; // expected-warning {{stack memory associated with local variable 'x' is returned}} + return p ?: throw 1; // expected-note {{returned here}} +} diff --git a/clang/test/Sema/LifetimeSafety/safety.cpp b/clang/test/Sema/LifetimeSafety/safety.cpp index 3c954cebda820..619cae086a25f 100644 --- a/clang/test/Sema/LifetimeSafety/safety.cpp +++ b/clang/test/Sema/LifetimeSafety/safety.cpp @@ -1250,6 +1250,88 @@ void conditional_operator_lifetimebound_nested_deep(bool cond) { (void)*p; // expected-note 4 {{later used here}} } +// GNU binary conditional operator `a ?: b`. +void binary_conditional_false_unsafe(MyObj* in) { + MyObj* p; + { + MyObj temp; + p = in ?: &temp; // expected-warning {{local variable 'temp' does not live long enough}} + } // expected-note {{destroyed here}} + (void)*p; // expected-note {{later used here}} +} + +void binary_conditional_common_unsafe(MyObj* fallback) { + MyObj* p; + { + MyObj temp; + MyObj* t = &temp; // expected-warning {{local variable 'temp' does not live long enough}} + p = t ?: fallback; + } // expected-note {{destroyed here}} + (void)*p; // expected-note {{later used here}} +} + +void binary_conditional_safe(MyObj* in) { + MyObj fallback; + MyObj* p = in ?: &fallback; + (void)*p; // no-warning +} + +void binary_conditional_nested(MyObj* a, MyObj* b) { + MyObj* p; + { + MyObj temp; + p = a ?: b ?: &temp; // expected-warning {{local variable 'temp' does not live long enough}} + } // expected-note {{destroyed here}} + (void)*p; // expected-note {{later used here}} +} + +void binary_conditional_masked_by_conditional(bool cond, MyObj* in) { + MyObj safe; + MyObj* keep = &safe; + MyObj* p; + { + MyObj temp; + p = cond ? keep : (in ?: &temp); // expected-warning {{local variable 'temp' does not live long enough}} + } // expected-note {{destroyed here}} + (void)*p; // expected-note {{later used here}} +} + +void binary_conditional_use_after_free(int* in) { + int* h = new int; // expected-warning {{allocated object does not live long enough}} + int* p = in ?: h; + delete h; // expected-note {{freed here}} + (void)*p; // expected-note {{later used here}} +} + +int** binary_conditional_double_ptr(int** in) { + int* local = nullptr; + int** p = in ?: &local; // expected-warning {{stack memory associated with local variable 'local' is returned}} + return p; // expected-note {{returned here}} +} + +// A constexpr operator bool folds the condition and prunes an arm; the surviving +// value must still flow without tripping the origin shape invariant. +struct [[gsl::Pointer]] TrueView { + const int *p; + constexpr TrueView(const int &x [[clang::lifetimebound]]) : p(&x) {} + constexpr explicit operator bool() const { return true; } +}; +struct [[gsl::Pointer]] FalseView { + const int *p; + constexpr FalseView(const int &x [[clang::lifetimebound]]) : p(&x) {} + constexpr explicit operator bool() const { return false; } +}; + +TrueView binary_conditional_folded_true(TrueView fb) { + int local = 0; + return TrueView(local) ?: fb; // expected-warning {{stack memory associated with local variable 'local' is returned}} expected-note {{returned here}} +} + +FalseView binary_conditional_folded_false(FalseView fb) { + int local = 0; + return FalseView(local) ?: fb; // no-warning (result is always fb) +} + // FIXME: Diagnostic output does not handle ParenExpr correctly, causing alias // information to be missed (local variable 'p' aliases the storage of local variable 'b'). void simpleparen() { `````````` </details> https://github.com/llvm/llvm-project/pull/204439 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
