[clang] Refine the temporay object member access filtering for GSL pointer

[hokein]

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.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

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 --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

[usx95]

Thanks. LGTM. Left one comment.

[hokein]

Discovered a new false positive, similar to #114213,

struct Bar {};

struct Foo {
   std::unique_ptr<Bar> bar;
};

struct Test {
  Test(Foo foo) : bar(foo.bar.get()), // bogus diagnostic: initializing pointer member 'bar' with the stack address of parameter 'foo'
      storage(std::move(foo.bar)) {};

  Bar* bar;
  std::unique_ptr<Bar> storage;
};

[hokein]

Discovered a new false positive, similar to #114213,

struct Bar {};

struct Foo {
   std::unique_ptr<Bar> bar;
};

struct Test {
  Test(Foo foo) : bar(foo.bar.get()), // bogus diagnostic: initializing pointer member 'bar' with the stack address of parameter 'foo'
      storage(std::move(foo.bar)) {};

  Bar* bar;
  std::unique_ptr<Bar> storage;
};

Fixed in the latest commit (3276f4e). Please take another look @usx95, @Xazax-hun

[Xazax-hun]

Thanks for fixing this! Looks good to me with the latest changes.