[clang] [alpha.webkit.NoDeleteChecker] Blame the code that is actually unsafe (PR #224728)

[Balázs Benics via cfe-commits]

================
@@ -168,35 +168,52 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> {
   }
 
   // The offending statement is often just the nearest call to a function that
-  // is itself unsafe several levels down. Point at the function at the bottom
-  // of that chain, since that is where the fix belongs.
-  void addRootCauseNote(BasicBugReport &Report,
-                        const NonTrivialityReason &Reason) const {
-    const FunctionDecl *RootCause = Reason.RootCause;
-    // Implicit special members have nothing worth pointing at.
-    if (!RootCause || !RootCause->getLocation().isValid())
+  // is itself unsafe several levels down. Walk the whole chain of calls, one
+  // note per function, down to the code that destructs an object or the
+  // function without a visible definition, since that is where the fix belongs.
+  void addCallStackNotes(BasicBugReport &Report,
+                         const NonTrivialityReason &Reason) const {
+    ArrayRef<NonTrivialityReason::Frame> CallStack = Reason.CallStack;
+    if (CallStack.empty())
       return;
 
-    // Nothing to add when the offending statement is the call to the root
-    // cause; the primary diagnostic already points right at it.
+    // Nothing to add when the offending statement is the call to a function
+    // that is opaque or rejected outright; a note would only point back at its
+    // declaration, which the primary diagnostic already names.
+    const auto &First = CallStack.front();
     const FunctionDecl *Callee = getDirectCallee(Reason.OffendingStmt);
-    if (Callee && Callee->getCanonicalDecl() == RootCause->getCanonicalDecl())
+    if (CallStack.size() == 1 && !First.OffendingStmt && Callee &&
+        Callee->getCanonicalDecl() == First.Callee->getCanonicalDecl())
       return;
 
-    SmallString<100> Buf;
-    llvm::raw_svector_ostream Os(Buf);
-    printQuotedName(Os, RootCause);
-    if (RootCause->doesThisDeclarationHaveABody()) {
-      Os << " could destruct an object.";
-    } else {
-      Os << " has no visible definition here, so it is assumed to destruct an "
-            "object. Annotate it with "
-            "[[clang::annotate_type(\"webkit.nodelete\")]] if it does not.";
-    }
+    for (size_t I = 0; I < CallStack.size(); ++I) {
+      const FunctionDecl *Fn = CallStack[I].Callee;
+      const Stmt *Offender = CallStack[I].OffendingStmt;
+      // Implicit special members have nothing worth pointing at.
+      if (!Offender && !Fn->getLocation().isValid())
+        continue;
+
+      SmallString<100> Buf;
+      llvm::raw_svector_ostream Os(Buf);
+      printQuotedName(Os, Fn);
+      if (I + 1 < CallStack.size()) {
+        Os << " calls ";
+        printQuotedName(Os, CallStack[I + 1].Callee);
----------------
steakhal wrote:

I think this is format is pretty verbose. In this context, the user actually know what function it currently is and (usually) knows the callee because it's spelled right there (except for function pointers - which this checker doesn't handle anyway).

So technically, only the location of this note is important.
Another perspective is that as we chain these notes, (currently) we would spell each frame name twice.
Like 'x' -> 'y', then 'y' -> 'z'.  This is why I suggested to only mention one side of the edge, the callee (because that is at least spelled out on the line against which we issue the note)

https://github.com/llvm/llvm-project/pull/224728