Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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(this)]] to STL container methods.
void inferLifetimeCaptureByAttribute(FunctionDecl *FD);

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

Expand Down
34 changes: 34 additions & 0 deletions clang/lib/Sema/SemaAttr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Collaborator

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

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

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

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

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

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

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;
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;
Copy link
Collaborator

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

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.

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
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
5 changes: 5 additions & 0 deletions clang/test/Sema/Inputs/lifetime-analysis.h
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
79 changes: 79 additions & 0 deletions clang/test/Sema/warn-lifetime-analysis-capture-by.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading