[PATCH] D156192: [-Wunsafe-buffer-usage] Stop generating incorrect fix-its for variable declarations with unsupported specifiers

[Ziqing Luo via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
ziqingluo-90 marked an inline comment as done.
Closed by commit rGb58e52889808: [-Wunsafe-buffer-usage] Stop generating 
incorrect fix-its for variable… (authored by ziqingluo-90).

Changed prior to commit:
  https://reviews.llvm.org/D156192?vs=543760&id=552162#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156192/new/

https://reviews.llvm.org/D156192

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
@@ -58,12 +58,46 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:24}:"std::span<int const> const q"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+  [[deprecated]] const int * x = a;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:18-[[@LINE-1]]:33}:"std::span<int const> x"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:34-[[@LINE-2]]:34}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:35-[[@LINE-3]]:35}:", 10}"
+  const int * y [[deprecated]];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int const> y"
+
   int tmp;
+
   tmp = p[5];
   tmp = q[5];
+  tmp = x[5];
+  tmp = y[5];
 }
 
 
+void local_variable_unsupported_specifiers() {
+  int a[10];
+  const int * p [[deprecated]] = a; //  not supported because the attribute overlaps the source range of the declaration
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  static const int * q = a; //  storage specifier not supported yet
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  extern int * x; //  storage specifier not supported yet
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  constexpr int * y = 0; //  `constexpr` specifier not supported yet
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  int tmp;
+
+  tmp = p[5];
+  tmp = q[5];
+  tmp = x[5];
+  tmp = y[5];
+}
+
+
+
 void local_array_subscript_variable_extent() {
   int n = 10;
   int tmp;
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1416,6 +1416,27 @@
   return getRangeText({ParmIdentBeginLoc, ParmIdentEndLoc}, SM, LangOpts);
 }
 
+// We cannot fix a variable declaration if it has some other specifiers than the
+// type specifier.  Because the source ranges of those specifiers could overlap
+// with the source range that is being replaced using fix-its.  Especially when
+// we often cannot obtain accurate source ranges of cv-qualified type
+// specifiers.
+// FIXME: also deal with type attributes
+static bool hasUnsupportedSpecifiers(const VarDecl *VD,
+                                     const SourceManager &SM) {
+  // AttrRangeOverlapping: true if at least one attribute of `VD` overlaps the
+  // source range of `VD`:
+  bool AttrRangeOverlapping = llvm::any_of(VD->attrs(), [&](Attr *At) -> bool {
+    return !(SM.isBeforeInTranslationUnit(At->getRange().getEnd(),
+                                          VD->getBeginLoc())) &&
+           !(SM.isBeforeInTranslationUnit(VD->getEndLoc(),
+                                          At->getRange().getBegin()));
+  });
+  return VD->isInlineSpecified() || VD->isConstexpr() ||
+         VD->hasConstantInitialization() || !VD->hasLocalStorage() ||
+         AttrRangeOverlapping;
+}
+
 // Returns the text of the pointee type of `T` from a `VarDecl` of a pointer
 // type. The text is obtained through from `TypeLoc`s.  Since `TypeLoc` does not
 // have source ranges of qualifiers ( The `QualifiedTypeLoc` looks hacky too me
@@ -1841,8 +1862,11 @@
 //    the non-empty fix-it list, if fix-its are successfuly generated; empty
 //    list otherwise.
 static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
-                                         const StringRef UserFillPlaceHolder,
-                                         UnsafeBufferUsageHandler &Handler) {
+					 const StringRef UserFillPlaceHolder,
+					 UnsafeBufferUsageHandler &Handler) {
+  if (hasUnsupportedSpecifiers(D, Ctx.getSourceManager()))
+    return {};
+
   FixItList FixIts{};
   std::optional<std::string> SpanTyText = createSpanTypeForVarDecl(D, Ctx);
 
@@ -2076,6 +2100,10 @@
 // `createOverloadsForFixedParams`).
 static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
                                   UnsafeBufferUsageHandler &Handler) {
+  if (hasUnsupportedSpecifiers(PVD, Ctx.getSourceManager())) {
+    DEBUG_NOTE_DECL_FAIL(PVD, " : has unsupport specifier(s)");
+    return {};
+  }
   if (PVD->hasDefaultArg()) {
     // FIXME: generate fix-its for default values:
     DEBUG_NOTE_DECL_FAIL(PVD, " : has default arg");

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits