[clang] [clang] Refine the temporay object member access filtering for GSL pointer (PR #122088)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

<details>
<summary>Changes</summary>

We currently have ad-hoc filtering logic for temporary object member access in `VisitGSLPointerArg`. This logic filters out more cases than it should, leading to false negatives. Furthermore, this location lacks sufficient context to implement a more accurate solution.  

This patch refines the filtering logic by moving it to the central filtering location, `analyzePathForGSLPointer`, consolidating the logic and avoiding scattered filtering across multiple places. As a result, the special handling for conditional operators (#<!-- -->120233) is no longer necessary.  

This change also resolves #<!-- -->120543.  



---
Full diff: https://github.com/llvm/llvm-project/pull/122088.diff


3 Files Affected:

- (modified) clang/lib/Sema/CheckExprLifetime.cpp (+29-13) 
- (modified) clang/test/Sema/Inputs/lifetime-analysis.h (+1-1) 
- (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+28) 


``````````diff
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 7109de03cadd12..d0d05e4ed980d5 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -200,6 +200,7 @@ struct IndirectLocalPathEntry {
     LifetimeBoundCall,
     TemporaryCopy,
     LambdaCaptureInit,
+    MemberExpr,
     GslReferenceInit,
     GslPointerInit,
     GslPointerAssignment,
@@ -578,19 +579,6 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
     Path.pop_back();
   };
   auto VisitGSLPointerArg = [&](const FunctionDecl *Callee, Expr *Arg) {
-    // We are not interested in the temporary base objects of gsl Pointers:
-    //   Temp().ptr; // Here ptr might not dangle.
-    if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
-      return;
-    // Avoid false positives when the object is constructed from a conditional
-    // operator argument. A common case is:
-    //   // 'ptr' might not be owned by the Owner object.
-    //   std::string_view s = cond() ? Owner().ptr : sv;
-    if (const auto *Cond =
-            dyn_cast<AbstractConditionalOperator>(Arg->IgnoreImpCasts());
-        Cond && isPointerLikeType(Cond->getType()))
-      return;
-
     auto ReturnType = Callee->getReturnType();
 
     // Once we initialized a value with a non gsl-owner reference, it can no
@@ -710,6 +698,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
         Init = ILE->getInit(0);
     }
 
+    if (MemberExpr *ME = dyn_cast<MemberExpr>(Init->IgnoreImpCasts()))
+      Path.push_back(
+          {IndirectLocalPathEntry::MemberExpr, ME, ME->getMemberDecl()});
     // Step over any subobject adjustments; we may have a materialized
     // temporary inside them.
     Init = const_cast<Expr *>(Init->skipRValueSubobjectAdjustments());
@@ -1103,6 +1094,8 @@ shouldLifetimeExtendThroughPath(const IndirectLocalPath &Path) {
   for (auto Elem : Path) {
     if (Elem.Kind == IndirectLocalPathEntry::DefaultInit)
       return PathLifetimeKind::Extend;
+    if (Elem.Kind == IndirectLocalPathEntry::MemberExpr)
+      continue;
     if (Elem.Kind != IndirectLocalPathEntry::LambdaCaptureInit)
       return PathLifetimeKind::NoExtend;
   }
@@ -1122,6 +1115,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
     case IndirectLocalPathEntry::GslPointerInit:
     case IndirectLocalPathEntry::GslPointerAssignment:
     case IndirectLocalPathEntry::ParenAggInit:
+    case IndirectLocalPathEntry::MemberExpr:
       // These exist primarily to mark the path as not permitting or
       // supporting lifetime extension.
       break;
@@ -1151,6 +1145,7 @@ static bool pathOnlyHandlesGslPointer(const IndirectLocalPath &Path) {
     case IndirectLocalPathEntry::VarInit:
     case IndirectLocalPathEntry::AddressOf:
     case IndirectLocalPathEntry::LifetimeBoundCall:
+    case IndirectLocalPathEntry::MemberExpr:
       continue;
     case IndirectLocalPathEntry::GslPointerInit:
     case IndirectLocalPathEntry::GslReferenceInit:
@@ -1184,6 +1179,26 @@ static AnalysisResult analyzePathForGSLPointer(const IndirectLocalPath &Path,
   // At this point, Path represents a series of operations involving a
   // GSLPointer, either in the process of initialization or assignment.
 
+  // Process  temporary base objects for MemberExpr cases, e.g. Temp().field.
+  for (const auto &E : Path) {
+    if (E.Kind == IndirectLocalPathEntry::MemberExpr) {
+      // Avoid interfering  with the local base object.
+      if (pathContainsInit(Path))
+        return Abandon;
+
+      // We are not interested in the temporary base objects of gsl Pointers:
+      //   auto p1 = Temp().ptr; // Here p1 might not dangle.
+      // However, we want to diagnose for gsl owner fields:
+      //   auto p2 = Temp().owner; // Here p2 is dangling.
+      if (const auto *FD = llvm::dyn_cast_or_null<FieldDecl>(E.D);
+          FD && !FD->getType()->isReferenceType() &&
+          isRecordWithAttr<OwnerAttr>(FD->getType())) {
+        return Report;
+      }
+      return Abandon;
+    }
+  }
+
   // Note: A LifetimeBoundCall can appear interleaved in this sequence.
   // For example:
   //    const std::string& Ref(const std::string& a [[clang::lifetimebound]]);
@@ -1510,6 +1525,7 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
 
       case IndirectLocalPathEntry::LifetimeBoundCall:
       case IndirectLocalPathEntry::TemporaryCopy:
+      case IndirectLocalPathEntry::MemberExpr:
       case IndirectLocalPathEntry::GslPointerInit:
       case IndirectLocalPathEntry::GslReferenceInit:
       case IndirectLocalPathEntry::GslPointerAssignment:
diff --git a/clang/test/Sema/Inputs/lifetime-analysis.h b/clang/test/Sema/Inputs/lifetime-analysis.h
index f888e6ab94bb6a..d318033ff0cc45 100644
--- a/clang/test/Sema/Inputs/lifetime-analysis.h
+++ b/clang/test/Sema/Inputs/lifetime-analysis.h
@@ -52,7 +52,7 @@ struct vector {
 
   void push_back(const T&);
   void push_back(T&&);
-  
+  const T& back() const;
   void insert(iterator, T&&);
 };
 
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 4c19367bb7f3dd..3b271af9dfa13f 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -806,3 +806,31 @@ std::string_view test2(int c, std::string_view sv) {
 }
 
 } // namespace GH120206
+
+namespace GH120543 {
+struct S {
+  std::string_view sv;
+  std::string s;
+};
+struct Q {
+  const S* get() const [[clang::lifetimebound]];
+};
+void test1() {
+  std::string_view k1 = S().sv; // OK
+  std::string_view k2 = S().s; // expected-warning {{object backing the pointer will}}
+  
+  std::string_view k3 = Q().get()->sv; // OK
+  std::string_view k4  = Q().get()->s; // expected-warning {{object backing the pointer will}}
+}
+
+struct Bar {};
+struct Foo {
+  std::vector<Bar> v;
+};
+Foo getFoo();
+void test2() {
+  const Foo& foo = getFoo();
+  const Bar& bar = foo.v.back(); // OK
+}
+
+} // namespace GH120543

``````````

</details>


https://github.com/llvm/llvm-project/pull/122088