Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Add a facility for debugging low fixit coverage
Browse files Browse the repository at this point in the history 
Differential Revision: https://reviews.llvm.org/D154880
  • Loading branch information
@t-rasmud
t-rasmud committed Jul 27, 2023
1 parent 9a67c6b commit a6ae740
Show file tree
Hide file tree
Showing 5 changed files with 220 additions and 16 deletions.
33 changes: 33 additions & 0 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "clang/AST/Decl.h"
#include "clang/AST/Stmt.h"
#include "llvm/Support/Debug.h"

namespace clang {

Expand All @@ -24,6 +25,18 @@ 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
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;
Expand All @@ -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;

Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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">;

Expand Down
123 changes: 107 additions & 16 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -565,7 +574,11 @@ class PointerInitGadget : public FixableGadget {

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};
Expand Down Expand Up @@ -613,7 +626,11 @@ class PointerAssignmentGadget : public FixableGadget {

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};
Expand Down Expand Up @@ -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<VarDecl>(D)) {
Expand Down Expand Up @@ -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<T>` type and transforms the initializer, if present, to a span
Expand All @@ -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!");

Expand All @@ -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);
Expand All @@ -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()));
Expand Down Expand Up @@ -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<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 = ">";
Expand Down Expand Up @@ -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 {};
}

Expand All @@ -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
Expand All @@ -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
Expand All @@ -2025,10 +2077,12 @@ fixVariable(const VarDecl *VD, Strategy::Kind K,
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.
Expand All @@ -2041,8 +2095,10 @@ fixVariable(const VarDecl *VD, Strategy::Kind K,
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) {
Expand All @@ -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:
Expand Down Expand Up @@ -2113,6 +2170,12 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
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 {
Expand Down Expand Up @@ -2198,6 +2261,10 @@ getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &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.
Expand Down Expand Up @@ -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<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;
}
}
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/Sema/AnalysisBasedWarnings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit a6ae740

Please sign in to comment.