[clang] 6341301 - [-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers
via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 10 12:09:18 PDT 2023
Author: ziqingluo-90
Date: 2023-04-10T12:08:30-07:00
New Revision: 63413015099bc8aaa45cba7551e8fe432b2b795f
URL: https://github.com/llvm/llvm-project/commit/63413015099bc8aaa45cba7551e8fe432b2b795f
DIFF: https://github.com/llvm/llvm-project/commit/63413015099bc8aaa45cba7551e8fe432b2b795f.diff
LOG: [-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers
For a local pointer declaration of the form `T * p = 0` or `T * p = std::nullptr`,
Before this patch, we generate fix-its that convert the declaration to
`std::span<T> p{nullptr, <# placeholder #>}`, in cases where `p` is
used in some unsafe operations. This patch improves the fix-its to
result in a simpler form `std::span<T> p`. It gets rid of the
placeholder and keeps the result concise.
Reviewed by: NoQ (Artem Dergachev)
Differential Revision: https://reviews.llvm.org/D143680
Added:
Modified:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 9fb928f6c4c68..fdc7584ad6ff1 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1007,14 +1007,30 @@ static std::string getAPIntText(APInt Val) {
template <typename NodeTy>
static SourceLocation getEndCharLoc(const NodeTy *Node, const SourceManager &SM,
const LangOptions &LangOpts) {
- return Lexer::getLocForEndOfToken(Node->getEndLoc(), 1, SM, LangOpts);
+ unsigned TkLen = Lexer::MeasureTokenLength(Node->getEndLoc(), SM, LangOpts);
+ SourceLocation Loc = Node->getEndLoc().getLocWithOffset(TkLen - 1);
+
+ // We expect `Loc` to be valid. The location is obtained by moving forward
+ // from the beginning of the token 'len(token)-1' characters. The file ID of
+ // the locations within a token must be consistent.
+ assert(Loc.isValid() && "Expected the source location of the last"
+ "character of an AST Node to be always valid");
+ return Loc;
}
// Return the source location just past the last character of the AST `Node`.
template <typename NodeTy>
static SourceLocation getPastLoc(const NodeTy *Node, const SourceManager &SM,
const LangOptions &LangOpts) {
- return Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts);
+ SourceLocation Loc =
+ Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts);
+
+ // We expect `Loc` to be valid as it is either associated to a file ID or
+ // it must be the end of a macro expansion. (see
+ // `Lexer::getLocForEndOfToken`)
+ assert(Loc.isValid() && "Expected the source location just past the last "
+ "character of an AST Node to be always valid");
+ return Loc;
}
// Return text representation of an `Expr`.
@@ -1179,9 +1195,25 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
static FixItList
populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx,
const StringRef UserFillPlaceHolder) {
- FixItList FixIts{};
const SourceManager &SM = Ctx.getSourceManager();
const LangOptions &LangOpts = Ctx.getLangOpts();
+
+ // If `Init` has a constant value that is (or equivalent to) a
+ // NULL pointer, we use the default constructor to initialize the span
+ // object, i.e., a `std:span` variable declaration with no initializer.
+ // So the fix-it is just to remove the initializer.
+ if (Init->isNullPointerConstant(
+ std::remove_const_t<ASTContext &>(Ctx),
+ // FIXME: Why does this function not ask for `const ASTContext
+ // &`? It should. Maybe worth an NFC patch later.
+ Expr::NullPointerConstantValueDependence::
+ NPC_ValueDependentIsNotNull)) {
+ SourceRange SR(Init->getBeginLoc(), getEndCharLoc(Init, SM, LangOpts));
+
+ return {FixItHint::CreateRemoval(SR)};
+ }
+
+ FixItList FixIts{};
std::string ExtentText = UserFillPlaceHolder.data();
StringRef One = "1";
@@ -1254,8 +1286,8 @@ static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx,
FixItList InitFixIts =
populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder);
- if (InitFixIts.empty())
- return {}; // Something wrong with fixing initializer, give up
+ assert(!InitFixIts.empty() &&
+ "Unable to fix initializer of unsafe variable declaration");
// The loc right before the initializer:
ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1);
FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
@@ -1318,8 +1350,8 @@ static bool overlapWithMacro(const FixItList &FixIts) {
// FIXME: For now we only check if the range (or the first token) is (part of)
// a macro expansion. Ideally, we want to check for all tokens in the range.
return llvm::any_of(FixIts, [](const FixItHint &Hint) {
- auto BeginLoc = Hint.RemoveRange.getBegin();
- if (BeginLoc.isMacroID())
+ auto Range = Hint.RemoveRange;
+ if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
// If the range (or the first token) is (part of) a macro expansion:
return true;
return false;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
index dacd8673d58bf..ca3f7a4b172a8 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -130,6 +130,38 @@ void explict_cast() {
tmp = (int) s[5];
}
+void null_init() {
+#define NULL 0
+ int tmp;
+ int * my_null = 0;
+ int * p = 0;
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}}
+ int * g = NULL; // cannot handle fix-its involving macros for now
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ int * f = nullptr;
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> f"
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}}
+
+ // In case of value dependencies, we give up
+ int * q = my_null;
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", <# placeholder #>}"
+ int * r = my_null + 0;
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> r"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
+
+ tmp = p[5]; // `p[5]` will cause crash after `p` being transformed to be a `std::span`
+ tmp = q[5]; // Similar for the rests.
+ tmp = r[5];
+ tmp = g[5];
+ tmp = f[5];
+#undef NULL
+}
+
+
void unsupported_multi_decl(int * x) {
int * p = x, * q = new int[10];
// CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]
More information about the cfe-commits
mailing list