[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 1 13:59:09 PST 2024
https://github.com/jkorous-apple updated https://github.com/llvm/llvm-project/pull/80084
>From 463a9904c1ae85fbdc0bd6029c6effea3fb16ea6 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/12] [-Wunsafe-buffer-usage] Move Strategy class to the
header
It needs to be used from handleUnsafeVariableGroup function.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 37 +++
clang/lib/Analysis/UnsafeBufferUsage.cpp | 238 ++++++++----------
2 files changed, 140 insertions(+), 135 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index aca1ad998822c..622c094f5b1eb 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -42,6 +42,43 @@ class VariableGroupsManager {
virtual VarGrpRef getGroupOfParms() const =0;
};
+// FixitStrategy is a map from variables to the way we plan to emit fixes for
+// these variables. It is figured out gradually by trying different fixes
+// for different variables depending on gadgets in which these variables
+// participate.
+class FixitStrategy {
+public:
+ enum class Kind {
+ Wontfix, // We don't plan to emit a fixit for this variable.
+ Span, // We recommend replacing the variable with std::span.
+ Iterator, // We recommend replacing the variable with std::span::iterator.
+ Array, // We recommend replacing the variable with std::array.
+ Vector // We recommend replacing the variable with std::vector.
+ };
+
+private:
+ using MapTy = llvm::DenseMap<const VarDecl *, Kind>;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy &operator=(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy &operator=(FixitStrategy &&) = default;
+
+ void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
+
+ Kind lookup(const VarDecl *VD) const {
+ auto I = Map.find(VD);
+ if (I == Map.end())
+ return Kind::Wontfix;
+
+ return I->second;
+ }
+};
+
/// The interface that lets the caller handle unsafe buffer usage analysis
/// results by overriding this class's handle... methods.
class UnsafeBufferUsageHandler {
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 823cd2a7b9969..924d677786275 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -407,9 +407,6 @@ using DeclUseList = SmallVector<const DeclRefExpr *, 1>;
// Convenience typedef.
using FixItList = SmallVector<FixItHint, 4>;
-
-// Defined below.
-class Strategy;
} // namespace
namespace {
@@ -486,7 +483,7 @@ class FixableGadget : public Gadget {
/// Returns a fixit that would fix the current gadget according to
/// the current strategy. Returns std::nullopt if the fix cannot be produced;
/// returns an empty list if no fixes are necessary.
- virtual std::optional<FixItList> getFixits(const Strategy &) const {
+ virtual std::optional<FixItList> getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &S) const override;
virtual const Stmt *getBaseStmt() const override {
// FIXME: This needs to be the entire DeclStmt, assuming that this method
@@ -789,7 +787,8 @@ class PointerAssignmentGadget : public FixableGadget {
return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
}
- virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &S) const override;
virtual const Stmt *getBaseStmt() const override {
// FIXME: This should be the binary operator, assuming that this method
@@ -892,7 +891,8 @@ class ULCArraySubscriptGadget : public FixableGadget {
return expr(isInUnspecifiedLvalueContext(Target));
}
- virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &S) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -932,7 +932,8 @@ class UPCStandalonePointerGadget : public FixableGadget {
return stmt(isInUnspecifiedPointerContext(target));
}
- virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &S) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -976,7 +977,8 @@ class PointerDereferenceGadget : public FixableGadget {
virtual const Stmt *getBaseStmt() const final { return Op; }
- virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &S) const override;
};
// Represents expressions of the form `&DRE[any]` in the Unspecified Pointer
@@ -1009,7 +1011,8 @@ class UPCAddressofArraySubscriptGadget : public FixableGadget {
.bind(UPCAddressofArraySubscriptTag)))));
}
- virtual std::optional<FixItList> getFixits(const Strategy &) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -1088,46 +1091,6 @@ class DeclUseTracker {
};
} // namespace
-namespace {
-// Strategy is a map from variables to the way we plan to emit fixes for
-// these variables. It is figured out gradually by trying different fixes
-// for different variables depending on gadgets in which these variables
-// participate.
-class Strategy {
-public:
- enum class Kind {
- Wontfix, // We don't plan to emit a fixit for this variable.
- Span, // We recommend replacing the variable with std::span.
- Iterator, // We recommend replacing the variable with std::span::iterator.
- Array, // We recommend replacing the variable with std::array.
- Vector // We recommend replacing the variable with std::vector.
- };
-
-private:
- using MapTy = llvm::DenseMap<const VarDecl *, Kind>;
-
- MapTy Map;
-
-public:
- Strategy() = default;
- Strategy(const Strategy &) = delete; // Let's avoid copies.
- Strategy &operator=(const Strategy &) = delete;
- Strategy(Strategy &&) = default;
- Strategy &operator=(Strategy &&) = default;
-
- void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
-
- Kind lookup(const VarDecl *VD) const {
- auto I = Map.find(VD);
- if (I == Map.end())
- return Kind::Wontfix;
-
- return I->second;
- }
-};
-} // namespace
-
-
// Representing a pointer type expression of the form `++Ptr` in an Unspecified
// Pointer Context (UPC):
class UPCPreIncrementGadget : public FixableGadget {
@@ -1159,7 +1122,8 @@ class UPCPreIncrementGadget : public FixableGadget {
).bind(UPCPreIncrementTag)))));
}
- virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &S) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -1204,7 +1168,8 @@ class UUCAddAssignGadget : public FixableGadget {
// clang-format on
}
- virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &S) const override;
virtual const Stmt *getBaseStmt() const override { return Node; }
@@ -1254,7 +1219,8 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
// clang-format on
}
- virtual std::optional<FixItList> getFixits(const Strategy &s) const final;
+ virtual std::optional<FixItList>
+ getFixits(const FixitStrategy &s) const final;
// TODO remove this method from FixableGadget interface
virtual const Stmt *getBaseStmt() const final { return nullptr; }
@@ -1464,38 +1430,38 @@ bool clang::internal::anyConflict(const SmallVectorImpl<FixItHint> &FixIts,
}
std::optional<FixItList>
-PointerAssignmentGadget::getFixits(const Strategy &S) const {
+PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
switch (S.lookup(LeftVD)) {
- case Strategy::Kind::Span:
- if (S.lookup(RightVD) == Strategy::Kind::Span)
- return FixItList{};
- return std::nullopt;
- case Strategy::Kind::Wontfix:
- return std::nullopt;
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
- llvm_unreachable("unsupported strategies for FixableGadgets");
+ case FixitStrategy::Kind::Span:
+ if (S.lookup(RightVD) == FixitStrategy::Kind::Span)
+ return FixItList{};
+ return std::nullopt;
+ case FixitStrategy::Kind::Wontfix:
+ return std::nullopt;
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Vector:
+ llvm_unreachable("unsupported strategies for FixableGadgets");
}
return std::nullopt;
}
std::optional<FixItList>
-PointerInitGadget::getFixits(const Strategy &S) const {
+PointerInitGadget::getFixits(const FixitStrategy &S) const {
const auto *LeftVD = PtrInitLHS;
const auto *RightVD = cast<VarDecl>(PtrInitRHS->getDecl());
switch (S.lookup(LeftVD)) {
- case Strategy::Kind::Span:
- if (S.lookup(RightVD) == Strategy::Kind::Span)
- return FixItList{};
- return std::nullopt;
- case Strategy::Kind::Wontfix:
- return std::nullopt;
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
+ case FixitStrategy::Kind::Span:
+ if (S.lookup(RightVD) == FixitStrategy::Kind::Span)
+ return FixItList{};
+ return std::nullopt;
+ case FixitStrategy::Kind::Wontfix:
+ return std::nullopt;
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
return std::nullopt;
@@ -1512,12 +1478,12 @@ static bool isNonNegativeIntegerExpr(const Expr *Expr, const VarDecl *VD,
}
std::optional<FixItList>
-ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
+ULCArraySubscriptGadget::getFixits(const FixitStrategy &S) const {
if (const auto *DRE =
dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts()))
if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
switch (S.lookup(VD)) {
- case Strategy::Kind::Span: {
+ case FixitStrategy::Kind::Span: {
// If the index has a negative constant value, we give up as no valid
// fix-it can be generated:
@@ -1528,10 +1494,10 @@ ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
// no-op is a good fix-it, otherwise
return FixItList{};
}
- case Strategy::Kind::Wontfix:
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Wontfix:
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
}
@@ -1542,17 +1508,17 @@ static std::optional<FixItList> // forward declaration
fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node);
std::optional<FixItList>
-UPCAddressofArraySubscriptGadget::getFixits(const Strategy &S) const {
+UPCAddressofArraySubscriptGadget::getFixits(const FixitStrategy &S) const {
auto DREs = getClaimedVarUseSites();
const auto *VD = cast<VarDecl>(DREs.front()->getDecl());
switch (S.lookup(VD)) {
- case Strategy::Kind::Span:
+ case FixitStrategy::Kind::Span:
return fixUPCAddressofArraySubscriptWithSpan(Node);
- case Strategy::Kind::Wontfix:
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
+ case FixitStrategy::Kind::Wontfix:
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
return std::nullopt; // something went wrong, no fix-it
@@ -1803,10 +1769,10 @@ getSpanTypeText(StringRef EltTyText,
}
std::optional<FixItList>
-DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
+DerefSimplePtrArithFixableGadget::getFixits(const FixitStrategy &s) const {
const VarDecl *VD = dyn_cast<VarDecl>(BaseDeclRefExpr->getDecl());
- if (VD && s.lookup(VD) == Strategy::Kind::Span) {
+ if (VD && s.lookup(VD) == FixitStrategy::Kind::Span) {
ASTContext &Ctx = VD->getASTContext();
// std::span can't represent elements before its begin()
if (auto ConstVal = Offset->getIntegerConstantExpr(Ctx))
@@ -1866,10 +1832,10 @@ DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
}
std::optional<FixItList>
-PointerDereferenceGadget::getFixits(const Strategy &S) const {
+PointerDereferenceGadget::getFixits(const FixitStrategy &S) const {
const VarDecl *VD = cast<VarDecl>(BaseDeclRefExpr->getDecl());
switch (S.lookup(VD)) {
- case Strategy::Kind::Span: {
+ case FixitStrategy::Kind::Span: {
ASTContext &Ctx = VD->getASTContext();
SourceManager &SM = Ctx.getSourceManager();
// Required changes: *(ptr); => (ptr[0]); and *ptr; => ptr[0]
@@ -1884,11 +1850,11 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const {
}
break;
}
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
- llvm_unreachable("Strategy not implemented yet!");
- case Strategy::Kind::Wontfix:
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Vector:
+ llvm_unreachable("FixitStrategy not implemented yet!");
+ case FixitStrategy::Kind::Wontfix:
llvm_unreachable("Invalid strategy!");
}
@@ -1897,27 +1863,26 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const {
// Generates fix-its replacing an expression of the form UPC(DRE) with
// `DRE.data()`
-std::optional<FixItList> UPCStandalonePointerGadget::getFixits(const Strategy &S)
- const {
+std::optional<FixItList>
+UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
const auto VD = cast<VarDecl>(Node->getDecl());
switch (S.lookup(VD)) {
- case Strategy::Kind::Span: {
- ASTContext &Ctx = VD->getASTContext();
- SourceManager &SM = Ctx.getSourceManager();
- // Inserts the .data() after the DRE
- std::optional<SourceLocation> EndOfOperand =
- getPastLoc(Node, SM, Ctx.getLangOpts());
-
- if (EndOfOperand)
- return FixItList{{FixItHint::CreateInsertion(
- *EndOfOperand, ".data()")}};
- // FIXME: Points inside a macro expansion.
- break;
+ case FixitStrategy::Kind::Span: {
+ ASTContext &Ctx = VD->getASTContext();
+ SourceManager &SM = Ctx.getSourceManager();
+ // Inserts the .data() after the DRE
+ std::optional<SourceLocation> EndOfOperand =
+ getPastLoc(Node, SM, Ctx.getLangOpts());
+
+ if (EndOfOperand)
+ return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}};
+ // FIXME: Points inside a macro expansion.
+ break;
}
- case Strategy::Kind::Wontfix:
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
+ case FixitStrategy::Kind::Wontfix:
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
@@ -1962,14 +1927,14 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
}
std::optional<FixItList>
-UUCAddAssignGadget::getFixits(const Strategy &S) const {
+UUCAddAssignGadget::getFixits(const FixitStrategy &S) const {
DeclUseList DREs = getClaimedVarUseSites();
if (DREs.size() != 1)
return std::nullopt; // In cases of `Ptr += n` where `Ptr` is not a DRE, we
// give up
if (const VarDecl *VD = dyn_cast<VarDecl>(DREs.front()->getDecl())) {
- if (S.lookup(VD) == Strategy::Kind::Span) {
+ if (S.lookup(VD) == FixitStrategy::Kind::Span) {
FixItList Fixes;
const Stmt *AddAssignNode = getBaseStmt();
@@ -2003,14 +1968,15 @@ UUCAddAssignGadget::getFixits(const Strategy &S) const {
return std::nullopt; // Not in the cases that we can handle for now, give up.
}
-std::optional<FixItList> UPCPreIncrementGadget::getFixits(const Strategy &S) const {
+std::optional<FixItList>
+UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const {
DeclUseList DREs = getClaimedVarUseSites();
if (DREs.size() != 1)
return std::nullopt; // In cases of `++Ptr` where `Ptr` is not a DRE, we
// give up
if (const VarDecl *VD = dyn_cast<VarDecl>(DREs.front()->getDecl())) {
- if (S.lookup(VD) == Strategy::Kind::Span) {
+ if (S.lookup(VD) == FixitStrategy::Kind::Span) {
FixItList Fixes;
std::stringstream SS;
const Stmt *PreIncNode = getBaseStmt();
@@ -2261,7 +2227,7 @@ static bool hasConflictingOverload(const FunctionDecl *FD) {
// }
//
static std::optional<FixItList>
-createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD,
+createOverloadsForFixedParams(const FixitStrategy &S, const FunctionDecl *FD,
const ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler) {
// FIXME: need to make this conflict checking better:
@@ -2278,9 +2244,9 @@ createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD,
for (unsigned i = 0; i < NumParms; i++) {
const ParmVarDecl *PVD = FD->getParamDecl(i);
- if (S.lookup(PVD) == Strategy::Kind::Wontfix)
+ if (S.lookup(PVD) == FixitStrategy::Kind::Wontfix)
continue;
- if (S.lookup(PVD) != Strategy::Kind::Span)
+ if (S.lookup(PVD) != FixitStrategy::Kind::Span)
// Not supported, not suppose to happen:
return std::nullopt;
@@ -2291,7 +2257,8 @@ createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD,
if (!PteTyText)
// something wrong in obtaining the text of the pointee type, give up
return std::nullopt;
- // FIXME: whether we should create std::span type depends on the Strategy.
+ // FIXME: whether we should create std::span type depends on the
+ // FixitStrategy.
NewTysTexts[i] = getSpanTypeText(*PteTyText, PteTyQuals);
ParmsMask[i] = true;
AtLeastOneParmToFix = true;
@@ -2498,7 +2465,7 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
// TODO: we should be consistent to use `std::nullopt` to represent no-fix due
// to any unexpected problem.
static FixItList
-fixVariable(const VarDecl *VD, Strategy::Kind K,
+fixVariable(const VarDecl *VD, FixitStrategy::Kind K,
/* The function decl under analysis */ const Decl *D,
const DeclUseTracker &Tracker, ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler) {
@@ -2529,7 +2496,7 @@ fixVariable(const VarDecl *VD, Strategy::Kind K,
}
switch (K) {
- case Strategy::Kind::Span: {
+ case FixitStrategy::Kind::Span: {
if (VD->getType()->isPointerType()) {
if (const auto *PVD = dyn_cast<ParmVarDecl>(VD))
return fixParamWithSpan(PVD, Ctx, Handler);
@@ -2540,11 +2507,11 @@ fixVariable(const VarDecl *VD, Strategy::Kind K,
DEBUG_NOTE_DECL_FAIL(VD, " : not a pointer");
return {};
}
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
- llvm_unreachable("Strategy not implemented yet!");
- case Strategy::Kind::Wontfix:
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Vector:
+ llvm_unreachable("FixitStrategy not implemented yet!");
+ case FixitStrategy::Kind::Wontfix:
llvm_unreachable("Invalid strategy!");
}
llvm_unreachable("Unknown strategy!");
@@ -2605,7 +2572,8 @@ static void eraseVarsForUnfixableGroupMates(
static FixItList createFunctionOverloadsForParms(
std::map<const VarDecl *, FixItList> &FixItsForVariable /* mutable */,
const VariableGroupsManager &VarGrpMgr, const FunctionDecl *FD,
- const Strategy &S, ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) {
+ const FixitStrategy &S, ASTContext &Ctx,
+ UnsafeBufferUsageHandler &Handler) {
FixItList FixItsSharedByParms{};
std::optional<FixItList> OverloadFixes =
@@ -2625,8 +2593,8 @@ static FixItList createFunctionOverloadsForParms(
// Constructs self-contained fix-its for each variable in `FixablesForAllVars`.
static std::map<const VarDecl *, FixItList>
-getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
- ASTContext &Ctx,
+getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S,
+ ASTContext &Ctx,
/* The function decl under analysis */ const Decl *D,
const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
const VariableGroupsManager &VarGrpMgr) {
@@ -2724,11 +2692,11 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
}
template <typename VarDeclIterTy>
-static Strategy
+static FixitStrategy
getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
- Strategy S;
+ FixitStrategy S;
for (const VarDecl *VD : UnsafeVars) {
- S.set(VD, Strategy::Kind::Span);
+ S.set(VD, FixitStrategy::Kind::Span);
}
return S;
}
@@ -3027,7 +2995,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
// We assign strategies to variables that are 1) in the graph and 2) can be
// fixed. Other variables have the default "Won't fix" strategy.
- Strategy NaiveStrategy = getNaiveStrategy(llvm::make_filter_range(
+ FixitStrategy NaiveStrategy = getNaiveStrategy(llvm::make_filter_range(
VisitedVars, [&FixablesForAllVars](const VarDecl *V) {
// If a warned variable has no "Fixable", it is considered unfixable:
return FixablesForAllVars.byVar.count(V);
>From 38255d1ebe412a770053bc132072d2fdb56af62b Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Tue, 23 Jan 2024 16:24:59 -0800
Subject: [PATCH 02/12] [-Wunsafe-buffer-usage] Implement fixits from const
size array to std::array
Array subscript on a const size array is not bounds-checked. The idiomatic
replacement is std::array which is bounds-safe in hardened mode of libc++.
This commit extends the fixit-producing machine to consider std::array as a
transformation target type and teaches it to handle the array subscript on const
size arrays with a trivial (empty) fixit.
The transformation is straightforward for most cases:
T array[N]; => std::array<T, N> array;
but for array of const pointers it needs to be:
T* const array[N]; => const std::array<T*, N> array;
This case and other special cases are to be implemented in follow-up
patches.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 10 +-
clang/lib/Analysis/UnsafeBufferUsage.cpp | 85 ++++++++++++++++-
clang/lib/Sema/AnalysisBasedWarnings.cpp | 16 +++-
.../warn-unsafe-buffer-usage-array.cpp | 24 +++++
.../warn-unsafe-buffer-usage-debug.cpp | 12 +--
...fe-buffer-usage-fixits-local-var-array.cpp | 95 +++++++++++++++++++
.../test/SemaCXX/warn-unsafe-buffer-usage.cpp | 5 +
7 files changed, 230 insertions(+), 17 deletions(-)
create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 622c094f5b1eb..2c9ebf3fb42d0 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -95,6 +95,8 @@ class UnsafeBufferUsageHandler {
#endif
public:
+ enum class TargetType { Span, Array };
+
UnsafeBufferUsageHandler() = default;
virtual ~UnsafeBufferUsageHandler() = default;
@@ -112,9 +114,11 @@ class UnsafeBufferUsageHandler {
///
/// `D` is the declaration of the callable under analysis that owns `Variable`
/// and all of its group mates.
- virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
- const VariableGroupsManager &VarGrpMgr,
- FixItList &&Fixes, const Decl *D) = 0;
+ virtual void
+ handleUnsafeVariableGroup(const VarDecl *Variable,
+ const VariableGroupsManager &VarGrpMgr,
+ FixItList &&Fixes, const Decl *D,
+ const FixitStrategy &VarTargetTypes) = 0;
#ifndef NDEBUG
public:
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 924d677786275..6c99a23aeb9e8 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1495,6 +1495,7 @@ ULCArraySubscriptGadget::getFixits(const FixitStrategy &S) const {
return FixItList{};
}
case FixitStrategy::Kind::Array:
+ return FixItList{};
case FixitStrategy::Kind::Wontfix:
case FixitStrategy::Kind::Iterator:
case FixitStrategy::Kind::Vector:
@@ -2462,6 +2463,71 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
+ UnsafeBufferUsageHandler &Handler) {
+ 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);
+
+ // For most types the transformation is simple:
+ // T foo[10]; => std::array<T, 10> foo;
+ // Cv-specifiers are straigtforward:
+ // const T foo[10]; => std::array<const T, 10> foo;
+ // Pointers are straightforward:
+ // T * foo[10]; => std::array<T *, 10> foo;
+ //
+ // However, for const pointers the transformation is different:
+ // T * const foo[10]; => const std::array<T *, 10> foo;
+ if (ArrayEltT->isPointerType() && ArrayEltT.isConstQualified()) {
+ DEBUG_NOTE_DECL_FAIL(D, " : const size array of const pointers");
+ // FIXME: implement the support
+ // FIXME: bail if the const pointer is a typedef
+ return {};
+ }
+
+ std::optional<StringRef> IdentText =
+ getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
+
+ if (!IdentText) {
+ DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
+ return {};
+ }
+
+ OS << "std::array<" << ArrayEltT.getAsString() << ", "
+ << getAPIntText(CAT->getSize()) << "> " << IdentText->str();
+
+ 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, Handler);
+}
+
// TODO: we should be consistent to use `std::nullopt` to represent no-fix due
// to any unexpected problem.
static FixItList
@@ -2507,7 +2573,13 @@ fixVariable(const VarDecl *VD, FixitStrategy::Kind K,
DEBUG_NOTE_DECL_FAIL(VD, " : not a pointer");
return {};
}
- case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Array: {
+ if (VD->isLocalVarDecl() && isa<clang::ConstantArrayType>(VD->getType()))
+ return fixVariableWithArray(VD, Tracker, Ctx, Handler);
+
+ DEBUG_NOTE_DECL_FAIL(VD, " : not a local const-size array");
+ return {};
+ }
case FixitStrategy::Kind::Iterator:
case FixitStrategy::Kind::Vector:
llvm_unreachable("FixitStrategy not implemented yet!");
@@ -2696,7 +2768,10 @@ static FixitStrategy
getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
FixitStrategy S;
for (const VarDecl *VD : UnsafeVars) {
- S.set(VD, FixitStrategy::Kind::Span);
+ if (isa<ConstantArrayType>(VD->getType()))
+ S.set(VD, FixitStrategy::Kind::Array);
+ else
+ S.set(VD, FixitStrategy::Kind::Span);
}
return S;
}
@@ -3018,9 +3093,9 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
auto FixItsIt = FixItsForVariableGroup.find(VD);
Handler.handleUnsafeVariableGroup(VD, VarGrpMgr,
FixItsIt != FixItsForVariableGroup.end()
- ? std::move(FixItsIt->second)
- : FixItList{},
- D);
+ ? std::move(FixItsIt->second)
+ : FixItList{},
+ D, NaiveStrategy);
for (const auto &G : WarningGadgets) {
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true,
D->getASTContext());
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 78b9f324e1390..8239ba49429d3 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2297,7 +2297,8 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
void handleUnsafeVariableGroup(const VarDecl *Variable,
const VariableGroupsManager &VarGrpMgr,
- FixItList &&Fixes, const Decl *D) override {
+ FixItList &&Fixes, const Decl *D,
+ const FixitStrategy &VarTargetTypes) override {
assert(!SuggestSuggestions &&
"Unsafe buffer usage fixits displayed without suggestions!");
S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
@@ -2312,7 +2313,18 @@ 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 = 0;
+ switch (VarTargetTypes.lookup(Variable)) {
+ case clang::FixitStrategy::Kind::Span:
+ FixItStrategy = 0;
+ break;
+ case clang::FixitStrategy::Kind::Array:
+ FixItStrategy = 1;
+ break;
+ default:
+ assert(false && "We support only std::span and std::array");
+ };
+
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-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
new file mode 100644
index 0000000000000..12fd5c69c78f0
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -verify %s
+
+// CHECK-NOT: [-Wunsafe-buffer-usage]
+
+
+void foo(unsigned idx) {
+ int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds}}
+ // expected-note at -1{{change type of 'buffer' to 'std::array' to preserve bounds information}}
+ buffer[idx] = 0; // expected-note{{used in buffer access here}}
+}
+
+int global_buffer[10]; // expected-warning{{'global_buffer' is an unsafe buffer that does not perform bounds}}
+void foo2(unsigned idx) {
+ global_buffer[idx] = 0; // expected-note{{used in buffer access here}}
+}
+
+struct Foo {
+ int member_buffer[10];
+};
+void foo2(Foo& f, unsigned idx) {
+ f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
index e08f70d97e391..518fafcda9d37 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -32,13 +32,11 @@ 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_decl(const int* out, unsigned idx) {
+ const int* const a[10] = {nullptr}; // 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' : const size array of const pointers}}
+
+ out = a[idx]; // expected-note{{used in buffer access here}}
}
void failed_multiple_decl() {
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
new file mode 100644
index 0000000000000..a14a88ac08636
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -0,0 +1,95 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array(unsigned idx) {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+ buffer[idx] = 0;
+}
+
+void unsupported_multi_decl1(unsigned idx) {
+ int a, buffer[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ buffer[idx] = 0;
+}
+
+void unsupported_multi_decl2(unsigned idx) {
+ int buffer[10], b;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ buffer[idx] = 0;
+}
+
+void local_array_ptr_to_const(unsigned idx, const int*& a) {
+ const int * buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array<const int *, 10> buffer"
+ a = buffer[idx];
+}
+
+void local_array_const_ptr(unsigned idx, int*& a) {
+ int * const buffer[10] = {a};
+// FIXME: implement support
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+ a = buffer[idx];
+
+}
+
+void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
+ const int * const buffer[10] = {a};
+// FIXME: implement support
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+ a = buffer[idx];
+
+}
+
+template<typename T>
+void local_array_in_template(unsigned idx) {
+ T buffer[10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+ buffer[idx] = 0;
+}
+// Instantiate the template function to force its analysis.
+template void local_array_in_template<int>(unsigned); // FIXME: expected note {{in instantiation of}}
+
+void macro_as_identifier(unsigned idx) {
+#define MY_BUFFER buffer
+ // Although fix-its include macros, the macros do not overlap with
+ // the bounds of the source range of these fix-its. So these fix-its
+ // are valid.
+
+ int MY_BUFFER[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:20}:"std::array<int, 10> MY_BUFFER"
+ MY_BUFFER[idx] = 0;
+
+#undef MY_BUFFER
+}
+
+void unsupported_fixit_overlapping_macro(unsigned idx) {
+#define MY_INT int
+ MY_INT buffer[10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ buffer[idx] = 0;
+#undef MY_INT
+}
+
+void subscript_negative() {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+
+ // For constant-size arrays any negative index will lead to buffer underflow.
+ // std::array::operator[] has unsigned parameter so the value will be casted to unsigned.
+ // This will very likely be buffer overflow but hardened std::array catch these at runtime.
+ buffer[-5] = 0;
+}
+
+void subscript_signed(int signed_idx) {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+
+ // For constant-size arrays any negative index will lead to buffer underflow.
+ // std::array::operator[] has unsigned parameter so the value will be casted to unsigned.
+ // This will very likely be buffer overflow but hardened std::array catch these at runtime.
+ buffer[signed_idx] = 0;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
index c5f0a9ef92937..a70d4bda45c72 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}}
}
>From 83e31ad1c55ffa7967e940da18c534bce5bcf8ef Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 24 Jan 2024 14:22:01 -0800
Subject: [PATCH 03/12] [-Wunsafe-buffer-usage] Don't assert Array strategy is
unimplemented
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 6c99a23aeb9e8..7b9edbd605650 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1442,6 +1442,7 @@ PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
return std::nullopt;
case FixitStrategy::Kind::Iterator:
case FixitStrategy::Kind::Array:
+ return std::nullopt;
case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
@@ -1461,6 +1462,7 @@ PointerInitGadget::getFixits(const FixitStrategy &S) const {
return std::nullopt;
case FixitStrategy::Kind::Iterator:
case FixitStrategy::Kind::Array:
+ return std::nullopt;
case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
@@ -1519,6 +1521,7 @@ UPCAddressofArraySubscriptGadget::getFixits(const FixitStrategy &S) const {
case FixitStrategy::Kind::Wontfix:
case FixitStrategy::Kind::Iterator:
case FixitStrategy::Kind::Array:
+ return std::nullopt;
case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
@@ -1853,6 +1856,7 @@ PointerDereferenceGadget::getFixits(const FixitStrategy &S) const {
}
case FixitStrategy::Kind::Iterator:
case FixitStrategy::Kind::Array:
+ return std::nullopt;
case FixitStrategy::Kind::Vector:
llvm_unreachable("FixitStrategy not implemented yet!");
case FixitStrategy::Kind::Wontfix:
@@ -1883,6 +1887,7 @@ UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
case FixitStrategy::Kind::Wontfix:
case FixitStrategy::Kind::Iterator:
case FixitStrategy::Kind::Array:
+ return std::nullopt;
case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
>From da47129c2987295afbc5432e4199bcbefaa115e7 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Fri, 26 Jan 2024 16:22:50 -0800
Subject: [PATCH 04/12] [-Wunsafe-buffer-usage] Preserve spelling of array size
in std::array fixits
Fixits related to arrays generally change type from T [N] to
std::array<T, N>. N has to be constant but doesn't have to be an integer
literal. It can be for example a constant.
The fixits need to preserve the spelling of the array size.
The assumption is that not specifying the numerical value directly was a
conscious decision of the original author and it'll very likely still apply to
the transformed code.
E. g. for
int buffer[SIZE];
if `buffer` is unsafe it should become:
std::array<int, SIZE> buffer;
rather than:
std::array<int, 42> buffer;
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 23 ++++++++++---
...fe-buffer-usage-fixits-local-var-array.cpp | 32 +++++++++++++------
2 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 7b9edbd605650..9ad532aedfa20 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2475,10 +2475,6 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
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);
// For most types the transformation is simple:
// T foo[10]; => std::array<T, 10> foo;
@@ -2496,6 +2492,21 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
return {};
}
+ // Find the '[' token.
+ std::optional<Token> NextTok = Lexer::findNextToken(getVarDeclIdentifierLoc(D), Ctx.getSourceManager(), Ctx.getLangOpts());
+ while (NextTok && !NextTok->is(tok::l_square))
+ NextTok = Lexer::findNextToken(NextTok->getLocation(), Ctx.getSourceManager(), Ctx.getLangOpts());
+ if (!NextTok)
+ return {};
+ const SourceLocation LSqBracketLoc = NextTok->getLocation();
+
+ // Get the spelling of the array size as written in the source file (including macros, etc.).
+ auto MaybeArraySizeTxt = getRangeText({LSqBracketLoc.getLocWithOffset(1), D->getTypeSpecEndLoc()}, Ctx.getSourceManager(), Ctx.getLangOpts());
+ if (!MaybeArraySizeTxt)
+ return {};
+
+ const std::string ArraySizeTxt = MaybeArraySizeTxt->str();
+
std::optional<StringRef> IdentText =
getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
@@ -2504,8 +2515,10 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
return {};
}
+ SmallString<32> Replacement;
+ raw_svector_ostream OS(Replacement);
OS << "std::array<" << ArrayEltT.getAsString() << ", "
- << getAPIntText(CAT->getSize()) << "> " << IdentText->str();
+ << ArraySizeTxt << "> " << IdentText->str();
FixIts.push_back(FixItHint::CreateReplacement(
SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str()));
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index a14a88ac08636..6f6b1a8aa5c66 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -10,6 +10,18 @@ void local_array(unsigned idx) {
buffer[idx] = 0;
}
+void weird_whitespace_in_declaration(unsigned idx) {
+ int buffer_w [ 10 ] ;
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:35}:"std::array<int, 10 > buffer_w"
+ buffer_w[idx] = 0;
+}
+
+void weird_comments_in_declaration(unsigned idx) {
+ int /* [ ] */ buffer_w /* [ ] */ [ /* [ ] */ 10 /* [ ] */ ] ;
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:67}:"std::array<int, /* [ ] */ 10 /* [ ] */ > buffer_w"
+ buffer_w[idx] = 0;
+}
+
void unsupported_multi_decl1(unsigned idx) {
int a, buffer[10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
@@ -55,23 +67,25 @@ template void local_array_in_template<int>(unsigned); // FIXME: expected note {{
void macro_as_identifier(unsigned idx) {
#define MY_BUFFER buffer
- // Although fix-its include macros, the macros do not overlap with
- // the bounds of the source range of these fix-its. So these fix-its
- // are valid.
-
int MY_BUFFER[10];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:20}:"std::array<int, 10> MY_BUFFER"
MY_BUFFER[idx] = 0;
-
#undef MY_BUFFER
}
-void unsupported_fixit_overlapping_macro(unsigned idx) {
-#define MY_INT int
- MY_INT buffer[10];
+void macro_as_size(unsigned idx) {
+#define MY_TEN 10
+ int buffer[MY_TEN];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
buffer[idx] = 0;
-#undef MY_INT
+#undef MY_TEN
+}
+
+void constant_as_size(unsigned idx) {
+ const unsigned my_const = 10;
+ int buffer[my_const];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::array<int, my_const> buffer"
+ buffer[idx] = 0;
}
void subscript_negative() {
>From e3c2483fa637ac5e0b16f137e658bae8561691b9 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Fri, 26 Jan 2024 16:52:41 -0800
Subject: [PATCH 05/12] [-Wunsafe-buffer-usage] Preserve array element type
spelling in std::array fixits
In other words - don't use the type that the source code gets expanded
to in the AST as there's very likely a reason why it is represented via
a typedef or macro in the sourcode.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 23 +++++++++++++++++--
...fe-buffer-usage-fixits-local-var-array.cpp | 19 +++++++++++++--
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 9ad532aedfa20..1a06eff32f039 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -12,6 +12,8 @@
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/StmtVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/CharInfo.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Lex/Lexer.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/APSInt.h"
@@ -2492,8 +2494,25 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
return {};
}
+ const SourceLocation IdentifierLoc = getVarDeclIdentifierLoc(D);
+
+ // Get the spelling of the element type as written in the source file (including macros, etc.).
+ auto MaybeElemTypeTxt = getRangeText({D->getBeginLoc(), IdentifierLoc}, Ctx.getSourceManager(), Ctx.getLangOpts());
+ if (!MaybeElemTypeTxt)
+ return {};
+ std::string ElemTypeTxt = MaybeElemTypeTxt->str();
+ // Trim whitespace from the type spelling.
+ unsigned TrailingWhitespace = 0;
+ for (auto It = ElemTypeTxt.rbegin(); It < ElemTypeTxt.rend(); ++It) {
+ if (!isWhitespace(*It))
+ break;
+ ++TrailingWhitespace;
+ }
+ if (TrailingWhitespace > 0)
+ ElemTypeTxt.erase(ElemTypeTxt.length() - TrailingWhitespace);
+
// Find the '[' token.
- std::optional<Token> NextTok = Lexer::findNextToken(getVarDeclIdentifierLoc(D), Ctx.getSourceManager(), Ctx.getLangOpts());
+ std::optional<Token> NextTok = Lexer::findNextToken(IdentifierLoc, Ctx.getSourceManager(), Ctx.getLangOpts());
while (NextTok && !NextTok->is(tok::l_square))
NextTok = Lexer::findNextToken(NextTok->getLocation(), Ctx.getSourceManager(), Ctx.getLangOpts());
if (!NextTok)
@@ -2517,7 +2536,7 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
SmallString<32> Replacement;
raw_svector_ostream OS(Replacement);
- OS << "std::array<" << ArrayEltT.getAsString() << ", "
+ OS << "std::array<" << ElemTypeTxt << ", "
<< ArraySizeTxt << "> " << IdentText->str();
FixIts.push_back(FixItHint::CreateReplacement(
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index 6f6b1a8aa5c66..2b44e238b03ad 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -18,7 +18,7 @@ void weird_whitespace_in_declaration(unsigned idx) {
void weird_comments_in_declaration(unsigned idx) {
int /* [ ] */ buffer_w /* [ ] */ [ /* [ ] */ 10 /* [ ] */ ] ;
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:67}:"std::array<int, /* [ ] */ 10 /* [ ] */ > buffer_w"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:67}:"std::array<int /* [ ] */, /* [ ] */ 10 /* [ ] */ > buffer_w"
buffer_w[idx] = 0;
}
@@ -65,6 +65,21 @@ void local_array_in_template(unsigned idx) {
// Instantiate the template function to force its analysis.
template void local_array_in_template<int>(unsigned); // FIXME: expected note {{in instantiation of}}
+typedef unsigned int uint;
+void typedef_as_elem_type(unsigned idx) {
+ uint buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:18}:"std::array<uint, 10> buffer"
+ buffer[idx] = 0;
+}
+
+void macro_as_elem_type(unsigned idx) {
+#define MY_INT int
+ MY_INT buffer[10];
+// FIXME: implement support
+ buffer[idx] = 0;
+#undef MY_INT
+}
+
void macro_as_identifier(unsigned idx) {
#define MY_BUFFER buffer
int MY_BUFFER[10];
@@ -76,7 +91,7 @@ void macro_as_identifier(unsigned idx) {
void macro_as_size(unsigned idx) {
#define MY_TEN 10
int buffer[MY_TEN];
-// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array<int, MY_TEN> buffer"
buffer[idx] = 0;
#undef MY_TEN
}
>From d19a2c30194d33c0a6602b9cd59f09e61e93b9bc Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Mon, 29 Jan 2024 12:54:10 -0800
Subject: [PATCH 06/12] [-Wunsafe-buffer-usage] Fix whitespace handling in
array types
Specifically applies to text with array element type and array size for
constant size arrays. Trimming there strings on both sides and using
llvm::StringRef::trim().
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 15 +++------------
...unsafe-buffer-usage-fixits-local-var-array.cpp | 12 ++++++------
2 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 1a06eff32f039..6570d339c3494 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -18,6 +18,7 @@
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
#include <memory>
#include <optional>
#include <queue>
@@ -2500,16 +2501,7 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
auto MaybeElemTypeTxt = getRangeText({D->getBeginLoc(), IdentifierLoc}, Ctx.getSourceManager(), Ctx.getLangOpts());
if (!MaybeElemTypeTxt)
return {};
- std::string ElemTypeTxt = MaybeElemTypeTxt->str();
- // Trim whitespace from the type spelling.
- unsigned TrailingWhitespace = 0;
- for (auto It = ElemTypeTxt.rbegin(); It < ElemTypeTxt.rend(); ++It) {
- if (!isWhitespace(*It))
- break;
- ++TrailingWhitespace;
- }
- if (TrailingWhitespace > 0)
- ElemTypeTxt.erase(ElemTypeTxt.length() - TrailingWhitespace);
+ const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
// Find the '[' token.
std::optional<Token> NextTok = Lexer::findNextToken(IdentifierLoc, Ctx.getSourceManager(), Ctx.getLangOpts());
@@ -2523,8 +2515,7 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
auto MaybeArraySizeTxt = getRangeText({LSqBracketLoc.getLocWithOffset(1), D->getTypeSpecEndLoc()}, Ctx.getSourceManager(), Ctx.getLangOpts());
if (!MaybeArraySizeTxt)
return {};
-
- const std::string ArraySizeTxt = MaybeArraySizeTxt->str();
+ const llvm::StringRef ArraySizeTxt = MaybeArraySizeTxt->trim();
std::optional<StringRef> IdentText =
getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index 2b44e238b03ad..052a085395055 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -10,15 +10,15 @@ void local_array(unsigned idx) {
buffer[idx] = 0;
}
-void weird_whitespace_in_declaration(unsigned idx) {
- int buffer_w [ 10 ] ;
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:35}:"std::array<int, 10 > buffer_w"
+void whitespace_in_declaration(unsigned idx) {
+ int buffer_w [ 10 ];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:35}:"std::array<int, 10> buffer_w"
buffer_w[idx] = 0;
}
-void weird_comments_in_declaration(unsigned idx) {
- int /* [ ] */ buffer_w /* [ ] */ [ /* [ ] */ 10 /* [ ] */ ] ;
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:67}:"std::array<int /* [ ] */, /* [ ] */ 10 /* [ ] */ > buffer_w"
+void comments_in_declaration(unsigned idx) {
+ int /* [A] */ buffer_w /* [B] */ [ /* [C] */ 10 /* [D] */ ] ;
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:69}:"std::array<int /* [A] */, /* [C] */ 10 /* [D] */> buffer_w"
buffer_w[idx] = 0;
}
>From 849f6431dcf14e7b245615c8b8e19256c1f41f48 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Mon, 29 Jan 2024 13:24:24 -0800
Subject: [PATCH 07/12] [-Wnsafe-buffer-usage][NFC] Fix and refactor tests
---
...fe-buffer-usage-fixits-local-var-array.cpp | 32 ++++++++++++-------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index 052a085395055..c39c9fd754071 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -4,7 +4,7 @@
typedef int * Int_ptr_t;
typedef int Int_t;
-void local_array(unsigned idx) {
+void simple(unsigned idx) {
int buffer[10];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
buffer[idx] = 0;
@@ -22,15 +22,19 @@ void comments_in_declaration(unsigned idx) {
buffer_w[idx] = 0;
}
-void unsupported_multi_decl1(unsigned idx) {
+void multi_decl1(unsigned idx) {
int a, buffer[10];
- // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// FIXME: implement support
+
buffer[idx] = 0;
}
-void unsupported_multi_decl2(unsigned idx) {
+void multi_decl2(unsigned idx) {
int buffer[10], b;
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// FIXME: implement support
+
buffer[idx] = 0;
}
@@ -42,40 +46,44 @@ void local_array_ptr_to_const(unsigned idx, const int*& a) {
void local_array_const_ptr(unsigned idx, int*& a) {
int * const buffer[10] = {a};
-// FIXME: implement support
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+// FIXME: implement support
+
a = buffer[idx];
}
void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
const int * const buffer[10] = {a};
-// FIXME: implement support
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+// FIXME: implement support
+
a = buffer[idx];
}
template<typename T>
-void local_array_in_template(unsigned idx) {
+void unsupported_local_array_in_template(unsigned idx) {
T buffer[10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
buffer[idx] = 0;
}
// Instantiate the template function to force its analysis.
-template void local_array_in_template<int>(unsigned); // FIXME: expected note {{in instantiation of}}
+template void unsupported_local_array_in_template<int>(unsigned);
-typedef unsigned int uint;
+typedef unsigned int my_uint;
void typedef_as_elem_type(unsigned idx) {
- uint buffer[10];
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:18}:"std::array<uint, 10> buffer"
+ my_uint buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array<my_uint, 10> buffer"
buffer[idx] = 0;
}
void macro_as_elem_type(unsigned idx) {
#define MY_INT int
MY_INT buffer[10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
// FIXME: implement support
+
buffer[idx] = 0;
#undef MY_INT
}
@@ -119,6 +127,6 @@ void subscript_signed(int signed_idx) {
// For constant-size arrays any negative index will lead to buffer underflow.
// std::array::operator[] has unsigned parameter so the value will be casted to unsigned.
- // This will very likely be buffer overflow but hardened std::array catch these at runtime.
+ // This will very likely be buffer overflow but hardened std::array catches these at runtime.
buffer[signed_idx] = 0;
}
>From b6688fb091b86a365ace76a264ec6766d934a116 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Mon, 29 Jan 2024 14:39:14 -0800
Subject: [PATCH 08/12] [-Wunsafe-buffer-usage] Bail on fixits for arrays whose
size is auto-deduced
C++ allows the array size to be omitted in declaration if it can be deduced from the
initializer.
E. g.:
int array [] = { 1, 2, 3 };
std::array<T, N> on the other hand requires the size N to be provided.
Currently the fixit produced would use empty space for N which would lead to a
compilation failure. This patch makes the fixit-producing machine just
bail in such cases and adds a FIXME to be addressed later.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 9 +++++++++
...nsafe-buffer-usage-fixits-local-var-array.cpp | 16 ++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 6570d339c3494..18d0ca7a17d3c 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2516,6 +2516,15 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
if (!MaybeArraySizeTxt)
return {};
const llvm::StringRef ArraySizeTxt = MaybeArraySizeTxt->trim();
+ if (ArraySizeTxt.empty()) {
+ // FIXME: Support array size getting determined from the initializer.
+ // Examples:
+ // int arr1[] = {0, 1, 2};
+ // int arr2{3, 4, 5};
+ // We might be able to preserve the non-specified size with `auto` and `std::to_array`:
+ // auto arr1 = std::to_array<int>({0, 1, 2});
+ return {};
+ }
std::optional<StringRef> IdentText =
getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index c39c9fd754071..c0688380c08a4 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -22,6 +22,22 @@ void comments_in_declaration(unsigned idx) {
buffer_w[idx] = 0;
}
+void auto_size(unsigned idx) {
+ int buffer[] = {0, 1, 2};
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// FIXME: implement support
+
+ buffer[idx] = 0;
+}
+
+void universal_initialization(unsigned idx) {
+ int buffer[] {0, 1, 2};
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// FIXME: implement support
+
+ buffer[idx] = 0;
+}
+
void multi_decl1(unsigned idx) {
int a, buffer[10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
>From fd791ccdbf919c4810a92e700665a4766ef402a7 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Mon, 29 Jan 2024 14:39:58 -0800
Subject: [PATCH 09/12] [-Wunsafe-buffer-usage][NFC] Add testcases
---
...fe-buffer-usage-fixits-local-var-array.cpp | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index c0688380c08a4..7e83d8e68a9ee 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -22,6 +22,17 @@ void comments_in_declaration(unsigned idx) {
buffer_w[idx] = 0;
}
+void initializer(unsigned idx) {
+ int buffer[3] = {0};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::array<int, 3> buffer"
+
+ int buffer2[3] = {0, 1, 2};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 3> buffer2"
+
+ buffer[idx] = 0;
+ buffer2[idx] = 0;
+}
+
void auto_size(unsigned idx) {
int buffer[] = {0, 1, 2};
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
@@ -66,7 +77,15 @@ void local_array_const_ptr(unsigned idx, int*& a) {
// FIXME: implement support
a = buffer[idx];
+}
+void local_array_const_ptr_via_typedef(unsigned idx, int*& a) {
+ typedef int * const my_const_ptr;
+ my_const_ptr buffer[10] = {a};
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+// FIXME: implement support
+
+ a = buffer[idx];
}
void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
>From 9183a63f3aed8e27563381d037bec0e170cbf795 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 31 Jan 2024 14:13:58 -0800
Subject: [PATCH 10/12] [-Wunsafe-buffer-usage] Clang-format std::array fixits
work
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 42 ++++++++++++++----------
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 18d0ca7a17d3c..30723b97f6268 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1886,13 +1886,13 @@ UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}};
// FIXME: Points inside a macro expansion.
break;
- }
- case FixitStrategy::Kind::Wontfix:
- case FixitStrategy::Kind::Iterator:
- case FixitStrategy::Kind::Array:
- return std::nullopt;
- case FixitStrategy::Kind::Vector:
- llvm_unreachable("unsupported strategies for FixableGadgets");
+ }
+ case FixitStrategy::Kind::Wontfix:
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Array:
+ return std::nullopt;
+ case FixitStrategy::Kind::Vector:
+ llvm_unreachable("unsupported strategies for FixableGadgets");
}
return std::nullopt;
@@ -2008,7 +2008,6 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const {
return std::nullopt; // Not in the cases that we can handle for now, give up.
}
-
// For a non-null initializer `Init` of `T *` type, this function returns
// `FixItHint`s producing a list initializer `{Init, S}` as a part of a fix-it
// to output stream.
@@ -2497,22 +2496,30 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
const SourceLocation IdentifierLoc = getVarDeclIdentifierLoc(D);
- // Get the spelling of the element type as written in the source file (including macros, etc.).
- auto MaybeElemTypeTxt = getRangeText({D->getBeginLoc(), IdentifierLoc}, Ctx.getSourceManager(), Ctx.getLangOpts());
+ // Get the spelling of the element type as written in the source file
+ // (including macros, etc.).
+ auto MaybeElemTypeTxt =
+ getRangeText({D->getBeginLoc(), IdentifierLoc}, Ctx.getSourceManager(),
+ Ctx.getLangOpts());
if (!MaybeElemTypeTxt)
return {};
const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
// Find the '[' token.
- std::optional<Token> NextTok = Lexer::findNextToken(IdentifierLoc, Ctx.getSourceManager(), Ctx.getLangOpts());
+ std::optional<Token> NextTok = Lexer::findNextToken(
+ IdentifierLoc, Ctx.getSourceManager(), Ctx.getLangOpts());
while (NextTok && !NextTok->is(tok::l_square))
- NextTok = Lexer::findNextToken(NextTok->getLocation(), Ctx.getSourceManager(), Ctx.getLangOpts());
+ NextTok = Lexer::findNextToken(NextTok->getLocation(),
+ Ctx.getSourceManager(), Ctx.getLangOpts());
if (!NextTok)
return {};
const SourceLocation LSqBracketLoc = NextTok->getLocation();
- // Get the spelling of the array size as written in the source file (including macros, etc.).
- auto MaybeArraySizeTxt = getRangeText({LSqBracketLoc.getLocWithOffset(1), D->getTypeSpecEndLoc()}, Ctx.getSourceManager(), Ctx.getLangOpts());
+ // Get the spelling of the array size as written in the source file
+ // (including macros, etc.).
+ auto MaybeArraySizeTxt = getRangeText(
+ {LSqBracketLoc.getLocWithOffset(1), D->getTypeSpecEndLoc()},
+ Ctx.getSourceManager(), Ctx.getLangOpts());
if (!MaybeArraySizeTxt)
return {};
const llvm::StringRef ArraySizeTxt = MaybeArraySizeTxt->trim();
@@ -2521,7 +2528,8 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
// Examples:
// int arr1[] = {0, 1, 2};
// int arr2{3, 4, 5};
- // We might be able to preserve the non-specified size with `auto` and `std::to_array`:
+ // We might be able to preserve the non-specified size with `auto` and
+ // `std::to_array`:
// auto arr1 = std::to_array<int>({0, 1, 2});
return {};
}
@@ -2536,8 +2544,8 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
SmallString<32> Replacement;
raw_svector_ostream OS(Replacement);
- OS << "std::array<" << ElemTypeTxt << ", "
- << ArraySizeTxt << "> " << IdentText->str();
+ OS << "std::array<" << ElemTypeTxt << ", " << ArraySizeTxt << "> "
+ << IdentText->str();
FixIts.push_back(FixItHint::CreateReplacement(
SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str()));
>From 04abb733381bdde6aa823a527b7dbd3b4ced7b85 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 31 Jan 2024 17:04:30 -0800
Subject: [PATCH 11/12] [-Wunsafe-buffer-usage] Fix note message suggesting to
use std::array
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 4 ++--
clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp | 6 +++---
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp | 10 +++++-----
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1c0ebbe4e6834..c2239f9785944 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12113,9 +12113,9 @@ def warn_unsafe_buffer_operation : Warning<
def note_unsafe_buffer_operation : Note<
"used%select{| in pointer arithmetic| in buffer access}0 here">;
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">;
+ "change type of %0 to '%select{std::span' to preserve bounds information|std::array' to harden it|std::span::iterator' to preserve bounds information}1%select{|, and change %2 to '%select{std::span|std::array|std::span::iterator}1' to propagate bounds information between them}3">;
def note_unsafe_buffer_variable_fixit_together : Note<
- "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information"
+ "change type of %0 to '%select{std::span' to preserve bounds information|std::array' to harden it|std::span::iterator' to preserve bounds information}1"
"%select{|, and change %2 to safe types to make function %4 bounds-safe}3">;
def note_safe_buffer_usage_suggestions_disabled : Note<
"pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 12fd5c69c78f0..ed12f360dc445 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -6,12 +6,12 @@
void foo(unsigned idx) {
- int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds}}
- // expected-note at -1{{change type of 'buffer' to 'std::array' to preserve bounds information}}
+ int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds checks}}
+ // expected-note at -1{{change type of 'buffer' to 'std::array' to harden it}}
buffer[idx] = 0; // expected-note{{used in buffer access here}}
}
-int global_buffer[10]; // expected-warning{{'global_buffer' is an unsafe buffer that does not perform bounds}}
+int global_buffer[10]; // expected-warning{{'global_buffer' is an unsafe buffer that does not perform bounds checks}}
void foo2(unsigned idx) {
global_buffer[idx] = 0; // expected-note{{used in buffer access here}}
}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
index a70d4bda45c72..6226114bad7a3 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -61,7 +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}}
+ // expected-note at -1{{change type of 'a' to 'std::array' to harden it}}
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}}
@@ -175,7 +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}}
+ // expected-note at -1{{change type of 'b' to 'std::array' to harden it}}
int c[10];
auto Lam1 = [a]() {
@@ -193,9 +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}}
+ // expected-note at -1{{change type of 'a' to 'std::array' to harden it}}
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}}
+ // expected-note at -1{{change type of 'b' to 'std::array' to harden it}}
auto Lam1 = [=]() {
return a[1]; // expected-note{{used in buffer access here}}
@@ -348,7 +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}}
+ // expected-note at -1{{change type of 'ar' to 'std::array' to harden it}}
foo(ar[5]); // expected-note{{used in buffer access here}}
}
>From f3ba5be3a27a9d71c52ebff1223c908bcb773efa Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 1 Feb 2024 13:58:34 -0800
Subject: [PATCH 12/12] [-Wunsafe-buffer-usage] Enable fixits for const ptr
array declarations
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 16 ----------------
...nsafe-buffer-usage-fixits-local-var-array.cpp | 9 +++------
2 files changed, 3 insertions(+), 22 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 30723b97f6268..fce0716781de4 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2478,22 +2478,6 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
const QualType &ArrayEltT = CAT->getElementType();
assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
- // For most types the transformation is simple:
- // T foo[10]; => std::array<T, 10> foo;
- // Cv-specifiers are straigtforward:
- // const T foo[10]; => std::array<const T, 10> foo;
- // Pointers are straightforward:
- // T * foo[10]; => std::array<T *, 10> foo;
- //
- // However, for const pointers the transformation is different:
- // T * const foo[10]; => const std::array<T *, 10> foo;
- if (ArrayEltT->isPointerType() && ArrayEltT.isConstQualified()) {
- DEBUG_NOTE_DECL_FAIL(D, " : const size array of const pointers");
- // FIXME: implement the support
- // FIXME: bail if the const pointer is a typedef
- return {};
- }
-
const SourceLocation IdentifierLoc = getVarDeclIdentifierLoc(D);
// Get the spelling of the element type as written in the source file
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index 7e83d8e68a9ee..496f618c496aa 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -73,8 +73,7 @@ void local_array_ptr_to_const(unsigned idx, const int*& a) {
void local_array_const_ptr(unsigned idx, int*& a) {
int * const buffer[10] = {a};
-// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
-// FIXME: implement support
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array<int * const, 10> buffer"
a = buffer[idx];
}
@@ -82,16 +81,14 @@ void local_array_const_ptr(unsigned idx, int*& a) {
void local_array_const_ptr_via_typedef(unsigned idx, int*& a) {
typedef int * const my_const_ptr;
my_const_ptr buffer[10] = {a};
-// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
-// FIXME: implement support
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:26}:"std::array<my_const_ptr, 10> buffer"
a = buffer[idx];
}
void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
const int * const buffer[10] = {a};
-// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
-// FIXME: implement support
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:31}:"std::array<const int * const, 10> buffer"
a = buffer[idx];
More information about the cfe-commits
mailing list