[clang] 0683c4e - Revert "[clang] Diagnose dangling issues for the "Container<GSLPointer>" case. (#107213)"
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 12 00:25:06 PDT 2024
Author: Haojian Wu
Date: 2024-09-12T09:24:32+02:00
New Revision: 0683c4e839524c37fe4ddfa1bce1e31ba556041b
URL: https://github.com/llvm/llvm-project/commit/0683c4e839524c37fe4ddfa1bce1e31ba556041b
DIFF: https://github.com/llvm/llvm-project/commit/0683c4e839524c37fe4ddfa1bce1e31ba556041b.diff
LOG: Revert "[clang] Diagnose dangling issues for the "Container<GSLPointer>" case. (#107213)"
This reverts commit e50131aa068f74daa70d4135c92020aadae3af33.
It introduces a new false positive, see comment https://github.com/llvm/llvm-project/pull/107213#issuecomment-2345465256
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/AttrDocs.td
clang/lib/Sema/CheckExprLifetime.cpp
clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9860b25f2e7fa6..22749e96a7e3d3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -301,8 +301,6 @@ 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 9f72456d2da678..546e5100b79dd9 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6690,20 +6690,6 @@ 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 77c73f47658fe1..f62e18543851c1 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -267,26 +267,6 @@ 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()))
@@ -295,7 +275,7 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
return false;
if (!isRecordWithAttr<PointerAttr>(
Callee->getFunctionObjectParameterType()) &&
- !isGSLOwner(Callee->getFunctionObjectParameterType()))
+ !isRecordWithAttr<OwnerAttr>(Callee->getFunctionObjectParameterType()))
return false;
if (Callee->getReturnType()->isPointerType() ||
isRecordWithAttr<PointerAttr>(Callee->getReturnType())) {
@@ -433,7 +413,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() &&
- !isGSLOwner(ReturnType->getPointeeType())) {
+ !isRecordWithAttr<OwnerAttr>(ReturnType->getPointeeType())) {
for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit ||
PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
@@ -488,17 +468,12 @@ 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)) {
- 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]);
+ } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
+ CCE &&
+ CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) {
+ VisitGSLPointerArg(CCE->getConstructor(), Args[0]);
}
}
}
@@ -1010,12 +985,13 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
// int &p = *localUniquePtr;
// someContainer.add(std::move(localUniquePtr));
// return p;
- IsLocalGslOwner = isGSLOwner(L->getType());
+ IsLocalGslOwner = isRecordWithAttr<OwnerAttr>(L->getType());
if (pathContainsInit(Path) || !IsLocalGslOwner)
return false;
} else {
IsGslPtrValueFromGslTempOwner =
- MTE && !MTE->getExtendingDecl() && isGSLOwner(MTE->getType());
+ MTE && !MTE->getExtendingDecl() &&
+ isRecordWithAttr<OwnerAttr>(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.
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 234e06f069074b..59357d0730a7d9 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -158,30 +158,17 @@ 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;
};
@@ -216,21 +203,11 @@ 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 {
@@ -576,57 +553,3 @@ 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(), // 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}}
-
- 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}}
- 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()};
- 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::string_view();
- std::optional<std::string_view> n4 = std::make_optional(std::string_view());
- const char* b = "";
- 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) {
- 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());
-}
-
-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
More information about the cfe-commits
mailing list