-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang] Implement lifetime analysis for lifetime_capture_by(X) #115921
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.
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
Changes from 4 commits
2cef37e
8522c58
f349aa8
ab30f17
1036946
c1ac443
b56631a
9a2999d
46ca05c
1b3f6d8
37259f2
5cd4938
feedd49
cfe3362
ea301d7
6039d2f
7c28943
c8843e5
3768a55
774b43b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,10 +45,14 @@ enum LifetimeKind { | |
/// a default member initializer), the program is ill-formed. | ||
LK_MemInitializer, | ||
|
||
/// The lifetime of a temporary bound to this entity probably ends too soon, | ||
/// The lifetime of a temporary bound to this entity may end too soon, | ||
/// because the entity is a pointer and we assign the address of a temporary | ||
/// object to it. | ||
LK_Assignment, | ||
|
||
/// The lifetime of a temporary bound to this entity may end too soon, | ||
/// because the entity may capture the reference to a temporary object. | ||
LK_LifetimeCapture, | ||
}; | ||
using LifetimeResult = | ||
llvm::PointerIntPair<const InitializedEntity *, 3, LifetimeKind>; | ||
|
@@ -193,6 +197,7 @@ struct IndirectLocalPathEntry { | |
VarInit, | ||
LValToRVal, | ||
LifetimeBoundCall, | ||
LifetimeCapture, | ||
TemporaryCopy, | ||
LambdaCaptureInit, | ||
GslReferenceInit, | ||
|
@@ -249,7 +254,7 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path, | |
LocalVisitor Visit); | ||
|
||
template <typename T> static bool isRecordWithAttr(QualType Type) { | ||
if (auto *RD = Type->getAsCXXRecordDecl()) | ||
if (auto *RD = Type.getNonReferenceType()->getAsCXXRecordDecl()) | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return RD->hasAttr<T>(); | ||
return false; | ||
} | ||
|
@@ -1049,6 +1054,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I, | |
case IndirectLocalPathEntry::AddressOf: | ||
case IndirectLocalPathEntry::LValToRVal: | ||
case IndirectLocalPathEntry::LifetimeBoundCall: | ||
case IndirectLocalPathEntry::LifetimeCapture: | ||
case IndirectLocalPathEntry::TemporaryCopy: | ||
case IndirectLocalPathEntry::GslReferenceInit: | ||
case IndirectLocalPathEntry::GslPointerInit: | ||
|
@@ -1082,6 +1088,7 @@ static bool pathOnlyHandlesGslPointer(const IndirectLocalPath &Path) { | |
case IndirectLocalPathEntry::VarInit: | ||
case IndirectLocalPathEntry::AddressOf: | ||
case IndirectLocalPathEntry::LifetimeBoundCall: | ||
case IndirectLocalPathEntry::LifetimeCapture: | ||
continue; | ||
case IndirectLocalPathEntry::GslPointerInit: | ||
case IndirectLocalPathEntry::GslReferenceInit: | ||
|
@@ -1110,12 +1117,13 @@ static bool shouldRunGSLAssignmentAnalysis(const Sema &SemaRef, | |
isAssignmentOperatorLifetimeBound(Entity.AssignmentOperator))); | ||
} | ||
|
||
static void checkExprLifetimeImpl(Sema &SemaRef, | ||
const InitializedEntity *InitEntity, | ||
const InitializedEntity *ExtendingEntity, | ||
LifetimeKind LK, | ||
const AssignedEntity *AEntity, Expr *Init) { | ||
static void | ||
checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity, | ||
const InitializedEntity *ExtendingEntity, LifetimeKind LK, | ||
const AssignedEntity *AEntity, | ||
const CapturingEntity *CapEntity, Expr *Init) { | ||
assert((AEntity && LK == LK_Assignment) || | ||
(CapEntity && LK == LK_LifetimeCapture) || | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(InitEntity && LK != LK_Assignment)); | ||
// If this entity doesn't have an interesting lifetime, don't bother looking | ||
// for temporaries within its initializer. | ||
|
@@ -1199,6 +1207,23 @@ static void checkExprLifetimeImpl(Sema &SemaRef, | |
break; | ||
} | ||
|
||
case LK_LifetimeCapture: { | ||
// The captured entity has lifetime beyond the full-expression, | ||
// and the capturing entity does too, so don't warn. | ||
if (!MTE) | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return false; | ||
assert(shouldLifetimeExtendThroughPath(Path) == | ||
PathLifetimeKind::NoExtend && | ||
"No lifetime extension in function calls"); | ||
if (CapEntity->Entity) | ||
SemaRef.Diag(DiagLoc, diag::warn_dangling_reference_captured) | ||
<< CapEntity->Entity << DiagRange; | ||
else | ||
SemaRef.Diag(DiagLoc, diag::warn_dangling_reference_captured_by_unknown) | ||
<< DiagRange; | ||
return false; | ||
} | ||
|
||
case LK_Assignment: { | ||
if (!MTE || pathContainsInit(Path)) | ||
return false; | ||
|
@@ -1359,6 +1384,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef, | |
break; | ||
|
||
case IndirectLocalPathEntry::LifetimeBoundCall: | ||
case IndirectLocalPathEntry::LifetimeCapture: | ||
case IndirectLocalPathEntry::TemporaryCopy: | ||
case IndirectLocalPathEntry::GslPointerInit: | ||
case IndirectLocalPathEntry::GslReferenceInit: | ||
|
@@ -1411,7 +1437,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef, | |
// warnings or errors on inner temporaries within this one's initializer. | ||
return false; | ||
}; | ||
|
||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool HasReferenceBinding = Init->isGLValue(); | ||
llvm::SmallVector<IndirectLocalPathEntry, 8> Path; | ||
if (LK == LK_Assignment && | ||
shouldRunGSLAssignmentAnalysis(SemaRef, *AEntity)) { | ||
|
@@ -1420,9 +1446,18 @@ static void checkExprLifetimeImpl(Sema &SemaRef, | |
? IndirectLocalPathEntry::LifetimeBoundCall | ||
: IndirectLocalPathEntry::GslPointerAssignment, | ||
Init}); | ||
} else if (LK == LK_LifetimeCapture) { | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Path.push_back({IndirectLocalPathEntry::LifetimeCapture, Init}); | ||
if (isRecordWithAttr<PointerAttr>(Init->getType())) | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
HasReferenceBinding = false; | ||
// Skip the top MTE if it is a temporary object of the pointer-like type | ||
// itself. | ||
if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init); | ||
MTE && isPointerLikeType(Init->getType())) | ||
Init = MTE->getSubExpr(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that this code originated from my patch that added assignment support for "container." That patch has some issues: although it passed the unit tests, it failed to diagnose cases in real STL headers. I'm not sure I fully understand the implementation here (my initial idea was to leverage the existing code paths for --- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -1212,9 +1212,9 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
// and the capturing entity does too, so don't warn.
if (!MTE)
return false;
- assert(shouldLifetimeExtendThroughPath(Path) ==
- PathLifetimeKind::NoExtend &&
- "No lifetime extension in function calls");
+ // assert(shouldLifetimeExtendThroughPath(Path) ==
+ // PathLifetimeKind::NoExtend &&
+ // "No lifetime extension in function calls");
if (CapEntity->Entity)
SemaRef.Diag(DiagLoc, diag::warn_dangling_reference_captured)
<< CapEntity->Entity << DiagRange;
@@ -1451,14 +1451,8 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
break;
}
case LK_LifetimeCapture: {
- Path.push_back({IndirectLocalPathEntry::LifetimeCapture, Init});
- if (isRecordWithAttr<PointerAttr>(Init->getType()))
- HasReferenceBinding = false;
- // Skip the top MTE if it is a temporary object of the pointer-like type
- // itself.
- if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init);
- MTE && isPointerLikeType(Init->getType()))
- Init = MTE->getSubExpr();
+ if (isPointerLikeType(Init->getType()))
+ Path.push_back({IndirectLocalPathEntry::GslPointerInit, Init});
break;
}
default: The tests passed, and it also fixes the known false positives:
Would you mind taking it further? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. Simplified. I have kept the
I think it is important to stop lifetime extension. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assert is being triggered because Path is empty. It seems that lifetime extension is only relevant in the Additionally, I’d prefer not to introduce a new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I see this method being used only for But this feels safe now. Thanks for digging into this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that the latest revision doesn't contain the change. |
||
} | ||
|
||
if (Init->isGLValue()) | ||
if (HasReferenceBinding) | ||
visitLocalsRetainedByReferenceBinding(Path, Init, RK_ReferenceBinding, | ||
TemporaryVisitor); | ||
else | ||
|
@@ -1438,13 +1473,13 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity, | |
LifetimeKind LK = LTResult.getInt(); | ||
const InitializedEntity *ExtendingEntity = LTResult.getPointer(); | ||
checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, | ||
/*AEntity*/ nullptr, Init); | ||
/*AEntity=*/nullptr, /*CapEntity=*/nullptr, Init); | ||
} | ||
|
||
void checkExprLifetimeMustTailArg(Sema &SemaRef, | ||
const InitializedEntity &Entity, Expr *Init) { | ||
checkExprLifetimeImpl(SemaRef, &Entity, nullptr, LK_MustTail, | ||
/*AEntity*/ nullptr, Init); | ||
/*AEntity=*/nullptr, /*CapEntity=*/nullptr, Init); | ||
} | ||
|
||
void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity, | ||
|
@@ -1460,7 +1495,15 @@ void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity, | |
|
||
checkExprLifetimeImpl(SemaRef, /*InitEntity=*/nullptr, | ||
/*ExtendingEntity=*/nullptr, LK_Assignment, &Entity, | ||
Init); | ||
/*CapEntity=*/nullptr, Init); | ||
} | ||
|
||
void checkExprLifetime(Sema &SemaRef, const CapturingEntity &Entity, | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Expr *Init) { | ||
return checkExprLifetimeImpl(SemaRef, /*InitEntity=*/nullptr, | ||
/*ExtendingEntity=*/nullptr, LK_LifetimeCapture, | ||
/*AEntity=*/nullptr, | ||
/*CapEntity=*/&Entity, Init); | ||
} | ||
|
||
} // namespace clang::sema |
Uh oh!
There was an error while loading. Please reload this page.