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

Haojian Wu via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 20 01:42:00 PDT 2024


https://github.com/hokein created https://github.com/llvm/llvm-project/pull/104906

In the current lifetime analysis, we have two parallel code paths: one for lifetimebound and another for GSL. These paths perform the same logic, both determining whether to continue visiting subexpressions.

This PR merges the two paths into a single code path. As a result, we'll reduce the overhead by eliminating a redundant visit to subexpressions. The change is mostly NFC (No Functional Change). The only notable difference is that when a subexpression is visited due to either lifetimebound or GSL, we will prioritize the lifetimebound path. This means the final diagnostic will be -Wdangling (rather than both `-Wdangling` and `-Wdangling-gsl`)

This might cause a slight change in behavior if the -Wdangling diagnostic is disabled, but I think this is not a major concern since both diagnostics are enabled by default.

Fixes #93386 

>From 773a03b25a94d991206f4b517eefdf3693e6a287 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Tue, 20 Aug 2024 10:22:44 +0200
Subject: [PATCH] [clang] Merge the lifetimebound and GSL code paths for
 lifetime analysis.

---
 clang/docs/ReleaseNotes.rst                   |   2 +
 clang/lib/Sema/CheckExprLifetime.cpp          | 129 ++++++++----------
 .../Sema/warn-lifetime-analysis-nocfg.cpp     |  13 ++
 3 files changed, 72 insertions(+), 72 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 249249971dec7c..924968452819d1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -225,6 +225,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).
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 7389046eaddde1..06eb909659d05d 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,35 @@ 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 +449,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 +462,19 @@ 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) {
+      if (I == 0) {
+        if (shouldTrackFirstArgument(Callee)) { // gsl
+          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 +547,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 +823,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..86ee90ed6df8dd 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -479,3 +479,16 @@ 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}}
+}



More information about the cfe-commits mailing list