diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 6766ba8ec2777..13f28076c6f4d 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/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 @@ using DefMapTy = llvm::DenseMap>; /// 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; + using DebugNoteList = std::vector; + using DebugNoteByVar = std::map; + DebugNoteByVar DebugNotesByVar; +#endif + public: UnsafeBufferUsageHandler() = default; virtual ~UnsafeBufferUsageHandler() = default; @@ -43,6 +56,26 @@ class UnsafeBufferUsageHandler { 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; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index c88f25209fc0f..b531babf0449c 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11872,6 +11872,12 @@ 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">; diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 7b1c5107a7e04..781dc13c898d2 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -316,6 +316,15 @@ class Gadget { 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 @@ class PointerInitGadget : public FixableGadget { virtual std::optional 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 @@ class PointerAssignmentGadget : public FixableGadget { virtual std::optional 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 @@ class DeclUseTracker { }); } + 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(D)) { @@ -1703,6 +1730,13 @@ populateInitializerFixItWithSpan(const Expr *Init, ASTContext &Ctx, 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` type and transforms the initializer, if present, to a span @@ -1717,7 +1751,9 @@ populateInitializerFixItWithSpan(const Expr *Init, ASTContext &Ctx, // 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 @@ static FixItList fixVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx, 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 @@ static FixItList fixVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx, 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(unsigned ParmIdx, StringRef NewTyText, // `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(PVD->getDeclContext()); - if (!FD) + if (!FD) { + DEBUG_NOTE_DECL_FAIL(PVD, " : invalid func decl"); return {}; + } std::optional PteTyQualifiers = std::nullopt; std::optional PteTyText = getPointeeTypeText( PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers); - if (!PteTyText) + if (!PteTyText) { + DEBUG_NOTE_DECL_FAIL(PVD, " : invalid pointee type"); return {}; + } std::optional 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 @@ static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx, Fixes.append(*OverloadFix); return Fixes; } + DEBUG_NOTE_DECL_FAIL(PVD, " : invalid number of parameters"); return {}; } @@ -2005,6 +2055,7 @@ static FixItList fixVariableWithSpan(const VarDecl *VD, 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 @@ static FixItList fixVariableWithSpan(const VarDecl *VD, (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 @@ fixVariable(const VarDecl *VD, Strategy::Kind K, UnsafeBufferUsageHandler &Handler) { if (const auto *PVD = dyn_cast(VD)) { auto *FD = dyn_cast(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 @@ fixVariable(const VarDecl *VD, Strategy::Kind K, isa(FD) || // skip when the function body is a try-block (FD->hasBody() && isa(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 @@ fixVariable(const VarDecl *VD, Strategy::Kind K, 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 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, for (const auto &F : Fixables) { std::optional 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 @@ getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) { 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 @@ void clang::checkUnsafeBufferUsage(const Decl *D, for (auto it = FixablesForAllVars.byVar.cbegin(); it != FixablesForAllVars.byVar.cend();) { // FIXME: need to deal with global variables later - if ((!it->first->isLocalVarDecl() && !isa(it->first)) || - Tracker.hasUnclaimedUses(it->first) || it->first->isInitCapture()) { - it = FixablesForAllVars.byVar.erase(it); - } else { + if ((!it->first->isLocalVarDecl() && !isa(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; } } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 43b13e0ec4d24..a2eb7707a19ba 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2276,6 +2276,12 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { 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 { diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp new file mode 100644 index 0000000000000..79031cdb7691d --- /dev/null +++ b/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}} +}