Skip to content

Commit

Permalink
Revert D150338, "[-Wunsafe-buffer-usage] Improving insertion of the […
Browse files Browse the repository at this point in the history 
…[clang::unsafe_buffer_usage]] attribute"

clangAnalysis should not depend on clangSema.

This reverts commit 5b012bf.
  • Loading branch information
@chapuni
chapuni committed Jul 15, 2023
1 parent 143e2c2 commit c915908
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 153 deletions.
13 changes: 10 additions & 3 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@

#include "clang/AST/Decl.h"
#include "clang/AST/Stmt.h"
#include "clang/Sema/Sema.h"

namespace clang {
class Sema;

using DefMapTy = llvm::DenseMap<const VarDecl *, std::vector<const VarDecl *>>;

Expand Down Expand Up @@ -47,12 +45,21 @@ class UnsafeBufferUsageHandler {

/// Returns a reference to the `Preprocessor`:
virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0;

/// Returns the text indicating that the user needs to provide input there:
virtual std::string
getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") const {
std::string s = std::string("<# ");
s += HintTextToUser;
s += " #>";
return s;
}
};

// This function invokes the analysis and allows the caller to react to it
// through the handler class.
void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler,
bool EmitSuggestions, Sema &Sema);
bool EmitSuggestions);

namespace internal {
// Tests if any two `FixItHint`s in `FixIts` conflict. Two `FixItHint`s
Expand Down
69 changes: 21 additions & 48 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1290,14 +1290,6 @@ static StringRef getEndOfLine() {
return EOL;
}

// Returns the text indicating that the user needs to provide input there:
std::string getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") {
std::string s = std::string("<# ");
s += HintTextToUser;
s += " #>";
return s;
}

// Return the text representation of the given `APInt Val`:
static std::string getAPIntText(APInt Val) {
SmallVector<char> Txt;
Expand Down Expand Up @@ -1764,28 +1756,11 @@ static bool hasConflictingOverload(const FunctionDecl *FD) {
}

// Returns the text representation of clang::unsafe_buffer_usage attribute.
// `WSSuffix` holds customized "white-space"s, e.g., newline or whilespace
// characters.
static std::string
getUnsafeBufferUsageAttributeTextAt(SourceLocation Loc, Sema &S,
StringRef WSSuffix = "") {
Preprocessor &PP = S.getPreprocessor();
TokenValue ClangUnsafeBufferUsageTokens[] = {
tok::l_square,
tok::l_square,
PP.getIdentifierInfo("clang"),
tok::coloncolon,
PP.getIdentifierInfo("unsafe_buffer_usage"),
tok::r_square,
tok::r_square};

StringRef MacroName;

// The returned macro (it returns) is guaranteed not to be function-like:
MacroName = PP.getLastMacroWithSpelling(Loc, ClangUnsafeBufferUsageTokens);
if (MacroName.empty())
MacroName = "[[clang::unsafe_buffer_usage]]";
return MacroName.str() + WSSuffix.str();
static std::string getUnsafeBufferUsageAttributeText() {
static const char *const RawAttr = "[[clang::unsafe_buffer_usage]]";
std::stringstream SS;
SS << RawAttr << getEndOfLine().str();
return SS.str();
}

// For a `FunDecl`, one of whose `ParmVarDecl`s is being changed to have a new
Expand Down Expand Up @@ -1830,11 +1805,11 @@ getUnsafeBufferUsageAttributeTextAt(SourceLocation Loc, Sema &S,
static std::optional<FixItList>
createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
const FunctionDecl *FD, const ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler, Sema &Sema) {
UnsafeBufferUsageHandler &Handler) {
// FIXME: need to make this conflict checking better:
if (hasConflictingOverload(FD))
return std::nullopt;

const SourceManager &SM = Ctx.getSourceManager();
const LangOptions &LangOpts = Ctx.getLangOpts();
// FIXME Respect indentation of the original code.
Expand Down Expand Up @@ -1884,7 +1859,7 @@ createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
// A lambda that creates the text representation of a function definition with
// the original signature:
const auto OldOverloadDefCreator =
[&SM, &Sema,
[&Handler, &SM,
&LangOpts](const FunctionDecl *FD, unsigned ParmIdx,
StringRef NewTypeText) -> std::optional<std::string> {
std::stringstream SS;
Expand All @@ -1894,8 +1869,7 @@ createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
if (auto FDPrefix = getRangeText(
SourceRange(FD->getBeginLoc(), FD->getBody()->getBeginLoc()), SM,
LangOpts))
SS << getUnsafeBufferUsageAttributeTextAt(FD->getBeginLoc(), Sema, " ")
<< FDPrefix->str() << "{";
SS << getUnsafeBufferUsageAttributeText() << FDPrefix->str() << "{";
else
return std::nullopt;
// Append: "return" func-name "("
Expand All @@ -1916,7 +1890,7 @@ createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
if (i == ParmIdx)
// This is our spanified paramter!
SS << NewTypeText.str() << "(" << Parm->getIdentifier()->getName().str() << ", "
<< getUserFillPlaceHolder("size") << ")";
<< Handler.getUserFillPlaceHolder("size") << ")";
else
SS << Parm->getIdentifier()->getName().str();
if (i != NumParms - 1)
Expand Down Expand Up @@ -1947,8 +1921,7 @@ createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
// Adds the unsafe-buffer attribute (if not already there) to `FReDecl`:
if (!FReDecl->hasAttr<UnsafeBufferUsageAttr>()) {
FixIts.emplace_back(FixItHint::CreateInsertion(
FReDecl->getBeginLoc(), getUnsafeBufferUsageAttributeTextAt(
FReDecl->getBeginLoc(), Sema, " ")));
FReDecl->getBeginLoc(), getUnsafeBufferUsageAttributeText()));
}
// Inserts a declaration with the new signature to the end of `FReDecl`:
if (auto NewOverloadDecl =
Expand All @@ -1965,7 +1938,7 @@ createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
// new overload of the function so that the change is self-contained (see
// `createOverloadsForFixedParams`).
static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler, Sema &Sema) {
UnsafeBufferUsageHandler &Handler) {
if (PVD->hasDefaultArg())
// FIXME: generate fix-its for default values:
return {};
Expand Down Expand Up @@ -2017,7 +1990,7 @@ static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
}
if (ParmIdx < FD->getNumParams())
if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, SpanTyText,
FD, Ctx, Handler, Sema)) {
FD, Ctx, Handler)) {
Fixes.append(*OverloadFix);
return Fixes;
}
Expand All @@ -2040,7 +2013,7 @@ 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, Handler.getUserFillPlaceHolder());
}

