[clang] Make lifetimebound and GSL analysis more coherent #105884
Merged
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
Member
|
@llvm/pr-subscribers-clang Author: Haojian Wu (hokein) ChangesReland the remaining change of #104906, with a proper fix for the regression. Patch is 22.58 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105884.diff 2 Files Affected:
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index c1362559536962..ef43a0d25e0c9e 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -237,13 +237,11 @@ static bool pathContainsInit(IndirectLocalPath &Path) {
static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
Expr *Init, LocalVisitor Visit,
- bool RevisitSubinits,
- bool EnableLifetimeWarnings);
+ bool RevisitSubinits);
static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
Expr *Init, ReferenceKind RK,
- LocalVisitor Visit,
- bool EnableLifetimeWarnings);
+ LocalVisitor Visit);
template <typename T> static bool isRecordWithAttr(QualType Type) {
if (auto *RD = Type->getAsCXXRecordDecl())
@@ -365,8 +363,7 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
// Visit lifetimebound or gsl-pointer arguments.
static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
- LocalVisitor Visit,
- bool EnableLifetimeWarnings) {
+ LocalVisitor Visit) {
const FunctionDecl *Callee;
ArrayRef<Expr *> Args;
@@ -381,6 +378,8 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
if (!Callee)
return;
+ bool EnableGSLAnalysis = !Callee->getASTContext().getDiagnostics().isIgnored(
+ diag::warn_dangling_lifetime_pointer, SourceLocation());
Expr *ObjectArg = nullptr;
if (isa<CXXOperatorCallExpr>(Call) && Callee->isCXXInstanceMember()) {
ObjectArg = Args[0];
@@ -393,11 +392,9 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
Path.push_back({IndirectLocalPathEntry::LifetimeBoundCall, Arg, D});
if (Arg->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
- Visit,
- /*EnableLifetimeWarnings=*/false);
+ Visit);
else
- visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
- /*EnableLifetimeWarnings=*/false);
+ visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
Path.pop_back();
};
auto VisitGSLPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
@@ -408,7 +405,8 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
// 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)
+ if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit ||
+ PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
continue;
if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
@@ -421,11 +419,9 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
Arg, D});
if (Arg->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
- Visit,
- /*EnableLifetimeWarnings=*/true);
+ Visit);
else
- visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
- /*EnableLifetimeWarnings=*/true);
+ visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
Path.pop_back();
};
@@ -448,7 +444,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
CheckCoroObjArg = false;
if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg)
VisitLifetimeBoundArg(Callee, ObjectArg);
- else if (EnableLifetimeWarnings) {
+ else if (EnableGSLAnalysis) {
if (auto *CME = dyn_cast<CXXMethodDecl>(Callee);
CME && shouldTrackImplicitObjectArg(CME))
VisitGSLPointerArg(Callee, ObjectArg,
@@ -461,15 +457,15 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
I != N; ++I) {
if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
- else if (EnableLifetimeWarnings && I == 0) {
+ else if (EnableGSLAnalysis && I == 0) { // GSL
if (shouldTrackFirstArgument(Callee)) {
VisitGSLPointerArg(Callee, Args[0],
!Callee->getReturnType()->isReferenceType());
- } else {
- if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
- CCE && CCE->getConstructor()->getParent()->hasAttr<PointerAttr>())
- VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0],
- true);
+ } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
+ CCE &&
+ CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) {
+ VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0],
+ true);
}
}
}
@@ -479,8 +475,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
/// glvalue expression \c Init.
static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
Expr *Init, ReferenceKind RK,
- LocalVisitor Visit,
- bool EnableLifetimeWarnings) {
+ LocalVisitor Visit) {
RevertToOldSizeRAII RAII(Path);
// Walk past any constructs which we can lifetime-extend across.
@@ -517,8 +512,7 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
else
// We can't lifetime extend through this but we might still find some
// retained temporaries.
- return visitLocalsRetainedByInitializer(Path, Init, Visit, true,
- EnableLifetimeWarnings);
+ return visitLocalsRetainedByInitializer(Path, Init, Visit, true);
}
// Step into CXXDefaultInitExprs so we can diagnose cases where a
@@ -532,21 +526,18 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init)) {
if (Visit(Path, Local(MTE), RK))
- visitLocalsRetainedByInitializer(Path, MTE->getSubExpr(), Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, MTE->getSubExpr(), Visit, true);
}
if (auto *M = dyn_cast<MemberExpr>(Init)) {
// Lifetime of a non-reference type field is same as base object.
if (auto *F = dyn_cast<FieldDecl>(M->getMemberDecl());
F && !F->getType()->isReferenceType())
- visitLocalsRetainedByInitializer(Path, M->getBase(), Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, M->getBase(), Visit, true);
}
if (isa<CallExpr>(Init))
- return visitFunctionCallArguments(Path, Init, Visit,
- EnableLifetimeWarnings);
+ return visitFunctionCallArguments(Path, Init, Visit);
switch (Init->getStmtClass()) {
case Stmt::DeclRefExprClass: {
@@ -565,8 +556,7 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
} else if (VD->getInit() && !isVarOnPath(Path, VD)) {
Path.push_back({IndirectLocalPathEntry::VarInit, DRE, VD});
visitLocalsRetainedByReferenceBinding(Path, VD->getInit(),
- RK_ReferenceBinding, Visit,
- EnableLifetimeWarnings);
+ RK_ReferenceBinding, Visit);
}
}
break;
@@ -578,15 +568,13 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
// handling all sorts of rvalues passed to a unary operator.
const UnaryOperator *U = cast<UnaryOperator>(Init);
if (U->getOpcode() == UO_Deref)
- visitLocalsRetainedByInitializer(Path, U->getSubExpr(), Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, U->getSubExpr(), Visit, true);
break;
}
case Stmt::ArraySectionExprClass: {
- visitLocalsRetainedByInitializer(Path,
- cast<ArraySectionExpr>(Init)->getBase(),
- Visit, true, EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(
+ Path, cast<ArraySectionExpr>(Init)->getBase(), Visit, true);
break;
}
@@ -594,11 +582,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
case Stmt::BinaryConditionalOperatorClass: {
auto *C = cast<AbstractConditionalOperator>(Init);
if (!C->getTrueExpr()->getType()->isVoidType())
- visitLocalsRetainedByReferenceBinding(Path, C->getTrueExpr(), RK, Visit,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByReferenceBinding(Path, C->getTrueExpr(), RK, Visit);
if (!C->getFalseExpr()->getType()->isVoidType())
- visitLocalsRetainedByReferenceBinding(Path, C->getFalseExpr(), RK, Visit,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByReferenceBinding(Path, C->getFalseExpr(), RK, Visit);
break;
}
@@ -621,8 +607,7 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
/// the prvalue expression \c Init.
static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
Expr *Init, LocalVisitor Visit,
- bool RevisitSubinits,
- bool EnableLifetimeWarnings) {
+ bool RevisitSubinits) {
RevertToOldSizeRAII RAII(Path);
Expr *Old;
@@ -663,18 +648,16 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
if (VD && VD->getType().isConstQualified() && VD->getInit() &&
!isVarOnPath(Path, VD)) {
Path.push_back({IndirectLocalPathEntry::VarInit, DRE, VD});
- visitLocalsRetainedByInitializer(
- Path, VD->getInit(), Visit, true, EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, VD->getInit(), Visit,
+ true);
}
} else if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L)) {
if (MTE->getType().isConstQualified())
visitLocalsRetainedByInitializer(Path, MTE->getSubExpr(),
- Visit, true,
- EnableLifetimeWarnings);
+ Visit, true);
}
return false;
- },
- EnableLifetimeWarnings);
+ });
// We assume that objects can be retained by pointers cast to integers,
// but not if the integer is cast to floating-point type or to _Complex.
@@ -703,9 +686,8 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
// Model array-to-pointer decay as taking the address of the array
// lvalue.
Path.push_back({IndirectLocalPathEntry::AddressOf, CE});
- return visitLocalsRetainedByReferenceBinding(Path, CE->getSubExpr(),
- RK_ReferenceBinding, Visit,
- EnableLifetimeWarnings);
+ return visitLocalsRetainedByReferenceBinding(
+ Path, CE->getSubExpr(), RK_ReferenceBinding, Visit);
default:
return;
@@ -720,8 +702,7 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
// lifetime of the array exactly like binding a reference to a temporary.
if (auto *ILE = dyn_cast<CXXStdInitializerListExpr>(Init))
return visitLocalsRetainedByReferenceBinding(Path, ILE->getSubExpr(),
- RK_StdInitializerList, Visit,
- EnableLifetimeWarnings);
+ RK_StdInitializerList, Visit);
if (InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) {
// We already visited the elements of this initializer list while
@@ -732,14 +713,12 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
if (ILE->isTransparent())
return visitLocalsRetainedByInitializer(Path, ILE->getInit(0), Visit,
- RevisitSubinits,
- EnableLifetimeWarnings);
+ RevisitSubinits);
if (ILE->getType()->isArrayType()) {
for (unsigned I = 0, N = ILE->getNumInits(); I != N; ++I)
visitLocalsRetainedByInitializer(Path, ILE->getInit(I), Visit,
- RevisitSubinits,
- EnableLifetimeWarnings);
+ RevisitSubinits);
return;
}
@@ -752,14 +731,12 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
if (RD->isUnion() && ILE->getInitializedFieldInUnion() &&
ILE->getInitializedFieldInUnion()->getType()->isReferenceType())
visitLocalsRetainedByReferenceBinding(Path, ILE->getInit(0),
- RK_ReferenceBinding, Visit,
- EnableLifetimeWarnings);
+ RK_ReferenceBinding, Visit);
else {
unsigned Index = 0;
for (; Index < RD->getNumBases() && Index < ILE->getNumInits(); ++Index)
visitLocalsRetainedByInitializer(Path, ILE->getInit(Index), Visit,
- RevisitSubinits,
- EnableLifetimeWarnings);
+ RevisitSubinits);
for (const auto *I : RD->fields()) {
if (Index >= ILE->getNumInits())
break;
@@ -768,14 +745,13 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
Expr *SubInit = ILE->getInit(Index);
if (I->getType()->isReferenceType())
visitLocalsRetainedByReferenceBinding(Path, SubInit,
- RK_ReferenceBinding, Visit,
- EnableLifetimeWarnings);
+ RK_ReferenceBinding, Visit);
else
// This might be either aggregate-initialization of a member or
// initialization of a std::initializer_list object. Regardless,
// we should recursively lifetime-extend that initializer.
- visitLocalsRetainedByInitializer(
- Path, SubInit, Visit, RevisitSubinits, EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, SubInit, Visit,
+ RevisitSubinits);
++Index;
}
}
@@ -796,10 +772,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
Path.push_back({IndirectLocalPathEntry::LambdaCaptureInit, E, &Cap});
if (E->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, E, RK_ReferenceBinding,
- Visit, EnableLifetimeWarnings);
+ Visit);
else
- visitLocalsRetainedByInitializer(Path, E, Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, E, Visit, true);
if (Cap.capturesVariable())
Path.pop_back();
}
@@ -813,16 +788,14 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
Expr *Arg = MTE->getSubExpr();
Path.push_back({IndirectLocalPathEntry::TemporaryCopy, Arg,
CCE->getConstructor()});
- visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
- /*EnableLifetimeWarnings*/ false);
+ visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
Path.pop_back();
}
}
}
if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init))
- return visitFunctionCallArguments(Path, Init, Visit,
- EnableLifetimeWarnings);
+ return visitFunctionCallArguments(Path, Init, Visit);
switch (Init->getStmtClass()) {
case Stmt::UnaryOperatorClass: {
@@ -838,8 +811,7 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
Path.push_back({IndirectLocalPathEntry::AddressOf, UO});
visitLocalsRetainedByReferenceBinding(Path, UO->getSubExpr(),
- RK_ReferenceBinding, Visit,
- EnableLifetimeWarnings);
+ RK_ReferenceBinding, Visit);
}
break;
}
@@ -852,11 +824,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
break;
if (BO->getLHS()->getType()->isPointerType())
- visitLocalsRetainedByInitializer(Path, BO->getLHS(), Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, BO->getLHS(), Visit, true);
else if (BO->getRHS()->getType()->isPointerType())
- visitLocalsRetainedByInitializer(Path, BO->getRHS(), Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, BO->getRHS(), Visit, true);
break;
}
@@ -866,11 +836,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
// In C++, we can have a throw-expression operand, which has 'void' type
// and isn't interesting from a lifetime perspective.
if (!C->getTrueExpr()->getType()->isVoidType())
- visitLocalsRetainedByInitializer(Path, C->getTrueExpr(), Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, C->getTrueExpr(), Visit, true);
if (!C->getFalseExpr()->getType()->isVoidType())
- visitLocalsRetainedByInitializer(Path, C->getFalseExpr(), Visit, true,
- EnableLifetimeWarnings);
+ visitLocalsRetainedByInitializer(Path, C->getFalseExpr(), Visit, true);
break;
}
@@ -972,8 +940,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
const InitializedEntity *InitEntity,
const InitializedEntity *ExtendingEntity,
LifetimeKind LK,
- const AssignedEntity *AEntity, Expr *Init,
- bool EnableLifetimeWarnings) {
+ const AssignedEntity *AEntity, Expr *Init) {
assert((AEntity && LK == LK_Assignment) ||
(InitEntity && LK != LK_Assignment));
// If this entity doesn't have an interesting lifetime, don't bother looking
@@ -1267,19 +1234,20 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
};
llvm::SmallVector<IndirectLocalPathEntry, 8> Path;
- if (EnableLifetimeWarnings && LK == LK_Assignment &&
+ if (!S...
[truncated]
|
hokein
commented
Aug 23, 2024
| Visit); | ||
| else | ||
| visitLocalsRetainedByInitializer(Path, Arg, Visit, true, | ||
| /*EnableLifetimeWarnings=*/false); |
Collaborator
Author
There was a problem hiding this comment.
We change the behavior on this line and the line above. (Previously, we never run the GSL code path for the Arg).
The fix is on Line 409.
Collaborator
There was a problem hiding this comment.
I think the PR needs to be renamed. From a PR with the title Remove the EnableLifetimeWarnings flag in lifetime analysis. I'd expect either an NFC or removing a feature, not an actual bug fix.
I think we should either split this up in two PRs, or rename the title to describe the bugfix aspect.
Collaborator
Author
There was a problem hiding this comment.
Good point. It is hard to split in two PRs, refined the PR to focus on the bug fixes rather than the refactoring change.
usx95
reviewed
Aug 26, 2024
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>()) | ||
| VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]); | ||
| else if (EnableLifetimeWarnings && I == 0) { | ||
| else if (EnableGSLAnalysis && I == 0) { // GSL |
| if (!Value) { | ||
| for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) { | ||
| if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit) | ||
| if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit || |
Contributor
There was a problem hiding this comment.
Can you move this to a separate function (like we do for pathOnlyHandlesGslPointer) and document what it is supposed to check.
Collaborator
Author
There was a problem hiding this comment.
This code isn't used elsewhere, so I think it's fine to leave it as is (and I'm not sure it's a good idea to expose it as a function).
I think this section could benefit from better documentation. Essentially, it bails out cases where we construct a GSL pointer from a reference, and before this change, the path could only contain GSL-specific IndirectLocalPathEntry. With this change, we should also consider the LifetimeBoundCall.
There’s also some refactoring and enhancement in this part of the code in #104556, and I’ll add more documentation there.
Contributor
There was a problem hiding this comment.
Alright. Thanks. Yeah the main idea is to add more documentation. Should be fine if you are already on it.
usx95
approved these changes
Aug 26, 2024
Xazax-hun
approved these changes
Aug 27, 2024
Collaborator
Xazax-hun
left a comment
Xazax-hun
left a comment
There was a problem hiding this comment.
Thanks, this is a nice cleanup, but I'd prefer either the PR to be split up, or renamed to emphasize the bug fix aspect of the PR.
hokein
force-pushed
the
remove-lb-flag
branch
from
0e81c91 to
ba4f1bf
Compare
hokein
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This allows clang to detect more use-after-free bugs (shown in the #100549).
This relands the remaining change (removing the EnableLifetimeWarnings flag) in #104906, with a proper fix for the regression.
Fixes #100549