[clang] Infer lifetime_capture_by for STL containers #117122

usx95 · 2024-11-21T07:22:19Z

This is behind -Wdangling-capture warning which is disabled by default.

llvmbot · 2024-11-21T07:22:55Z

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

This is behind -Wdangling-capture warning which is disabled by default.

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

5 Files Affected:

(modified) clang/include/clang/Sema/Sema.h (+3)
(modified) clang/lib/Sema/SemaAttr.cpp (+34)
(modified) clang/lib/Sema/SemaDecl.cpp (+2)
(modified) clang/test/Sema/Inputs/lifetime-analysis.h (+5)
(modified) clang/test/Sema/warn-lifetime-analysis-capture-by.cpp (+79)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 6ea6c67447b6f0..9bafcfec5d4786 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -1757,6 +1757,9 @@ class Sema final : public SemaBase {
   /// Add [[clang:::lifetimebound]] attr for std:: functions and methods.
   void inferLifetimeBoundAttribute(FunctionDecl *FD);
 
+  /// Add [[clang:::lifetime_capture(this)]] to STL container methods.
+  void inferLifetimeCaptureByAttribute(FunctionDecl *FD);
+
   /// Add [[gsl::Pointer]] attributes for std:: types.
   void inferGslPointerAttribute(TypedefNameDecl *TD);
 
diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
index 9fbad7ed67ccbe..507f7c40d58782 100644
--- a/clang/lib/Sema/SemaAttr.cpp
+++ b/clang/lib/Sema/SemaAttr.cpp
@@ -268,6 +268,40 @@ void Sema::inferLifetimeBoundAttribute(FunctionDecl *FD) {
   }
 }
 
