ziqingluo-90 updated this revision to Diff 498494.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143680/new/
https://reviews.llvm.org/D143680
Files:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -131,6 +131,38 @@
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-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", <# placeholder #>}"
+ int * r = my_null + 0;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> r"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK-DAG: 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]]
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1068,14 +1068,30 @@
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`.
@@ -1180,9 +1196,25 @@
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";
@@ -1255,8 +1287,8 @@
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()),
@@ -1319,8 +1351,8 @@
// 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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
