Skip to content

[clang] Infer lifetime_capture_by for STL containers #117122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 22, 2024
Merged

[clang] Infer lifetime_capture_by for STL containers #117122

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a57223
[clang] Infer lifetime_capture_by for STL containers
usx95 Nov 21, 2024
2c8f670
add ast tests
usx95 Nov 22, 2024
3ea45b1
new line
usx95 Nov 22, 2024
ab9a998
add // CHECK-NOT: LifetimeCaptureByAttr
usx95 Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_by(this)]] to STL container methods.
void inferLifetimeCaptureByAttribute(FunctionDecl *FD);

/// Add [[gsl::Pointer]] attributes for std:: types.
void inferGslPointerAttribute(TypedefNameDecl *TD);

Expand Down
39 changes: 39 additions & 0 deletions clang/lib/Sema/SemaAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
//
//===----------------------------------------------------------------------===//

#include "CheckExprLifetime.h"
#include "clang/AST/ASTConsumer.h"
#include "clang/AST/Attr.h"
#include "clang/AST/Expr.h"
Expand Down Expand Up @@ -268,6 +269,44 @@ 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;
if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD))
RD = CTSD->getSpecializedTemplate()->getTemplatedDecl();
Copy link
Collaborator

@hokein hokein Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@usx95 usx95 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@usx95 usx95 Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@hokein hokein Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@usx95 usx95 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Collaborator

@hokein hokein Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@usx95 usx95 Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
// FIXME: Infer for operator[] for map-like containers. For example:
// std::map<string_view, ...> m;
// m[ReturnString(..)] = ...;
static const llvm::StringSet<> CapturingMethods{"insert", "push",
"push_front", "push_back"};
if (!CapturingMethods.contains(MD->getName()))
return;
// Do not infer if any parameter is explicitly annotated.
for (ParmVarDecl *PVD : MD->parameters())
if (PVD->hasAttr<LifetimeCaptureByAttr>())
return;
Copy link
Collaborator

@Xazax-hun Xazax-hun Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@usx95 usx95 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was intentional.

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

for (ParmVarDecl *PVD : MD->parameters()) {
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",
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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
Expand Down
103 changes: 103 additions & 0 deletions clang/test/AST/attr-lifetime-capture-by.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,106 @@ struct S {
};

// CHECK: CXXMethodDecl {{.*}}clang::lifetime_capture_by(a, b, global)

// ****************************************************************************
// Infer annotation for STL container methods.
// ****************************************************************************
namespace __gnu_cxx {
template <typename T>
struct basic_iterator {};
}

namespace std {
template<typename T> class allocator {};
template <typename T, typename Alloc = allocator<T>>
struct vector {
typedef __gnu_cxx::basic_iterator<T> iterator;
iterator begin();

vector();

void push_back(const T&);
void push_back(T&&);

void insert(iterator, T&&);
};
} // namespace std
struct [[gsl::Pointer()]] View {};
std::vector<View> views;
// CHECK: ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
// CHECK: TemplateArgument type 'View'
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} push_back 'void (const View &)'
// CHECK: ParmVarDecl {{.*}} 'const View &'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} push_back 'void (View &&)'
// CHECK: ParmVarDecl {{.*}} 'View &&'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit

// CHECK: CXXMethodDecl {{.*}} insert 'void (iterator, View &&)'
// CHECK: ParmVarDecl {{.*}} 'iterator'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK: ParmVarDecl {{.*}} 'View &&'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK-NOT: LifetimeCaptureByAttr

template <class T> struct [[gsl::Pointer()]] ViewTemplate {};
std::vector<ViewTemplate<int>> templated_views;
// CHECK: ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
// CHECK: TemplateArgument type 'ViewTemplate<int>'
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} push_back 'void (const ViewTemplate<int> &)'
// CHECK: ParmVarDecl {{.*}} 'const ViewTemplate<int> &'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} push_back 'void (ViewTemplate<int> &&)'
// CHECK: ParmVarDecl {{.*}} 'ViewTemplate<int> &&'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit

// CHECK: CXXMethodDecl {{.*}} insert 'void (iterator, ViewTemplate<int> &&)'
// CHECK: ParmVarDecl {{.*}} 'iterator'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK: ParmVarDecl {{.*}} 'ViewTemplate<int> &&'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK-NOT: LifetimeCaptureByAttr

std::vector<int*> pointers;
// CHECK: ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
// CHECK: TemplateArgument type 'int *'
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} push_back 'void (int *const &)'
// CHECK: ParmVarDecl {{.*}} 'int *const &'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} push_back 'void (int *&&)'
// CHECK: ParmVarDecl {{.*}} 'int *&&'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit

// CHECK: CXXMethodDecl {{.*}} insert 'void (iterator, int *&&)'
// CHECK: ParmVarDecl {{.*}} 'iterator'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK: ParmVarDecl {{.*}} 'int *&&'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK-NOT: LifetimeCaptureByAttr

std::vector<int> ints;
// CHECK: ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
// CHECK: TemplateArgument type 'int'

// CHECK: CXXMethodDecl {{.*}} push_back 'void (const int &)'
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} push_back 'void (int &&)'
// CHECK-NOT: LifetimeCaptureByAttr

// CHECK: CXXMethodDecl {{.*}} insert 'void (iterator, int &&)'
// CHECK: ParmVarDecl {{.*}} 'iterator'
// CHECK: LifetimeCaptureByAttr {{.*}} Implicit
// CHECK-NOT: LifetimeCaptureByAttr
5 changes: 5 additions & 0 deletions clang/test/Sema/Inputs/lifetime-analysis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
45 changes: 45 additions & 0 deletions clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,3 +366,48 @@ 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);

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());

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(&local);
}

namespace with_span {
// Templated view types.
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}); // expected-warning {{object whose reference is captured by 'spans' will be destroyed at the end of the full-expression}}
std::vector<int> local;
spans.push_back(local);
}
} // namespace with_span
} // namespace inferred_capture_by
Loading