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 rGcfcf76c6ad72: [-Wunsafe-buffer-usage] Ignore the
FixableGadgets that will not be fixed at an… (authored by ziqingluo-90).
Changed prior to commit:
https://reviews.llvm.org/D155524?vs=542199&id=544151#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155524/new/
https://reviews.llvm.org/D155524
Files:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
@@ -107,3 +107,128 @@
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;
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1090,16 +1090,6 @@
}
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 @@
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 @@
}
}
+ // 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 =
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
