[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
https://github.com/jkorous-apple closed https://github.com/llvm/llvm-project/pull/80084 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -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 harden it}}
jkorous-apple wrote:
Went with "to label it for hardening".
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/17] [-Wunsafe-buffer-usage] Move Strategy class to the
header
It needs to be used from handleUnsafeVariableGroup function.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 37 +++
clang/lib/Analysis/UnsafeBufferUsage.cpp | 238 --
2 files changed, 140 insertions(+), 135 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index aca1ad998822c5..622c094f5b1eb1 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -42,6 +42,43 @@ class VariableGroupsManager {
virtual VarGrpRef getGroupOfParms() const =0;
};
+// FixitStrategy is a map from variables to the way we plan to emit fixes for
+// these variables. It is figured out gradually by trying different fixes
+// for different variables depending on gadgets in which these variables
+// participate.
+class FixitStrategy {
+public:
+ enum class Kind {
+Wontfix, // We don't plan to emit a fixit for this variable.
+Span, // We recommend replacing the variable with std::span.
+Iterator, // We recommend replacing the variable with std::span::iterator.
+Array,// We recommend replacing the variable with std::array.
+Vector// We recommend replacing the variable with std::vector.
+ };
+
+private:
+ using MapTy = llvm::DenseMap;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(FixitStrategy &&) = default;
+
+ void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
+
+ Kind lookup(const VarDecl *VD) const {
+auto I = Map.find(VD);
+if (I == Map.end())
+ return Kind::Wontfix;
+
+return I->second;
+ }
+};
+
/// The interface that lets the caller handle unsafe buffer usage analysis
/// results by overriding this class's handle... methods.
class UnsafeBufferUsageHandler {
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 823cd2a7b99691..924d677786275d 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -407,9 +407,6 @@ using DeclUseList = SmallVector;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
https://github.com/jkorous-apple edited https://github.com/llvm/llvm-project/pull/80084 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
jkorous-apple wrote:
Sorry for the confusion but I actually changed the conditions in the callers in
the commit that addresses the type sugar.
https://github.com/llvm/llvm-project/pull/80084/commits/d49d5fd7e80463223aee9eac4ebd9e3c934cf9bd
My reasoning is that the layers above just check what kind of variable they are
dealing with (e. g. pointer vs array) for which they need canonical type.
These rely on more specific functions to produce the fixits in a given context.
Ultimately I see it as responsibility of `fixVarDeclWithArray` to handle any
local variable of an array type and it shouldn't expect only canonical arrays.
Maybe there will be some cases of type sugar for which it'll provide fixits in
the future.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
https://github.com/jkorous-apple edited https://github.com/llvm/llvm-project/pull/80084 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
+ 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 {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
+
+// Find the '[' token.
+std::optional 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)
+ 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 llvm::StringRef ArraySizeTxt = MaybeArraySizeTxt->trim();
+if (ArraySizeTxt.empty()) {
+ // FIXME: Support array size getting determined from the initializer.
jkorous-apple wrote:
It is technically doable and the code would compile but if we want to preserve
the "spirit of the code" then we can do a better job:
```
int arr1[] = {0, 1, 2};
// = fixit ===>
auto arr1 = std::to_array({0, 1, 2});
```
The author of the original probably had a reason why they didn't hardcode the
size in the first place.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/16] [-Wunsafe-buffer-usage] Move Strategy class to the
header
It needs to be used from handleUnsafeVariableGroup function.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 37 +++
clang/lib/Analysis/UnsafeBufferUsage.cpp | 238 --
2 files changed, 140 insertions(+), 135 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index aca1ad998822c5..622c094f5b1eb1 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -42,6 +42,43 @@ class VariableGroupsManager {
virtual VarGrpRef getGroupOfParms() const =0;
};
+// FixitStrategy is a map from variables to the way we plan to emit fixes for
+// these variables. It is figured out gradually by trying different fixes
+// for different variables depending on gadgets in which these variables
+// participate.
+class FixitStrategy {
+public:
+ enum class Kind {
+Wontfix, // We don't plan to emit a fixit for this variable.
+Span, // We recommend replacing the variable with std::span.
+Iterator, // We recommend replacing the variable with std::span::iterator.
+Array,// We recommend replacing the variable with std::array.
+Vector// We recommend replacing the variable with std::vector.
+ };
+
+private:
+ using MapTy = llvm::DenseMap;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(FixitStrategy &&) = default;
+
+ void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
+
+ Kind lookup(const VarDecl *VD) const {
+auto I = Map.find(VD);
+if (I == Map.end())
+ return Kind::Wontfix;
+
+return I->second;
+ }
+};
+
/// The interface that lets the caller handle unsafe buffer usage analysis
/// results by overriding this class's handle... methods.
class UnsafeBufferUsageHandler {
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 823cd2a7b99691..924d677786275d 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -407,9 +407,6 @@ using DeclUseList = SmallVector;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
jkorous-apple wrote:
You're right, for example it's possible to mix CSA with C variable length
arrays.
https://clang.llvm.org/doxygen/classclang_1_1VariableArrayType.html
```
void foo(unsigned u) {
int foo[5][u];
}
```
Let me fix this and add a testcase.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
jkorous-apple wrote:
...and on top of that the `if` should check for canonical type to not be fooled
by typedefs & co.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
+ 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 {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
+
+// Find the '[' token.
+std::optional 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)
+ 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 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({0, 1, 2});
+ return {};
+}
+
+std::optional IdentText =
+getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
+
+if (!IdentText) {
+ DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
+ return {};
+}
+
+SmallString<32> Replacement;
+raw_svector_ostream OS(Replacement);
+OS << "std::array<" << ElemTypeTxt << ", " << ArraySizeTxt << "> "
+ << IdentText->str();
+
+FixIts.push_back(FixItHint::CreateReplacement(
+SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str()));
jkorous-apple wrote:
Excellent question!
It is indeed possible so I added a couple more testcases and verified that the
implementation doesn't produce nonsense when encounters type sugar.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
https://github.com/ziqingluo-90 approved this pull request. I left all my comments here. I didn't think quite through for some of them so feel free to ignore if they don't make sense to you. Other parts LGTM! https://github.com/llvm/llvm-project/pull/80084 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
+ 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 {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
+
+// Find the '[' token.
+std::optional 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)
+ 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 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({0, 1, 2});
+ return {};
+}
+
+std::optional IdentText =
+getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
+
+if (!IdentText) {
+ DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
+ return {};
+}
+
+SmallString<32> Replacement;
+raw_svector_ostream OS(Replacement);
+OS << "std::array<" << ElemTypeTxt << ", " << ArraySizeTxt << "> "
+ << IdentText->str();
+
+FixIts.push_back(FixItHint::CreateReplacement(
+SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str()));
ziqingluo-90 wrote:
and this maybe?
```
typedef int ArrayT[10];
ArrayT a;
```
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
ziqingluo-90 wrote:
nitpick: can we replace this check with an assertion? Because it is already
checked at the caller:
```
case FixitStrategy::Kind::Array: {
if (VD->isLocalVarDecl() && isa(VD->getType()))
return fixVariableWithArray(VD, Tracker, Ctx, Handler);
```
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
+ 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 {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
+
+// Find the '[' token.
+std::optional 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)
+ 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 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({0, 1, 2});
+ return {};
+}
+
+std::optional IdentText =
+getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
+
+if (!IdentText) {
+ DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
+ return {};
+}
+
+SmallString<32> Replacement;
+raw_svector_ostream OS(Replacement);
+OS << "std::array<" << ElemTypeTxt << ", " << ArraySizeTxt << "> "
+ << IdentText->str();
+
+FixIts.push_back(FixItHint::CreateReplacement(
+SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str()));
ziqingluo-90 wrote:
I'm wondering if it is possible to declare a constant array in a way that the
type specifier is completely at the left-hand side of the identifier? Like
the form `T a;` while `T` represents a constant array type.
How about this?
```
int a[10];
decltype(a) b;
```
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
+ 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 {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
+
+// Find the '[' token.
+std::optional 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)
+ 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 llvm::StringRef ArraySizeTxt = MaybeArraySizeTxt->trim();
+if (ArraySizeTxt.empty()) {
+ // FIXME: Support array size getting determined from the initializer.
ziqingluo-90 wrote:
In this case, maybe we can get the concrete size value from the array type
directly since there is no spelling text for us to preserve?
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2471,100 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+// FIXME: support multi-dimensional arrays
+if (isa(ArrayEltT))
ziqingluo-90 wrote:
is it possible that the element type is an array type but not a constant array
type?
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2470,97 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+
+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 {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
+
+// Find the '[' token.
+std::optional 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)
+ return {};
+const SourceLocation LSqBracketLoc = NextTok->getLocation();
+
+// Get the spelling of the array size as written in the source file
+// (including macros, etc.).
jkorous-apple wrote:
I agree. I expect we'll get a better way of doing this and more when we adopt
transformer library in the future. Getting the array size spelling might be
already implemented or at least I expect to find better means of doing that
than dealing with tokens.
For now I'm happy with a temporary solution while we bootstrap the rest of the
machine.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -0,0 +1,164 @@
+// 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 simple(unsigned idx) {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
buffer"
+ buffer[idx] = 0;
+}
+
+void whitespace_in_declaration(unsigned idx) {
+ int buffer_w [ 10 ];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:35}:"std::array
buffer_w"
+ buffer_w[idx] = 0;
+}
+
+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 buffer_w"
+ buffer_w[idx] = 0;
+}
+
+void initializer(unsigned idx) {
+ int buffer[3] = {0};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::array
buffer"
+
+ int buffer2[3] = {0, 1, 2};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
buffer2"
+
+ buffer[idx] = 0;
+ buffer2[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]]
+// FIXME: implement support
+
+ buffer[idx] = 0;
+}
+
+void multi_decl2(unsigned idx) {
+ int buffer[10], b;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// FIXME: implement support
+
+ 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 buffer"
+ a = buffer[idx];
+}
+
+void local_array_const_ptr(unsigned idx, int*& a) {
+ int * const buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array buffer"
+
+ 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:
fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:26}:"std::array
buffer"
+
+ a = buffer[idx];
+}
+
+void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
+ const int * const buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:31}:"std::array buffer"
+
+ a = buffer[idx];
+
+}
+
+template
+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 unsupported_local_array_in_template(unsigned);
+
+typedef unsigned int my_uint;
+void typedef_as_elem_type(unsigned idx) {
+ my_uint buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array 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
+}
+
+void macro_as_identifier(unsigned idx) {
+#define MY_BUFFER buffer
+ int MY_BUFFER[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:20}:"std::array
MY_BUFFER"
+ MY_BUFFER[idx] = 0;
+#undef MY_BUFFER
+}
+
+void macro_as_size(unsigned idx) {
+#define MY_TEN 10
+ int buffer[MY_TEN];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array buffer"
+ buffer[idx] = 0;
+#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 buffer"
+ buffer[idx] = 0;
+}
+
+void subscript_negative() {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
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
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 catches
these at runtime.
+ buffer[signed_idx] = 0;
+}
jkorous-apple wrote:
Added couple tests and an explicit check when we're creating fixit for unsafe
array declaration.
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/14] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/13] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/14] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -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 harden it}}
jkorous-apple wrote:
"so it can benefit from libc++ hardening"?
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -0,0 +1,164 @@
+// 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 simple(unsigned idx) {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
buffer"
+ buffer[idx] = 0;
+}
+
+void whitespace_in_declaration(unsigned idx) {
+ int buffer_w [ 10 ];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:35}:"std::array
buffer_w"
+ buffer_w[idx] = 0;
+}
+
+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 buffer_w"
+ buffer_w[idx] = 0;
+}
+
+void initializer(unsigned idx) {
+ int buffer[3] = {0};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::array
buffer"
+
+ int buffer2[3] = {0, 1, 2};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
buffer2"
+
+ buffer[idx] = 0;
+ buffer2[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]]
+// FIXME: implement support
+
+ buffer[idx] = 0;
+}
+
+void multi_decl2(unsigned idx) {
+ int buffer[10], b;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// FIXME: implement support
+
+ 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 buffer"
+ a = buffer[idx];
+}
+
+void local_array_const_ptr(unsigned idx, int*& a) {
+ int * const buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array buffer"
+
+ 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:
fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:26}:"std::array
buffer"
+
+ a = buffer[idx];
+}
+
+void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
+ const int * const buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:31}:"std::array buffer"
+
+ a = buffer[idx];
+
+}
+
+template
+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 unsupported_local_array_in_template(unsigned);
+
+typedef unsigned int my_uint;
+void typedef_as_elem_type(unsigned idx) {
+ my_uint buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array 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
+}
+
+void macro_as_identifier(unsigned idx) {
+#define MY_BUFFER buffer
+ int MY_BUFFER[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:20}:"std::array
MY_BUFFER"
+ MY_BUFFER[idx] = 0;
+#undef MY_BUFFER
+}
+
+void macro_as_size(unsigned idx) {
+#define MY_TEN 10
+ int buffer[MY_TEN];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array buffer"
+ buffer[idx] = 0;
+#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 buffer"
+ buffer[idx] = 0;
+}
+
+void subscript_negative() {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
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
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 catches
these at runtime.
+ buffer[signed_idx] = 0;
+}
jkorous-apple wrote:
Excellent point!
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -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 harden it}}
jkorous-apple wrote:
I guess what we really want to say is "change the type of 'foo' to std::array
so when you build the code with hardened libc++ all subscript operations on
'foo' will be automatically bounds-safe" but that is too long and too complex
for this particular communication channel.
How about:
"to enable hardening"
"to get it hardened"
"to designate it for hardening"
"to register it for hardening"
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2470,97 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = CAT->getElementType();
+assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+
+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 {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
+
+// Find the '[' token.
+std::optional 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)
+ return {};
+const SourceLocation LSqBracketLoc = NextTok->getLocation();
+
+// Get the spelling of the array size as written in the source file
+// (including macros, etc.).
ziqingluo-90 wrote:
I wish we could have a more elegant way to get the spelling of the array size.
I feel like `TypeLoc` is suppose to offer it but it does not behave as I expect
in this case. (`TypeLoc` also disappointed me in dealing with cv-qualifiers.
`¯\_(ツ)_/¯`)
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -0,0 +1,164 @@
+// 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 simple(unsigned idx) {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
buffer"
+ buffer[idx] = 0;
+}
+
+void whitespace_in_declaration(unsigned idx) {
+ int buffer_w [ 10 ];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:35}:"std::array
buffer_w"
+ buffer_w[idx] = 0;
+}
+
+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 buffer_w"
+ buffer_w[idx] = 0;
+}
+
+void initializer(unsigned idx) {
+ int buffer[3] = {0};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::array
buffer"
+
+ int buffer2[3] = {0, 1, 2};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
buffer2"
+
+ buffer[idx] = 0;
+ buffer2[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]]
+// FIXME: implement support
+
+ buffer[idx] = 0;
+}
+
+void multi_decl2(unsigned idx) {
+ int buffer[10], b;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+// FIXME: implement support
+
+ 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 buffer"
+ a = buffer[idx];
+}
+
+void local_array_const_ptr(unsigned idx, int*& a) {
+ int * const buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array buffer"
+
+ 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:
fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:26}:"std::array
buffer"
+
+ a = buffer[idx];
+}
+
+void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
+ const int * const buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:31}:"std::array buffer"
+
+ a = buffer[idx];
+
+}
+
+template
+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 unsupported_local_array_in_template(unsigned);
+
+typedef unsigned int my_uint;
+void typedef_as_elem_type(unsigned idx) {
+ my_uint buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array 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
+}
+
+void macro_as_identifier(unsigned idx) {
+#define MY_BUFFER buffer
+ int MY_BUFFER[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:20}:"std::array
MY_BUFFER"
+ MY_BUFFER[idx] = 0;
+#undef MY_BUFFER
+}
+
+void macro_as_size(unsigned idx) {
+#define MY_TEN 10
+ int buffer[MY_TEN];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array buffer"
+ buffer[idx] = 0;
+#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 buffer"
+ buffer[idx] = 0;
+}
+
+void subscript_negative() {
+ int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array
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
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 catches
these at runtime.
+ buffer[signed_idx] = 0;
+}
ziqingluo-90 wrote:
maybe we can have a test showing that multi-dimentional arrays will not be
fixed?
https://github.com/llvm/llvm-project/pull/80084
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -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 harden it}}
haoNoQ wrote:
I think it's important to make sure it's not actively misleading, even if we
plan to figure out the perfect phrasing later. Maybe `to incapsulate it for
hardening purposes`?
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2470,113 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = 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 foo;
+// Cv-specifiers are straigtforward:
+// const T foo[10]; => std::array foo;
+// Pointers are straightforward:
+// T * foo[10]; => std::array foo;
+//
+// However, for const pointers the transformation is different:
+// T * const foo[10]; => const std::array 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
+// (including macros, etc.).
+auto MaybeElemTypeTxt =
+getRangeText({D->getBeginLoc(), IdentifierLoc}, Ctx.getSourceManager(),
+ Ctx.getLangOpts());
+if (!MaybeElemTypeTxt)
+ return {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
jkorous-apple wrote:
For now I would leave it as is but if we later find that callers need to trim
so often it gets annoying then let's reconsider.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -58,6 +95,8 @@ class UnsafeBufferUsageHandler {
#endif
public:
+ enum class TargetType { Span, Array };
jkorous-apple wrote:
Totally. Removed. Thanks!
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/13] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/12] [-Wunsafe-buffer-usage] Move Strategy class to the
header
It needs to be used from handleUnsafeVariableGroup function.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 37 +++
clang/lib/Analysis/UnsafeBufferUsage.cpp | 238 --
2 files changed, 140 insertions(+), 135 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index aca1ad998822c..622c094f5b1eb 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -42,6 +42,43 @@ class VariableGroupsManager {
virtual VarGrpRef getGroupOfParms() const =0;
};
+// FixitStrategy is a map from variables to the way we plan to emit fixes for
+// these variables. It is figured out gradually by trying different fixes
+// for different variables depending on gadgets in which these variables
+// participate.
+class FixitStrategy {
+public:
+ enum class Kind {
+Wontfix, // We don't plan to emit a fixit for this variable.
+Span, // We recommend replacing the variable with std::span.
+Iterator, // We recommend replacing the variable with std::span::iterator.
+Array,// We recommend replacing the variable with std::array.
+Vector// We recommend replacing the variable with std::vector.
+ };
+
+private:
+ using MapTy = llvm::DenseMap;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -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 harden it}}
jkorous-apple wrote:
I am fine with broader discussion later but don't want to block the PR on it.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2470,113 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = 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 foo;
+// Cv-specifiers are straigtforward:
+// const T foo[10]; => std::array foo;
+// Pointers are straightforward:
+// T * foo[10]; => std::array foo;
+//
+// However, for const pointers the transformation is different:
+// T * const foo[10]; => const std::array foo;
jkorous-apple wrote:
You're right, let me simplify this.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2470,113 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = 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 foo;
+// Cv-specifiers are straigtforward:
+// const T foo[10]; => std::array foo;
+// Pointers are straightforward:
+// T * foo[10]; => std::array foo;
+//
+// However, for const pointers the transformation is different:
+// T * const foo[10]; => const std::array foo;
haoNoQ wrote:
Hmm, wouldn't `std::array` have the exact same behavior in
practice? (Though I completely agree that `const std::array` is more
readable.) (Testbed: https://godbolt.org/z/aeohe6jW9)
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -2495,10 +2470,113 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext ,
+ UnsafeBufferUsageHandler ) {
+ FixItList FixIts{};
+
+ if (auto CAT = dyn_cast(D->getType())) {
+const QualType = 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 foo;
+// Cv-specifiers are straigtforward:
+// const T foo[10]; => std::array foo;
+// Pointers are straightforward:
+// T * foo[10]; => std::array foo;
+//
+// However, for const pointers the transformation is different:
+// T * const foo[10]; => const std::array 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
+// (including macros, etc.).
+auto MaybeElemTypeTxt =
+getRangeText({D->getBeginLoc(), IdentifierLoc}, Ctx.getSourceManager(),
+ Ctx.getLangOpts());
+if (!MaybeElemTypeTxt)
+ return {};
+const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim();
haoNoQ wrote:
Should `getRangeText()` trim automatically?
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -58,6 +95,8 @@ class UnsafeBufferUsageHandler {
#endif
public:
+ enum class TargetType { Span, Array };
haoNoQ wrote:
Looks like a bit of dead code?
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
@@ -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 harden it}}
haoNoQ wrote:
We'll probably have a broader discussion about wording but before I forget: "to
harden it" may be misleading because we don't know/check whether library
hardening is actually turned on.
https://github.com/llvm/llvm-project/pull/80084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/11] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/11] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
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
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 01/10] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 86cd2fbdfe67d70a7fe061ed5d3a644f50f070f5
fd791ccdbf919c4810a92e700665a4766ef402a7 --
clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 18d0ca7a17..30723b97f6 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1886,13 +1886,13 @@ UPCStandalonePointerGadget::getFixits(const
FixitStrategy ) 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 )
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 ,
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 NextTok = Lexer::findNextToken(IdentifierLoc,
Ctx.getSourceManager(), Ctx.getLangOpts());
+std::optional 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 ,
// 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({0, 1, 2});
return {};
}
@@ -2536,8 +2544,8 @@ static FixItList fixVarDeclWithArray(const VarDecl *D,
const ASTContext ,
SmallString<32> Replacement;
raw_svector_ostream OS(Replacement);
-OS << "std::array<" << ElemTypeTxt << ", "
- << ArraySizeTxt << "> " << IdentText->str();
+OS << "std::array<" << ElemTypeTxt << ", " << ArraySizeTxt << "> "
+
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
llvmbot wrote:
@llvm/pr-subscribers-clang-analysis
Author: None (jkorous-apple)
Changes
---
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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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
,
- FixItList &, const Decl *D) = 0;
+ virtual void
+ handleUnsafeVariableGroup(const VarDecl *Variable,
+const VariableGroupsManager ,
+FixItList &, const Decl *D,
+const FixitStrategy ) = 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
#include
#include
@@ -407,9 +410,6 @@ using DeclUseList = SmallVector;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +737,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
virtual const Stmt *getBaseStmt() const override {
//
[clang] [-Wunsafe-buffer-usage] Introduce std::array fixits (PR #80084)
https://github.com/jkorous-apple created
https://github.com/llvm/llvm-project/pull/80084
None
>From 463a9904c1ae85fbdc0bd6029c6effea3fb16ea6 Mon Sep 17 00:00:00 2001
From: Jan Korous
Date: Tue, 23 Jan 2024 16:16:10 -0800
Subject: [PATCH 1/9] [-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;
+
+ MapTy Map;
+
+public:
+ FixitStrategy() = default;
+ FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies.
+ FixitStrategy =(const FixitStrategy &) = delete;
+ FixitStrategy(FixitStrategy &&) = default;
+ FixitStrategy =(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;
// Convenience typedef.
using FixItList = SmallVector;
-
-// 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 getFixits(const Strategy &) const {
+ virtual std::optional getFixits(const FixitStrategy &) const {
return std::nullopt;
}
@@ -737,7 +734,8 @@ class PointerInitGadget : public FixableGadget {
return stmt(PtrInitStmt);
}
- virtual std::optional getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) 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 getFixits(const Strategy ) const override;
+ virtual std::optional
+ getFixits(const FixitStrategy ) const override;
};
//
