[-Wunsafe-buffer-usage] Emit Fix-Its only for C++20 and later standards

[jkorous]

Summary

The transformation strategy we are bringing up relies heavily on std::span which was introduced as part of C++20.

[NoQ]

Yeah that's important! Do we need to cherry-pick this to llvm-16 release candidate?

clang/lib/Sema/AnalysisBasedWarnings.cpp

2521–2522

Also what does this monstrous invocation do that isn't accomplished by S.getLangOpts().CPlusPlus20?

[jkorous]

clang/lib/Sema/AnalysisBasedWarnings.cpp

2521–2522

You are right!
I missed the "(or later)".
https://clang.llvm.org/doxygen/structclang_1_1LangStandard.html#a16afe72612546f41ab5f9afd2b52c7e9

[jkorous]

We actually have only commits with warnings in release/16.x - no Fix-Its. So no need to cherry-pick.

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") {
     s += HintTextToUser;
     s += " #>";
     return s;
   }
 };
 
 // This function invokes the analysis and allows the caller to react to it
 // through the handler class.
-void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
+void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler,
+                            bool EmitFixits);
 
 namespace internal {
 // Tests if any two `FixItHint`s in `FixIts` conflict.  Two `FixItHint`s
 // conflict if they have overlapping source ranges.
 bool anyConflict(const llvm::SmallVectorImpl<FixItHint> &FixIts,
                  const SourceManager &SM);
 } // namespace internal
 } // end namespace clang
 
 #endif /* LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H */

clang/lib/Analysis/UnsafeBufferUsage.cpp

getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
   Strategy S;
   for (const VarDecl *VD : UnsafeVars) {
     S.set(VD, Strategy::Kind::Span);
   }
   return S;
 }
 
 void clang::checkUnsafeBufferUsage(const Decl *D,
-                                   UnsafeBufferUsageHandler &Handler) {
+                                   UnsafeBufferUsageHandler &Handler,
+                                   bool EmitFixits) {
   assert(D && D->getBody());
 
   WarningGadgetSets UnsafeOps;
   FixableGadgetSets FixablesForUnsafeVars;
   DeclUseTracker Tracker;
 
   {
+    // FIXME: We could skip even matching Fixables' matchers if EmitFixits ==
+    // false.
     auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D, Handler);
     UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
     FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets));
     Tracker = std::move(TrackerRes);
   }
 
+  std::map<const VarDecl *, FixItList> FixItsForVariable;
+
+  if (EmitFixits) {
     // Filter out non-local vars and vars with unclaimed DeclRefExpr-s.
     for (auto it = FixablesForUnsafeVars.byVar.cbegin();
          it != FixablesForUnsafeVars.byVar.cend();) {
       // FIXME: Support ParmVarDecl as well.
       if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) {
         it = FixablesForUnsafeVars.byVar.erase(it);
       } else {
         ++it;
       }
     }
 
     llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
     for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar)
       UnsafeVars.push_back(VD);
 
     Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
-  std::map<const VarDecl *, FixItList> FixItsForVariable =
-      getFixIts(FixablesForUnsafeVars, NaiveStrategy, Tracker,
+    FixItsForVariable = getFixIts(FixablesForUnsafeVars, NaiveStrategy, Tracker,
                                   D->getASTContext(), Handler);
 
     // FIXME Detect overlapping FixIts.
+  }
 
   for (const auto &G : UnsafeOps.noVar) {
     Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
   }
 
   for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
-    auto FixItsIt = FixItsForVariable.find(VD);
+    auto FixItsIt =
+        EmitFixits ? FixItsForVariable.find(VD) : FixItsForVariable.end();
     Handler.handleFixableVariable(VD, FixItsIt != FixItsForVariable.end()
                                           ? std::move(FixItsIt->second)
                                           : FixItList{});
     for (const auto &G : WarningGadgets) {
       Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
     }
   }
 }

clang/lib/Sema/AnalysisBasedWarnings.cpp

if (!Diags.isIgnored(diag::warn_throw_in_noexcept_func, D->getBeginLoc()))
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
       if (S.getLangOpts().CPlusPlus && isNoexcept(FD))
         checkThrowInNonThrowingFunc(S, FD, AC);
 
   // Emit unsafe buffer usage warnings and fixits.
   if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, D->getBeginLoc()) ||
       !Diags.isIgnored(diag::warn_unsafe_buffer_variable, D->getBeginLoc())) {
     UnsafeBufferUsageReporter R(S);
-    checkUnsafeBufferUsage(D, R);
+    checkUnsafeBufferUsage(
+        D, R,
+        /*EmitFixits=*/S.getLangOpts().CPlusPlus20);
   }
 
   // If none of the previous checks caused a CFG build, trigger one here
   // for the logical error handler.
   if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
     AC.getCFG();
   }
 
   // Collect statistics about the CFG if it was built.

clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp

+// RUN: %clang_cc1 -x c -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x c -std=c89 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=gnu89 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=iso9899:1990 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x c -std=c17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=gnu17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=iso9899:2017 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=c2x -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x c++ -std=c++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=gnu++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=c++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=gnu++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x objective-c++ -std=c++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x objective-c++ -std=gnu++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x objective-c++ -std=c++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x objective-c++ -std=gnu++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// CHECK-NOT: fix-it:
+
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p;
+  const int *q;
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x;
+  Int_ptr_t y;
+  Int_t * z;
+  Int_t * w;
+
+  tmp = x[5];
+  tmp = y[5];
+  tmp = z[5];
+  tmp = w[5];
+}
+
+void local_ptr_to_array() {
+  int tmp;
+  int n = 10;
+  int a[10];
+  int b[n];
+  int *p = a;
+  int *q = b;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+  int var;
+  int * q = &var;
+  var = q[5];
+}
+
+void decl_without_init() {
+  int tmp;
+  int * p;
+  Int_ptr_t q;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void explict_cast() {
+  int tmp;
+  int * p;
+  tmp = p[5];
+
+  int a;
+  char * q = (char *)&a;
+  tmp = (int) q[5];
+
+  void * r = &a;
+  char * s = (char *) r;
+  tmp = (int) s[5];
+}