[-Wunsafe-buffer-usage] Add a facility for debugging low fixit coverage.

[t-rasmud]

Summary

This patch adds extra notes to -Wunsafe-buffer-usage warnings, which explain why a fixit wasn't produced. When applied to a large body of real-world code, it'll help us gather statistics that will help us figure out which fixable gadgets (or other features of the fixit machine) to "invest" into.

This is a debugging facility intended for developer use only; it is activated by passing -mllvm -debug-only=SafeBuffers to clang, so it's carefully hidden and undiscoverable, and it's only available in builds with assertions.

Offline we've identified the following sources of false negatives which these notes can help us categorize:

1. unsafe operation not performed on a supported kind of variable (eg. member variable);
2. use site of the unsafe variable not claimed by any fixable gadgets (so we need to cover it with a new fixable);
3. one of the "implicated" variables has unclaimed uses (so we can't build the implication graph);
4. fixit generation for the declaration of the variable has failed (eg. declaration is in a macro);
5. fixit generation for one of the fixable gadgets has failed (eg. the use is in a macro).

Currently this patch covers #5; it probably makes sense to cover all of these except maybe #1 (this one's usually obvious).

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

578

I changed this to make F->getBaseStmt() available more often, but ran into more problems of this kind that were much more strange than this one. I'll probably investigate more.

[NoQ]

private: => public:

(for some reason it didn't complain until I did a full rebuild)

[NoQ]

Add the other missing base statement. Debugger was acting weirdly so I thought we had bigger problems, but it's just the other thing missing. It's likely that we ultimately want a better interface for that anyway.

[t-rasmud]

I will add debug notes for the rest of the cases not addressed in this patch.

[t-rasmud]

This patch addresses cases 2, 3, and 4 described in the summary (i.e) adds debug notes for unclaimed uses of variables and for failed fixit generation of variable declarations.

[NoQ]

Awesome!!

Did you try running it on some real code? Does this actually cover most cases? (I suspect that (1.) is going to be the most popular case, but that's also the easiest case to diagnose visually. We might still want a note if we wanted to prioritize among non-variables, but that's some work, maybe we can get back to this later.)

I have a few minor nitpicks, but generally LGTM!

clang/lib/Analysis/UnsafeBufferUsage.cpp

1727–1728

We might want to silence -Wunused-parameter in release builds.

Maybe it's better to put the entire parameter under #ifndef NDEBUG, but it's definitely more clumsy.

There's also __attribute__((unused)) aka [[maybe_unused]] but it looks like we're not supposed to use it in LLVM for variables:

173 // Some compilers warn about unused functions. When a function is sometimes
174 // used or not depending on build settings (e.g. a function only called from
175 // within "assert"), this attribute can be used to suppress such warnings.
176 //
177 // However, it shouldn't be used for unused *variables*, as those have a much
178 // more portable solution:
179 //   (void)unused_var_name;
180 // Prefer cast-to-void wherever it is sufficient.
181 #if __has_attribute(unused)
182 #define LLVM_ATTRIBUTE_UNUSED __attribute__((__unused__))
183 #else
184 #define LLVM_ATTRIBUTE_UNUSED
185 #endif

1741–1742

This comment is probably duplicated everywhere by accident; it doesn't make much sense in most of these places.

1763

2278

getName() crashes on various anonymous variables, whereas getNameAsString() always gives an answer (even if it's not always a "good" answer), which is more suitable for diagnostics.

2279

Do you want to include a "failde to produce fixit..." text in this message and the next message, for consistency?

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2126–2127

This is my original comment right? It's actually also outdated and should be removed, I already erased the runtime check, and I already moved the architectural recommendation to implementation of getBaseStmt() in individual gadgets for which it doesn't make sense.

[ziqingluo-90]

This is a lot of work, thank you @t-rasmud & @NoQ !

I have a minor suggestion: can we use some macros to make the debug stub even shorter?
The prefix "failed to produce fixit for declaration" is used in many places so probably we do not have to repeat it everywhere. And, maybe some prefixes could be a bit more blurry so that they can be shared. For example, we can just replace "failed to produce fixit for parm var decl" with "failed to produce fixit for declaration". We have source location and more specific message attached to the note so we are not losing information I think.

I'm imagining something like this:

#define DEBUG_NOTE_DECL_FAIL(D, Msg)  \
Handler.addDebugNoteForVar((D), (D)->getBeginLoc(),  "failed to produce fixit for declaration "##Msg)

#define DEBUG_NOTE_GADGET_FAIL(Gadget, Msg)  ...

Does it make sense to you?

[t-rasmud]

In D154880#4533035, @ziqingluo-90 wrote:

This is a lot of work, thank you @t-rasmud & @NoQ !

I have a minor suggestion: can we use some macros to make the debug stub even shorter?
The prefix "failed to produce fixit for declaration" is used in many places so probably we do not have to repeat it everywhere. And, maybe some prefixes could be a bit more blurry so that they can be shared. For example, we can just replace "failed to produce fixit for parm var decl" with "failed to produce fixit for declaration". We have source location and more specific message attached to the note so we are not losing information I think.

I'm imagining something like this:

#define DEBUG_NOTE_DECL_FAIL(D, Msg)  \
Handler.addDebugNoteForVar((D), (D)->getBeginLoc(),  "failed to produce fixit for declaration "##Msg)

#define DEBUG_NOTE_GADGET_FAIL(Gadget, Msg)  ...

Does it make sense to you?

I like this suggestion. I've made changes to replace Handler.addDebugNoteForVar for declarations. The Gadget case appears just once as of now, so I've left it as is.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1712–1713

Ooo that's actually really nice! Maybe you can go even further and add this extra harness, so that to eliminate the need for #ifndef NDEBUG at every use.

[NoQ]

LGTM! I'm excited to learn what this new facility discovers!

clang/lib/Analysis/UnsafeBufferUsage.cpp

1712–1717

An extra bit of paranoia.

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H
 #define LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/Stmt.h"
+#include "llvm/Support/Debug.h"
 
 namespace clang {
 
 using DefMapTy = llvm::DenseMap<const VarDecl *, std::vector<const VarDecl *>>;
 
 /// The interface that lets the caller handle unsafe buffer usage analysis
 /// results by overriding this class's handle... methods.
 class UnsafeBufferUsageHandler {
+#ifndef NDEBUG
+private:
+  // A self-debugging facility that you can use to notify the user when
+  // suggestions or fixits are incomplete.
+  // Uses std::function to avoid computing the message when it won't
+  // actually be displayed.
+  using DebugNote = std::pair<SourceLocation, std::string>;
+  using DebugNoteList = std::vector<DebugNote>;
+  using DebugNoteByVar = std::map<const VarDecl *, DebugNoteList>;
+  DebugNoteByVar DebugNotesByVar;
+#endif
+
 public:
   UnsafeBufferUsageHandler() = default;
   virtual ~UnsafeBufferUsageHandler() = default;
 
   /// This analyses produces large fixits that are organized into lists
   /// of primitive fixits (individual insertions/removals/replacements).
   using FixItList = llvm::SmallVectorImpl<FixItHint>;
 
   /// Invoked when an unsafe operation over raw pointers is found.
   virtual void handleUnsafeOperation(const Stmt *Operation,
                                      bool IsRelatedToDecl) = 0;
 
   /// Invoked when a fix is suggested against a variable. This function groups
   /// all variables that must be fixed together (i.e their types must be changed to the
   /// same target type to prevent type mismatches) into a single fixit.
   virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
                                          const DefMapTy &VarGrpMap,
                                          FixItList &&Fixes) = 0;
 
+#ifndef NDEBUG
+public:
+  bool areDebugNotesRequested() {
+    DEBUG_WITH_TYPE("SafeBuffers", return true);
+    return false;
+  }
+
+  void addDebugNoteForVar(const VarDecl *VD, SourceLocation Loc,
+                          std::string Text) {
+    if (areDebugNotesRequested())
+      DebugNotesByVar[VD].push_back(std::make_pair(Loc, Text));
+  }
+
+  void clearDebugNotes() {
+    if (areDebugNotesRequested())
+      DebugNotesByVar.clear();
+  }
+#endif
+
+public:
   /// Returns a reference to the `Preprocessor`:
   virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0;
 
   /// Returns the text indicating that the user needs to provide input there:
   virtual std::string
   getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") const {
     std::string s = std::string("<# ");
     s += HintTextToUser;

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_unsafe_buffer_operation : Warning<
   "unsafe buffer access|function introduces unsafe buffer manipulation}0">,
   InGroup<UnsafeBufferUsage>, DefaultIgnore;
 def note_unsafe_buffer_operation : Note<
   "used%select{| in pointer arithmetic| in buffer access}0 here">;
 def note_unsafe_buffer_variable_fixit_group : Note<
   "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information%select{|, and change %2 to '%select{std::span|std::array|std::span::iterator}1' to propagate bounds information between them}3">;
 def note_safe_buffer_usage_suggestions_disabled : Note<
   "pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
+#ifndef NDEBUG
+// Not a user-facing diagnostic. Useful for debugging false negatives in
+// -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).
+def note_safe_buffer_debug_mode : Note<"safe buffers debug: %0">;
+#endif
+
 def err_loongarch_builtin_requires_la32 : Error<
   "this builtin requires target: loongarch32">;
 
 def err_builtin_pass_in_regs_non_class : Error<
   "argument %0 is not an unqualified class type">;
 
 
 // WebAssembly reference type and table diagnostics.

clang/lib/Analysis/UnsafeBufferUsage.cpp

#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
   /// Useful for implementing the static matcher() methods
   /// that are expected from all non-abstract subclasses.
   using Matcher = decltype(stmt());
 
   Gadget(Kind K) : K(K) {}
 
   Kind getKind() const { return K; }
 
+#ifndef NDEBUG
+  StringRef getDebugName() const {
+    switch (K) {
+#define GADGET(x) case Kind::x: return #x;
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+    }
+  }
+#endif
+
   virtual bool isWarningGadget() const = 0;
   virtual const Stmt *getBaseStmt() const = 0;
 
   /// Returns the list of pointer-type variables on which this gadget performs
   /// its operation. Typically, there's only one variable. This isn't a list
   /// of all DeclRefExprs in the gadget's AST!
   virtual DeclUseList getClaimedVarUseSites() const = 0;
 
auto PtrInitStmt = declStmt(hasSingleDecl(varDecl(
                                                   bind(PointerInitRHSTag)))).
                                               bind(PointerInitLHSTag)));
 
     return stmt(PtrInitStmt);
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
 
-  virtual const Stmt *getBaseStmt() const override { return nullptr; }
+  virtual const Stmt *getBaseStmt() const override { return PtrInitRHS; }
 
   virtual DeclUseList getClaimedVarUseSites() const override {
     return DeclUseList{PtrInitRHS};
   }
 
   virtual std::optional<std::pair<const VarDecl *, const VarDecl *>>
   getStrategyImplications() const override {
       return std::make_pair(PtrInitLHS,
populateInitializerFixItWithSpan(const Expr *Init, ASTContext &Ctx,
 
   StrBuffer.append(", ");
   StrBuffer.append(ExtentText);
   StrBuffer.append("}");
   FixIts.push_back(FixItHint::CreateInsertion(*LocPassInit, StrBuffer.str()));
   return FixIts;
 }
 
 // For a `VarDecl` of the form `T  * var (= Init)?`, this
 // function generates a fix-it for the declaration, which re-declares `var` to
 // be of `span<T>` type and transforms the initializer, if present, to a span
 // constructor---`span<T> var {Init, Extent}`, where `Extent` may need the user
 // to fill in.
 //
 // FIXME: support Multi-level pointers
 //
 // Parameters:
 //   `D` a pointer the variable declaration node
 //   `Ctx` a reference to the ASTContext
 // Returns:
 //    the generated fix-it
 static FixItList fixVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
                                     const StringRef UserFillPlaceHolder) {
   const QualType &SpanEltT = D->getType()->getPointeeType();
   assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");
 
   FixItList FixIts{};
   std::optional<SourceLocation>
       ReplacementLastLoc; // the inclusive end location of the replacement
   const SourceManager &SM = Ctx.getSourceManager();
 
   if (const Expr *Init = D->getInit()) {
     FixItList InitFixIts =
         populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder);
 
     if (InitFixIts.empty())
       return {};
 
     // The loc right before the initializer:
     ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1);
     FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
                   std::make_move_iterator(InitFixIts.end()));
   } else
     ReplacementLastLoc = getEndCharLoc(D, SM, Ctx.getLangOpts());
 
   // Producing fix-it for variable declaration---make `D` to be of span type:
   SmallString<32> Replacement;
   raw_svector_ostream OS(Replacement);
 
   OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName();
 
   if (!ReplacementLastLoc)
     return {};
 
   FixIts.push_back(FixItHint::CreateReplacement(
       SourceRange{D->getBeginLoc(), *ReplacementLastLoc}, OS.str()));
   return FixIts;
 }
 
 static bool hasConflictingOverload(const FunctionDecl *FD) {
   return !FD->getDeclContext()->lookup(FD->getDeclName()).isSingleResult();
 }
 
 // Returns the text representation of clang::unsafe_buffer_usage attribute.
 static std::string getUnsafeBufferUsageAttributeText() {
   static const char *const RawAttr = "[[clang::unsafe_buffer_usage]]";
   std::stringstream SS;
   SS << RawAttr << getEndOfLine().str();
if (FixItsForVariable[VD].empty()) {
       FixItsForVariable.erase(VD);
       continue;
     }
     bool ImpossibleToFix = false;
     llvm::SmallVector<FixItHint, 16> FixItsForVD;
     for (const auto &F : Fixables) {
       std::optional<FixItList> Fixits = F->getFixits(S);
       if (!Fixits) {
+#ifndef NDEBUG
+        // FIXME: F->getBaseStmt() should never be null!
+        // (Or we should build a better interface for this.)
+        Handler.addDebugNoteForVar(
+            VD,
+            F->getBaseStmt() ? F->getBaseStmt()->getBeginLoc()
+                             : VD->getBeginLoc(),
+            ("gadget '" + F->getDebugName() + "' refused to produce a fix")
+                .str());
+#endif
         ImpossibleToFix = true;
         break;
       } else {
         const FixItList CorrectFixes = Fixits.value();
         FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(),
                            CorrectFixes.end());
       }
     }
for (const VarDecl *VD : UnsafeVars) {
     S.set(VD, Strategy::Kind::Span);
   }
   return S;
 }
 
 void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler,
                                    bool EmitSuggestions) {
+#ifndef NDEBUG
+  Handler.clearDebugNotes();
+#endif
+
   assert(D && D->getBody());
 
   // Do not emit fixit suggestions for functions declared in an
   // extern "C" block.
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
       for (FunctionDecl *FReDecl : FD->redecls()) {
       if (FReDecl->isExternC()) {
         EmitSuggestions = false;
for (auto it = FixablesForAllVars.byVar.cbegin();
           Tracker.hasUnclaimedUses(it->first)) {
       it = FixablesForAllVars.byVar.erase(it);
     } else {
       ++it;
     }
   }
 
   llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
   for (const auto &[VD, ignore] : FixablesForAllVars.byVar)
     UnsafeVars.push_back(VD);
 
   // Fixpoint iteration for pointer assignments
   using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>;
   DepMapTy DependenciesMap{};
   DepMapTy PtrAssignmentGraph{};
 
   for (auto it : FixablesForAllVars.byVar) {
     for (const FixableGadget *fixable : it.second) {

clang/lib/Sema/AnalysisBasedWarnings.cpp

if (!Fixes.empty()) {
         FD << AllVars << 1;
       } else {
         FD << "" << 0;
       }
 
       for (const auto &F : Fixes)
         FD << F;
     }
+
+#ifndef NDEBUG
+    if (areDebugNotesRequested())
+      for (const DebugNote &Note: DebugNotesByVar[Variable])
+        S.Diag(Note.first, diag::note_safe_buffer_debug_mode) << Note.second;
+#endif
   }
 
   bool isSafeBufferOptOut(const SourceLocation &Loc) const override {
     return S.PP.isSafeBufferOptOut(S.getSourceManager(), Loc);
   }
 };
 } // namespace
 

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

