[clang] Emit -Wdangling diagnoses for cases where a gsl-pointer is construct from a gsl-owner object in a container. #104556
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Haojian Wu (hokein) ChangesThe warning is not emitted for the case This bailout was introduced in this commit to address a false positive with Fixes #100384. Full diff: https://github.com/llvm/llvm-project/pull/104556.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ffdd063ec99037..c9864abc593e5e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -214,6 +214,8 @@ Improvements to Clang's diagnostics
- Clang now diagnoses the use of ``main`` in an ``extern`` context as invalid according to [basic.start.main] p3. Fixes #GH101512.
+- Clang now diagnoses dangling cases where a gsl-pointer is constructed from a gsl-owner object inside a container (#GH100384).
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 7389046eaddde1..b543c5e67f9550 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -191,7 +191,6 @@ struct IndirectLocalPathEntry {
LifetimeBoundCall,
TemporaryCopy,
LambdaCaptureInit,
- GslReferenceInit,
GslPointerInit,
GslPointerAssignment,
} Kind;
@@ -328,25 +327,13 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) {
static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
LocalVisitor Visit) {
- auto VisitPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
+ auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
// We are not interested in the temporary base objects of gsl Pointers:
// Temp().ptr; // Here ptr might not dangle.
if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
return;
- // Once we initialized a value with a reference, it can no longer dangle.
- if (!Value) {
- for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
- if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
- continue;
- if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
- PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
- return;
- break;
- }
- }
- Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
- : IndirectLocalPathEntry::GslReferenceInit,
- Arg, D});
+
+ Path.push_back({IndirectLocalPathEntry::GslPointerInit, Arg, D});
if (Arg->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
Visit,
@@ -360,29 +347,28 @@ static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
const auto *MD = cast_or_null<CXXMethodDecl>(MCE->getDirectCallee());
if (MD && shouldTrackImplicitObjectArg(MD))
- VisitPointerArg(MD, MCE->getImplicitObjectArgument(),
- !MD->getReturnType()->isReferenceType());
+ VisitPointerArg(MD, MCE->getImplicitObjectArgument());
return;
} else if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(Call)) {
FunctionDecl *Callee = OCE->getDirectCallee();
if (Callee && Callee->isCXXInstanceMember() &&
shouldTrackImplicitObjectArg(cast<CXXMethodDecl>(Callee)))
- VisitPointerArg(Callee, OCE->getArg(0),
- !Callee->getReturnType()->isReferenceType());
+ VisitPointerArg(Callee, OCE->getArg(0));
return;
} else if (auto *CE = dyn_cast<CallExpr>(Call)) {
FunctionDecl *Callee = CE->getDirectCallee();
if (Callee && shouldTrackFirstArgument(Callee))
- VisitPointerArg(Callee, CE->getArg(0),
- !Callee->getReturnType()->isReferenceType());
+ VisitPointerArg(Callee, CE->getArg(0));
return;
}
if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
const auto *Ctor = CCE->getConstructor();
const CXXRecordDecl *RD = Ctor->getParent();
- if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())
- VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0], true);
+ if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>() &&
+ isRecordWithAttr<OwnerAttr>(
+ CCE->getArg(0)->IgnoreImpCasts()->getType()))
+ VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]);
}
}
@@ -944,7 +930,6 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
case IndirectLocalPathEntry::LValToRVal:
case IndirectLocalPathEntry::LifetimeBoundCall:
case IndirectLocalPathEntry::TemporaryCopy:
- case IndirectLocalPathEntry::GslReferenceInit:
case IndirectLocalPathEntry::GslPointerInit:
case IndirectLocalPathEntry::GslPointerAssignment:
// These exist primarily to mark the path as not permitting or
@@ -975,7 +960,6 @@ static bool pathOnlyHandlesGslPointer(IndirectLocalPath &Path) {
case IndirectLocalPathEntry::LifetimeBoundCall:
continue;
case IndirectLocalPathEntry::GslPointerInit:
- case IndirectLocalPathEntry::GslReferenceInit:
case IndirectLocalPathEntry::GslPointerAssignment:
return true;
default:
@@ -1242,7 +1226,6 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
case IndirectLocalPathEntry::LifetimeBoundCall:
case IndirectLocalPathEntry::TemporaryCopy:
case IndirectLocalPathEntry::GslPointerInit:
- case IndirectLocalPathEntry::GslReferenceInit:
case IndirectLocalPathEntry::GslPointerAssignment:
// FIXME: Consider adding a note for these.
break;
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 09dfb2b5d96a89..c0fb2ed042cc7b 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -475,6 +475,18 @@ std::vector<int>::iterator noFalsePositiveWithVectorOfPointers() {
return iters.at(0);
}
+// GH100384
+std::string_view ContainerWithAnnotatedElements() {
+ std::string_view c1 = std::vector<std::string>().at(0); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+ c1 = std::vector<std::string>().at(0); // expected-warning {{object backing the pointer}}
+
+ // no warning on constructing from gsl-pointer
+ std::string_view c2 = std::vector<std::string_view>().at(0);
+
+ std::vector<std::string> local;
+ return local.at(0); // expected-warning {{address of stack memory associated with local variable 'local' returned}}
+}
+
void testForBug49342()
{
auto it = std::iter<char>{} - 2; // Used to be false positive.
|
|
Testing it a bit more, unfortunately, it has a false positive on the following case: |
hokein
force-pushed
the
lb-owner-container
branch
2 times, most recently
from
e0f1fca to
ea86000
Compare
|
I've refined the PR, and the new implementation eliminates the false positive mentioned above (I've also added a few more tests to verify this). Please take a look. The changes include some refactoring (I'm happy to split those out if they make the review process harder). |
Xazax-hun
approved these changes
Aug 19, 2024
usx95
approved these changes
Aug 20, 2024
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
Comment on lines
351
to
353
| Path.push_back({!ReturnType->isReferenceType() | ||
| ? IndirectLocalPathEntry::GslPointerInit | ||
| : IndirectLocalPathEntry::GslReferenceInit, |
There was a problem hiding this comment.
nit: simplify without !
ReturnType->isReferenceType() ? IndirectLocalPathEntry::GslReferenceInit: IndirectLocalPathEntry::GslPointerInit
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) { | ||
| const auto *Ctor = CCE->getConstructor(); | ||
| const CXXRecordDecl *RD = Ctor->getParent(); | ||
| if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>()) | ||
| VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0], true); | ||
| VisitPointerArg(Ctor, CCE->getArgs()[0]); |
There was a problem hiding this comment.
Is this a no-op change ? i.e., passing Ctor instead of the param decl ?
There was a problem hiding this comment.
Yeah, we changed the Decl here, but I think it is NFC -- the lifetimebound code path uses it for emitting diagnostic notes, but we don't use it for GSL code path. And it is unclear why we only pass the parameter del for this particular case (the other cases in this function, we all pass the Callee).
This was referenced Aug 23, 2024
hokein
force-pushed
the
lb-owner-container
branch
from
ea86000 to
aa8dda3
Compare
construct from a gsl-owner object in a container.
hokein
force-pushed
the
lb-owner-container
branch
from
aa8dda3 to
aba7fdb
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/139/builds/3380 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The warning is not emitted for the case
string_view c = std::vector<std::string>({""}).at(0);because we bail out during the visit of the LHS at this point in the code.This bailout was introduced in this commit to address a false positive with
vector<vector::iterator>({""}).at(0);. This PR refines that fix by ensuring that, for initialization involving a gsl-pointer, we only consider constructor calls that take the gsl-owner object.Fixes #100384.