[-Wunsafe-buffer-usage] Add AST info to the unclaimed DRE debug notes for analysis

[ziqingluo-90]

Summary

The debug note for an unclaimed DRE highlights a usage of an unsafe pointer that is not supported by our analysis.

For a better understand of what the unsupported cases are, we add more information to the debug note---a string of ancestor AST nodes of the unclaimed DRE. For example, an unclaimed DRE p in an expression *(p++) will result in a string starting with DRE, UnaryOperator(++), Paren, UnaryOperator(*).

To find out the most common patterns of those unsupported use cases, we add a simple script to build a prefix tree over those strings and count each prefix. The script reads input line by line, assumes a line is a list of words separated by ,s, and builds a prefix tree over those lists.

[ziqingluo-90]

Add a test

[NoQ]

I like where this is going, and I appreciate the test! Tests for tiny debug utilities aren't critical for the compiler, but they're a great way to demonstrate and document how to invoke the script and how it's supposed to work.

clang/lib/Analysis/UnsafeBufferUsage.cpp

38–39

Maybe just use something else as a separator, like ==> ? (Python .split() method will eat that happily.)

56

This looks like a manual reimplementation of a StmtVisitor. Maybe just use it directly?

class StmtDebugPrinter : public ConstStmtVisitor<StmtDebugPrinter, std::optional<std::string>> {
public:
  std::optional<std::string> VisitStmt(const Stmt *S) {
    return S->getStmtClassName();
  }
  std::optional<std::string> VisitBinaryOperator(const BinaryOperator *BO) {
    // Customize...
  }
};

clang/test/SemaCXX/warn-unsafe-buffer-usage-debug-unclaimed.cpp

10 ↗

(On Diff #556070)

You probably need to do something similar to test/Analysis/exploded-graph-rewriter/lit.local.cfg, to properly discover the right python executable which may not be straightforward on some buildbots (imagine Windows!).

clang/utils/analyze_safe_buffer_debug_notes.py

30–34

A bit more idiomatic I think (might behave differently around linebreaks).

[ziqingluo-90]

Address comments.

[NoQ]

clang/test/SemaCXX/warn-unsafe-buffer-usage-debug-unclaimed/lit.local.cfg

7

I might be overthinking/cargo-culting, but folks seem to never expose the python executable on its own, instead make a substitution for the entire tool. They also seem to use config.python_executable instead of sys.executable, though it's probably always the same no matter how they override it. Still, I might be missing some weird interactions. I think it's better to just do whatever everyone else does, something along the lines of

config.substitutions.append(
  (
    "%analyze_safe_buffer_debug_notes",
    "'%s' %s % (
      config.python_executable,
      os.path.join(config.clang_src_dir, "utils", "analyze_safe_buffer_debug_notes.py")
    )
  )
)

Also forward slashes will probably fail on Windows, so I can see how os.path.join is necessary. Though I suspect we will anyway end up with UNSUPPORTED: system-windows or REQUIRES: shell one way or another.

[ziqingluo-90]

address comments

[NoQ]

Thanks! Looks great now, let's try to land this!

clang/lib/Analysis/UnsafeBufferUsage.cpp

 //===- UnsafeBufferUsage.cpp - Replace pointers with modern C++ -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/StmtVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/SmallVector.h"
 #include <memory>
 #include <optional>
 #include <sstream>
 #include <queue>
 
 using namespace llvm;
 using namespace clang;
 using namespace ast_matchers;
 
