[-Wunsafe-buffer-usage] Ignore the FixableGadgets that will not be fi…

…xed at an earlier stage

`FixableGadget`s are not always associated with variables that are unsafe
(warned). For example, they could be associated with variables whose
unsafe operations are suppressed or that are not used in any unsafe
operation. Such `FixableGadget`s will not be fixed. Removing these
`FixableGadget` as early as possible helps improve the performance
and stability of the analysis.

Reviewed by: NoQ (Artem Dergachev), t-rasmud (Rashmi Mudduluru)

Differential revision: https://reviews.llvm.org/D155524

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -1090,16 +1090,6 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
   }
 
   M.match(*D->getBody(), D->getASTContext());
-
-  // Gadgets "claim" variables they're responsible for. Once this loop finishes,
-  // the tracker will only track DREs that weren't claimed by any gadgets,
-  // i.e. not understood by the analysis.
-  for (const auto &G : CB.FixableGadgets) {
-    for (const auto *DRE : G->getClaimedVarUseSites()) {
-      CB.Tracker.claimUse(DRE);
-    }
-  }
-
   return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
           std::move(CB.Tracker)};
 }
@@ -2250,6 +2240,33 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     return;
   }
 
+  // If no `WarningGadget`s ever matched, there is no unsafe operations in the
+  //  function under the analysis. No need to fix any Fixables.
+  if (!WarningGadgets.empty()) {
+    // Gadgets "claim" variables they're responsible for. Once this loop
+    // finishes, the tracker will only track DREs that weren't claimed by any
+    // gadgets, i.e. not understood by the analysis.
+    for (const auto &G : FixableGadgets) {
+      for (const auto *DRE : G->getClaimedVarUseSites()) {
+        Tracker.claimUse(DRE);
+      }
+    }
+  }
+
+  // If no `WarningGadget`s ever matched, there is no unsafe operations in the
+  // function under the analysis.  Thus, it early returns here as there is
+  // nothing needs to be fixed.
+  //
+  // Note this claim is based on the assumption that there is no unsafe
+  // variable whose declaration is invisible from the analyzing function.
+  // Otherwise, we need to consider if the uses of those unsafe varuables needs
+  // fix.
+  // So far, we are not fixing any global variables or class members. And,
+  // lambdas will be analyzed along with the enclosing function. So this early
+  // return is correct for now.
+  if (WarningGadgets.empty())
+    return;
+
   UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
   FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets));
 
@@ -2356,6 +2373,34 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     }
   }
 
+  // Remove a `FixableGadget` if the associated variable is not in the graph
+  // computed above.  We do not want to generate fix-its for such variables,
+  // since they are neither warned nor reachable from a warned one.
+  //
+  // Note a variable is not warned if it is not directly used in any unsafe
+  // operation. A variable `v` is NOT reachable from an unsafe variable, if it
+  // does not exist another variable `u` such that `u` is warned and fixing `u`
+  // (transitively) implicates fixing `v`.
+  //
+  // For example,
+  // ```
+  // void f(int * p) {
+  //   int * a = p; *p = 0;
+  // }
+  // ```
+  // `*p = 0` is a fixable gadget associated with a variable `p` that is neither
+  // warned nor reachable from a warned one.  If we add `a[5] = 0` to the end of
+  // the function above, `p` becomes reachable from a warned variable.
+  for (auto I = FixablesForAllVars.byVar.begin();
+       I != FixablesForAllVars.byVar.end();) {
+    // Note `VisitedVars` contain all the variables in the graph:
+    if (VisitedVars.find((*I).first) == VisitedVars.end()) {
+      // no such var in graph:
+      I = FixablesForAllVars.byVar.erase(I);
+    } else
+      ++I;
+  }
+
   Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
 
   FixItsForVariableGroup =

clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp

@@ -107,3 +107,128 @@ void noteGoesWithVarDeclWarning() {
 
   p[5]; // not to note since the associated warning is suppressed
 }
+
+
+// Test suppressing interacts with variable grouping:
+
+// The implication edges are: `a` -> `b` -> `c`.
+// If the unsafe operation on `a` is supressed, none of the variables
+// will be fixed.
+void suppressedVarInGroup() {
+  int * a;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * b;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * c;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+#pragma clang unsafe_buffer_usage begin
+  a[5] = 5;
+#pragma clang unsafe_buffer_usage end
+  a = b;
+  b = c;
+}
+
+// To show that if `a[5]` is not suppressed in the
+// `suppressedVarInGroup` function above, all variables will be fixed.
+void suppressedVarInGroup_control() {
+  int * a;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> a"
+  int * b;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> b"
+  int * c;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> c"
+
+  a[5] = 5;
+  a = b;
+  b = c;
+}
+
+// The implication edges are: `a` -> `b` -> `c`.
+// The unsafe operation on `b` is supressed, while the unsafe
+// operation on `a` is not suppressed. Variable `b` will still be
+// fixed when fixing `a`.
+void suppressedVarInGroup2() {
+  int * a;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> a"
+  int * b;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> b"
+  int * c;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> c"
+
+  a[5] = 5;
+#pragma clang unsafe_buffer_usage begin
+  b[5] = 5;
+#pragma clang unsafe_buffer_usage end
+  a = b;
+  b = c;
+}
+
+// The implication edges are: `a` -> `b` -> `c`.
+// The unsafe operation on `b` is supressed, while the unsafe
+// operation on `c` is not suppressed. Only variable `c` will be fixed
+// then.
+void suppressedVarInGroup3() {
+  int * a;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * b;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * c;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> c"
+
+  c[5] = 5;
+#pragma clang unsafe_buffer_usage begin
+  b[5] = 5;
+#pragma clang unsafe_buffer_usage end
+  a = b;
+  b = c;
+}
+
+// The implication edges are: `a` -> `b` -> `c` -> `a`.
+// The unsafe operation on `b` is supressed, while the unsafe
+// operation on `c` is not suppressed. Since the implication graph
+// forms a cycle, all variables will be fixed.
+void suppressedVarInGroup4() {
+  int * a;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> a"
+  int * b;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> b"
+  int * c;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> c"
+
+  c[5] = 5;
+#pragma clang unsafe_buffer_usage begin
+  b[5] = 5;
+#pragma clang unsafe_buffer_usage end
+  a = b;
+  b = c;
+  c = a;
+}
+
+// There are two groups: `a` -> `b` -> `c` and `d` -> `e` -> `f`.
+// Suppressing unsafe operations on variables in one group does not
+// affect other groups.
+void suppressedVarInGroup5() {
+  int * a;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * b;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * c;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * d;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> d"
+  int * e;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> e"
+  int * f;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> f"
+
+#pragma clang unsafe_buffer_usage begin
+  a[5] = 5;
+#pragma clang unsafe_buffer_usage end
+  a = b;
+  b = c;
+
+  d[5] = 5;
+  d = e;
+  e = f;
+}