================
@@ -11173,6 +11173,14 @@ def warn_lifetime_safety_field_origin_escape
: Warning<"return value depends on the object referenced by %0, not on
'*this'">,
InGroup<LifetimeSafetyFieldOriginEscape>,
DefaultIgnore;
+def warn_lifetime_safety_destructor_use_after_scope
+ : Warning<"%0 does not live long enough">,
+ InGroup<LifetimeSafetyDestructorUse>,
+ DefaultIgnore;
+def warn_lifetime_safety_destructor_use_after_invalidation
+ : Warning<"%0 is later invalidated">,
+ InGroup<LifetimeSafetyDestructorUse>,
+ DefaultIgnore;
----------------
usx95 wrote:
For implicit scope-exit uses, I feel `later used here` might be confusing. The
note points at the closing brace, and does not mention the borrow. Could we use
a note that names the action and the variable? For example:
- `later used by the destructor of 'r' at scope exit`
- `later used by the cleanup function of 'g' at scope exit`
This could also replace the bool `IsDestructorUse` that is threaded through the
overloads.
<details>
<summary>
Here is a idea (from AI) which I like:
</summary>
The idea is to describe a use once, in a `UseSite`, and let `UseFact` own it.
1. `Facts.h`: add UseKind and UseSite, and have UseFact hold one
```cpp
enum class UseKind { Explicit, Destructor, Cleanup };
struct UseSite {
SourceLocation Loc;
SourceRange Range; // empty for implicit uses
const VarDecl *ScopeExitVD = nullptr; // set for Destructor/Cleanup
UseKind Kind = UseKind::Explicit;
static UseSite fromExpr(const Expr *E) {
return {E->getExprLoc(), E->getSourceRange(), nullptr, UseKind::Explicit};
}
static UseSite scopeExit(SourceLocation Loc, const VarDecl *VD,
UseKind Kind) {
return {Loc, SourceRange(), VD, Kind};
}
bool isImplicit() const { return Kind != UseKind::Explicit; }
};
class UseFact : public Fact {
const Expr *UseExpr = nullptr; // still used by buildOriginFlowChain / dump
UseSite Site;
const OriginList *OList;
public:
static bool classof(const Fact *F) { return F->getKind() == Kind::Use; }
UseFact(const Expr *UseExpr, const OriginList *OList)
: Fact(Kind::Use), UseExpr(UseExpr), Site(UseSite::fromExpr(UseExpr)),
OList(OList) {}
UseFact(UseSite Site, const OriginList *OList)
: Fact(Kind::Use), Site(Site), OList(OList) {}
const OriginList *getUsedOrigins() const { return OList; }
const Expr *getUseExpr() const { return UseExpr; } // null if implicit
const UseSite &getUseSite() const { return Site; }
...
};
```
This replaces ImplicitLoc, IsDestructorUse, getUseLoc() and isDestructorUse().
2. `FactsGenerator.cpp`: pick the kind in one place
```cpp
if (HasCleanup || IsDestructorUse)
if (OriginList *List = getOriginsList(*LifetimeEndsVD))
CurrentBlockFacts.push_back(FactMgr.createFact<UseFact>(
UseSite::scopeExit(LifetimeEnds.getTriggerStmt()->getEndLoc(),
LifetimeEndsVD,
HasCleanup ? UseKind::Cleanup : UseKind::Destructor),
List));
```
3. `LifetimeSafety.h`: one overload per report
```cpp
virtual void reportUseAfterScope(const Expr *IssueExpr, const UseSite &Use,
const Expr *MovedExpr, SourceLocation FreeLoc,
llvm::ArrayRef<const Expr *> ExprChain) {}
virtual void
reportUseAfterInvalidation(const Expr *IssueExpr, const UseSite &Use,
const Expr *InvalidationExpr,
llvm::ArrayRef<const Expr *> ExprChain) {}
virtual void
reportUseAfterInvalidation(const ParmVarDecl *PVD, const UseSite &Use,
const Expr *InvalidationExpr,
llvm::ArrayRef<const Expr *> ExprChain) {}
```
4. `Checker.cpp`: drop the !UF->getUseExpr() branch
```cpp
if (const auto *UF = dyn_cast<const UseFact *>(CausingFact)) {
ExprChain = ...;
const UseSite &Use = UF->getUseSite();
if (Warning.InvalidatedByExpr) {
if (IssueExpr)
SemaHelper->reportUseAfterInvalidation(IssueExpr, Use,
Warning.InvalidatedByExpr,
ExprChain);
else if (InvalidatedPVD)
SemaHelper->reportUseAfterInvalidation(InvalidatedPVD, Use,
Warning.InvalidatedByExpr,
ExprChain);
} else
SemaHelper->reportUseAfterScope(IssueExpr, Use, MovedExpr, ExpiryLoc,
ExprChain);
}
```
`LiveOrigins.cpp::GetFactLoc` becomes `UF->getUseSite().Loc`.
5. `DiagnosticSemaKinds.td`: new notes
```cpp
def note_lifetime_safety_used_by_destructor
: Note<"later used by the destructor of %0 at scope exit">;
def note_lifetime_safety_used_by_cleanup
: Note<"later used by the cleanup function of %0 at scope exit">;
```
6. `SemaLifetimeSafety.h`: single emission point for the note
```cpp
void reportUseNote(const UseSite &U) {
switch (U.Kind) {
case UseKind::Explicit:
S.Diag(U.Loc, diag::note_lifetime_safety_used_here) << U.Range;
break;
case UseKind::Destructor:
S.Diag(U.Loc, diag::note_lifetime_safety_used_by_destructor)
<< U.ScopeExitVD;
break;
case UseKind::Cleanup:
S.Diag(U.Loc, diag::note_lifetime_safety_used_by_cleanup)
<< U.ScopeExitVD;
break;
}
}
static unsigned getInvalidationWarnDiag(bool IsFree, UseKind Kind) {
if (Kind == UseKind::Destructor)
return IsFree ? diag::warn_lifetime_safety_destructor_use_after_scope
:
diag::warn_lifetime_safety_destructor_use_after_invalidation;
return IsFree ? diag::warn_lifetime_safety_use_after_free
: diag::warn_lifetime_safety_invalidation;
}
```
`reportUseAfterScope` and `reportUseAfterInvalidationImpl` call
`reportUseNote(Use)` instead of emitting `note_lifetime_safety_used_here`
inline. `reportUseAfterInvalidationImpl` takes `const UseSite &` instead of
`UseLoc, UseRange, IsDestructorUse`. It calls
`getInvalidationWarnDiag(isa<CXXDeleteExpr>(InvalidationExpr), Use.Kind)`
instead of the nested if/else. In `reportUseAfterScope`, the destructor
diagnostic is chosen when `Use.Kind == UseKind::Destructor`. Cleanup keeps the
existing groups.
7. Tests
In `safety.cpp` and `invalidations.cpp`, replace the destructor `{{later used
here}}` with `{{later used by the destructor of 'r' at scope exit}}`.
Replace the cleanup `{{later used here}}` with `{{later used by the cleanup
function of 'g' at scope exit}}`.
`CapturedByThis` cases keep later used here.
</summary>
</details>
https://github.com/llvm/llvm-project/pull/204650
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