+// RUN: %clang_cc1 -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions \
+// RUN:            -std=c++20 -verify=expected %s
+// RUN: %clang_cc1 -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions \
+// RUN:            -mllvm -debug-only=SafeBuffers \
+// RUN:            -std=c++20 -verify=expected,debug %s
+
+// A generic -debug would also enable our notes. This is probably fine.
+//
+// RUN: %clang_cc1 -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions \
+// RUN:            -std=c++20 -mllvm -debug \
+// RUN:            -verify=expected,debug %s
+
+// This test file checks the behavior under the assumption that no fixits
+// were emitted for the test cases. If -Wunsafe-buffer-usage is improved
+// to support these cases (thus failing the test), the test should be changed
+// to showcase a different unsupported example.
+//
+// RUN: %clang_cc1 -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions \
+// RUN:            -mllvm -debug-only=SafeBuffers \
+// RUN:            -std=c++20 -fdiagnostics-parseable-fixits %s \
+// RUN:            2>&1 | FileCheck %s
+// CHECK-NOT: fix-it:
+
+// This debugging facility is only available in debug builds.
+//
+// REQUIRES: asserts
+
+void foo() {
+  int *x = new int[10]; // expected-warning{{'x' is an unsafe pointer used for buffer access}}
+  x[5] = 10;            // expected-note{{used in buffer access here}}
+  int z = x[-1];        // expected-note{{used in buffer access here}} \
+                        // debug-note{{safe buffers debug: gadget 'ULCArraySubscript' refused to produce a fix}}
+}