+static bool IsPointerLikeType(QualType QT) {
+  QT = QT.getNonReferenceType();
+  if (QT->isPointerType())
+    return true;
+  auto *RD = QT->getAsCXXRecordDecl();
+  if (!RD)
+    return false;
+  RD = RD->getCanonicalDecl();
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD))
+    RD = CTSD->getSpecializedTemplate()->getTemplatedDecl();
+  return RD->hasAttr<PointerAttr>();
+}
+
+void Sema::inferLifetimeCaptureByAttribute(FunctionDecl *FD) {
+  if (!FD)
+    return;
+  auto *MD = dyn_cast<CXXMethodDecl>(FD);
+  if (!MD || !MD->getIdentifier() || !MD->getParent()->isInStdNamespace())
+    return;
+  static const llvm::StringSet<> CapturingMethods{"insert", "push",
+                                                  "push_front", "push_back"};
+  if (!CapturingMethods.contains(MD->getName()))
+    return;
+  for (ParmVarDecl *PVD : MD->parameters()) {
+    if (PVD->hasAttr<LifetimeCaptureByAttr>())
+      return;
+    if (IsPointerLikeType(PVD->getType())) {
+      int CaptureByThis[] = {LifetimeCaptureByAttr::THIS};
+      PVD->addAttr(
+          LifetimeCaptureByAttr::CreateImplicit(Context, CaptureByThis, 1));
+    }
+  }
+}
+
 void Sema::inferNullableClassAttribute(CXXRecordDecl *CRD) {
   static const llvm::StringSet<> Nullable{
       "auto_ptr",         "shared_ptr", "unique_ptr",         "exception_ptr",
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index be570f3a1829d0..5b30d0f2c22d16 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -11913,6 +11913,7 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
   NamedDecl *OldDecl = nullptr;
   bool MayNeedOverloadableChecks = false;
 
+  inferLifetimeCaptureByAttribute(NewFD);
   // Merge or overload the declaration with an existing declaration of
   // the same name, if appropriate.
   if (!Previous.empty()) {
@@ -16716,6 +16717,7 @@ void Sema::AddKnownFunctionAttributes(FunctionDecl *FD) {
 
   LazyProcessLifetimeCaptureByParams(FD);
   inferLifetimeBoundAttribute(FD);
+  inferLifetimeCaptureByAttribute(FD);
   AddKnownFunctionAttributesForReplaceableGlobalAllocationFunction(FD);
 
   // If C++ exceptions are enabled but we are told extern "C" functions cannot
diff --git a/clang/test/Sema/Inputs/lifetime-analysis.h b/clang/test/Sema/Inputs/lifetime-analysis.h
index 41d1e2f074cc83..5c151385b1fe5a 100644
--- a/clang/test/Sema/Inputs/lifetime-analysis.h
+++ b/clang/test/Sema/Inputs/lifetime-analysis.h
@@ -49,6 +49,11 @@ struct vector {
 	vector(InputIterator first, InputIterator __last);
 
   T &at(int n);
+
+  void push_back(const T&);
+  void push_back(T&&);
+  
+  void insert(iterator, T&&);
 };
 
 template<typename T>
diff --git a/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp b/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
index b3fde386b8616c..462cb2d3f3fd6e 100644
--- a/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
@@ -366,3 +366,82 @@ void use() {
   capture3(std::string(), x3); // expected-warning {{object whose reference is captured by 'x3' will be destroyed at the end of the full-expression}}
 }
 } // namespace temporary_views
+
+// ****************************************************************************
+// Inferring annotation for STL containers
+// ****************************************************************************
+namespace inferred_capture_by {
+const std::string* getLifetimeBoundPointer(const std::string &s [[clang::lifetimebound]]);
+const std::string* getNotLifetimeBoundPointer(const std::string &s);
+
+namespace with_string_views {
+std::string_view getLifetimeBoundView(const std::string& s [[clang::lifetimebound]]);
+std::string_view getNotLifetimeBoundView(const std::string& s);
+void use() {
+  std::string local;
+  std::vector<std::string_view> views;
+  views.push_back(std::string()); // expected-warning {{object whose reference is captured by 'views' will be destroyed at the end of the full-expression}}
+  views.insert(views.begin(), 
+            std::string()); // expected-warning {{object whose reference is captured by 'views' will be destroyed at the end of the full-expression}}
+  views.push_back(getLifetimeBoundView(std::string())); // expected-warning {{object whose reference is captured by 'views' will be destroyed at the end of the full-expression}}
+  views.push_back(getNotLifetimeBoundView(std::string()));
+  views.push_back(local);
+  views.insert(views.end(), local);
+
+  std::vector<std::string> strings;
+  strings.push_back(std::string());
+  strings.insert(strings.begin(), std::string());
+}
+} // namespace with_string_views
+
+namespace with_pointers {
+const std::string* getLifetimeBoundPointer(const std::string &s [[clang::lifetimebound]]);
+const std::string* getLifetimeBoundPointer(std::string_view s [[clang::lifetimebound]]);
+const std::string* getNotLifetimeBoundPointer(const std::string &s);
+std::string_view getLifetimeBoundView(const std::string& s [[clang::lifetimebound]]);
+
+void use() {
+  std::string local;
+  std::vector<const std::string*> pointers;
+  pointers.push_back(getLifetimeBoundPointer(std::string())); // expected-warning {{object whose reference is captured by 'pointers' will be destroyed at the end of the full-expression}}
+  pointers.push_back(getLifetimeBoundPointer(*getLifetimeBoundPointer(std::string()))); // expected-warning {{object whose reference is captured by 'pointers' will be destroyed at the end of the full-expression}}
+  pointers.push_back(getLifetimeBoundPointer(getLifetimeBoundView(std::string()))); // expected-warning {{object whose reference is captured by 'pointers' will be destroyed at the end of the full-expression}}
+  pointers.push_back(getLifetimeBoundPointer(local));
+
+  pointers.push_back(getLifetimeBoundPointer(*getNotLifetimeBoundPointer(std::string())));
+  pointers.push_back(getNotLifetimeBoundPointer(std::string()));
+}
+} // namespace with_pointers
+
+namespace with_optional {
+class [[gsl::Pointer()]] my_view : public std::string_view {};
+class non_pointer_view : public std::string_view {};
+
+std::optional<std::string> getOptionalString();
+std::optional<std::string_view> getOptionalView();
+std::optional<std::string_view> getOptionalViewLifetimebound(const std::string& s [[clang::lifetimebound]]);
+std::optional<my_view> getOptionalMyView();
+std::optional<non_pointer_view> getOptionalNonPointerView();
+my_view getMyView();
+non_pointer_view getNonPointerView();
+
+void use() {
+  std::string local;
+  std::vector<std::string_view> views;
+  
+  std::optional<std::string_view> optional;
+  views.push_back(optional.value());
+  views.push_back(getOptionalString().value()); // expected-warning {{object whose reference is captured by 'views' will be destroyed at the end of the full-expression}}
+  views.push_back(getOptionalView().value());
+  views.push_back(getOptionalViewLifetimebound(std::string()).value()); // FIXME: Diagnose it.
+  views.push_back(getOptionalMyView().value());
+
+  views.push_back(getOptionalNonPointerView().value());
+  views.push_back(getMyView());
+  views.push_back(getNonPointerView());
+  views.push_back(std::string_view{});
+  views.push_back(my_view{});
+  views.push_back(non_pointer_view{});
+}
+} // namespace with_optional
+} // namespace inferred_capture_by

clang/lib/Sema/SemaAttr.cpp

Xazax-hun · 2024-11-21T11:33:52Z

clang/lib/Sema/SemaAttr.cpp

+    return;
+  for (ParmVarDecl *PVD : MD->parameters()) {
+    if (PVD->hasAttr<LifetimeCaptureByAttr>())
+      return;


Is return intentional here? Do we want an explicit attr to turn off the inference for the rest of the params? If that is the case, I think we want to turn off the inference for the params before the explicit attr as well, not just after.

Yes, this was intentional.

Good catch. Disabled if any of the parameters is explicitly annotated.

clang/lib/Sema/SemaAttr.cpp

clang/include/clang/Sema/Sema.h

clang/lib/Sema/SemaAttr.cpp

hokein · 2024-11-21T13:53:24Z

clang/lib/Sema/SemaAttr.cpp

+    return false;
+  RD = RD->getCanonicalDecl();
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD))
+    RD = CTSD->getSpecializedTemplate()->getTemplatedDecl();


It’s unclear to me why this case needs to be handled separately, and we already have one in CheckExprLifetime.cpp

We fail to detect pointer types which are templates:

template <class T> struct [[gsl::Pointer()]] ViewTemplate {}; std::vector<ViewTemplate<int>> templated_views;

template<typename T> struct [[gsl::Pointer]] Span { Span(const std::vector<T> &V); }; void use() { std::vector<Span<int>> spans; spans.push_back(std::vector<int>{1, 2, 3}); // warning. }

Maybe a better fix would be to propagate this annotation to the template specialization in clang.

Thanks for the example. I think I understand what’s happening here.

The primary template Span is annotated with the gsl::Pointer attribute, so all its specializations should inherit this attribute.

However, in this example, at the point where we infer the lifetime_capture_by attribute for vector's method, we don’t yet have a fully completed ClassTemplateSpecializationDecl for Span<int> (and therefore, no PointerAttr). This is likely because the instantiation of Span<int> isn’t required for the statement std::vector<Span<int>> spans;.

I think the following case would work without this special handling.

void use() { Span<int> abc({}); // trigger an instantiation of `Span<int>`. std::vector<Span<int>> spans; spans.push_back(std::vector<int>{1, 2, 3}); // warning. }

I don’t have a better suggestion for addressing this issue directly. However, I think we should have a comment explaining it. (Should we do the same thing for the one in CheckExprLifetime.cpp?)

Should we do the same thing for the one in CheckExprLifetime.cpp?
Makes a lot of sense. Let me do this separately in parallel though.

That said, your example still doesn't work show the warning with the lines removed:

if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD)) RD = CTSD->getSpecializedTemplate()->getTemplatedDecl();

hmm, this is strange. I tested it locally, it works for me.

$ cat /t/t6.cpp #include <vector> template<typename T> struct [[gsl::Pointer]] Span { Span(const std::vector<T> &V); }; void use() { Span<int> pp(std::vector<int>{}); std::vector<Span<int>> spans; spans.push_back(std::vector<int>()); // warning. } $ ./bin/clang -Xclang -fsyntax-only -Wdangling-capture /t/t6.cpp <<< /t/t6.cpp:9:16: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl] 9 | Span<int> pp(std::vector<int>{}); | ^~~~~~~~~~~~~~~~~~ /t/t6.cpp:11:19: warning: object whose reference is captured by 'spans' will be destroyed at the end of the full-expression [-Wdangling-capture] 11 | spans.push_back(std::vector<int>()); // warning. | ^~~~~~~~~~~~~~~~~~ 2 warnings generated. /usr/bin/ld: /lib/x86_64-linux-gnu/Scrt1.o: in function `_start': (.text+0x17): undefined reference to `main' clang: error: linker command failed with exit code 1 (use -v to see invocation)

Hmm. I can reproduce with <vector> include but not with handcrafted vector in lit tests.

But I think you are right. I also suspected that we are just inspecting the template spec decl earlier than expected.

clang/test/Sema/warn-lifetime-analysis-capture-by.cpp

hokein · 2024-11-22T09:34:21Z

clang/lib/Sema/SemaAttr.cpp

+    return false;
+  RD = RD->getCanonicalDecl();
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD))
+    RD = CTSD->getSpecializedTemplate()->getTemplatedDecl();


