llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Yuan Suo (suoyuan666) <details> <summary>Changes</summary> After adding `buildOriginFlowChain`, we need to choose a diagnostic type that is as simple as possible to verify its feasibility during `Sema` diagnostics. I did not choose the annotation suggestions described in https://github.com/llvm/llvm-project/pull/188467/#issuecomment-4359071778 as the first target to implement, because it does not seem to occur within a single CFG block. The `IssueFact` always resides in the block preceding the `OriginEscapesFact`, which causes me to always get an empty `OriginFlowChain`. --- To be honest, I feel that the defined `Note<"%0 aliases the storage of %1">` might not be entirely appropriate. In short, since `buildOrginFlowChain` only yields `SrcOrigin`, I prefer not to modify its implementation for now. Consequently, I am simply retrieving the expression corresponding to `SrcOrigin`. However, expressions can vary significantly. If we want to extract a meaningful `Name` from them, it could get quite complicated. I'm not sure if there is already an existing public function that can help with this. --- Full diff: https://github.com/llvm/llvm-project/pull/199345.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h (+4-3) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1) - (modified) clang/lib/Analysis/LifetimeSafety/Checker.cpp (+54-2) - (modified) clang/lib/Sema/SemaLifetimeSafety.h (+26-3) - (modified) clang/test/Sema/warn-lifetime-safety.cpp (+19-17) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h index ec9f83748ad8d..5ff9a3d880369 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h @@ -61,9 +61,10 @@ class LifetimeSafetySemaHelper { LifetimeSafetySemaHelper() = default; virtual ~LifetimeSafetySemaHelper() = default; - virtual void reportUseAfterScope(const Expr *IssueExpr, const Expr *UseExpr, - const Expr *MovedExpr, - SourceLocation FreeLoc) {} + virtual void + reportUseAfterScope(const Expr *IssueExpr, const Expr *UseExpr, + const Expr *MovedExpr, SourceLocation FreeLoc, + llvm::SmallVector<const Expr *> OriginExprChain) {} virtual void reportUseAfterReturn(const Expr *IssueExpr, const Expr *ReturnExpr, diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 142f848f37e50..140385081e9bc 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11040,6 +11040,7 @@ def note_lifetime_safety_escapes_to_field_here: Note<"escapes to this field">; def note_lifetime_safety_escapes_to_global_here: Note<"escapes to this global storage">; def note_lifetime_safety_escapes_to_static_storage_here: Note<"escapes to this static storage">; def note_lifetime_safety_lifetimebound_here: Note<"'lifetimebound' attribute appears here on the definition">; +def note_lifetime_safety_note_alias_chain : Note<"%0 aliases the storage of %1">; def warn_lifetime_safety_intra_tu_param_suggestion : Warning<"parameter in intra-TU function should be marked " diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp index d6a15139aa4ea..91bcd6a22c81d 100644 --- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp @@ -260,10 +260,25 @@ class LifetimeChecker { SemaHelper->reportUseAfterInvalidation( InvalidatedPVD, UF->getUseExpr(), Warning.InvalidatedByExpr); - } else + } else { // Scope-based expiry (use-after-scope). + + llvm::SmallVector<OriginID> OriginFlowChain; + + for (const OriginList *Cur = UF->getUsedOrigins(); Cur; + Cur = Cur->peelOuterOrigin()) + if (LoanPropagation.getLoans(Cur->getOuterOriginID(), UF) + .contains(LID)) + OriginFlowChain = LoanPropagation.buildOriginFlowChain( + FactMgr, UF, Cur->getOuterOriginID(), LID); + + const llvm::SmallVector<const Expr *> OriginExprChain = + buildExprOrDeclChain(OriginFlowChain); SemaHelper->reportUseAfterScope(IssueExpr, UF->getUseExpr(), - MovedExpr, ExpiryLoc); + MovedExpr, ExpiryLoc, + OriginExprChain); + } + } else if (const auto *OEF = CausingFact.dyn_cast<const OriginEscapesFact *>()) { if (Warning.InvalidatedByExpr) { @@ -498,6 +513,43 @@ class LifetimeChecker { } } } + + /// Retrieve a list of reliable expressions from OriginIFlowChain that + /// can be used for Sema warnings. + /// + /// Although the AST node corresponding to Origin can be either a + /// `const Expr *` or a `const ValueDecl *`, `buildOriginFlowChain` only + /// collects Origins from RHS expressions. Therefore, we do not need to + /// handle non-expression cases here. + llvm::SmallVector<const Expr *> + buildExprOrDeclChain(llvm::ArrayRef<OriginID> OriginFlowChain) { + llvm::SmallVector<const Expr *> rs; + const SourceManager &SM = AST.getSourceManager(); + + auto InsertOrReplace = [&rs, &SM](const Expr *NewNode) { + if (!NewNode) + return; + SourceLocation NewLocation = NewNode->getExprLoc(); + if (NewLocation.isInvalid()) + return; + + const Expr *LastNode = rs.back(); + SourceLocation LastLocation = LastNode->getExprLoc(); + if (SM.getSpellingLineNumber(LastLocation) == + SM.getSpellingLineNumber(NewLocation)) + rs.back() = NewNode; + else + rs.push_back(NewNode); + }; + + for (const OriginID CurrOID : OriginFlowChain) + if (!rs.empty()) + InsertOrReplace(FactMgr.getOriginMgr().getOrigin(CurrOID).getExpr()); + else + rs.push_back(FactMgr.getOriginMgr().getOrigin(CurrOID).getExpr()); + + return rs; + } }; } // namespace diff --git a/clang/lib/Sema/SemaLifetimeSafety.h b/clang/lib/Sema/SemaLifetimeSafety.h index af5202c33fed0..882755bbd0529 100644 --- a/clang/lib/Sema/SemaLifetimeSafety.h +++ b/clang/lib/Sema/SemaLifetimeSafety.h @@ -54,14 +54,24 @@ inline bool IsLifetimeSafetyEnabled(Sema &S, const Decl *D) { return false; } +inline StringRef formatExpr(const Expr *E) { + const Expr *PureExpr = E->IgnoreImpCasts(); + if (const DeclRefExpr *DRExpr = dyn_cast<DeclRefExpr>(PureExpr)) + return DRExpr->getDecl()->getName(); + + // TODO: Add support for more expression types. + return ""; +} + class LifetimeSafetySemaHelperImpl : public LifetimeSafetySemaHelper { public: LifetimeSafetySemaHelperImpl(Sema &S) : S(S) {} - void reportUseAfterScope(const Expr *IssueExpr, const Expr *UseExpr, - const Expr *MovedExpr, - SourceLocation FreeLoc) override { + void reportUseAfterScope( + const Expr *IssueExpr, const Expr *UseExpr, const Expr *MovedExpr, + SourceLocation FreeLoc, + llvm::SmallVector<const Expr *> OriginExprChain) override { S.Diag(IssueExpr->getExprLoc(), MovedExpr ? diag::warn_lifetime_safety_use_after_scope_moved : diag::warn_lifetime_safety_use_after_scope) @@ -70,6 +80,19 @@ class LifetimeSafetySemaHelperImpl : public LifetimeSafetySemaHelper { S.Diag(MovedExpr->getExprLoc(), diag::note_lifetime_safety_moved_here) << MovedExpr->getSourceRange(); S.Diag(FreeLoc, diag::note_lifetime_safety_destroyed_here); + + StringRef IssueStr; + for (const Expr *CurrExpr : reverse(OriginExprChain)) { + if (IssueStr.empty()) { + IssueStr = formatExpr(CurrExpr); + continue; + } + + S.Diag(CurrExpr->getBeginLoc(), + diag::note_lifetime_safety_note_alias_chain) + << CurrExpr->getSourceRange() << formatExpr(CurrExpr) << IssueStr; + } + S.Diag(UseExpr->getExprLoc(), diag::note_lifetime_safety_used_here) << UseExpr->getSourceRange(); } diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp index 70ec968dfd5c1..25fde5a3f0544 100644 --- a/clang/test/Sema/warn-lifetime-safety.cpp +++ b/clang/test/Sema/warn-lifetime-safety.cpp @@ -92,7 +92,7 @@ void pointer_chain() { { MyObj s; p = &s; // expected-warning {{does not live long enough}} - q = p; + q = p; // expected-note {{p aliases the storage of s}} } // expected-note {{destroyed here}} (void)*q; // expected-note {{later used here}} } @@ -102,7 +102,7 @@ void propagation_gsl() { { MyObj s; v1 = s; // expected-warning {{object whose reference is captured does not live long enough}} - v2 = v1; + v2 = v1; // expected-note {{v1 aliases the storage of s}} } // expected-note {{destroyed here}} v2.use(); // expected-note {{later used here}} } @@ -696,7 +696,7 @@ void test_lifetimebound_multi_level() { int* p = nullptr; int** pp = &p; int*** ppp = &pp; // expected-warning {{object whose reference is captured does not live long enough}} - result = return_inner_ptr_addr(ppp); + result = return_inner_ptr_addr(ppp); // expected-note {{ppp aliases the storage of pp}} } // expected-note {{destroyed here}} (void)**result; // expected-note {{used here}} } @@ -828,7 +828,7 @@ void lifetimebound_multiple_args_definite() { View v; { MyObj obj1, obj2; - v = Choose(true, + v = Choose(true, // expected-note {{aliases the storage of obj1}} expected-note {{aliases the storage of obj2}} obj1, // expected-warning {{object whose reference is captured does not live long enough}} obj2); // expected-warning {{object whose reference is captured does not live long enough}} } // expected-note 2 {{destroyed here}} @@ -945,8 +945,8 @@ void lifetimebound_return_reference() { { MyObj obj; View temp_v = obj; // expected-warning {{object whose reference is captured does not live long enough}} - const MyObj& ref = GetObject(temp_v); - ptr = &ref; + const MyObj& ref = GetObject(temp_v); // expected-note {{temp_v aliases the storage of obj}} + ptr = &ref; // expected-note {{aliases the storage of obj}} } // expected-note {{destroyed here}} (void)*ptr; // expected-note {{later used here}} } @@ -1324,7 +1324,9 @@ void foobar() { View view; { StatusOr<MyObj> string_or = getStringOr(); - view = string_or. // expected-warning {{object whose reference is captured does not live long enough}} + view = string_or. // expected-warning {{object whose reference is captured does not live long enough}} \ + // expected-note {{aliases the storage of string_or}} \ + // expected-note {{aliases the storage of string_or}} value(); } // expected-note {{destroyed here}} (void)view; // expected-note {{later used here}} @@ -1759,7 +1761,7 @@ void test_temporary() { { S s; const std::string& zz = s.x(); // expected-warning {{object whose reference is captured does not live long enough}} - z = zz; + z = zz; // expected-note {{aliases the storage of s}} } // expected-note {{destroyed here}} (void)z; // expected-note {{later used here}} } @@ -1789,7 +1791,7 @@ void uaf() { { S str; S* p = &str; // expected-warning {{object whose reference is captured does not live long enough}} - view = p->s; + view = p->s; // expected-note {{p aliases the storage of str}} } // expected-note {{destroyed here}} (void)view; // expected-note {{later used here}} } @@ -1814,8 +1816,8 @@ void uaf_union() { std::string_view view; { U u = U{"hello"}; - U* up = &u; // expected-warning {{object whose reference is captured does not live long enough}} - view = up->s; + U* up = &u; // expected-warning {{object whose reference is captured does not live long enough}} + view = up->s; // expected-note {{up aliases the storage of u}} } // expected-note {{destroyed here}} (void)view; // expected-note {{later used here}} } @@ -1832,7 +1834,7 @@ void uaf_anonymous_union() { { AnonymousUnion au; AnonymousUnion* up = &au; // expected-warning {{object whose reference is captured does not live long enough}} - ip = &up->x; + ip = &up->x; // expected-note {{up aliases the storage of au}} } // expected-note {{destroyed here}} (void)ip; // expected-note {{later used here}} } @@ -2387,7 +2389,7 @@ void assignment_propagation() { { std::string str{"abc"}; a = getS(str); // expected-warning {{object whose reference is captured does not live long enough}} - b = a; + b = a; // expected-note {{a aliases the storage of str}} } // expected-note {{destroyed here}} use(b); // expected-note {{later used here}} } @@ -2397,7 +2399,7 @@ void chained_defaulted_assignment_propagation() { { std::string str{"abc"}; S a = getS(str); // expected-warning {{object whose reference is captured does not live long enough}} - c = b = a; + c = b = a; // expected-note {{a aliases the storage of str}} } // expected-note {{destroyed here}} use(c); // expected-note {{later used here}} } @@ -2630,8 +2632,8 @@ void nested_local_pointer() { { Bar v; p = Pointer(v); // expected-warning {{object whose reference is captured does not live long enough}} - pp = Pointer(p); - ppp = Pointer(pp); + pp = Pointer(p); // expected-note {{p aliases the storage of v}} + ppp = Pointer(pp); // expected-note {{pp aliases the storage of v}} } // expected-note {{destroyed here}} use(***ppp); // expected-note {{later used here}} } @@ -2758,7 +2760,7 @@ void new_pointer_from_pointer() { { MyObj obj; MyObj *q = &obj; // expected-warning {{object whose reference is captured does not live long enough}} - p = new MyObj *(q); + p = new MyObj *(q); // expected-note {{q aliases the storage of obj}} } // expected-note {{destroyed here}} (void)**p; // expected-note {{later used here}} } `````````` </details> https://github.com/llvm/llvm-project/pull/199345 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
