Magic-Mirror/llvm-capstone
1
0
Fork 0
mirror of https://github.com/capstone-engine/llvm-capstone.git synced 2024-11-27 23:51:56 +00:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

[-Wunsafe-buffer-usage] Reducing non-determinism in diagnostics output stream

Browse Source 
The -Wunsafe-buffer-usage analysis outputs diagnostics in the order of
pointer values to associated `VarDecl`s. This creates non-determinism
in the order of diagnostics in output since the order cannot be
guaranteed in pointer values. However, our fix-it tests were written
under the assumption that diagnostics are output in source location
order.  This results in non-deterministic failures in our tests.  This
patch fixes the problem by keeping analysis results sorted by source
locations.

Reviewed by: jkorous, NoQ

Differential revision: https://reviews.llvm.org/D145993
This commit is contained in:
ziqingluo-90 2023-03-13 17:17:30 -07:00
parent 93a455375c
commit 148dc8a2a8
2 changed files with 74 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

136
clang/lib/Analysis/UnsafeBufferUsage.cpp
View File

@ -9,8 +9,8 @@
#include "clang/Analysis/Analyses/UnsafeBufferUsage.h" #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/RecursiveASTVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Lex/Lexer.h" #include "clang/Lex/Lexer.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/SmallVector.h"
#include <memory> #include <memory>
#include <optional> #include <optional>
@ -113,13 +113,14 @@ private:
AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) { AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher); const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All); MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);
return Visitor.findMatch(DynTypedNode::create(Node)); return Visitor.findMatch(DynTypedNode::create(Node));
} }
// Matches a `Stmt` node iff the node is in a safe-buffer opt-out region // Matches a `Stmt` node iff the node is in a safe-buffer opt-out region
AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *, Handler) { AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *,
Handler) {
return !Handler->isSafeBufferOptOut(Node.getBeginLoc()); return !Handler->isSafeBufferOptOut(Node.getBeginLoc());
} }
@ -130,7 +131,7 @@ AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
// Returns a matcher that matches any expression 'e' such that `innerMatcher` // Returns a matcher that matches any expression 'e' such that `innerMatcher`
// matches 'e' and 'e' is in an Unspecified Lvalue Context. // matches 'e' and 'e' is in an Unspecified Lvalue Context.
static auto isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) { static auto isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) {
// clang-format off // clang-format off
return return
expr(anyOf( expr(anyOf(
implicitCastExpr( implicitCastExpr(
@ -331,7 +332,7 @@ public:
anyOf(hasPointerType(), hasArrayType()))), anyOf(hasPointerType(), hasArrayType()))),
unless(hasIndex(integerLiteral(equals(0))))) unless(hasIndex(integerLiteral(equals(0)))))
.bind(ArraySubscrTag)); .bind(ArraySubscrTag));
// clang-format on // clang-format on
} }
const ArraySubscriptExpr *getBaseStmt() const override { return ASE; } const ArraySubscriptExpr *getBaseStmt() const override { return ASE; }
@ -354,10 +355,10 @@ class PointerArithmeticGadget : public WarningGadget {
static constexpr const char *const PointerArithmeticTag = "ptrAdd"; static constexpr const char *const PointerArithmeticTag = "ptrAdd";
static constexpr const char *const PointerArithmeticPointerTag = "ptrAddPtr"; static constexpr const char *const PointerArithmeticPointerTag = "ptrAddPtr";
const BinaryOperator *PA; // pointer arithmetic expression const BinaryOperator *PA; // pointer arithmetic expression
const Expr * Ptr; // the pointer expression in `PA` const Expr *Ptr; // the pointer expression in `PA`
public: public:
PointerArithmeticGadget(const MatchFinder::MatchResult &Result) PointerArithmeticGadget(const MatchFinder::MatchResult &Result)
: WarningGadget(Kind::PointerArithmetic), : WarningGadget(Kind::PointerArithmetic),
PA(Result.Nodes.getNodeAs<BinaryOperator>(PointerArithmeticTag)), PA(Result.Nodes.getNodeAs<BinaryOperator>(PointerArithmeticTag)),
Ptr(Result.Nodes.getNodeAs<Expr>(PointerArithmeticPointerTag)) {} Ptr(Result.Nodes.getNodeAs<Expr>(PointerArithmeticPointerTag)) {}
@ -367,43 +368,42 @@ public:
} }
static Matcher matcher() { static Matcher matcher() {
auto HasIntegerType = anyOf( auto HasIntegerType = anyOf(hasType(isInteger()), hasType(enumType()));
hasType(isInteger()), hasType(enumType())); auto PtrAtRight =
auto PtrAtRight = allOf(hasOperatorName("+"), allOf(hasOperatorName("+"),
hasRHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)), hasRHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)),
hasLHS(HasIntegerType)); hasLHS(HasIntegerType));
auto PtrAtLeft = allOf( auto PtrAtLeft =
anyOf(hasOperatorName("+"), hasOperatorName("-"), allOf(anyOf(hasOperatorName("+"), hasOperatorName("-"),
hasOperatorName("+="), hasOperatorName("-=")), hasOperatorName("+="), hasOperatorName("-=")),
hasLHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)), hasLHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)),
hasRHS(HasIntegerType)); hasRHS(HasIntegerType));
return stmt(binaryOperator(anyOf(PtrAtLeft, PtrAtRight)).bind(PointerArithmeticTag)); return stmt(binaryOperator(anyOf(PtrAtLeft, PtrAtRight))
.bind(PointerArithmeticTag));
} }
const Stmt *getBaseStmt() const override { return PA; } const Stmt *getBaseStmt() const override { return PA; }
DeclUseList getClaimedVarUseSites() const override { DeclUseList getClaimedVarUseSites() const override {
if (const auto *DRE = if (const auto *DRE = dyn_cast<DeclRefExpr>(Ptr->IgnoreParenImpCasts())) {
dyn_cast<DeclRefExpr>(Ptr->IgnoreParenImpCasts())) {
return {DRE}; return {DRE};
} }
return {}; return {};
} }
// FIXME: pointer adding zero should be fine // FIXME: pointer adding zero should be fine
//FIXME: this gadge will need a fix-it // FIXME: this gadge will need a fix-it
}; };
/// A call of a function or method that performs unchecked buffer operations /// A call of a function or method that performs unchecked buffer operations
/// over one of its pointer parameters. /// over one of its pointer parameters.
class UnsafeBufferUsageAttrGadget : public WarningGadget { class UnsafeBufferUsageAttrGadget : public WarningGadget {
constexpr static const char *const OpTag = "call_expr"; constexpr static const char *const OpTag = "call_expr";
const CallExpr *Op; const CallExpr *Op;
public: public:
UnsafeBufferUsageAttrGadget(const MatchFinder::MatchResult &Result) UnsafeBufferUsageAttrGadget(const MatchFinder::MatchResult &Result)
: WarningGadget(Kind::UnsafeBufferUsageAttr), : WarningGadget(Kind::UnsafeBufferUsageAttr),
Op(Result.Nodes.getNodeAs<CallExpr>(OpTag)) {} Op(Result.Nodes.getNodeAs<CallExpr>(OpTag)) {}
@ -413,22 +413,20 @@ public:
static Matcher matcher() { static Matcher matcher() {
return stmt(callExpr(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))) return stmt(callExpr(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))))
.bind(OpTag)); .bind(OpTag));
} }
const Stmt *getBaseStmt() const override { return Op; } const Stmt *getBaseStmt() const override { return Op; }
DeclUseList getClaimedVarUseSites() const override { DeclUseList getClaimedVarUseSites() const override { return {}; }
return {};
}
}; };
// Represents expressions of the form `DRE[*]` in the Unspecified Lvalue // Represents expressions of the form `DRE[*]` in the Unspecified Lvalue
// Context (see `isInUnspecifiedLvalueContext`). // Context (see `isInUnspecifiedLvalueContext`).
// Note here `[]` is the built-in subscript operator. // Note here `[]` is the built-in subscript operator.
class ULCArraySubscriptGadget : public FixableGadget { class ULCArraySubscriptGadget : public FixableGadget {
private: private:
static constexpr const char *const ULCArraySubscriptTag = "ArraySubscriptUnderULC"; static constexpr const char *const ULCArraySubscriptTag =
"ArraySubscriptUnderULC";
const ArraySubscriptExpr *Node; const ArraySubscriptExpr *Node;
public: public:
@ -457,7 +455,8 @@ public:
virtual const Stmt *getBaseStmt() const override { return Node; } virtual const Stmt *getBaseStmt() const override { return Node; }
virtual DeclUseList getClaimedVarUseSites() const override { virtual DeclUseList getClaimedVarUseSites() const override {
if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) { if (const auto *DRE =
dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) {
return {DRE}; return {DRE};
} }
return {}; return {};
@ -530,11 +529,11 @@ namespace {
class Strategy { class Strategy {
public: public:
enum class Kind { enum class Kind {
Wontfix, // We don't plan to emit a fixit for this variable. Wontfix, // We don't plan to emit a fixit for this variable.
Span, // We recommend replacing the variable with std::span. Span, // We recommend replacing the variable with std::span.
Iterator, // We recommend replacing the variable with std::span::iterator. Iterator, // We recommend replacing the variable with std::span::iterator.
Array, // We recommend replacing the variable with std::array. Array, // We recommend replacing the variable with std::array.
Vector // We recommend replacing the variable with std::vector. Vector // We recommend replacing the variable with std::vector.
}; };
private: private:
@ -547,9 +546,7 @@ public:
Strategy(const Strategy &) = delete; // Let's avoid copies. Strategy(const Strategy &) = delete; // Let's avoid copies.
Strategy(Strategy &&) = default; Strategy(Strategy &&) = default;
void set(const VarDecl *VD, Kind K) { void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
Map[VD] = K;
}
Kind lookup(const VarDecl *VD) const { Kind lookup(const VarDecl *VD) const {
auto I = Map.find(VD); auto I = Map.find(VD);
@ -593,17 +590,17 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) {
// Figure out which matcher we've found, and call the appropriate // Figure out which matcher we've found, and call the appropriate
// subclass constructor. // subclass constructor.
// FIXME: Can we do this more logarithmically? // FIXME: Can we do this more logarithmically?
#define FIXABLE_GADGET(name) \ #define FIXABLE_GADGET(name) \
if (Result.Nodes.getNodeAs<Stmt>(#name)) { \ if (Result.Nodes.getNodeAs<Stmt>(#name)) { \
FixableGadgets.push_back(std::make_unique<name ## Gadget>(Result)); \ FixableGadgets.push_back(std::make_unique<name##Gadget>(Result)); \
NEXT; \ NEXT; \
} }
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
#define WARNING_GADGET(name) \ #define WARNING_GADGET(name) \
if (Result.Nodes.getNodeAs<Stmt>(#name)) { \ if (Result.Nodes.getNodeAs<Stmt>(#name)) { \
WarningGadgets.push_back(std::make_unique<name ## Gadget>(Result)); \ WarningGadgets.push_back(std::make_unique<name##Gadget>(Result)); \
NEXT; \ NEXT; \
} }
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
assert(numFound >= 1 && "Gadgets not found in match result!"); assert(numFound >= 1 && "Gadgets not found in match result!");
@ -657,11 +654,24 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) {
} }
} }
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)}; return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
} }
// Compares AST nodes by source locations.
template <typename NodeTy> struct CompareNode {
bool operator()(const NodeTy *N1, const NodeTy *N2) const {
return N1->getBeginLoc().getRawEncoding() <
N2->getBeginLoc().getRawEncoding();
}
};
struct WarningGadgetSets { struct WarningGadgetSets {
std::map<const VarDecl *, std::set<std::unique_ptr<WarningGadget>>> byVar; std::map<const VarDecl *, std::set<std::unique_ptr<WarningGadget>>,
// To keep keys sorted by their locations in the map so that the
// order is deterministic:
CompareNode<VarDecl>>
byVar;
// These Gadgets are not related to pointer variables (e. g. temporaries). // These Gadgets are not related to pointer variables (e. g. temporaries).
llvm::SmallVector<std::unique_ptr<WarningGadget>, 16> noVar; llvm::SmallVector<std::unique_ptr<WarningGadget>, 16> noVar;
}; };
@ -709,8 +719,8 @@ groupFixablesByVar(FixableGadgetList &&AllFixableOperations) {
return FixablesForUnsafeVars; return FixablesForUnsafeVars;
} }
bool clang::internal::anyConflict( bool clang::internal::anyConflict(const SmallVectorImpl<FixItHint> &FixIts,
const SmallVectorImpl<FixItHint> &FixIts, const SourceManager &SM) { const SourceManager &SM) {
// A simple interval overlap detection algorithm. Sorts all ranges by their // A simple interval overlap detection algorithm. Sorts all ranges by their
// begin location then finds the first overlap in one pass. // begin location then finds the first overlap in one pass.
std::vector<const FixItHint *> All; // a copy of `FixIts` std::vector<const FixItHint *> All; // a copy of `FixIts`
@ -742,7 +752,8 @@ bool clang::internal::anyConflict(
std::optional<FixItList> std::optional<FixItList>
ULCArraySubscriptGadget::getFixits(const Strategy &S) const { ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) if (const auto *DRE =
dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts()))
if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) { if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
switch (S.lookup(VD)) { switch (S.lookup(VD)) {
case Strategy::Kind::Span: { case Strategy::Kind::Span: {
@ -793,7 +804,7 @@ static SourceLocation getPastLoc(const NodeTy *Node, const SourceManager &SM,
// Return text representation of an `Expr`. // Return text representation of an `Expr`.
static StringRef getExprText(const Expr *E, const SourceManager &SM, static StringRef getExprText(const Expr *E, const SourceManager &SM,
const LangOptions &LangOpts) { const LangOptions &LangOpts) {
SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts); SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts);
return Lexer::getSourceText( return Lexer::getSourceText(
@ -850,8 +861,8 @@ populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx,
if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf && if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf &&
isa_and_present<DeclRefExpr>(AddrOfExpr->getSubExpr())) isa_and_present<DeclRefExpr>(AddrOfExpr->getSubExpr()))
ExtentText = One; ExtentText = One;
// TODO: we can handle more cases, e.g., `&a[0]`, `&a`, `std::addressof`, and explicit casting, etc. // TODO: we can handle more cases, e.g., `&a[0]`, `&a`, `std::addressof`,
// etc. // and explicit casting, etc. etc.
} }
SmallString<32> StrBuffer{}; SmallString<32> StrBuffer{};
@ -918,7 +929,7 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
assert(DS && "Fixing non-local variables not implemented yet!"); assert(DS && "Fixing non-local variables not implemented yet!");
if (!DS->isSingleDecl()) { if (!DS->isSingleDecl()) {
// FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt` // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
return{}; return {};
} }
// Currently DS is an unused variable but we'll need it when // Currently DS is an unused variable but we'll need it when
// non-single decls are implemented, where the pointee type name // non-single decls are implemented, where the pointee type name
@ -969,7 +980,8 @@ getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
UnsafeBufferUsageHandler &Handler) { UnsafeBufferUsageHandler &Handler) {
std::map<const VarDecl *, FixItList> FixItsForVariable; std::map<const VarDecl *, FixItList> FixItsForVariable;
for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
FixItsForVariable[VD] = fixVariable(VD, S.lookup(VD), Tracker, Ctx, Handler); FixItsForVariable[VD] =
fixVariable(VD, S.lookup(VD), Tracker, Ctx, Handler);
// If we fail to produce Fix-It for the declaration we have to skip the // If we fail to produce Fix-It for the declaration we have to skip the
// variable entirely. // variable entirely.
if (FixItsForVariable[VD].empty()) { if (FixItsForVariable[VD].empty()) {
@ -999,7 +1011,7 @@ getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
// a fix-it conflicts with another one // a fix-it conflicts with another one
if (overlapWithMacro(FixItsForVariable[VD]) || if (overlapWithMacro(FixItsForVariable[VD]) ||
clang::internal::anyConflict(FixItsForVariable[VD], clang::internal::anyConflict(FixItsForVariable[VD],
Ctx.getSourceManager())) { Ctx.getSourceManager())) {
FixItsForVariable.erase(VD); FixItsForVariable.erase(VD);
} }
} }

1
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
View File

@ -1,4 +1,3 @@
// REQUIRES: !system-windows
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s // RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
typedef int * Int_ptr_t; typedef int * Int_ptr_t;
typedef int Int_t; typedef int Int_t;