llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: PushkarSingh (iitianpushkar)
<details>
<summary>Changes</summary>
## Summary
Improve Lifetime Safety diagnostic notes by including descriptions of the
affected storage in destruction and invalidation notes.
Examples of the new diagnostics:
```cpp
{
int value;
ptr = &value;
}
```
Before:
```
note: destroyed here
```
After:
```
note: 'value' is destroyed here at the end of its scope
```
For temporaries:
```
note: temporary object is destroyed here at the end of the full-expression
```
For invalidations:
```
note: 'container' is invalidated here
```
## Implementation
This change:
* Threads `AccessPath` information from `LifetimeChecker` into Lifetime Safety
diagnostic reporting APIs.
* Uses `AccessPath` in `SemaLifetimeSafety` to generate storage-aware
diagnostic notes.
* Distinguishes between scope-based destruction and temporary lifetime
expiration.
* Updates regression tests for the new diagnostics.
Fixes #<!-- -->200234.
---
Full diff: https://github.com/llvm/llvm-project/pull/204900.diff
6 Files Affected:
- (modified)
clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h (+20-7)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4-2)
- (modified) clang/lib/Analysis/LifetimeSafety/Checker.cpp (+13-7)
- (modified) clang/lib/Sema/SemaLifetimeSafety.h (+68-47)
- (modified) clang/test/Sema/LifetimeSafety/invalidations.cpp (+1-1)
- (modified) clang/test/Sema/LifetimeSafety/safety.cpp (+2-2)
``````````diff
diff --git
a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h
b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h
index 28886b826f72f..3e03de134ea6b 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h
@@ -64,6 +64,7 @@ class LifetimeSafetySemaHelper {
virtual void reportUseAfterScope(const Expr *IssueExpr, const Expr *UseExpr,
const Expr *MovedExpr,
SourceLocation FreeLoc,
+ const internal::AccessPath &ExpiredPath,
llvm::ArrayRef<const Expr *> ExprChain) {}
virtual void reportUseAfterReturn(const Expr *IssueExpr,
@@ -82,24 +83,36 @@ class LifetimeSafetySemaHelper {
// Reports when a reference/iterator is used after the container operation
// that invalidated it.
- virtual void reportUseAfterInvalidation(const Expr *IssueExpr,
+ virtual void reportUseAfterInvalidation(const Expr *IssueExpr,
const Expr *UseExpr,
- const Expr *InvalidationExpr) {}
+ const Expr *InvalidationExpr,
+ const internal::AccessPath
+ &InvalidatedPath) {}
virtual void reportUseAfterInvalidation(const ParmVarDecl *PVD,
const Expr *UseExpr,
- const Expr *InvalidationExpr) {}
+ const Expr *InvalidationExpr,
+ const internal::AccessPath
+ &InvalidatedPath) {}
virtual void reportInvalidatedField(const Expr *IssueExpr,
const FieldDecl *Field,
- const Expr *InvalidationExpr) {}
+ const Expr *InvalidationExpr,
+ const internal::AccessPath
+ &InvalidatedPath) {}
virtual void reportInvalidatedField(const ParmVarDecl *PVD,
const FieldDecl *Field,
- const Expr *InvalidationExpr) {}
+ const Expr *InvalidationExpr,
+ const internal::AccessPath
+ &InvalidatedPath) {}
virtual void reportInvalidatedGlobal(const Expr *IssueExpr,
const VarDecl *Global,
- const Expr *InvalidationExpr) {}
+ const Expr *InvalidationExpr,
+ const internal::AccessPath
+ &InvalidatedPath) {}
virtual void reportInvalidatedGlobal(const ParmVarDecl *PVD,
const VarDecl *Global,
- const Expr *InvalidationExpr) {}
+ const Expr *InvalidationExpr,
+ const internal::AccessPath
+ &InvalidatedPath) {}
using EscapingTarget =
llvm::PointerUnion<const Expr *, const FieldDecl *, const VarDecl *>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f84cd8dca6d4c..92e80f299dbbb 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11052,8 +11052,10 @@ def warn_lifetime_safety_inapplicable_lifetimebound
DefaultIgnore;
def note_lifetime_safety_used_here : Note<"later used here">;
-def note_lifetime_safety_invalidated_here : Note<"invalidated here">;
-def note_lifetime_safety_destroyed_here : Note<"destroyed here">;
+def note_lifetime_safety_invalidated_here : Note<"%0 is invalidated here">;
+def note_lifetime_safety_destroyed_here : Note<
+ "%0 is destroyed here at the end of "
+ "%select{its scope|the full-expression}1">;
def note_lifetime_safety_freed_here : Note<"freed here">;
def note_lifetime_safety_returned_here : Note<"returned here">;
def note_lifetime_safety_moved_here : Note<"potentially moved here">;
diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp
b/clang/lib/Analysis/LifetimeSafety/Checker.cpp
index d41d6f43f837b..4c6542599ef31 100644
--- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp
@@ -258,16 +258,18 @@ class LifetimeChecker {
if (IssueExpr)
// Use-after-invalidation of an object on stack.
SemaHelper->reportUseAfterInvalidation(IssueExpr, UF->getUseExpr(),
- Warning.InvalidatedByExpr);
+ Warning.InvalidatedByExpr,
+ L->getAccessPath());
else if (InvalidatedPVD)
// Use-after-invalidation of a parameter.
SemaHelper->reportUseAfterInvalidation(
- InvalidatedPVD, UF->getUseExpr(), Warning.InvalidatedByExpr);
-
+ InvalidatedPVD, UF->getUseExpr(), Warning.InvalidatedByExpr,
+ L->getAccessPath());
} else
// Scope-based expiry (use-after-scope).
SemaHelper->reportUseAfterScope(
IssueExpr, UF->getUseExpr(), MovedExpr, ExpiryLoc,
+ L->getAccessPath(),
getExprChain(LoanPropagation.buildOriginFlowChain(UF, LID)));
} else if (const auto *OEF =
@@ -279,12 +281,14 @@ class LifetimeChecker {
// Invalidated object on stack escapes to a field.
SemaHelper->reportInvalidatedField(IssueExpr,
FieldEscape->getFieldDecl(),
- Warning.InvalidatedByExpr);
+ Warning.InvalidatedByExpr,
+ L->getAccessPath());
else if (InvalidatedPVD)
// Invalidated parameter escapes to a field.
SemaHelper->reportInvalidatedField(InvalidatedPVD,
FieldEscape->getFieldDecl(),
- Warning.InvalidatedByExpr);
+ Warning.InvalidatedByExpr,
+ L->getAccessPath());
} else if (const auto *GlobalEscape =
dyn_cast<GlobalEscapeFact>(OEF)) {
// Invalidated object escapes to global or static storage.
@@ -293,12 +297,14 @@ class LifetimeChecker {
// storage.
SemaHelper->reportInvalidatedGlobal(IssueExpr,
GlobalEscape->getGlobal(),
- Warning.InvalidatedByExpr);
+ Warning.InvalidatedByExpr,
+ L->getAccessPath());
else if (InvalidatedPVD)
// Invalidated parameter escapes to global or static storage.
SemaHelper->reportInvalidatedGlobal(InvalidatedPVD,
GlobalEscape->getGlobal(),
- Warning.InvalidatedByExpr);
+ Warning.InvalidatedByExpr,
+ L->getAccessPath());
} else if (isa<ReturnEscapeFact>(OEF)) {
// FIXME: Diagnose invalidated return escapes separately.
} else
diff --git a/clang/lib/Sema/SemaLifetimeSafety.h
b/clang/lib/Sema/SemaLifetimeSafety.h
index a8bde363e3397..e67478afcf351 100644
--- a/clang/lib/Sema/SemaLifetimeSafety.h
+++ b/clang/lib/Sema/SemaLifetimeSafety.h
@@ -82,6 +82,7 @@ class LifetimeSafetySemaHelperImpl : public
LifetimeSafetySemaHelper {
void reportUseAfterScope(const Expr *IssueExpr, const Expr *UseExpr,
const Expr *MovedExpr, SourceLocation FreeLoc,
+ const internal::AccessPath &ExpiredPath,
llvm::ArrayRef<const Expr *> ExprChain) override {
unsigned DiagID = MovedExpr
? diag::warn_lifetime_safety_use_after_scope_moved
@@ -92,7 +93,10 @@ class LifetimeSafetySemaHelperImpl : public
LifetimeSafetySemaHelper {
if (MovedExpr)
S.Diag(MovedExpr->getExprLoc(), diag::note_lifetime_safety_moved_here)
<< MovedExpr->getSourceRange();
- S.Diag(FreeLoc, diag::note_lifetime_safety_destroyed_here);
+ S.Diag(FreeLoc, diag::note_lifetime_safety_destroyed_here)
+ << getDiagStorageDescription(ExpiredPath)
+ << (ExpiredPath.getKind() ==
+ internal::AccessPath::Kind::MaterializeTemporary);
reportAliasingChain(ExprChain);
@@ -161,87 +165,72 @@ class LifetimeSafetySemaHelperImpl : public
LifetimeSafetySemaHelper {
<< DanglingGlobal->getEndLoc();
}
- void reportUseAfterInvalidation(const Expr *IssueExpr, const Expr *UseExpr,
- const Expr *InvalidationExpr) override {
+ void reportUseAfterInvalidation(
+ const Expr *IssueExpr, const Expr *UseExpr, const Expr *InvalidationExpr,
+ const internal::AccessPath &InvalidatedPath) override {
auto WarnDiag = isa<CXXDeleteExpr>(InvalidationExpr)
? diag::warn_lifetime_safety_use_after_free
: diag::warn_lifetime_safety_invalidation;
- auto UseDiag = isa<CXXDeleteExpr>(InvalidationExpr)
- ? diag::note_lifetime_safety_freed_here
- : diag::note_lifetime_safety_invalidated_here;
S.Diag(IssueExpr->getExprLoc(), WarnDiag)
<< getDiagSubjectDescription(IssueExpr) << IssueExpr->getSourceRange();
- S.Diag(InvalidationExpr->getExprLoc(), UseDiag)
- << InvalidationExpr->getSourceRange();
+ reportInvalidationNote(InvalidationExpr, InvalidatedPath);
S.Diag(UseExpr->getExprLoc(), diag::note_lifetime_safety_used_here)
<< UseExpr->getSourceRange();
}
- void reportUseAfterInvalidation(const ParmVarDecl *PVD, const Expr *UseExpr,
- const Expr *InvalidationExpr) override {
+ void reportUseAfterInvalidation(
+ const ParmVarDecl *PVD, const Expr *UseExpr, const Expr
*InvalidationExpr,
+ const internal::AccessPath &InvalidatedPath) override {
auto WarnDiag = isa<CXXDeleteExpr>(InvalidationExpr)
? diag::warn_lifetime_safety_use_after_free
: diag::warn_lifetime_safety_invalidation;
- auto UseDiag = isa<CXXDeleteExpr>(InvalidationExpr)
- ? diag::note_lifetime_safety_freed_here
- : diag::note_lifetime_safety_invalidated_here;
S.Diag(PVD->getSourceRange().getBegin(), WarnDiag)
<< getDiagSubjectDescription(PVD) << PVD->getSourceRange();
- S.Diag(InvalidationExpr->getExprLoc(), UseDiag)
- << InvalidationExpr->getSourceRange();
+ reportInvalidationNote(InvalidationExpr, InvalidatedPath);
S.Diag(UseExpr->getExprLoc(), diag::note_lifetime_safety_used_here)
<< UseExpr->getSourceRange();
}
- void reportInvalidatedField(const Expr *IssueExpr,
- const FieldDecl *DanglingField,
- const Expr *InvalidationExpr) override {
- auto InvalidationDiag = isa<CXXDeleteExpr>(InvalidationExpr)
- ? diag::note_lifetime_safety_freed_here
- : diag::note_lifetime_safety_invalidated_here;
+ void
+ reportInvalidatedField(const Expr *IssueExpr, const FieldDecl *DanglingField,
+ const Expr *InvalidationExpr,
+ const internal::AccessPath &InvalidatedPath) override
{
S.Diag(IssueExpr->getExprLoc(),
diag::warn_lifetime_safety_invalidated_field)
<< getDiagSubjectDescription(IssueExpr)
<< getDiagSubjectDescription(DanglingField)
<< IssueExpr->getSourceRange();
- S.Diag(InvalidationExpr->getExprLoc(), InvalidationDiag)
- << InvalidationExpr->getSourceRange();
+ reportInvalidationNote(InvalidationExpr, InvalidatedPath);
S.Diag(DanglingField->getLocation(),
diag::note_lifetime_safety_dangling_field_here)
<< DanglingField->getEndLoc();
}
- void reportInvalidatedField(const ParmVarDecl *PVD,
- const FieldDecl *DanglingField,
- const Expr *InvalidationExpr) override {
- auto InvalidationDiag = isa<CXXDeleteExpr>(InvalidationExpr)
- ? diag::note_lifetime_safety_freed_here
- : diag::note_lifetime_safety_invalidated_here;
+ void
+ reportInvalidatedField(const ParmVarDecl *PVD, const FieldDecl
*DanglingField,
+ const Expr *InvalidationExpr,
+ const internal::AccessPath &InvalidatedPath) override
{
S.Diag(PVD->getSourceRange().getBegin(),
diag::warn_lifetime_safety_invalidated_field)
<< getDiagSubjectDescription(PVD)
<< getDiagSubjectDescription(DanglingField) << PVD->getSourceRange();
- S.Diag(InvalidationExpr->getExprLoc(), InvalidationDiag)
- << InvalidationExpr->getSourceRange();
+ reportInvalidationNote(InvalidationExpr, InvalidatedPath);
S.Diag(DanglingField->getLocation(),
diag::note_lifetime_safety_dangling_field_here)
<< DanglingField->getEndLoc();
}
- void reportInvalidatedGlobal(const Expr *IssueExpr,
- const VarDecl *DanglingGlobal,
- const Expr *InvalidationExpr) override {
- auto InvalidationDiag = isa<CXXDeleteExpr>(InvalidationExpr)
- ? diag::note_lifetime_safety_freed_here
- : diag::note_lifetime_safety_invalidated_here;
+ void reportInvalidatedGlobal(
+ const Expr *IssueExpr, const VarDecl *DanglingGlobal,
+ const Expr *InvalidationExpr,
+ const internal::AccessPath &InvalidatedPath) override {
S.Diag(IssueExpr->getExprLoc(),
diag::warn_lifetime_safety_invalidated_global)
<< getDiagSubjectDescription(IssueExpr)
<< getDiagSubjectDescription(DanglingGlobal)
<< IssueExpr->getSourceRange();
- S.Diag(InvalidationExpr->getExprLoc(), InvalidationDiag)
- << InvalidationExpr->getSourceRange();
+ reportInvalidationNote(InvalidationExpr, InvalidatedPath);
if (DanglingGlobal->isStaticLocal() ||
DanglingGlobal->isStaticDataMember())
S.Diag(DanglingGlobal->getLocation(),
diag::note_lifetime_safety_dangling_static_here)
@@ -252,18 +241,15 @@ class LifetimeSafetySemaHelperImpl : public
LifetimeSafetySemaHelper {
<< DanglingGlobal->getEndLoc();
}
- void reportInvalidatedGlobal(const ParmVarDecl *PVD,
- const VarDecl *DanglingGlobal,
- const Expr *InvalidationExpr) override {
- auto InvalidationDiag = isa<CXXDeleteExpr>(InvalidationExpr)
- ? diag::note_lifetime_safety_freed_here
- : diag::note_lifetime_safety_invalidated_here;
+ void reportInvalidatedGlobal(
+ const ParmVarDecl *PVD, const VarDecl *DanglingGlobal,
+ const Expr *InvalidationExpr,
+ const internal::AccessPath &InvalidatedPath) override {
S.Diag(PVD->getSourceRange().getBegin(),
diag::warn_lifetime_safety_invalidated_global)
<< getDiagSubjectDescription(PVD)
<< getDiagSubjectDescription(DanglingGlobal) << PVD->getSourceRange();
- S.Diag(InvalidationExpr->getExprLoc(), InvalidationDiag)
- << InvalidationExpr->getSourceRange();
+ reportInvalidationNote(InvalidationExpr, InvalidatedPath);
if (DanglingGlobal->isStaticLocal() ||
DanglingGlobal->isStaticDataMember())
S.Diag(DanglingGlobal->getLocation(),
diag::note_lifetime_safety_dangling_static_here)
@@ -441,6 +427,41 @@ class LifetimeSafetySemaHelperImpl : public
LifetimeSafetySemaHelper {
}
private:
+ std::string
+ getDiagStorageDescription(const internal::AccessPath &AccessPath) {
+ if (const auto *VD = AccessPath.getAsValueDecl()) {
+ std::string Res;
+ llvm::raw_string_ostream OS(Res);
+ OS << "'";
+ VD->getNameForDiagnostic(OS, S.getPrintingPolicy(), /*Qualified=*/false);
+ OS << "'";
+ return Res;
+ }
+ if (AccessPath.getAsMaterializeTemporaryExpr())
+ return "temporary object";
+ if (const auto *PVD = AccessPath.getAsPlaceholderParam())
+ return getDiagSubjectDescription(PVD);
+ if (AccessPath.getAsPlaceholderThis())
+ return "the object referenced by 'this'";
+ if (AccessPath.getAsNewAllocation())
+ return "allocated object";
+ return "object";
+ }
+
+ void reportInvalidationNote(const Expr *InvalidationExpr,
+ const internal::AccessPath &InvalidatedPath) {
+ if (isa<CXXDeleteExpr>(InvalidationExpr)) {
+ S.Diag(InvalidationExpr->getExprLoc(),
+ diag::note_lifetime_safety_freed_here)
+ << InvalidationExpr->getSourceRange();
+ return;
+ }
+ S.Diag(InvalidationExpr->getExprLoc(),
+ diag::note_lifetime_safety_invalidated_here)
+ << getDiagStorageDescription(InvalidatedPath)
+ << InvalidationExpr->getSourceRange();
+ }
+
std::pair<SourceLocation, StringRef>
getLifetimeBoundFixIt(const ParmVarDecl *Decl) {
SourceLocation InsertionPoint = Lexer::getLocForEndOfToken(
diff --git a/clang/test/Sema/LifetimeSafety/invalidations.cpp
b/clang/test/Sema/LifetimeSafety/invalidations.cpp
index 301822f066de8..b5eb886980ce0 100644
--- a/clang/test/Sema/LifetimeSafety/invalidations.cpp
+++ b/clang/test/Sema/LifetimeSafety/invalidations.cpp
@@ -8,7 +8,7 @@ namespace SimpleResize {
void IteratorInvalidAfterResize(int new_size) {
std::vector<int> v;
auto it = std::begin(v); // expected-warning {{local variable 'v' is later
invalidated}}
- v.resize(new_size); // expected-note {{invalidated here}}
+ v.resize(new_size); // expected-note {{'v' is invalidated here}}
*it; // expected-note {{later used here}}
}
diff --git a/clang/test/Sema/LifetimeSafety/safety.cpp
b/clang/test/Sema/LifetimeSafety/safety.cpp
index 7a2644e46a6e1..4d78276fa4f78 100644
--- a/clang/test/Sema/LifetimeSafety/safety.cpp
+++ b/clang/test/Sema/LifetimeSafety/safety.cpp
@@ -55,7 +55,7 @@ void simple_case() {
{
MyObj s;
p = &s; // expected-warning {{local variable 's' does not live long
enough}}
- } // expected-note {{destroyed here}}
+ } // expected-note {{'s' is destroyed here at the end of its
scope}}
(void)*p; // expected-note {{later used here}}
}
@@ -1926,7 +1926,7 @@ const std::string& identity(const std::string& in
[[clang::lifetimebound]]);
const S& identity(const S& in [[clang::lifetimebound]]);
void test_temporary() {
- const std::string& x = S().x(); // expected-warning {{temporary object does
not live long enough}} expected-note {{destroyed here}} \
+ const std::string& x = S().x(); // expected-warning {{temporary object does
not live long enough}} expected-note {{temporary object is destroyed here at
the end of the full-expression}} \
// expected-note {{result of call to 'x'
aliases the storage of temporary object}}
(void)x; // expected-note {{later used here}}
``````````
</details>
https://github.com/llvm/llvm-project/pull/204900
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
