[clang] [-Wunsafe-buffer-usage] Fixits for array decayed to pointer (PR #80347)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 8 15:57:20 PST 2024
https://github.com/jkorous-apple updated https://github.com/llvm/llvm-project/pull/80347
>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/22] [-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 aca1ad998822c5..622c094f5b1eb1 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 823cd2a7b99691..924d677786275d 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/22] [-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 622c094f5b1eb1..2c9ebf3fb42d07 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 924d677786275d..6c99a23aeb9e80 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 78b9f324e13906..8239ba49429d3c 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 00000000000000..12fd5c69c78f06
--- /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 e08f70d97e3912..518fafcda9d375 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 00000000000000..a14a88ac08636d
--- /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 c5f0a9ef929371..a70d4bda45c72c 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/22] [-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 6c99a23aeb9e80..7b9edbd6056507 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/22] [-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 7b9edbd6056507..9ad532aedfa205 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 a14a88ac08636d..6f6b1a8aa5c665 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/22] [-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 9ad532aedfa205..1a06eff32f039e 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 6f6b1a8aa5c665..2b44e238b03ad0 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/22] [-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 1a06eff32f039e..6570d339c3494f 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 2b44e238b03ad0..052a0853950551 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/22] [-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 052a0853950551..c39c9fd7540718 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/22] [-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 6570d339c3494f..18d0ca7a17d3c8 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 c39c9fd7540718..c0688380c08a40 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/22] [-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 c0688380c08a40..7e83d8e68a9ee8 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/22] [-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 18d0ca7a17d3c8..30723b97f62689 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/22] [-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 1c0ebbe4e68343..c2239f97859447 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 12fd5c69c78f06..ed12f360dc4452 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 a70d4bda45c72c..6226114bad7a37 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 5540d7b3159182eb0fbd8d9b2f5cfc4ddcbcb51e 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/22] [-Wunsafe-buffer-usage] Enable fixits for const ptr
array declarations
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 16 ----------------
.../SemaCXX/warn-unsafe-buffer-usage-debug.cpp | 7 -------
...nsafe-buffer-usage-fixits-local-var-array.cpp | 9 +++------
3 files changed, 3 insertions(+), 29 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 30723b97f62689..fce0716781de42 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-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
index 518fafcda9d375..3a360566d1f3a5 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -32,13 +32,6 @@ void foo() {
// debug-note{{safe buffers debug: gadget 'ULCArraySubscript' refused to produce a fix}}
}
-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() {
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-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
index 7e83d8e68a9ee8..496f618c496aaa 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];
>From 7694a6d8f2b14b87447765c535f7877d63593ffa Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 1 Feb 2024 14:02:35 -0800
Subject: [PATCH 13/22] [-Wunsafe-buffer-usage] Remove unused TargetType enum
---
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 2c9ebf3fb42d07..5d16dcc824c50c 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -95,8 +95,6 @@ class UnsafeBufferUsageHandler {
#endif
public:
- enum class TargetType { Span, Array };
-
UnsafeBufferUsageHandler() = default;
virtual ~UnsafeBufferUsageHandler() = default;
>From 437a4f57da424faeab8865253653cfcefde8e631 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Tue, 6 Feb 2024 10:10:54 -0800
Subject: [PATCH 14/22] [-Wunsafe-buffer-usage] Explicitly unsupport fixits for
multidimensional C arrays
Proper support for multidimensional arrays is outside of the scope of
initial std::array fixits.
This change makes sure that we don't accidentally emit fixits for multidimensional
arrays until that work is all done. It adds a specific check when transforming
VarDecl type to std::array. Also adds corresponding several testscases.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 4 +++
...fe-buffer-usage-fixits-local-var-array.cpp | 26 +++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index fce0716781de42..bd7183d8be068c 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -19,6 +19,7 @@
#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
#include <memory>
#include <optional>
#include <queue>
@@ -2477,6 +2478,9 @@ 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!");
+ // FIXME: support multi-dimensional arrays
+ if (isa<clang::ConstantArrayType>(ArrayEltT))
+ return {};
const SourceLocation IdentifierLoc = getVarDeclIdentifierLoc(D);
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 496f618c496aaa..a3ba46f27a0dcd 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,32 @@ void simple(unsigned idx) {
buffer[idx] = 0;
}
+void array2d(unsigned idx) {
+ int buffer[10][10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ buffer[idx][idx] = 0;
+}
+
+void array2d_assign_from_elem(unsigned idx) {
+ int buffer[10][10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ int a = buffer[idx][idx];
+}
+
+void array2d_use(unsigned);
+void array2d_call(unsigned idx) {
+ int buffer[10][10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ array2d_use(buffer[idx][idx]);
+}
+
+void array2d_typedef(unsigned idx) {
+ typedef int ten_ints_t[10];
+ ten_ints_t buffer[10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ buffer[idx][idx] = 0;
+}
+
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"
>From d49d5fd7e80463223aee9eac4ebd9e3c934cf9bd Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 7 Feb 2024 17:22:29 -0800
Subject: [PATCH 15/22] [-Wunsafe-buffer-usage] Avoid type sugar in array
fixits
The code that creates fixits from C array to std::array assumes the array type is
not a sugared type like typedef as it would not produce correct fixits otherwise.
The patch adds a note about the condition under which the code works and testcases.
As a general precaution it also makes the format assumption explicit in iteration
over tokens of the declaration.
Contrary to that the higher layers need to use canonical (desugared) type in order
to decide which function is responsible for fixing an array VarDecl.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 10 +++++---
...fe-buffer-usage-fixits-local-var-array.cpp | 23 +++++++++++++++++++
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index bd7183d8be068c..df9a87d5f3099b 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2475,6 +2475,8 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler) {
FixItList FixIts{};
+ // Note: the code below expects the declaration to not use any type sugar like
+ // typedef.
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!");
@@ -2496,7 +2498,8 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
// Find the '[' token.
std::optional<Token> NextTok = Lexer::findNextToken(
IdentifierLoc, Ctx.getSourceManager(), Ctx.getLangOpts());
- while (NextTok && !NextTok->is(tok::l_square))
+ while (NextTok && !NextTok->is(tok::l_square) &&
+ NextTok->getLocation() <= D->getSourceRange().getEnd())
NextTok = Lexer::findNextToken(NextTok->getLocation(),
Ctx.getSourceManager(), Ctx.getLangOpts());
if (!NextTok)
@@ -2607,7 +2610,8 @@ fixVariable(const VarDecl *VD, FixitStrategy::Kind K,
return {};
}
case FixitStrategy::Kind::Array: {
- if (VD->isLocalVarDecl() && isa<clang::ConstantArrayType>(VD->getType()))
+ if (VD->isLocalVarDecl() &&
+ isa<clang::ConstantArrayType>(VD->getType().getCanonicalType()))
return fixVariableWithArray(VD, Tracker, Ctx, Handler);
DEBUG_NOTE_DECL_FAIL(VD, " : not a local const-size array");
@@ -2801,7 +2805,7 @@ static FixitStrategy
getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
FixitStrategy S;
for (const VarDecl *VD : UnsafeVars) {
- if (isa<ConstantArrayType>(VD->getType()))
+ if (isa<ConstantArrayType>(VD->getType().getCanonicalType()))
S.set(VD, FixitStrategy::Kind::Array);
else
S.set(VD, FixitStrategy::Kind::Span);
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 a3ba46f27a0dcd..75fd0e7de82d7a 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
@@ -136,6 +136,13 @@ void typedef_as_elem_type(unsigned idx) {
buffer[idx] = 0;
}
+void decltype_as_elem_type(unsigned idx) {
+ int a;
+ decltype(a) buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array<decltype(a), 10> buffer"
+ buffer[idx] = 0;
+}
+
void macro_as_elem_type(unsigned idx) {
#define MY_INT int
MY_INT buffer[10];
@@ -162,6 +169,22 @@ void macro_as_size(unsigned idx) {
#undef MY_TEN
}
+typedef unsigned int my_array[42];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+void typedef_as_array_type(unsigned idx) {
+ my_array buffer;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+ buffer[idx] = 0;
+}
+
+void decltype_as_array_type(unsigned idx) {
+ int buffer[42];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+ decltype(buffer) buffer2;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+ buffer2[idx] = 0;
+}
+
void constant_as_size(unsigned idx) {
const unsigned my_const = 10;
int buffer[my_const];
>From 17f7b9e3a0a92bc9bfcbb8836c0a0fae7a268b85 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 7 Feb 2024 17:50:42 -0800
Subject: [PATCH 16/22] [-Wunsafe-buffer-usage] Avoid multi-dimentional arrays
properly
Array element can be for example VLA which given its non-constant length can't be transformed to std::array.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 2 +-
...fe-buffer-usage-fixits-local-var-array.cpp | 19 +++++++++++++++++--
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index df9a87d5f3099b..be6f77c9ddfdba 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2481,7 +2481,7 @@ 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!");
// FIXME: support multi-dimensional arrays
- if (isa<clang::ConstantArrayType>(ArrayEltT))
+ if (isa<clang::ArrayType>(ArrayEltT.getCanonicalType()))
return {};
const SourceLocation IdentifierLoc = getVarDeclIdentifierLoc(D);
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 75fd0e7de82d7a..3adfc324dfbe30 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
@@ -16,21 +16,36 @@ void array2d(unsigned idx) {
buffer[idx][idx] = 0;
}
+void array2d_vla(unsigned sz, unsigned idx) {
+ int buffer1[10][sz];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ int buffer2[sz][10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ buffer1[idx][idx] = 0;
+ buffer2[idx][idx] = 0;
+}
+
void array2d_assign_from_elem(unsigned idx) {
int buffer[10][10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
int a = buffer[idx][idx];
}
-void array2d_use(unsigned);
+void array2d_use(int *);
void array2d_call(unsigned idx) {
int buffer[10][10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
- array2d_use(buffer[idx][idx]);
+ array2d_use(buffer[idx]);
+}
+void array2d_call_vla(unsigned sz, unsigned idx) {
+ int buffer[10][sz];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ array2d_use(buffer[idx]);
}
void array2d_typedef(unsigned idx) {
typedef int ten_ints_t[10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
ten_ints_t buffer[10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
buffer[idx][idx] = 0;
>From ad31dab794808ee852fee3a3a9ad37040c8d0707 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 24 Jan 2024 17:35:47 -0800
Subject: [PATCH 17/22] [-Wunsafe-buffer-usage] Emit fixits for array used as a
pointer
Covers cases where DeclRefExpr referring to a const-size array decays to a
pointer and is used "as a pointer" (e. g. passed to a pointer type
parameter).
Since std::array<T, N> doesn't implicitly convert to pointer to its element
type T* the cast needs to be done explicitly as part of the fixit
when we retrofit std::array to code that previously worked with constant
size array. std::array::data() method is used for the explicit
cast.
In terms of the fixit machine this covers the UPC(DRE) case for Array fixit strategy.
The emitted fixit inserts call to std::array::data() method similarly to
analogous fixit for Span strategy.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 2 +-
...afe-buffer-usage-fixits-pointer-access.cpp | 29 +++++++++++++++++++
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index be6f77c9ddfdba..bc9d3bae1214a7 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1876,6 +1876,7 @@ std::optional<FixItList>
UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
const auto VD = cast<VarDecl>(Node->getDecl());
switch (S.lookup(VD)) {
+ case FixitStrategy::Kind::Array:
case FixitStrategy::Kind::Span: {
ASTContext &Ctx = VD->getASTContext();
SourceManager &SM = Ctx.getSourceManager();
@@ -1890,7 +1891,6 @@ 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");
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index ca19702c7ec300..f8caff5b1a3ef7 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -83,12 +83,27 @@ void unsafe_method_invocation_single_param() {
}
+void unsafe_method_invocation_single_param_array() {
+ int p[32];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 32> p"
+
+ int tmp = p[5];
+ foo(p);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()"
+}
+
void safe_method_invocation_single_param() {
int* p = new int[10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}-[[@LINE-1]]:{{.*}}}
foo(p);
}
+void safe_method_invocation_single_param_array() {
+ int p[10];
+ foo(p);
+ // CHECK-NO: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}-[[@LINE-1]]:{{.*}}}:".data()"
+}
+
void unsafe_method_invocation_double_param() {
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
@@ -111,6 +126,20 @@ void unsafe_method_invocation_double_param() {
m1(q, q, 8);
}
+void unsafe_method_invocation_double_param_array() {
+ int p[14];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 14> p"
+
+ int q[40];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 40> q"
+
+ q[5] = p[5];
+
+ m1(p, p, 10);
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:".data()"
+}
+
void unsafe_access_in_lamda() {
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
>From 2d0f4cbaafe37ceb1cdbf6b18bfe31cbdc0cd81c Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Tue, 6 Feb 2024 14:51:17 -0800
Subject: [PATCH 18/22] [-Wunsafe-buffer-usage][NFC] Test more fixits of array
decayed to pointer
---
...afe-buffer-usage-fixits-pointer-access.cpp | 21 ++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index f8caff5b1a3ef7..f94072015ff87d 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -206,4 +206,23 @@ void fixits_in_lambda_capture_rename() {
};
p[5] = 10;
-}
+}
+
+bool ptr_comparison(int* ptr, unsigned idx) {
+ int arr[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::array<int, 10> arr"
+ arr[idx] = idx;
+
+ return arr > ptr;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:".data()"
+}
+
+int long long ptr_distance(int* ptr, unsigned idx) {
+ int arr[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::array<int, 10> arr"
+ arr[idx] = idx;
+
+ int long long dist = arr - ptr;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:27-[[@LINE-1]]:27}:".data()"
+ return dist;
+}
>From 46166ba9aaaf69a1a01fea824a2a12dcb8dcc4f9 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 8 Feb 2024 14:30:20 -0800
Subject: [PATCH 19/22] [-Wunsafe-buffer-usage][NFC] Factor out .data() fixit
to a function
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index bc9d3bae1214a7..1dcc8a1b157f7a 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1453,6 +1453,9 @@ PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
return std::nullopt;
}
+/// \returns fixit that adds .data() call after \DRE.
+static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE);
+
std::optional<FixItList>
PointerInitGadget::getFixits(const FixitStrategy &S) const {
const auto *LeftVD = PtrInitLHS;
@@ -1870,6 +1873,18 @@ PointerDereferenceGadget::getFixits(const FixitStrategy &S) const {
return std::nullopt;
}
+static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE) {
+ const SourceManager &SM = Ctx.getSourceManager();
+ // Inserts the .data() after the DRE
+ std::optional<SourceLocation> EndOfOperand =
+ getPastLoc(DRE, SM, Ctx.getLangOpts());
+
+ if (EndOfOperand)
+ return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}};
+
+ return std::nullopt;
+}
+
// Generates fix-its replacing an expression of the form UPC(DRE) with
// `DRE.data()`
std::optional<FixItList>
@@ -1878,14 +1893,7 @@ UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
switch (S.lookup(VD)) {
case FixitStrategy::Kind::Array:
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()")}};
+ return createDataFixit(VD->getASTContext(), Node);
// FIXME: Points inside a macro expansion.
break;
}
>From 0b6dcc73c60d4ed41ddfb3a2a04e36bd1caf1b66 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 8 Feb 2024 14:33:03 -0800
Subject: [PATCH 20/22] [-Wunsafe-buffer-usage][NFC] Rename PointerAssignment
gadget to PtrToPtrAssignment
---
.../Analysis/Analyses/UnsafeBufferUsageGadgets.def | 2 +-
clang/lib/Analysis/UnsafeBufferUsage.cpp | 11 ++++++-----
clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp | 2 +-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 07f805ebb11013..2babc1df93d515 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -45,7 +45,7 @@ FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Poin
FIXABLE_GADGET(UPCStandalonePointer)
FIXABLE_GADGET(UPCPreIncrement) // '++Ptr' in an Unspecified Pointer Context
FIXABLE_GADGET(UUCAddAssign) // 'Ptr += n' in an Unspecified Untyped Context
-FIXABLE_GADGET(PointerAssignment)
+FIXABLE_GADGET(PtrToPtrAssignment)
FIXABLE_GADGET(PointerInit)
#undef FIXABLE_GADGET
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 1dcc8a1b157f7a..a33595500c3d26 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -762,7 +762,8 @@ class PointerInitGadget : public FixableGadget {
/// \code
/// p = q;
/// \endcode
-class PointerAssignmentGadget : public FixableGadget {
+/// where both `p` and `q` are pointers.
+class PtrToPtrAssignmentGadget : public FixableGadget {
private:
static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
@@ -770,13 +771,13 @@ class PointerAssignmentGadget : public FixableGadget {
const DeclRefExpr * PtrRHS; // the RHS pointer expression in `PA`
public:
- PointerAssignmentGadget(const MatchFinder::MatchResult &Result)
- : FixableGadget(Kind::PointerAssignment),
+ PtrToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
+ : FixableGadget(Kind::PtrToPtrAssignment),
PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
static bool classof(const Gadget *G) {
- return G->getKind() == Kind::PointerAssignment;
+ return G->getKind() == Kind::PtrToPtrAssignment;
}
static Matcher matcher() {
@@ -1434,7 +1435,7 @@ bool clang::internal::anyConflict(const SmallVectorImpl<FixItHint> &FixIts,
}
std::optional<FixItList>
-PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
+PtrToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
switch (S.lookup(LeftVD)) {
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
index 3a360566d1f3a5..a25a0b50a61f1b 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -53,7 +53,7 @@ void unclaimed_use() {
void implied_unclaimed_var(int *b) { // expected-warning{{'b' is an unsafe pointer used for buffer access}}
int *a = new int[3]; // expected-warning{{'a' is an unsafe pointer used for buffer access}}
a[4] = 7; // expected-note{{used in buffer access here}}
- a = b; // debug-note{{safe buffers debug: gadget 'PointerAssignment' refused to produce a fix}}
+ a = b; // debug-note{{safe buffers debug: gadget 'PtrToPtrAssignment' refused to produce a fix}}
b++; // expected-note{{used in pointer arithmetic here}} \
// debug-note{{safe buffers debug: failed to produce fixit for 'b' : has an unclaimed use}}
}
>From 25a0f30c23bba935ebe6a5f63a4e00230d035637 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 8 Feb 2024 14:47:13 -0800
Subject: [PATCH 21/22] [-Wunsafe-buffer-usage] Implement fixits for const size
array assigned to pointer
Introducing a new FixableGadget. It needs to be distinguished from PtrToPtr because their strategy implications are different.
Key observations:
- const size array can be assigned to std::span and bounds are propagated
- std::array can be assigned to std::span and bounds are propagated
Because const size arrays (both C and std) propagate bounds their assignment has no implications for transformation strategy.
---
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 88 +++++++++++++++++++
...uffer-usage-fixits-array-assign-to-ptr.cpp | 43 +++++++++
3 files changed, 132 insertions(+)
create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 2babc1df93d515..3273c642eed517 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -46,6 +46,7 @@ FIXABLE_GADGET(UPCStandalonePointer)
FIXABLE_GADGET(UPCPreIncrement) // '++Ptr' in an Unspecified Pointer Context
FIXABLE_GADGET(UUCAddAssign) // 'Ptr += n' in an Unspecified Untyped Context
FIXABLE_GADGET(PtrToPtrAssignment)
+FIXABLE_GADGET(CArrayToPtrAssignment)
FIXABLE_GADGET(PointerInit)
#undef FIXABLE_GADGET
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index a33595500c3d26..7b1ff896286e01 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -7,11 +7,14 @@
//===----------------------------------------------------------------------===//
#include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
+#include "clang/AST/ASTContext.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
#include "clang/AST/StmtVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Basic/CharInfo.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Lex/Lexer.h"
@@ -812,6 +815,59 @@ class PtrToPtrAssignmentGadget : public FixableGadget {
}
};
+/// An assignment expression of the form:
+/// \code
+/// ptr = array;
+/// \endcode
+/// where `p` is a pointer and `array` is a constant size array.
+class CArrayToPtrAssignmentGadget : public FixableGadget {
+private:
+ static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
+ static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
+ const DeclRefExpr * PtrLHS; // the LHS pointer expression in `PA`
+ const DeclRefExpr * PtrRHS; // the RHS pointer expression in `PA`
+
+public:
+ CArrayToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
+ : FixableGadget(Kind::CArrayToPtrAssignment),
+ PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
+ PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
+
+ static bool classof(const Gadget *G) {
+ return G->getKind() == Kind::CArrayToPtrAssignment;
+ }
+
+ static Matcher matcher() {
+ auto PtrAssignExpr = binaryOperator(allOf(hasOperatorName("="),
+ hasRHS(ignoringParenImpCasts(declRefExpr(hasType(hasCanonicalType(constantArrayType())),
+ toSupportedVariable()).
+ bind(PointerAssignRHSTag))),
+ hasLHS(declRefExpr(hasPointerType(),
+ toSupportedVariable()).
+ bind(PointerAssignLHSTag))));
+
+ return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
+ }
+
+ 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
+ // makes sense at all on a FixableGadget.
+ return PtrLHS;
+ }
+
+ virtual DeclUseList getClaimedVarUseSites() const override {
+ return DeclUseList{PtrLHS, PtrRHS};
+ }
+
+ virtual std::optional<std::pair<const VarDecl *, const VarDecl *>>
+ getStrategyImplications() const override {
+ return {};
+ }
+};
+
/// A call of a function or method that performs unchecked buffer operations
/// over one of its pointer parameters.
class UnsafeBufferUsageAttrGadget : public WarningGadget {
@@ -1457,6 +1513,38 @@ PtrToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
/// \returns fixit that adds .data() call after \DRE.
static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE);
+std::optional<FixItList>
+CArrayToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
+ const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
+ const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
+ switch (S.lookup(LeftVD)) {
+ case FixitStrategy::Kind::Span: {
+ switch (S.lookup(RightVD)) {
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Vector:
+ case FixitStrategy::Kind::Wontfix:
+ return FixItList{};
+ default: break;
+ }
+ break;
+ }
+ case FixitStrategy::Kind::Wontfix: {
+ switch (S.lookup(RightVD)) {
+ case FixitStrategy::Kind::Wontfix:
+ return FixItList{}; // tautological
+ case FixitStrategy::Kind::Array:
+ case FixitStrategy::Kind::Span:
+ return createDataFixit(LeftVD->getASTContext(), PtrRHS);
+ // FIXME: Points inside a macro expansion.
+ default: break;
+ }
+ break;
+ }
+ default: break;
+ }
+ return std::nullopt;
+}
+
std::optional<FixItList>
PointerInitGadget::getFixits(const FixitStrategy &S) const {
const auto *LeftVD = PtrInitLHS;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
new file mode 100644
index 00000000000000..93715952d516c0
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void safe_array_assigned_to_safe_ptr(unsigned idx) {
+ int buffer[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ int* ptr;
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ ptr = buffer;
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+}
+
+void safe_array_assigned_to_unsafe_ptr(unsigned idx) {
+ int buffer[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ int* ptr;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+ ptr = buffer;
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ ptr[idx] = 0;
+}
+
+void unsafe_array_assigned_to_safe_ptr(unsigned idx) {
+ int buffer[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+ int* ptr;
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ ptr = buffer;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:15-[[@LINE-1]]:15}:".data()"
+ buffer[idx] = 0;
+}
+
+void unsafe_array_assigned_to_unsafe_ptr(unsigned idx) {
+ int buffer[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+ int* ptr;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+ ptr = buffer;
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}
+ buffer[idx] = 0;
+ ptr[idx] = 0;
+}
>From b7471ab0ec3e7d20e971ed61dea727b55c944d5a Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 8 Feb 2024 15:52:08 -0800
Subject: [PATCH 22/22] [-Wunsafe-buffer-usage] Fix fixits for std::span
initialization from const size array
Example:
int arr[10];
int * ptr = arr;
If ptr is unsafe and we transform it to std::span then the fixit we'd currently provide transforms the code to:
std::span<int> ptr{arr, 10};
However, the idiomatic transformation should rely on one of many span constructors can get just the array and let template parameter autodeduction correctly handle the span size.
The transformed code should look like:
std::span<int> ptr = arr;
This patch changes the initializer handling to the desired (empty) fixit.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 28 ++++++++++---------
...fe-buffer-usage-fixits-array-inits-ptr.cpp | 19 +++++++++++++
...afe-buffer-usage-fixits-local-var-span.cpp | 8 ------
3 files changed, 34 insertions(+), 21 deletions(-)
create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 7b1ff896286e01..32fae495774df0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2112,13 +2112,15 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const {
// In many cases, this function cannot figure out the actual extent `S`. It
// then will use a place holder to replace `S` to ask users to fill `S` in. The
// initializer shall be used to initialize a variable of type `std::span<T>`.
+// In some cases (e. g. constant size array) the initializer should remain unchanged and the function returns empty list.
+// In case the function can't provide the right fixit it will return nullopt.
//
// FIXME: Support multi-level pointers
//
// Parameters:
// `Init` a pointer to the initializer expression
// `Ctx` a reference to the ASTContext
-static FixItList
+static std::optional<FixItList>
FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
const StringRef UserFillPlaceHolder) {
const SourceManager &SM = Ctx.getSourceManager();
@@ -2136,11 +2138,11 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
std::optional<SourceLocation> InitLocation =
getEndCharLoc(Init, SM, LangOpts);
if (!InitLocation)
- return {};
+ return std::nullopt;
SourceRange SR(Init->getBeginLoc(), *InitLocation);
- return {FixItHint::CreateRemoval(SR)};
+ return FixItList{FixItHint::CreateRemoval(SR)};
}
FixItList FixIts{};
@@ -2159,7 +2161,7 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
if (!Ext->HasSideEffects(Ctx)) {
std::optional<StringRef> ExtentString = getExprText(Ext, SM, LangOpts);
if (!ExtentString)
- return {};
+ return std::nullopt;
ExtentText = *ExtentString;
}
} else if (!CxxNew->isArray())
@@ -2168,10 +2170,10 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
ExtentText = One;
} else if (const auto *CArrTy = Ctx.getAsConstantArrayType(
Init->IgnoreImpCasts()->getType())) {
- // In cases `Init` is of an array type after stripping off implicit casts,
- // the extent is the array size. Note that if the array size is not a
- // constant, we cannot use it as the extent.
- ExtentText = getAPIntText(CArrTy->getSize());
+ // std::span has a single parameter constructor for initialization with constant size array.
+ // The size is auto-deduced as the constructor is a function template.
+ // The correct fixit is empty - no changes should happen.
+ return FixItList{};
} else {
// In cases `Init` is of the form `&Var` after stripping of implicit
// casts, where `&` is the built-in operator, the extent is 1.
@@ -2187,7 +2189,7 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
std::optional<SourceLocation> LocPassInit = getPastLoc(Init, SM, LangOpts);
if (!LocPassInit)
- return {};
+ return std::nullopt;
StrBuffer.append(", ");
StrBuffer.append(ExtentText);
@@ -2277,12 +2279,12 @@ static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
}
// Fix the initializer if it exists:
if (const Expr *Init = D->getInit()) {
- FixItList InitFixIts =
+ std::optional<FixItList> InitFixIts =
FixVarInitializerWithSpan(Init, Ctx, UserFillPlaceHolder);
- if (InitFixIts.empty())
+ if (!InitFixIts)
return {};
- FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
- std::make_move_iterator(InitFixIts.end()));
+ FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts->begin()),
+ std::make_move_iterator(InitFixIts->end()));
// If the declaration has the form `T *ident = init`, we want to replace
// `T *ident = ` with `std::span<T> ident`:
EndLocForReplacement = Init->getBeginLoc().getLocWithOffset(-1);
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp
new file mode 100644
index 00000000000000..2044056cf6eea4
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void safe_array_initing_safe_ptr(unsigned idx) {
+ int buffer[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ int* ptr = buffer;
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+}
+
+void safe_array_initing_unsafe_ptr(unsigned idx) {
+ int buffer[123321123];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ int* ptr = buffer;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:13}:"std::span<int> ptr"
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}123321123
+ ptr[idx + 1] = 0;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
index 114ceaad56e451..ca5dab15a0dff4 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -52,12 +52,8 @@ void local_variable_qualifiers_specifiers() {
int a[10];
const int * p = a;
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:18}:"std::span<int const> p"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:19}:"{"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", 10}"
const int * const q = a;
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:24}:"std::span<int const> const q"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:"{"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
int tmp;
tmp = p[5];
tmp = q[5];
@@ -88,12 +84,8 @@ void local_ptr_to_array() {
int b[n]; // If the extent expression does not have a constant value, we cannot fill the extent for users...
int *p = a;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", 10}"
int *q = b;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
// No way to know if `n` is ever mutated since `int b[n];`, so no way to figure out the extent
tmp = p[5];
tmp = q[5];
More information about the cfe-commits
mailing list