Thanks for the example. I think I understand what’s happening here.

The primary template Span is annotated with the gsl::Pointer attribute, so all its specializations should inherit this attribute.

However, in this example, at the point where we infer the lifetime_capture_by attribute for vector's method, we don’t yet have a fully completed ClassTemplateSpecializationDecl for Span<int> (and therefore, no PointerAttr). This is likely because the instantiation of Span<int> isn’t required for the statement std::vector<Span<int>> spans;.

I think the following case would work without this special handling.

void use() { Span<int> abc({}); // trigger an instantiation of `Span<int>`. std::vector<Span<int>> spans; spans.push_back(std::vector<int>{1, 2, 3}); // warning. }

I don’t have a better suggestion for addressing this issue directly. However, I think we should have a comment explaining it. (Should we do the same thing for the one in CheckExprLifetime.cpp?)

clang/test/AST/attr-lifetime-capture-by.cpp

Xazax-hun

Once @hokein's comments are addressed, it looks good to me.

clang/test/AST/attr-lifetime-capture-by.cpp

llvm-ci · 2024-11-22T15:13:16Z

LLVM Buildbot has detected a new failure on builder openmp-offload-amdgpu-runtime running on omp-vega20-0 while building clang at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/10839

Here is the relevant piece of the build log for the reference

Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./bin/clang -fopenmp   -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/openmp/runtime/test -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/openmp/runtime/test/ompt /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./bin/clang -fopenmp -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/openmp/runtime/test -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/openmp/runtime/test/ompt /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# note: command had no output on stdout or stderr
# executed command: /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************

[clang] Infer lifetime_capture_by for STL containers

9a57223

usx95 requested a review from Endilll as a code owner November 21, 2024 07:22

usx95 requested review from Xazax-hun and removed request for Endilll November 21, 2024 07:22

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 21, 2024

usx95 requested a review from hokein November 21, 2024 07:22

usx95 commented Nov 21, 2024

View reviewed changes

clang/lib/Sema/SemaAttr.cpp Show resolved Hide resolved

Xazax-hun reviewed Nov 21, 2024

View reviewed changes

hokein reviewed Nov 21, 2024

View reviewed changes

add ast tests

2c8f670

usx95 requested review from Xazax-hun and hokein November 22, 2024 08:45

hokein reviewed Nov 22, 2024

View reviewed changes

Xazax-hun approved these changes Nov 22, 2024

View reviewed changes

clang/test/AST/attr-lifetime-capture-by.cpp Outdated Show resolved Hide resolved

usx95 added 2 commits November 22, 2024 11:05

new line

3ea45b1

add // CHECK-NOT: LifetimeCaptureByAttr

ab9a998

hokein approved these changes Nov 22, 2024

View reviewed changes

clang/test/AST/attr-lifetime-capture-by.cpp Show resolved Hide resolved

usx95 merged commit 912c502 into llvm:main Nov 22, 2024
8 checks passed

[clang] Infer lifetime_capture_by for STL containers #117122

[clang] Infer lifetime_capture_by for STL containers #117122

Uh oh!

Conversation

usx95 commented Nov 21, 2024

Uh oh!

llvmbot commented Nov 21, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usx95 Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usx95 Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Xazax-hun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvm-ci commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

usx95 Nov 22, 2024 •

edited

Loading

usx95 Nov 22, 2024 •

edited

Loading