[clang] Fix the post-filtering heuristics for GSLPointer case.

Author: hokein

clang/docs/ReleaseNotes.rst

@@ -464,6 +464,8 @@ Improvements to Clang's diagnostics
 
 - Clang now diagnoses ``[[deprecated]]`` attribute usage on local variables (#GH90073).
 
+- Fix false positives when `[[gsl::Owner/Pointer]]` and `[[clang::lifetimebound]]` are used together.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/lib/Sema/CheckExprLifetime.cpp

@@ -1093,6 +1093,87 @@ static bool pathOnlyHandlesGslPointer(const IndirectLocalPath &Path) {
   }
   return false;
 }
+// Result of analyzing the Path for GSLPointer.
+enum AnalysisResult {
+  // Path does not correspond to a GSLPointer.
+  NotGSLPointer,
+
+  // A relevant case was identified.
+  Report,
+  // Stop the entire traversal.
+  Abandon,
+  // Skip this step and continue traversing inner AST nodes.
+  Skip,
+};
+// Analyze cases where a GSLPointer is initialized or assigned from a
+// temporary owner object.
+static AnalysisResult analyzePathForGSLPointer(const IndirectLocalPath &Path,
+                                               Local L) {
+  if (!pathOnlyHandlesGslPointer(Path))
+    return NotGSLPointer;
+
+  // At this point, Path represents a series of operations involving a
+  // GSLPointer, either in the process of initialization or assignment.
+
+  // Note: A LifetimeBoundCall can appear interleaved in this sequence.
+  // For example:
+  //    const std::string& Ref(const std::string& a [[clang::lifetimebound]]);
+  //    string_view abc = Ref(std::string());
+  // The "Path" is [GSLPointerInit, LifetimeboundCall], where "L" is the
+  // temporary "std::string()" object. We need to check if the function with the
+  // lifetimebound attribute returns a "owner" type.
+  if (Path.back().Kind == IndirectLocalPathEntry::LifetimeBoundCall) {
+    // The lifetimebound applies to the implicit object parameter of a method.
+    if (const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Path.back().D)) {
+      if (Method->getReturnType()->isReferenceType() &&
+          isRecordWithAttr<OwnerAttr>(
+              Method->getReturnType()->getPointeeType()))
+        return Report;
+      return Abandon;
+    }
+    // The lifetimebound applies to a function parameter.
+    const auto *PD = llvm::dyn_cast<ParmVarDecl>(Path.back().D);
+    if (const auto *FD = llvm::dyn_cast<FunctionDecl>(PD->getDeclContext())) {
+      if (isa<CXXConstructorDecl>(FD)) {
+        // Constructor case: the parameter is annotated with lifetimebound
+        //   e.g., GSLPointer(const S& s [[clang::lifetimebound]])
+        // We still respect this case even the type S is not an owner.
+        return Report;
+      }
+      // For regular functions, check if the return type has an Owner attribute.
+      //   e.g., const GSLOwner& func(const Foo& foo [[clang::lifetimebound]])
+      if (FD->getReturnType()->isReferenceType() &&
+          isRecordWithAttr<OwnerAttr>(FD->getReturnType()->getPointeeType()))
+        return Report;
+    }
+    return Abandon;
+  }
+
+  if (isa<DeclRefExpr>(L)) {
+    // We do not want to follow the references when returning a pointer
+    // originating from a local owner to avoid the following false positive:
+    //   int &p = *localUniquePtr;
+    //   someContainer.add(std::move(localUniquePtr));
+    //   return p;
+    if (!pathContainsInit(Path) && isRecordWithAttr<OwnerAttr>(L->getType()))
+      return Report;
+    return Abandon;
+  }
+
+  // The GSLPointer is from a temporary object.
+  auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
+
+  bool IsGslPtrValueFromGslTempOwner =
+      MTE && !MTE->getExtendingDecl() &&
+      isRecordWithAttr<OwnerAttr>(MTE->getType());
+  // Skipping a chain of initializing gsl::Pointer annotated objects.
+  // We are looking only for the final source to find out if it was
+  // a local or temporary owner or the address of a local
+  // variable/param.
+  if (!IsGslPtrValueFromGslTempOwner)
+    return Skip;
+  return Report;
+}
 
 static bool isAssignmentOperatorLifetimeBound(CXXMethodDecl *CMD) {
   if (!CMD)
@@ -1131,27 +1212,17 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
 
     auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
 
-    bool IsGslPtrValueFromGslTempOwner = false;
-    if (pathOnlyHandlesGslPointer(Path)) {
-      if (isa<DeclRefExpr>(L)) {
-        // We do not want to follow the references when returning a pointer
-        // originating from a local owner to avoid the following false positive:
-        //   int &p = *localUniquePtr;
-        //   someContainer.add(std::move(localUniquePtr));
-        //   return p;
-        if (pathContainsInit(Path) ||
-            !isRecordWithAttr<OwnerAttr>(L->getType()))
-          return false;
-      } else {
-        IsGslPtrValueFromGslTempOwner =
-            MTE && !MTE->getExtendingDecl() &&
-            isRecordWithAttr<OwnerAttr>(MTE->getType());
-        // Skipping a chain of initializing gsl::Pointer annotated objects.
-        // We are looking only for the final source to find out if it was
-        // a local or temporary owner or the address of a local variable/param.
-        if (!IsGslPtrValueFromGslTempOwner)
-          return true;
-      }
+    bool IsGslPtrValueFromGslTempOwner = true;
+    switch (analyzePathForGSLPointer(Path, L)) {
+    case Abandon:
+       return false;
+    case Skip:
+       return true;
+    case NotGSLPointer:
+      IsGslPtrValueFromGslTempOwner = false;
+      LLVM_FALLTHROUGH;
+    case Report:
+      break;
     }
 
     switch (LK) {

clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

@@ -727,8 +727,9 @@ struct [[gsl::Pointer]] Span {
 
 // Pointer from Owner<Pointer>
 std::string_view test5() {
-  std::string_view a = StatusOr<std::string_view>().valueLB(); // expected-warning {{object backing the pointer will be dest}}
-  return StatusOr<std::string_view>().valueLB(); // expected-warning {{returning address of local temporary}}
+  // The Owner<Pointer> doesn't own the object which its inner pointer points to.
+  std::string_view a = StatusOr<std::string_view>().valueLB(); // OK
+  return StatusOr<std::string_view>().valueLB(); // OK
 
   // No dangling diagnostics on non-lifetimebound methods.
   std::string_view b = StatusOr<std::string_view>().valueNoLB();
@@ -775,7 +776,7 @@ Span<std::string> test10(StatusOr<std::vector<std::string>> aa) {
 
 // Pointer<Owner>> from Owner<Pointer<Owner>>
 Span<std::string> test11(StatusOr<Span<std::string>> aa) {
-  return aa.valueLB(); // expected-warning {{address of stack memory}}
+  return aa.valueLB(); // OK
   return aa.valueNoLB(); // OK.
 }
 
@@ -793,3 +794,44 @@ void test13() {
 }
 
 } // namespace GH100526
+
+namespace LifetimeboundInterleave {
+
+const std::string& Ref(const std::string& abc [[clang::lifetimebound]]);
+std::string_view test1() {
+  std::string_view t1 = Ref(std::string()); // expected-warning {{object backing}}
+  t1 = Ref(std::string()); // expected-warning {{object backing}}
+  return Ref(std::string()); // expected-warning {{returning address}}
+}
+
+template <typename T>
+struct Foo {
+  const T& get() const [[clang::lifetimebound]];
+  const T& getNoLB() const;
+};
+std::string_view test2(Foo<std::string> r1, Foo<std::string_view> r2) {
+  std::string_view t1 = Foo<std::string>().get(); // expected-warning {{object backing}}
+  t1 = Foo<std::string>().get(); // expected-warning {{object backing}}
+  return r1.get(); // expected-warning {{address of stack}}
+  
+  std::string_view t2 = Foo<std::string_view>().get();
+  t2 = Foo<std::string_view>().get();
+  return r2.get();
+
+  // no warning on no-LB-annotated method.
+  std::string_view t3 = Foo<std::string>().getNoLB(); 
+  t3 = Foo<std::string>().getNoLB(); 
+  return r1.getNoLB(); 
+}
+
+struct Bar {};
+struct [[gsl::Pointer]] Pointer {
+  Pointer(const Bar & bar [[clang::lifetimebound]]);
+};
+Pointer test3(Bar bar) {
+  Pointer p = Pointer(Bar()); // expected-warning {{temporary}}
+  p = Pointer(Bar()); // expected-warning {{object backing}}
+  return bar; // expected-warning {{address of stack}}
+}
+
+} // namespace LifetimeboundInterleave