Reland "[clang] Merge lifetimebound and GSL code paths for lifetime analysis (#104906)"

[hokein]

Reland without the EnableLifetimeWarnings removal. I will remove the EnableLifetimeWarnings in a follow-up patch.

I have added a test to prevent regression.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

Reland without the EnableLifetimeWarnings removal. I will remove the EnableLifetimeWarnings in a follow-up patch.

I have added a test to prevent regression.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+54-72)
• (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+20)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 70ff5dedab217f..a75798ad6bd251 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -231,6 +231,8 @@ Improvements to Clang's diagnostics
 
 - Clang now diagnoses when the result of a [[nodiscard]] function is discarded after being cast in C. Fixes #GH104391.
 
+- Don't emit duplicated dangling diagnostics. (#GH93386).
+
 - Improved diagnostic when trying to befriend a concept. (#GH45182).
 
 Improvements to Clang's time-trace
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 7389046eaddde1..0459ead46a1716 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -326,66 +326,6 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) {
   return false;
 }
 
-static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
-                                    LocalVisitor Visit) {
-  auto VisitPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
-    // 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;
-    // Once we initialized a value with a reference, it can no longer dangle.
-    if (!Value) {
-      for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
-        if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
-          continue;
-        if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
-            PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
-          return;
-        break;
-      }
-    }
-    Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
-                          : IndirectLocalPathEntry::GslReferenceInit,
-                    Arg, D});
-    if (Arg->isGLValue())
-      visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-                                            Visit,
-                                            /*EnableLifetimeWarnings=*/true);
-    else
-      visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
-                                       /*EnableLifetimeWarnings=*/true);
-    Path.pop_back();
-  };
-
-  if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
-    const auto *MD = cast_or_null<CXXMethodDecl>(MCE->getDirectCallee());
-    if (MD && shouldTrackImplicitObjectArg(MD))
-      VisitPointerArg(MD, MCE->getImplicitObjectArgument(),
-                      !MD->getReturnType()->isReferenceType());
-    return;
-  } else if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(Call)) {
-    FunctionDecl *Callee = OCE->getDirectCallee();
-    if (Callee && Callee->isCXXInstanceMember() &&
-        shouldTrackImplicitObjectArg(cast<CXXMethodDecl>(Callee)))
-      VisitPointerArg(Callee, OCE->getArg(0),
-                      !Callee->getReturnType()->isReferenceType());
-    return;
-  } else if (auto *CE = dyn_cast<CallExpr>(Call)) {
-    FunctionDecl *Callee = CE->getDirectCallee();
-    if (Callee && shouldTrackFirstArgument(Callee))
-      VisitPointerArg(Callee, CE->getArg(0),
-                      !Callee->getReturnType()->isReferenceType());
-    return;
-  }
-
-  if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
-    const auto *Ctor = CCE->getConstructor();
-    const CXXRecordDecl *RD = Ctor->getParent();
-    if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())
-      VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0], true);
-  }
-}
-
 static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
   const TypeSourceInfo *TSI = FD->getTypeSourceInfo();
   if (!TSI)
@@ -423,8 +363,10 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
   return false;
 }
 
-static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
-                                        LocalVisitor Visit) {
+// Visit lifetimebound or gsl-pointer arguments.
+static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
+                                       LocalVisitor Visit,
+                                       bool EnableLifetimeWarnings) {
   const FunctionDecl *Callee;
   ArrayRef<Expr *> Args;
 
@@ -458,6 +400,34 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
                                        /*EnableLifetimeWarnings=*/false);
     Path.pop_back();
   };
+  auto VisitGSLPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
+    // 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;
+    // Once we initialized a value with a reference, it can no longer dangle.
+    if (!Value) {
+      for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
+        if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
+          continue;
+        if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
+            PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
+          return;
+        break;
+      }
+    }
+    Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
+                          : IndirectLocalPathEntry::GslReferenceInit,
+                    Arg, D});
+    if (Arg->isGLValue())
+      visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
+                                            Visit,
+                                            /*EnableLifetimeWarnings=*/true);
+    else
+      visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
+                                       /*EnableLifetimeWarnings=*/true);
+    Path.pop_back();
+  };
 
   bool CheckCoroCall = false;
   if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
@@ -478,6 +448,12 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
       CheckCoroObjArg = false;
     if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg)
       VisitLifetimeBoundArg(Callee, ObjectArg);
+    else if (EnableLifetimeWarnings) {
+      if (auto *CME = dyn_cast<CXXMethodDecl>(Callee);
+          CME && shouldTrackImplicitObjectArg(CME))
+        VisitGSLPointerArg(Callee, ObjectArg,
+                        !Callee->getReturnType()->isReferenceType());
+    }
   }
 
   for (unsigned I = 0,
@@ -485,6 +461,17 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
        I != N; ++I) {
     if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
       VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
+    else if (EnableLifetimeWarnings && I == 0) {
+      if (shouldTrackFirstArgument(Callee)) {
+        VisitGSLPointerArg(Callee, Args[0],
+                           !Callee->getReturnType()->isReferenceType());
+      } else {
+        if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
+            CCE && CCE->getConstructor()->getParent()->hasAttr<PointerAttr>())
+          VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0],
+                             true);
+      }
+    }
   }
 }
 
@@ -557,11 +544,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
                                        EnableLifetimeWarnings);
   }
 
-  if (isa<CallExpr>(Init)) {
-    if (EnableLifetimeWarnings)
-      handleGslAnnotatedTypes(Path, Init, Visit);
-    return visitLifetimeBoundArguments(Path, Init, Visit);
-  }
+  if (isa<CallExpr>(Init))
+    return visitFunctionCallArguments(Path, Init, Visit,
+                                      EnableLifetimeWarnings);
 
   switch (Init->getStmtClass()) {
   case Stmt::DeclRefExprClass: {
@@ -835,11 +820,8 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
     }
   }
 
-  if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init)) {
-    if (EnableLifetimeWarnings)
-      handleGslAnnotatedTypes(Path, Init, Visit);
-    return visitLifetimeBoundArguments(Path, Init, Visit);
-  }
+  if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init))
+    return visitFunctionCallArguments(Path, Init, Visit, EnableLifetimeWarnings);
 
   switch (Init->getStmtClass()) {
   case Stmt::UnaryOperatorClass: {
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 09dfb2b5d96a89..cd1904db327105 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -479,3 +479,23 @@ void testForBug49342()
 {
   auto it = std::iter<char>{} - 2; // Used to be false positive.
 }
+
+namespace GH93386 {
+// verify no duplicated diagnostics are emitted.
+struct [[gsl::Pointer]] S {
+  S(const std::vector<int>& abc [[clang::lifetimebound]]);
+};
+
+S test(std::vector<int> a) {
+  return S(a);  // expected-warning {{address of stack memory associated with}}
+}
+
+auto s = S(std::vector<int>()); // expected-warning {{temporary whose address is used as value of local variable}}
+
+// Verify no regression on the follow case.
+std::string_view test2(int i, std::optional<std::string_view> a) {
+  if (i)
+    return std::move(*a);
+  return std::move(a.value());
+}
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[usx95]

LGTM. Thanks.