[-Wunsafe-buffer-usage] Ignore the FixableGadgets that will not be fixed at an earlier stage

[ziqingluo-90]

Summary

FixableGadgets are not always associated with variables that is 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 FixableGadgets will not be fixed. Removing these FixableGadget as early as possible could help improve the performance and stability of the analysis.

The variable grouping algorithm introduced in D145739 nearly completes this task.
For a variable that is neither warned about nor reachable from a warned variable, it does not exist in the graph computed by the algorithm.
So this patch simply removes FixableGadgets whose variables are not present in the graph computed for variable grouping.

[t-rasmud]

LGTM!

clang/lib/Analysis/UnsafeBufferUsage.cpp

2378

Nit: Maybe also mention when a variable is neither warned nor is reachable? Are there scenarios besides variables inside pragmas where this constraint is satisfied?

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2378

good question! I add some explanation in the comment.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2242–2269

Maybe early-return here when there are no gadgets?

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1092–1093

Let's also skip this part when there are no warning gadgets? Your initial patch was clearing the FixableGadgets list so it was naturally skipped, but now it's active again.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1092–1093

Oh, I intentionally chose not to skip it:

• 1. To keep the Tracker consistent with CB.FixableGadgets in case in the future we want to use these two objects even if there is no WarningGadget;
• 2. The findGadgets function can stay as straightforward as its name suggests. It doesn't have to know the specific optimization for empty WarningGadgets.

But the two thoughts above probably aren't important enough. I will change it back to skipping the loop when there is no warnings.

[ziqingluo-90]

Address comments

[NoQ]

LGTM, thanks!!

clang/lib/Analysis/UnsafeBufferUsage.cpp

1092–1093

Hmm, I agree that this turns into a confusing contract. Maybe move this loop out of the function, to the call site, so that it was more obvious that we skip it? This would leave the tracker in a mildly inconsistent state at return, so the caller will have to make it consistent by doing the claiming, but that's arguably less confusing because we clearly communicate that this is an optional step. So the function will do exactly what it says it'll do: find gadgets and that's it.

Separately, we can probably consider not searching for fixable gadgets at all when no warning gadgets are found. This could be a performance improvement, but definitely a story for another time.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1092–1093

Thanks for the suggestion. I moved the loop to the end of the call to findGadgets and landed it.

clang/lib/Analysis/UnsafeBufferUsage.cpp

#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
             declStmt().bind("any_ds")
           )))
       ),
       &CB
     );
     // clang-format on
   }
 
   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)};
 }
 
 // Compares AST nodes by source locations.
 template <typename NodeTy> struct CompareNode {
   bool operator()(const NodeTy *N1, const NodeTy *N2) const {
     return N1->getBeginLoc().getRawEncoding() <
            N2->getBeginLoc().getRawEncoding();
if (!EmitSuggestions) {
     }
 
     // This return guarantees that most of the machine doesn't run when
     // suggestions aren't requested.
     assert(FixableGadgets.size() == 0 &&
            "Fixable gadgets found but suggestions not requested!");
     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));
 
   std::map<const VarDecl *, FixItList> FixItsForVariableGroup;
   DefMapTy VariableGroupsMap{};
 
   // Filter out non-local vars and vars with unclaimed DeclRefExpr-s.
   for (auto it = FixablesForAllVars.byVar.cbegin();
if (VisitedVars.find(Var) == VisitedVars.end()) {
       for (const VarDecl * V : VarGroup) {
         if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) {
           VariableGroupsMap[V] = VarGroup;
         }
       }
     }
   }
 
+  // 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 =
       getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
                 Tracker, Handler, VariableGroupsMap);
 
   // FIXME Detect overlapping FixIts.
 

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

 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunsafe-buffer-usage"
   int *p = new int[10]; // not to warn
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
 #pragma clang diagnostic pop
 
   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;
+}