[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 30 16:27:01 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: None (jkorous-apple)
<details>
<summary>Changes</summary>
---
Patch is 39.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80084.diff
7 Files Affected:
- (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h (+44-3)
- (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+218-138)
- (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+14-2)
- (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp (+24)
- (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp (+5-7)
- (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp (+167)
- (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp (+5)
``````````diff
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index aca1ad998822c..2c9ebf3fb42d0 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 {
@@ -58,6 +95,8 @@ class UnsafeBufferUsageHandler {
#endif
public:
+ enum class TargetType { Span, Array };
+
UnsafeBufferUsageHandler() = default;
virtual ~UnsafeBufferUsageHandler() = default;
@@ -75,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 823cd2a7b9969..18d0ca7a17d3c 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -12,10 +12,13 @@
#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"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
#include <memory>
#include <optional>
#include <queue>
@@ -407,9 +410,6 @@ using DeclUseList = SmallVector<const DeclRefExpr *, 1>;
// Convenience typedef.
using FixItList = SmallVector<FixItHint, 4>;
-
-// Defined below.
-class Strategy;
} // namespace
namespace {
@@ -486,7 +486,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 +737,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 +790,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 +894,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 +935,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 +980,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 +1014,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 +1094,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 +1125,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 +1171,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 +1222,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 +1433,40 @@ 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:
+ return std::nullopt;
+ 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:
+ return std::nullopt;
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
return std::nullopt;
@@ -1512,12 +1483,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 +1499,11 @@ 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:
+ return FixItList{};
+ case FixitStrategy::Kind::Wontfix:
+ case FixitStrategy::Kind::Iterator:
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
}
@@ -1542,17 +1514,18 @@ 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:
+ return std::nullopt;
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
return std::nullopt; // something went wrong, no fix-it
@@ -1803,10 +1776,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 +1839,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 +1857,12 @@ 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:
+ return std::nullopt;
+ case FixitStrategy::Kind::Vector:
+ llvm_unreachable("FixitStrategy not implemented yet!");
+ case FixitStrategy::Kind::Wontfix:
llvm_unreachable("Invalid strategy!");
}
@@ -1897,27 +1871,27 @@ 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:
+ return std::nullopt;
+ case FixitStrategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
@@ -1962,14 +1936,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 +1977,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 +2236,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 +2253,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 +2266,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;
@@ -2495,10 +2471,104 @@ 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-...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/80084
More information about the cfe-commits
mailing list