Skip to content

[clang] Don't infer lifetime_capture-by for reference of raw pointer types. #122240

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 1 commit into from
Jan 9, 2025
Merged

[clang] Don't infer lifetime_capture-by for reference of raw pointer types. #122240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions clang/lib/Sema/CheckExprLifetime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,9 +281,11 @@ template <typename T> static bool isRecordWithAttr(QualType Type) {
return Result;
}

bool isPointerLikeType(QualType QT) {
return isRecordWithAttr<PointerAttr>(QT) || QT->isPointerType() ||
QT->isNullPtrType();
// Tells whether the type is annotated with [[gsl::Pointer]].
bool isGLSPointerType(QualType QT) { return isRecordWithAttr<PointerAttr>(QT); }

static bool isPointerLikeType(QualType QT) {
return isGLSPointerType(QT) || QT->isPointerType() || QT->isNullPtrType();
}

// Decl::isInStdNamespace will return false for iterators in some STL
Expand Down
5 changes: 2 additions & 3 deletions clang/lib/Sema/CheckExprLifetime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@

namespace clang::sema {

// Tells whether the type is annotated with [[gsl::Pointer]] or is a pointer
// type.
bool isPointerLikeType(QualType QT);
// Tells whether the type is annotated with [[gsl::Pointer]].
bool isGLSPointerType(QualType QT);

/// Describes an entity that is being assigned.
struct AssignedEntity {
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ void Sema::inferLifetimeCaptureByAttribute(FunctionDecl *FD) {
// We only apply the lifetime_capture_by attribute to parameters of
// pointer-like reference types (`const T&`, `T&&`).
if (PVD->getType()->isReferenceType() &&
sema::isPointerLikeType(PVD->getType().getNonReferenceType())) {
sema::isGLSPointerType(PVD->getType().getNonReferenceType())) {
int CaptureByThis[] = {LifetimeCaptureByAttr::THIS};
PVD->addAttr(
LifetimeCaptureByAttr::CreateImplicit(Context, CaptureByThis, 1));
Expand Down
6 changes: 3 additions & 3 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 @@ -80,16 +80,16 @@ std::vector<int*> pointers;

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

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

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

std::vector<int> ints;
// CHECK: ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
Expand Down
23 changes: 22 additions & 1 deletion 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 @@ -406,7 +406,7 @@ void use() {
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(getLifetimeBoundPointer(std::string()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a follow-up PR, but I wonder if we can actually get this diagnostic back in the future. I think in case the pointer originates from a gsl owner, it is a strong indication that we care about the lifetime of the pointee and not the pointer itself and the capture_by annotation is what we actually want.

But I am also OK never supporting this scenario as it starts to get a bit convoluted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I completely agree that it would be ideal to restore the diagnostic without introducing false positives. However, I currently don’t have a simple solution for how to achieve this.

pointers.push_back(&local);
}

Expand Down Expand Up @@ -451,3 +451,24 @@ void test() {
T2(1, a); // expected-warning {{object whose reference is captured by}}
}
} // namespace on_constructor

namespace GH121391 {

struct Foo {};

template <typename T>
struct Container {
const T& tt() [[clang::lifetimebound]];
};
template<typename T>
struct StatusOr {
T* get() [[clang::lifetimebound]];
};
StatusOr<Container<const Foo*>> getContainer();

void test() {
std::vector<const Foo*> vv;
vv.push_back(getContainer().get()->tt()); // OK
}

} // namespace GH121391
Loading