NoQ created this revision.
NoQ added reviewers: jkorous, t-rasmud, ziqingluo-90, malavikasamak.
Herald added subscribers: steakhal, martong.
Herald added a project: All.
NoQ requested review of this revision.
Herald added a subscriber: wangpc.
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).
Repository:
rC Clang
https://reviews.llvm.org/D154880
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -0,0 +1,33 @@
+// 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}}
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2276,6 +2276,12 @@
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 {
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -319,6 +319,15 @@
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;
@@ -566,7 +575,7 @@
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};
@@ -2113,6 +2122,16 @@
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 {
@@ -2198,6 +2217,10 @@
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
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11841,6 +11841,12 @@
"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">;
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
===================================================================
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -16,6 +16,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/Stmt.h"
+#include "llvm/Support/Debug.h"
namespace clang {
@@ -24,6 +25,18 @@
/// 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;
@@ -43,6 +56,26 @@
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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
