This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa6ae740e743a: [-Wunsafe-buffer-usage] Add a facility for
debugging low fixit coverage (authored by t-rasmud).
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D154880?vs=544547&id=544561#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154880/new/
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,68 @@
+// 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}}
+}
+
+void failed_decl() {
+ int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} \
+ // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : not a pointer}}
+
+ for (int i = 0; i < 10; i++) {
+ a[i] = i; // expected-note{{used in buffer access here}}
+ }
+}
+
+void failed_multiple_decl() {
+ int *a = new int[4], b; // expected-warning{{'a' is an unsafe pointer used for buffer access}} \
+ // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : multiple VarDecls}}
+ a[4] = 3; // expected-note{{used in buffer access here}}
+}
+
+void failed_param_var_decl(int *a =new int[3]) { // expected-warning{{'a' is an unsafe pointer used for buffer access}} \
+ // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : has default arg}}
+ a[4] = 6; // expected-note{{used in buffer access here}}
+}
+
+void unclaimed_use() {
+ int *a = new int[3]; // expected-warning{{'a' is an unsafe pointer used for buffer access}}
+ a[2] = 9; // expected-note{{used in buffer access here}}
+ int *b = a++; // expected-note{{used in pointer arithmetic here}} \
+ // debug-note{{safe buffers debug: failed to produce fixit for 'a' : has an unclaimed use}}
+}
+
+void implied_unclaimed_var(int *b) { // expected-warning{{'b' is an unsafe pointer used for buffer access}}
+ int *a = new int[3]; // expected-warning{{'a' is an unsafe pointer used for buffer access}}
+ a[4] = 7; // expected-note{{used in buffer access here}}
+ a = b; // debug-note{{safe buffers debug: gadget 'PointerAssignment' refused to produce a fix}}
+ b++; // expected-note{{used in pointer arithmetic here}} \
+ // debug-note{{safe buffers debug: failed to produce fixit for 'b' : has an unclaimed use}}
+}
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
@@ -316,6 +316,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;
@@ -565,7 +574,11 @@
virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
- virtual const Stmt *getBaseStmt() const override { return nullptr; }
+ virtual const Stmt *getBaseStmt() const override {
+ // FIXME: This needs to be the entire DeclStmt, assuming that this method
+ // makes sense at all on a FixableGadget.
+ return PtrInitRHS;
+ }
virtual DeclUseList getClaimedVarUseSites() const override {
return DeclUseList{PtrInitRHS};
@@ -613,7 +626,11 @@
virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
- virtual const Stmt *getBaseStmt() const override { return nullptr; }
+ virtual const Stmt *getBaseStmt() const override {
+ // FIXME: This should be the binary operator, assuming that this method
+ // makes sense at all on a FixableGadget.
+ return PtrLHS;
+ }
virtual DeclUseList getClaimedVarUseSites() const override {
return DeclUseList{PtrLHS, PtrRHS};
@@ -845,6 +862,16 @@
});
}
+ UseSetTy getUnclaimedUses(const VarDecl *VD) const {
+ UseSetTy ReturnSet;
+ for (auto use : *Uses) {
+ if (use->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl()) {
+ ReturnSet.insert(use);
+ }
+ }
+ return ReturnSet;
+ }
+
void discoverDecl(const DeclStmt *DS) {
for (const Decl *D : DS->decls()) {
if (const auto *VD = dyn_cast<VarDecl>(D)) {
@@ -1703,6 +1730,13 @@
return FixIts;
}
+#ifndef NDEBUG
+#define DEBUG_NOTE_DECL_FAIL(D, Msg) \
+Handler.addDebugNoteForVar((D), (D)->getBeginLoc(), "failed to produce fixit for declaration '" + (D)->getNameAsString() + "'" + (Msg))
+#else
+#define DEBUG_NOTE_DECL_FAIL(D, Msg)
+#endif
+
// 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
@@ -1717,7 +1751,9 @@
// Returns:
// the generated fix-it
static FixItList fixVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
- const StringRef UserFillPlaceHolder) {
+ const StringRef UserFillPlaceHolder,
+ UnsafeBufferUsageHandler &Handler) {
+ (void)Handler; // Suppress unused variable warning in release builds.
const QualType &SpanEltT = D->getType()->getPointeeType();
assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");
@@ -1730,8 +1766,10 @@
FixItList InitFixIts =
populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder);
- if (InitFixIts.empty())
+ if (InitFixIts.empty()) {
+ DEBUG_NOTE_DECL_FAIL(D, " : empty initializer");
return {};
+ }
// The loc right before the initializer:
ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1);
@@ -1746,8 +1784,10 @@
OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName();
- if (!ReplacementLastLoc)
+ if (!ReplacementLastLoc) {
+ DEBUG_NOTE_DECL_FAIL(D, " : failed to get end char loc (macro)");
return {};
+ }
FixIts.push_back(FixItHint::CreateReplacement(
SourceRange{D->getBeginLoc(), *ReplacementLastLoc}, OS.str()));
@@ -1939,26 +1979,35 @@
// `createOverloadsForFixedParams`).
static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler) {
- if (PVD->hasDefaultArg())
+ if (PVD->hasDefaultArg()) {
// FIXME: generate fix-its for default values:
+ DEBUG_NOTE_DECL_FAIL(PVD, " : has default arg");
return {};
+ }
+
assert(PVD->getType()->isPointerType());
auto *FD = dyn_cast<FunctionDecl>(PVD->getDeclContext());
- if (!FD)
+ if (!FD) {
+ DEBUG_NOTE_DECL_FAIL(PVD, " : invalid func decl");
return {};
+ }
std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
std::optional<std::string> PteTyText = getPointeeTypeText(
PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
- if (!PteTyText)
+ if (!PteTyText) {
+ DEBUG_NOTE_DECL_FAIL(PVD, " : invalid pointee type");
return {};
+ }
std::optional<StringRef> PVDNameText = PVD->getIdentifier()->getName();
- if (!PVDNameText)
+ if (!PVDNameText) {
+ DEBUG_NOTE_DECL_FAIL(PVD, " : invalid identifier name");
return {};
+ }
std::string SpanOpen = "std::span<";
std::string SpanClose = ">";
@@ -1994,6 +2043,7 @@
Fixes.append(*OverloadFix);
return Fixes;
}
+ DEBUG_NOTE_DECL_FAIL(PVD, " : invalid number of parameters");
return {};
}
@@ -2005,6 +2055,7 @@
assert(DS && "Fixing non-local variables not implemented yet!");
if (!DS->isSingleDecl()) {
// FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
+ DEBUG_NOTE_DECL_FAIL(VD, " : multiple VarDecls");
return {};
}
// Currently DS is an unused variable but we'll need it when
@@ -2013,7 +2064,8 @@
(void)DS;
// FIXME: handle cases where DS has multiple declarations
- return fixVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder());
+ return fixVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(),
+ Handler);
}
// TODO: we should be consistent to use `std::nullopt` to represent no-fix due
@@ -2025,10 +2077,12 @@
UnsafeBufferUsageHandler &Handler) {
if (const auto *PVD = dyn_cast<ParmVarDecl>(VD)) {
auto *FD = dyn_cast<clang::FunctionDecl>(PVD->getDeclContext());
- if (!FD || FD != D)
+ if (!FD || FD != D) {
// `FD != D` means that `PVD` belongs to a function that is not being
// analyzed currently. Thus `FD` may not be complete.
+ DEBUG_NOTE_DECL_FAIL(VD, " : function not currently analyzed");
return {};
+ }
// TODO If function has a try block we can't change params unless we check
// also its catch block for their use.
@@ -2041,8 +2095,10 @@
isa<CXXMethodDecl>(FD) ||
// skip when the function body is a try-block
(FD->hasBody() && isa<CXXTryStmt>(FD->getBody())) ||
- FD->isOverloadedOperator())
+ FD->isOverloadedOperator()) {
+ DEBUG_NOTE_DECL_FAIL(VD, " : unsupported function decl");
return {}; // TODO test all these cases
+ }
}
switch (K) {
@@ -2054,6 +2110,7 @@
if (VD->isLocalVarDecl())
return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
}
+ DEBUG_NOTE_DECL_FAIL(VD, " : not a pointer");
return {};
}
case Strategy::Kind::Iterator:
@@ -2113,6 +2170,12 @@
for (const auto &F : Fixables) {
std::optional<FixItList> Fixits = F->getFixits(S);
if (!Fixits) {
+#ifndef NDEBUG
+ Handler.addDebugNoteForVar(
+ VD, F->getBaseStmt()->getBeginLoc(),
+ ("gadget '" + F->getDebugName() + "' refused to produce a fix")
+ .str());
+#endif
ImpossibleToFix = true;
break;
} else {
@@ -2198,6 +2261,10 @@
void clang::checkUnsafeBufferUsage(const Decl *D,
UnsafeBufferUsageHandler &Handler,
bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
assert(D && D->getBody());
// We do not want to visit a Lambda expression defined inside a method independently.
@@ -2277,10 +2344,34 @@
for (auto it = FixablesForAllVars.byVar.cbegin();
it != FixablesForAllVars.byVar.cend();) {
// FIXME: need to deal with global variables later
- if ((!it->first->isLocalVarDecl() && !isa<ParmVarDecl>(it->first)) ||
- Tracker.hasUnclaimedUses(it->first) || it->first->isInitCapture()) {
- it = FixablesForAllVars.byVar.erase(it);
- } else {
+ if ((!it->first->isLocalVarDecl() && !isa<ParmVarDecl>(it->first))) {
+#ifndef NDEBUG
+ Handler.addDebugNoteForVar(
+ it->first, it->first->getBeginLoc(),
+ ("failed to produce fixit for '" + it->first->getNameAsString() +
+ "' : neither local nor a parameter"));
+#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"));
+ }
+#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"));
+#endif
+ it = FixablesForAllVars.byVar.erase(it);
+ }else {
++it;
}
}
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11872,6 +11872,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
+public:
+ // 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