// TODO: we should be consistent to use `std::nullopt` to represent no-fix due
Expand All @@ -2049,7 +2022,7 @@ static FixItList
fixVariable(const VarDecl *VD, Strategy::Kind K,
/* The function decl under analysis */ const Decl *D,
const DeclUseTracker &Tracker, ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler, Sema &Sema) {
UnsafeBufferUsageHandler &Handler) {
if (const auto *PVD = dyn_cast<ParmVarDecl>(VD)) {
auto *FD = dyn_cast<clang::FunctionDecl>(PVD->getDeclContext());
if (!FD || FD != D)
Expand All @@ -2076,7 +2049,7 @@ fixVariable(const VarDecl *VD, Strategy::Kind K,
case Strategy::Kind::Span: {
if (VD->getType()->isPointerType()) {
if (const auto *PVD = dyn_cast<ParmVarDecl>(VD))
return fixParamWithSpan(PVD, Ctx, Handler, Sema);
return fixParamWithSpan(PVD, Ctx, Handler);

if (VD->isLocalVarDecl())
return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
Expand Down Expand Up @@ -2124,11 +2097,11 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
ASTContext &Ctx,
/* The function decl under analysis */ const Decl *D,
const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
const DefMapTy &VarGrpMap, Sema &Sema) {
const DefMapTy &VarGrpMap) {
std::map<const VarDecl *, FixItList> FixItsForVariable;
for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
FixItsForVariable[VD] =
fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler, Sema);
fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler);
// If we fail to produce Fix-It for the declaration we have to skip the
// variable entirely.
if (FixItsForVariable[VD].empty()) {
Expand Down Expand Up @@ -2198,7 +2171,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
FixItList GroupFix;
if (FixItsForVariable.find(Var) == FixItsForVariable.end()) {
GroupFix = fixVariable(Var, ReplacementTypeForVD, D,
Tracker, Var->getASTContext(), Handler, Sema);
Tracker, Var->getASTContext(), Handler);
} else {
GroupFix = FixItsForVariable[Var];
}
Expand All @@ -2224,7 +2197,7 @@ getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {

void clang::checkUnsafeBufferUsage(const Decl *D,
UnsafeBufferUsageHandler &Handler,
bool EmitSuggestions, Sema &Sema) {
bool EmitSuggestions) {
assert(D && D->getBody());

// Do not emit fixit suggestions for functions declared in an
Expand Down Expand Up @@ -2370,7 +2343,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,

FixItsForVariableGroup =
getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
Tracker, Handler, VariableGroupsMap, Sema);
Tracker, Handler, VariableGroupsMap);

// FIXME Detect overlapping FixIts.

Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/AnalysisBasedWarnings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2423,7 +2423,7 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
!Diags.isIgnored(diag::warn_unsafe_buffer_variable,
Node->getBeginLoc())) {
clang::checkUnsafeBufferUsage(Node, R,
UnsafeBufferUsageShouldEmitSuggestions, S);
UnsafeBufferUsageShouldEmitSuggestions);
}

// More analysis ...
Expand Down
69 changes: 0 additions & 69 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-attributes-spelling.cpp
View file

This file was deleted.

8 changes: 4 additions & 4 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-overload.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ namespace NS {
int tmp;
tmp = p[5];
}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void foo(int *p) {return foo(std::span<int>(p, <# size #>));}\n"
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid foo(int *p) {return foo(std::span<int>(p, <# size #>));}\n"

// Similarly, `NS::bar` is distinct from `bar`:
void bar(int *p);
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:3}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} "
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:3}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:19}:";\nvoid bar(std::span<int> p)"
} // end of namespace NS

Expand All @@ -60,7 +60,7 @@ void NS::bar(int *p) {
int tmp;
tmp = p[5];
}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void NS::bar(int *p) {return NS::bar(std::span<int>(p, <# size #>));}\n"
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS::bar(int *p) {return NS::bar(std::span<int>(p, <# size #>));}\n"

namespace NESTED {
void alpha(int);
Expand All @@ -74,7 +74,7 @@ namespace NESTED {
int tmp;
tmp = p[5];
}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:6-[[@LINE-1]]:6}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void alpha(int *p) {return alpha(std::span<int>(p, <# size #>));}\n"
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:6-[[@LINE-1]]:6}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid alpha(int *p) {return alpha(std::span<int>(p, <# size #>));}\n"
}
}

Expand Down
14 changes: 7 additions & 7 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-qualified-names.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@

namespace NS1 {
void foo(int *);
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:3}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} "
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:3}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n"
// CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-2]]:18-[[@LINE-2]]:18}:";\nvoid foo(std::span<int>)"
namespace NS2 {
void foo(int *);
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:5-[[@LINE-1]]:5}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} "
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:5-[[@LINE-1]]:5}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n"
// CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-2]]:20-[[@LINE-2]]:20}:";\nvoid foo(std::span<int>)"
namespace NS3 {
void foo(int *);
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:7-[[@LINE-1]]:7}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} "
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:7-[[@LINE-1]]:7}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n"
// CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-2]]:22-[[@LINE-2]]:22}:";\nvoid foo(std::span<int>)"
}
}
Expand All @@ -23,26 +23,26 @@ void NS1::foo(int *p) {
int tmp;
tmp = p[5];
}
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void NS1::foo(int *p) {return NS1::foo(std::span<int>(p, <# size #>));}\n"
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS1::foo(int *p) {return NS1::foo(std::span<int>(p, <# size #>));}\n"

void NS1::NS2::foo(int *p) {
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:20-[[@LINE-1]]:26}:"std::span<int> p"
int tmp;
tmp = p[5];
}
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void NS1::NS2::foo(int *p) {return NS1::NS2::foo(std::span<int>(p, <# size #>));}\n"
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS1::NS2::foo(int *p) {return NS1::NS2::foo(std::span<int>(p, <# size #>));}\n"

void NS1::NS2::NS3::foo(int *p) {
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:31}:"std::span<int> p"
int tmp;
tmp = p[5];
}
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void NS1::NS2::NS3::foo(int *p) {return NS1::NS2::NS3::foo(std::span<int>(p, <# size #>));}\n"
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS1::NS2::NS3::foo(int *p) {return NS1::NS2::NS3::foo(std::span<int>(p, <# size #>));}\n"


void f(NS1::MyType * x) {
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:8-[[@LINE-1]]:23}:"std::span<NS1::MyType> x"
NS1::MyType tmp;
tmp = x[5];
}
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void f(NS1::MyType * x) {return f(std::span<NS1::MyType>(x, <# size #>));}\n"
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid f(NS1::MyType * x) {return f(std::span<NS1::MyType>(x, <# size #>));}\n"

0 comments on commit c915908

Please sign in to comment.