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