+#ifndef NDEBUG
+namespace {
+class StmtDebugPrinter
+    : public ConstStmtVisitor<StmtDebugPrinter, std::string> {
+public:
+  std::string VisitStmt(const Stmt *S) { return S->getStmtClassName(); }
+
+  std::string VisitBinaryOperator(const BinaryOperator *BO) {
+    return "BinaryOperator(" + BO->getOpcodeStr().str() + ")";
+  }
+
+  std::string VisitUnaryOperator(const UnaryOperator *UO) {
+    return "UnaryOperator(" + UO->getOpcodeStr(UO->getOpcode()).str() + ")";
+  }
+
+  std::string VisitImplicitCastExpr(const ImplicitCastExpr *ICE) {
+    return "ImplicitCastExpr(" + std::string(ICE->getCastKindName()) + ")";
+  }
+};
+
+// Returns a string of ancestor `Stmt`s of the given `DRE` in such a form:
+// "DRE ==> parent-of-DRE ==> grandparent-of-DRE ==> ...".
+static std::string getDREAncestorString(const DeclRefExpr *DRE,
+                                        ASTContext &Ctx) {
+  std::stringstream SS;
+  const Stmt *St = DRE;
+  StmtDebugPrinter StmtPriner;
+
+  do {
+    SS << StmtPriner.Visit(St);
+
+    DynTypedNodeList StParents = Ctx.getParents(*St);
+
+    if (StParents.size() > 1)
+      return "unavailable due to multiple parents";
+    if (StParents.size() == 0)
+      break;
+    St = StParents.begin()->get<Stmt>();
+    if (St)
+      SS << " ==> ";
+  } while (St);
+  return SS.str();
+}
+} // namespace
+#endif /* NDEBUG */
+
 namespace clang::ast_matchers {
 // A `RecursiveASTVisitor` that traverses all descendants of a given node "n"
 // except for those belonging to a different callable of "n".
 class MatchDescendantVisitor
     : public RecursiveASTVisitor<MatchDescendantVisitor> {
 public:
   typedef RecursiveASTVisitor<MatchDescendantVisitor> VisitorBase;
 
#ifndef NDEBUG
                                     it->first->getNameAsString() +
                                     "' : has a reference type"));
 #endif
         it = FixablesForAllVars.byVar.erase(it);
       } else if (Tracker.hasUnclaimedUses(it->first)) {
 #ifndef NDEBUG
         auto AllUnclaimed = Tracker.getUnclaimedUses(it->first);
         for (auto UnclaimedDRE : AllUnclaimed) {
-          Handler.addDebugNoteForVar(
-              it->first, UnclaimedDRE->getBeginLoc(),
-                                     ("failed to produce fixit for '" + it->first->getNameAsString() +
-                                      "' : has an unclaimed use"));
-          }
+        std::string UnclaimedUseTrace =
+            getDREAncestorString(UnclaimedDRE, D->getASTContext());
+
+             "' : has an unclaimed use\nThe unclaimed DRE trace: " +
+             UnclaimedUseTrace));
 #endif
         it = FixablesForAllVars.byVar.erase(it);
       } else if (it->first->isInitCapture()) {
 #ifndef NDEBUG
         Handler.addDebugNoteForVar(
             it->first, it->first->getBeginLoc(),
                                    ("failed to produce fixit for '" + it->first->getNameAsString() +
                                     "' : init capture"));

clang/test/SemaCXX/warn-unsafe-buffer-usage-debug-unclaimed/lit.local.cfg

+# -*- Python -*-
+
+config.substitutions.append(
+  (
+    "%analyze_safe_buffer_debug_notes",
+    "'%s' %s" % (
+      config.python_executable,
+      os.path.join(config.clang_src_dir, "utils", "analyze_safe_buffer_debug_notes.py")
+    )
+  )
+)

clang/test/SemaCXX/warn-unsafe-buffer-usage-debug-unclaimed/warn-unsafe-buffer-usage-debug-unclaimed.cpp

+// RUN: %clang_cc1 -Wno-unused-value -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions \
+// RUN:            -mllvm -debug-only=SafeBuffers \
+// RUN:            -std=c++20 -verify=expected %s
+
+// RUN: %clang_cc1 -Wno-unused-value -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions \
+// RUN:            -mllvm -debug-only=SafeBuffers \
+// RUN:            -std=c++20 %s                  \
+// RUN:            2>&1 | grep 'The unclaimed DRE trace:' \
+// RUN:                 | sed 's/^The unclaimed DRE trace://' \
+// RUN:                 | %analyze_safe_buffer_debug_notes \
+// RUN:                 | FileCheck %s
+
+// This debugging facility is only available in debug builds.
+//
+// REQUIRES: asserts
+// REQUIRES: shell
+
+void test_unclaimed_use(int *p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  p++;           //  expected-note{{used in pointer arithmetic here}} \
+                     expected-note{{safe buffers debug: failed to produce fixit for 'p' : has an unclaimed use\n \
+ The unclaimed DRE trace: DeclRefExpr, UnaryOperator(++), CompoundStmt}}
+  *((p + 1) + 1); // expected-warning{{unsafe pointer arithmetic}}                      \
+                     expected-note{{used in pointer arithmetic here}}     \
+         expected-note{{safe buffers debug: failed to produce fixit for 'p' : has an unclaimed use\n \
+  The unclaimed DRE trace: DeclRefExpr, ImplicitCastExpr(LValueToRValue), BinaryOperator(+), ParenExpr, BinaryOperator(+), ParenExpr, UnaryOperator(*), CompoundStmt}}
+  p -= 1;         // expected-note{{used in pointer arithmetic here}} \
+         expected-note{{safe buffers debug: failed to produce fixit for 'p' : has an unclaimed use\n \
+  The unclaimed DRE trace: DeclRefExpr, BinaryOperator(-=), CompoundStmt}}
+  p--;            // expected-note{{used in pointer arithmetic here}} \
+         expected-note{{safe buffers debug: failed to produce fixit for 'p' : has an unclaimed use\n \
+  The unclaimed DRE trace: DeclRefExpr, UnaryOperator(--), CompoundStmt}}
+  p[5] = 5;       // expected-note{{used in buffer access here}}
+}
+
+// CHECK: Root # 1
+// CHECK: |- DeclRefExpr # 4
+// CHECK: |-- UnaryOperator(++) # 1
+// CHECK: |--- CompoundStmt # 1
+// CHECK: |-- ImplicitCastExpr(LValueToRValue) # 1
+// CHECK: |--- BinaryOperator(+) # 1
+// CHECK: |---- ParenExpr # 1
+// CHECK: |----- BinaryOperator(+) # 1
+// CHECK: |------ ParenExpr # 1
+// CHECK: |------- UnaryOperator(*) # 1
+// CHECK: |-------- CompoundStmt # 1
+// CHECK: |-- BinaryOperator(-=) # 1
+// CHECK: |--- CompoundStmt # 1
+// CHECK: |-- UnaryOperator(--) # 1
+// CHECK: |--- CompoundStmt # 1

clang/utils/analyze_safe_buffer_debug_notes.py

+import sys
+from collections import OrderedDict
+
+class Trie:
+    def __init__(self, name):
+        self.name = name
+        self.children = OrderedDict()
+        self.count = 1
+
+    def add(self, name):
+        if name in self.children:
+            self.children[name].count += 1
+        else:
+            self.children[name] = Trie(name)
+        return self.children[name]
+
+    def print(self, depth):
+        if depth > 0:
+            print('|', end="")
+        for i in range(depth):
+            print('-', end="")
+        if depth > 0:
+            print(end=" ")
+        print(self.name, '#', self.count)
+        for key, child in self.children.items():
+            child.print(depth + 1)
+
+
+Root = Trie("Root")
+
+if __name__ == "__main__":
+    for line in sys.stdin:
+        words = line.split('==>')
+        words = [word.strip() for word in words]
+        MyTrie = Root;
+        for word in words:
+            MyTrie = MyTrie.add(word)
+
+    Root.print(0)