https://github.com/hokein updated
https://github.com/llvm/llvm-project/pull/107213
>From 73ed8a2e42415b064cf8a35a84cc8c0c1dd20988 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein...@gmail.com>
Date: Wed, 28 Aug 2024 09:59:41 +0200
Subject: [PATCH 1/3] [clang] Diagnose dangling issues for
"Container<GSLPointer>" case.
We teach the GSL lifetime analysis to handle this code path, particuly
when constructing the container<GSLPointer> object from a GSLOwner.
---
clang/docs/ReleaseNotes.rst | 2 +
clang/include/clang/Basic/AttrDocs.td | 14 +++++
clang/lib/Sema/CheckExprLifetime.cpp | 34 +++++++++--
.../Sema/warn-lifetime-analysis-nocfg.cpp | 60 +++++++++++++++++++
4 files changed, 104 insertions(+), 6 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 250821a9f9c45c..fcc29ed2e2f04a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -298,6 +298,8 @@ Improvements to Clang's diagnostics
- Clang now warns for u8 character literals used in C23 with
``-Wpre-c23-compat`` instead of ``-Wpre-c++17-compat``.
+- Clang now diagnoses cases where a dangling `GSLOwner<GSLPointer>`` object is
constructed, e.g. `std::vector<string_view> v = {std::string}` (#GH100526).
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 546e5100b79dd9..4b56490b3af8ce 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6690,6 +6690,20 @@ When the Owner's lifetime ends, it will consider the
Pointer to be dangling.
P.getInt(); // P is dangling
}
+If a template class is annotated with [[gsl::Owner]], and the first
instantiated
+template argument is a pointer type (raw pointer, or [[gsl::Pointer]]), the
+analysis will consider the instantiated class as a container of the pointer.
+When constructing such an object from a GSL owner object, the analysis will
+assume that the container holds a pointer to the owner object. Consequently,
+when the owner object is destroyed, the pointer will be considered dangling.
+
+.. code-block:: c++
+
+ int f() {
+ std::vector<std::string_view> v = {std::string()}; // v holds a dangling
pointer.
+ std::optional<std::string_view> o = std::string(); // o holds a dangling
pointer.
+ }
+
}];
}
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp
b/clang/lib/Sema/CheckExprLifetime.cpp
index f1507ebb9a5068..5ad3d7afbff1ce 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -367,6 +367,21 @@ static bool implicitObjectParamIsLifetimeBound(const
FunctionDecl *FD) {
return isNormalAssignmentOperator(FD);
}
+// Returns true if the given Record decl is a form of `GSLOwner<Pointer>`
+// type, e.g. std::vector<string_view>, std::optional<string_view>.
+static bool isContainerOfPointer(const RecordDecl *Container) {
+ if (const auto *CTSD =
+ dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container)) {
+ if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type.
+ return false;
+ const auto &TAs = CTSD->getTemplateArgs();
+ return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
+ (isRecordWithAttr<PointerAttr>(TAs[0].getAsType()) ||
+ TAs[0].getAsType()->isPointerType());
+ }
+ return false;
+}
+
// Visit lifetimebound or gsl-pointer arguments.
static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
LocalVisitor Visit) {
@@ -470,10 +485,14 @@ static void visitFunctionCallArguments(IndirectLocalPath
&Path, Expr *Call,
else if (EnableGSLAnalysis && I == 0) {
if (shouldTrackFirstArgument(Callee)) {
VisitGSLPointerArg(Callee, Args[0]);
- } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
- CCE &&
- CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) {
- VisitGSLPointerArg(CCE->getConstructor(), Args[0]);
+ } else if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call)) {
+ const auto *ClassD = Ctor->getConstructor()->getParent();
+ // Two cases:
+ // a GSL pointer, e.g. std::string_view
+ // a container of GSL pointer, e.g. std::vector<string_view>
+ if (ClassD->hasAttr<PointerAttr>() ||
+ (isContainerOfPointer(ClassD) && Callee->getNumParams() == 1))
+ VisitGSLPointerArg(Ctor->getConstructor(), Args[0]);
}
}
}
@@ -990,13 +1009,16 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
// int &p = *localUniquePtr;
// someContainer.add(std::move(localUniquePtr));
// return p;
- IsLocalGslOwner = isRecordWithAttr<OwnerAttr>(L->getType());
+ IsLocalGslOwner =
+ isRecordWithAttr<OwnerAttr>(L->getType()) &&
+ !isContainerOfPointer(L->getType()->getAsRecordDecl());
if (pathContainsInit(Path) || !IsLocalGslOwner)
return false;
} else {
IsGslPtrValueFromGslTempOwner =
MTE && !MTE->getExtendingDecl() &&
- isRecordWithAttr<OwnerAttr>(MTE->getType());
+ isRecordWithAttr<OwnerAttr>(MTE->getType()) &&
+ !isContainerOfPointer(MTE->getType()->getAsRecordDecl());
// Skipping a chain of initializing gsl::Pointer annotated objects.
// We are looking only for the final source to find out if it was
// a local or temporary owner or the address of a local variable/param.
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 59357d0730a7d9..64f9f77428b62c 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -158,17 +158,30 @@ auto begin(C &c) -> decltype(c.begin());
template<typename T, int N>
T *begin(T (&array)[N]);
+using size_t = decltype(sizeof(0));
+
+template<typename T>
+struct initializer_list {
+ const T* ptr; size_t sz;
+};
template <typename T>
struct vector {
typedef __gnu_cxx::basic_iterator<T> iterator;
iterator begin();
iterator end();
const T *data() const;
+ vector();
+ vector(initializer_list<T> __l);
+
+ template<typename InputIterator>
+ vector(InputIterator first, InputIterator __last);
+
T &at(int n);
};
template<typename T>
struct basic_string_view {
+ basic_string_view();
basic_string_view(const T *);
const T *begin() const;
};
@@ -203,11 +216,21 @@ template<typename T>
struct optional {
optional();
optional(const T&);
+
+ template<typename U = T>
+ optional(U&& t);
+
+ template<typename U>
+ optional(optional<U>&& __t);
+
T &operator*() &;
T &&operator*() &&;
T &value() &;
T &&value() &&;
};
+template<typename T>
+optional<__decay(T)> make_optional(T&&);
+
template<typename T>
struct stack {
@@ -553,3 +576,40 @@ void test() {
std::string_view svjkk1 = ReturnStringView(StrCat("bar", "x")); //
expected-warning {{object backing the pointer will be destroyed at the end of
the full-expression}}
}
} // namespace GH100549
+
+namespace GH100526 {
+void test() {
+ std::vector<std::string_view> v1({std::string()}); // expected-warning
{{object backing the pointer will be destroyed at the end}}
+ std::vector<std::string_view> v2({std::string(), std::string_view()}); //
expected-warning {{object backing the pointer will be destroyed at the end}}
+ std::vector<std::string_view> v3({std::string_view(), std::string()}); //
expected-warning {{object backing the pointer will be destroyed at the end}}
+
+ std::optional<std::string_view> o1 = std::string(); // expected-warning
{{object backing the pointer}}
+
+ std::string s;
+ // This is a tricky use-after-free case, what it does:
+ // 1. make_optional creates a temporary "optional<string>"" object
+ // 2. the temporary object owns the underlying string which is copied from
s.
+ // 3. the t3 object holds the view to the underlying string of the
temporary object.
+ std::optional<std::string_view> o2 = std::make_optional(s); //
expected-warning {{object backing the pointer}}
+
+ // FIXME: should work for assignment cases
+ v1 = {std::string()};
+ o1 = std::string();
+
+ // no warning on copying pointers.
+ std::vector<std::string_view> n1 = {std::string_view()};
+ std::optional<std::string_view> n2 = {std::string_view()};
+ std::optional<std::string_view> n3 = std::make_optional(std::string_view());
+ const char* b = "";
+ std::optional<std::string_view> n4 = std::make_optional(b);
+ std::optional<std::string_view> n5 = std::make_optional("test");
+}
+
+std::vector<std::string_view> test2(int i) {
+ std::vector<std::string_view> t;
+ if (i)
+ return t; // this is fine, no dangling
+ return std::vector<std::string_view>(t.begin(), t.end());
+}
+
+} // namespace GH100526
>From 64e2c74bc34f6ea6ee583af2e4c0908a91a04b56 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein...@gmail.com>
Date: Fri, 6 Sep 2024 13:48:30 +0200
Subject: [PATCH 2/3] add more tests per comments.
---
clang/docs/ReleaseNotes.rst | 2 +-
clang/include/clang/Basic/AttrDocs.td | 6 ++---
.../Sema/warn-lifetime-analysis-nocfg.cpp | 27 +++++++++++++++----
3 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fcc29ed2e2f04a..f08e614e3b8367 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -298,7 +298,7 @@ Improvements to Clang's diagnostics
- Clang now warns for u8 character literals used in C23 with
``-Wpre-c23-compat`` instead of ``-Wpre-c++17-compat``.
-- Clang now diagnoses cases where a dangling `GSLOwner<GSLPointer>`` object is
constructed, e.g. `std::vector<string_view> v = {std::string}` (#GH100526).
+- Clang now diagnoses cases where a dangling ``GSLOwner<GSLPointer>`` object
is constructed, e.g. `std::vector<string_view> v = {std::string()};`
(#GH100526).
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 4b56490b3af8ce..9f72456d2da678 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6690,9 +6690,9 @@ When the Owner's lifetime ends, it will consider the
Pointer to be dangling.
P.getInt(); // P is dangling
}
-If a template class is annotated with [[gsl::Owner]], and the first
instantiated
-template argument is a pointer type (raw pointer, or [[gsl::Pointer]]), the
-analysis will consider the instantiated class as a container of the pointer.
+If a template class is annotated with ``[[gsl::Owner]]``, and the first
+instantiated template argument is a pointer type (raw pointer, or
``[[gsl::Pointer]]``),
+the analysis will consider the instantiated class as a container of the
pointer.
When constructing such an object from a GSL owner object, the analysis will
assume that the container holds a pointer to the owner object. Consequently,
when the owner object is destroyed, the pointer will be considered dangling.
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 64f9f77428b62c..234e06f069074b 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -580,8 +580,14 @@ void test() {
namespace GH100526 {
void test() {
std::vector<std::string_view> v1({std::string()}); // expected-warning
{{object backing the pointer will be destroyed at the end}}
- std::vector<std::string_view> v2({std::string(), std::string_view()}); //
expected-warning {{object backing the pointer will be destroyed at the end}}
- std::vector<std::string_view> v3({std::string_view(), std::string()}); //
expected-warning {{object backing the pointer will be destroyed at the end}}
+ std::vector<std::string_view> v2({
+ std::string(), // expected-warning {{object backing the pointer will be
destroyed at the end}}
+ std::string_view()
+ });
+ std::vector<std::string_view> v3({
+ std::string_view(),
+ std::string() // expected-warning {{object backing the pointer will be
destroyed at the end}}
+ });
std::optional<std::string_view> o1 = std::string(); // expected-warning
{{object backing the pointer}}
@@ -591,6 +597,8 @@ void test() {
// 2. the temporary object owns the underlying string which is copied from
s.
// 3. the t3 object holds the view to the underlying string of the
temporary object.
std::optional<std::string_view> o2 = std::make_optional(s); //
expected-warning {{object backing the pointer}}
+ std::optional<std::string_view> o3 = std::optional<std::string>(s); //
expected-warning {{object backing the pointer}}
+ std::optional<std::string_view> o4 = std::optional<std::string_view>(s);
// FIXME: should work for assignment cases
v1 = {std::string()};
@@ -599,10 +607,11 @@ void test() {
// no warning on copying pointers.
std::vector<std::string_view> n1 = {std::string_view()};
std::optional<std::string_view> n2 = {std::string_view()};
- std::optional<std::string_view> n3 = std::make_optional(std::string_view());
+ std::optional<std::string_view> n3 = std::string_view();
+ std::optional<std::string_view> n4 = std::make_optional(std::string_view());
const char* b = "";
- std::optional<std::string_view> n4 = std::make_optional(b);
- std::optional<std::string_view> n5 = std::make_optional("test");
+ std::optional<std::string_view> n5 = std::make_optional(b);
+ std::optional<std::string_view> n6 = std::make_optional("test");
}
std::vector<std::string_view> test2(int i) {
@@ -612,4 +621,12 @@ std::vector<std::string_view> test2(int i) {
return std::vector<std::string_view>(t.begin(), t.end());
}
+std::optional<std::string_view> test3(int i) {
+ std::string s;
+ std::string_view sv;
+ if (i)
+ return s; // expected-warning {{address of stack memory associated}}
+ return sv; // fine
+}
+
} // namespace GH100526
>From 1ad96667e4577e4c4a68a7c07477a054fe21ad96 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein...@gmail.com>
Date: Mon, 9 Sep 2024 21:26:00 +0200
Subject: [PATCH 3/3] address review comments.
---
clang/docs/ReleaseNotes.rst | 2 +-
clang/lib/Sema/CheckExprLifetime.cpp | 48 +++++++++++++++-------------
2 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f08e614e3b8367..59ccdf1e15cd81 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -298,7 +298,7 @@ Improvements to Clang's diagnostics
- Clang now warns for u8 character literals used in C23 with
``-Wpre-c23-compat`` instead of ``-Wpre-c++17-compat``.
-- Clang now diagnoses cases where a dangling ``GSLOwner<GSLPointer>`` object
is constructed, e.g. `std::vector<string_view> v = {std::string()};`
(#GH100526).
+- Clang now diagnoses cases where a dangling ``GSLOwner<GSLPointer>`` object
is constructed, e.g. ``std::vector<string_view> v = {std::string()};``
(#GH100526).
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp
b/clang/lib/Sema/CheckExprLifetime.cpp
index 5ad3d7afbff1ce..c8e703036c132c 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -267,6 +267,26 @@ static bool isInStlNamespace(const Decl *D) {
return DC->isStdNamespace();
}
+// Returns true if the given Record decl is a form of `GSLOwner<Pointer>`
+// type, e.g. std::vector<string_view>, std::optional<string_view>.
+static bool isContainerOfPointer(const RecordDecl *Container) {
+ if (const auto *CTSD =
+ dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container)) {
+ if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type.
+ return false;
+ const auto &TAs = CTSD->getTemplateArgs();
+ return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
+ (isRecordWithAttr<PointerAttr>(TAs[0].getAsType()) ||
+ TAs[0].getAsType()->isPointerType());
+ }
+ return false;
+}
+
+static bool isGSLOwner(QualType T) {
+ return isRecordWithAttr<OwnerAttr>(T) &&
+ !isContainerOfPointer(T->getAsRecordDecl());
+}
+
static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()))
@@ -275,7 +295,7 @@ static bool shouldTrackImplicitObjectArg(const
CXXMethodDecl *Callee) {
return false;
if (!isRecordWithAttr<PointerAttr>(
Callee->getFunctionObjectParameterType()) &&
- !isRecordWithAttr<OwnerAttr>(Callee->getFunctionObjectParameterType()))
+ !isGSLOwner(Callee->getFunctionObjectParameterType()))
return false;
if (Callee->getReturnType()->isPointerType() ||
isRecordWithAttr<PointerAttr>(Callee->getReturnType())) {
@@ -367,21 +387,6 @@ static bool implicitObjectParamIsLifetimeBound(const
FunctionDecl *FD) {
return isNormalAssignmentOperator(FD);
}
-// Returns true if the given Record decl is a form of `GSLOwner<Pointer>`
-// type, e.g. std::vector<string_view>, std::optional<string_view>.
-static bool isContainerOfPointer(const RecordDecl *Container) {
- if (const auto *CTSD =
- dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container)) {
- if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type.
- return false;
- const auto &TAs = CTSD->getTemplateArgs();
- return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
- (isRecordWithAttr<PointerAttr>(TAs[0].getAsType()) ||
- TAs[0].getAsType()->isPointerType());
- }
- return false;
-}
-
// Visit lifetimebound or gsl-pointer arguments.
static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
LocalVisitor Visit) {
@@ -428,7 +433,7 @@ static void visitFunctionCallArguments(IndirectLocalPath
&Path, Expr *Call,
// Once we initialized a value with a non gsl-owner reference, it can no
// longer dangle.
if (ReturnType->isReferenceType() &&
- !isRecordWithAttr<OwnerAttr>(ReturnType->getPointeeType())) {
+ !isGSLOwner(ReturnType->getPointeeType())) {
for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit ||
PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
@@ -483,6 +488,7 @@ static void visitFunctionCallArguments(IndirectLocalPath
&Path, Expr *Call,
if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
else if (EnableGSLAnalysis && I == 0) {
+ // Perform GSL analysis for the first argument
if (shouldTrackFirstArgument(Callee)) {
VisitGSLPointerArg(Callee, Args[0]);
} else if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call)) {
@@ -1009,16 +1015,12 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
// int &p = *localUniquePtr;
// someContainer.add(std::move(localUniquePtr));
// return p;
- IsLocalGslOwner =
- isRecordWithAttr<OwnerAttr>(L->getType()) &&
- !isContainerOfPointer(L->getType()->getAsRecordDecl());
+ IsLocalGslOwner = isGSLOwner(L->getType());
if (pathContainsInit(Path) || !IsLocalGslOwner)
return false;
} else {
IsGslPtrValueFromGslTempOwner =
- MTE && !MTE->getExtendingDecl() &&
- isRecordWithAttr<OwnerAttr>(MTE->getType()) &&
- !isContainerOfPointer(MTE->getType()->getAsRecordDecl());
+ MTE && !MTE->getExtendingDecl() && isGSLOwner(MTE->getType());
// Skipping a chain of initializing gsl::Pointer annotated objects.
// We are looking only for the final source to find out if it was
// a local or temporary owner or the address of a local variable/param.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
