[clang] [WIP][-Wunsafe-buffer-usage] Start emitting std::array fixits (PR #68037)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 2 13:52:02 PDT 2023
https://github.com/jkorous-apple created https://github.com/llvm/llvm-project/pull/68037
None
>From 553173411b33b4439d6d6c458c31e08ab0a08e28 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Mon, 2 Oct 2023 11:51:18 -0700
Subject: [PATCH] [WIP][-Wunsafe-buffer-usage] Start emitting std::array fixits
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 7 ++-
clang/lib/Analysis/UnsafeBufferUsage.cpp | 58 +++++++++++++++++--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 4 +-
.../warn-unsafe-buffer-usage-debug.cpp | 9 ---
.../test/SemaCXX/warn-unsafe-buffer-usage.cpp | 5 ++
5 files changed, 67 insertions(+), 16 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 8a2d56668e32f92..a2f7f84109b3fef 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -57,6 +57,11 @@ class UnsafeBufferUsageHandler {
#endif
public:
+ enum class TargetType {
+ Span,
+ Array
+ };
+
UnsafeBufferUsageHandler() = default;
virtual ~UnsafeBufferUsageHandler() = default;
@@ -76,7 +81,7 @@ class UnsafeBufferUsageHandler {
/// and all of its group mates.
virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
const VariableGroupsManager &VarGrpMgr,
- FixItList &&Fixes, const Decl *D) = 0;
+ FixItList &&Fixes, const Decl *D, TargetType Type) = 0;
#ifndef NDEBUG
public:
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 49cfa7c0d3e3b27..61da10b74f3981d 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1283,9 +1283,12 @@ ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
// no-op is a good fix-it, otherwise
return FixItList{};
}
+ case Strategy::Kind::Array: {
+ // FIXME: negative values
+ return FixItList{};
+ }
case Strategy::Kind::Wontfix:
case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
case Strategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
@@ -2211,6 +2214,43 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast<clang::ConstantArrayType>(D->getType())) {
+ const QualType &ArrayEltT = CAT->getElementType();
+ assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+ // Producing fix-it for variable declaration---make `D` to be of std::array type:
+ SmallString<32> Replacement;
+ raw_svector_ostream OS(Replacement);
+ OS << "std::array<" << ArrayEltT.getAsString() << ", " << getAPIntText(CAT->getSize()) << "> " << D->getName();
+ FixIts.push_back(FixItHint::CreateReplacement(
+ SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc() }, OS.str()));
+ }
+
+
+ return FixIts;
+}
+
+static FixItList fixVariableWithArray(const VarDecl *VD,
+ const DeclUseTracker &Tracker,
+ const ASTContext &Ctx,
+ UnsafeBufferUsageHandler &Handler) {
+ const DeclStmt *DS = Tracker.lookupDecl(VD);
+ assert(DS && "Fixing non-local variables not implemented yet!");
+ if (!DS->isSingleDecl()) {
+ // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
+ return {};
+ }
+ // Currently DS is an unused variable but we'll need it when
+ // non-single decls are implemented, where the pointee type name
+ // and the '*' are spread around the place.
+ (void)DS;
+
+ // FIXME: handle cases where DS has multiple declarations
+ return fixVarDeclWithArray(VD, Ctx);
+}
+
// TODO: we should be consistent to use `std::nullopt` to represent no-fix due
// to any unexpected problem.
static FixItList
@@ -2256,8 +2296,12 @@ fixVariable(const VarDecl *VD, Strategy::Kind K,
DEBUG_NOTE_DECL_FAIL(VD, " : not a pointer");
return {};
}
+ case Strategy::Kind::Array: {
+ if (VD->isLocalVarDecl() && isa<clang::ConstantArrayType>(VD->getType()))
+ return fixVariableWithArray(VD, Tracker, Ctx, Handler);
+ return {};
+ }
case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
case Strategy::Kind::Vector:
llvm_unreachable("Strategy not implemented yet!");
case Strategy::Kind::Wontfix:
@@ -2444,7 +2488,10 @@ static Strategy
getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
Strategy S;
for (const VarDecl *VD : UnsafeVars) {
- S.set(VD, Strategy::Kind::Span);
+ if (isa<ConstantArrayType>(VD->getType()))
+ S.set(VD, Strategy::Kind::Array);
+ else
+ S.set(VD, Strategy::Kind::Span);
}
return S;
}
@@ -2763,7 +2810,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
FixItsIt != FixItsForVariableGroup.end()
? std::move(FixItsIt->second)
: FixItList{},
- D);
+ D,
+ NaiveStrategy.lookup(VD) == Strategy::Kind::Span
+ ? UnsafeBufferUsageHandler::TargetType::Span
+ : UnsafeBufferUsageHandler::TargetType::Array);
for (const auto &G : WarningGadgets) {
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 77bb560eb6288f7..8a76b5531f6723f 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2267,7 +2267,7 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
void handleUnsafeVariableGroup(const VarDecl *Variable,
const VariableGroupsManager &VarGrpMgr,
- FixItList &&Fixes, const Decl *D) override {
+ FixItList &&Fixes, const Decl *D, TargetType Type) override {
assert(!SuggestSuggestions &&
"Unsafe buffer usage fixits displayed without suggestions!");
S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
@@ -2282,7 +2282,7 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
// NOT explain how the variables are grouped as the reason is non-trivial
// and irrelavant to users' experience:
const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Variable, &BriefMsg);
- unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy
+ unsigned FixItStrategy = Type == TargetType::Span ? 0 : 1;
const auto &FD =
S.Diag(Variable->getLocation(),
BriefMsg ? diag::note_unsafe_buffer_variable_fixit_together
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
index e08f70d97e3912f..3a360566d1f3a53 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -32,15 +32,6 @@ void foo() {
// 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}}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
index c5f0a9ef929371b..a70d4bda45c72ce 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -61,6 +61,7 @@ void testArraySubscripts(int *p, int **pp) {
);
int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+ // expected-note at -1{{change type of 'a' to 'std::array' to preserve bounds information}}
int b[10][10]; // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
foo(a[1], 1[a], // expected-note2{{used in buffer access here}}
@@ -174,6 +175,7 @@ auto file_scope_lambda = [](int *ptr) {
void testLambdaCapture() {
int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
int b[10]; // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+ // expected-note at -1{{change type of 'b' to 'std::array' to preserve bounds information}}
int c[10];
auto Lam1 = [a]() {
@@ -191,7 +193,9 @@ void testLambdaCapture() {
void testLambdaImplicitCapture() {
int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+ // expected-note at -1{{change type of 'a' to 'std::array' to preserve bounds information}}
int b[10]; // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+ // expected-note at -1{{change type of 'b' to 'std::array' to preserve bounds information}}
auto Lam1 = [=]() {
return a[1]; // expected-note{{used in buffer access here}}
@@ -344,6 +348,7 @@ template<typename T> void fArr(T t[]) {
// expected-warning at -1{{'t' is an unsafe pointer used for buffer access}}
foo(t[1]); // expected-note{{used in buffer access here}}
T ar[8]; // expected-warning{{'ar' is an unsafe buffer that does not perform bounds checks}}
+ // expected-note at -1{{change type of 'ar' to 'std::array' to preserve bounds information}}
foo(ar[5]); // expected-note{{used in buffer access here}}
}
More information about the cfe-commits
mailing list