ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: NoQ, t-rasmud, jkorous, malavikasamak.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
For a function `F` whose parameters need to be fixed, we group fix-its of its'
parameters together so that either all of the parameters get fixed or none of
them gets fixed.
The reason for doing so is to avoid generating low-quality fix-its. For
example,
void f(int * p, int * q) {
int i = p[5];
int j = q[5];
}
We would like to fix both parameters `p` and `q` with `std::span`. If we do
not group them together, they will be fixed in two steps: 1) fixing `p` first;
and then 2) fixing `q`. Step 1 yields a new overload of `f`:
void f(std::span<int> p, int *q) { ... }
[[unsafe_buffer_usage]] void f(int *p, int *q) {return f(std::span(p, <# size
#>), q);}
}
The new overload of `f` is, however, still an unsafe function. Then we apply
step 2 and have:
void f(std::span<int> p, std::span<int> q) { ... }
[[unsafe_buffer_usage]] void f(int *p, int *q) {return f(std::span<int>(p, <#
size #>), q);}
[[unsafe_buffer_usage]] void f(std::span<int> p, int *q) {return f(p,
std::span<int>(q, <# size #>));}
The "intermediate" overload `void f(std::span<int>, int *)` stays there but
usually is not useful. If there are more than two parameters need to be fixed,
there will be more such useless "intermediate" overloads.
With this patch, `p` and `q` will be fixed together: either selecting `p` or
`q` to fix will result in a safe function `void f(std::span<int> p,
std::span<int> q` in one step.
The implementation is based on this patch <https://reviews.llvm.org/D145739>,
which uses a directed graph to group variables that depend on each other. To
group parameters, I explicitly add directed edges between all parameters that
need fix so that they form a ring. As being a ring in the directed graph, no
such parameter will be missed regardless of where the search in the graph
starts, as long as at least one such parameter is reachable.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D153059
Files:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -36,7 +36,7 @@
void * voidPtrCall(void);
char * charPtrCall(void);
-void testArraySubscripts(int *p, int **pp) { // expected-note{{change type of 'pp' to 'std::span' to preserve bounds information}}
+void testArraySubscripts(int *p, int **pp) {
// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
// expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
foo(p[1], // expected-note{{used in buffer access here}}
@@ -109,7 +109,6 @@
sizeof(decltype(p[1]))); // no-warning
}
-// expected-note@+1{{change type of 'a' to 'std::span' to preserve bounds information}}
void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) {
// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
// expected-warning@-2{{'q' is an unsafe pointer used for buffer access}}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
@@ -29,8 +29,7 @@
int tmp;
tmp = p[5] + q[5];
}
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(std::span<int>(p, <# size #>), q);}\n"
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:2-[[@LINE-2]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(p, std::span<int>(q, <# size #>));}\n"
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(std::span<int>(p, <# size #>), std::span<int>(q, <# size #>));}\n"
void ptrToConst(const int * x) {
// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:17-[[@LINE-1]]:30}:"std::span<int const> x"
@@ -100,22 +99,30 @@
// namned
} NAMED_S;
+
// FIXME: `decltype(ANON_S)` represents an unnamed type but it can
// be referred as "`decltype(ANON_S)`", so the analysis should
// fix-it.
- void decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,
- decltype(NAMED_S) ** rr) {
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:26-[[@LINE-2]]:41}:"std::span<decltype(C)> p"
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:65-[[@LINE-3]]:86}:"std::span<decltype(NAMED_S)> r"
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:26-[[@LINE-3]]:49}:"std::span<decltype(NAMED_S) *> rr"
+ // As parameter `q` cannot be fixed, fixes to parameters are all being given up.
+ void decltypeSpecifierAnon(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,
+ decltype(NAMED_S) ** rr) {
+ // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
if (++p) {}
if (++q) {}
if (++r) {}
if (++rr) {}
}
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(std::span<decltype(C)>(p, <# size #>), q, r, rr);}\n
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, std::span<decltype(NAMED_S)>(r, <# size #>), rr);}\n"
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:4-[[@LINE-3]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, r, std::span<decltype(NAMED_S) *>(rr, <# size #>));}\n"
+ // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+ void decltypeSpecifier(decltype(C) * p, decltype(NAMED_S) * r, decltype(NAMED_S) ** rr) {
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:26-[[@LINE-1]]:41}:"std::span<decltype(C)> p"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:43-[[@LINE-2]]:64}:"std::span<decltype(NAMED_S)> r"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:66-[[@LINE-3]]:89}:"std::span<decltype(NAMED_S) *> rr"
+ if (++p) {}
+ if (++r) {}
+ if (++rr) {}
+ }
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(NAMED_S) * r, decltype(NAMED_S) ** rr) {return decltypeSpecifier(std::span<decltype(C)>(p, <# size #>), std::span<decltype(NAMED_S)>(r, <# size #>), std::span<decltype(NAMED_S) *>(rr, <# size #>));}\n
#define MACRO_TYPE(T) long T
@@ -125,8 +132,7 @@
int tmp = p[5];
tmp = q[5];
}
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(std::span<unsigned MACRO_TYPE(int)>(p, <# size #>), q);}\n"
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(p, std::span<unsigned MACRO_TYPE(long)>(q, <# size #>));}\n"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(std::span<unsigned MACRO_TYPE(int)>(p, <# size #>), std::span<unsigned MACRO_TYPE(long)>(q, <# size #>));}\n"
}
// The followings test various declarators:
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp
@@ -0,0 +1,251 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions \
+// RUN: %s 2>&1 | FileCheck %s
+
+// FIXME: what about possible diagnostic message non-determinism?
+
+// Parameters other than `r` all will be fixed
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+3]]:24-[[@LINE+3]]:30}:"std::span<int> p"
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+2]]:32-[[@LINE+2]]:39}:"std::span<int *> q"
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:41-[[@LINE+1]]:48}:"std::span<int> a"
+void * multiParmAllFix(int *p, int **q, int a[], int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'a' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' and 'a' to 'std::span' to propagate bounds information between them}} \
+ expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'a' to 'std::span' to propagate bounds information between them}} \
+ expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p' and 'q' to 'std::span' to propagate bounds information between them}}
+ int tmp;
+
+ tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = a[5]; // expected-note{{used in buffer access here}}
+ if (++q) {} // expected-note{{used in pointer arithmetic here}}
+ return r;
+}
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid * multiParmAllFix(int *p, int **q, int a[], int * r) {return multiParmAllFix(std::span<int>(p, <# size #>), std::span<int *>(q, <# size #>), std::span<int>(a, <# size #>), r);}\n"
+
+void * multiParmAllFix(int *p, int **q, int a[], int * r);
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n"
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:58-[[@LINE-2]]:58}:";\nvoid * multiParmAllFix(std::span<int> p, std::span<int *> q, std::span<int> a, int * r)"
+
+// Fixing local variables implicates fixing parameters
+void multiParmLocalAllFix(int *p, int * r) {
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:28-[[@LINE-1]]:34}:"std::span<int> p"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:36-[[@LINE-2]]:43}:"std::span<int> r"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> x"
+ int * x; // expected-warning{{'x' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'x' to 'std::span' to preserve bounds information, and change 'p', 'r', and 'z' to 'std::span' to propagate bounds information between them}}
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> z"
+ int * z; // expected-warning{{'z' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'z' to 'std::span' to preserve bounds information, and change 'x', 'p', 'r', to 'std::span' to propagate bounds information between them}}
+ int * y;
+
+ x = p;
+ y = x;
+ // `x` needs to be fixed so does the pointer assigned to `x`, i.e.,`p`
+ x[5] = 5; // expected-note{{used in buffer access here}}
+ z = r;
+ // `z` needs to be fixed so does the pointer assigned to `z`, i.e.,`r`
+ z[5] = 5; // expected-note{{used in buffer access here}}
+ // Since `p` and `r` are parameters need to be fixed together,
+ // fixing `x` involves fixing all `p`, `r`, and `z`. Similar for
+ // fixing `z`.
+}
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid multiParmLocalAllFix(int *p, int * r) {return multiParmLocalAllFix(std::span<int>(p, <# size #>), std::span<int>(r, <# size #>));}\n"
+
+
+// Fixing parameters implicates fixing local variables
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+2]]:29-[[@LINE+2]]:35}:"std::span<int> p"
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:37-[[@LINE+1]]:44}:"std::span<int> r"
+void multiParmLocalAllFix2(int *p, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'r', 'x', and 'z' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'x', and 'z' to 'std::span' to propagate bounds information between them}}
+ int * x = new int[10];
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> x"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+ int * z = new int[10];
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> z"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+ int * y;
+
+ p = x;
+ y = x;
+ p[5] = 5; // expected-note{{used in buffer access here}}
+ r = z;
+ r[5] = 5; // expected-note{{used in buffer access here}}
+}
+// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid multiParmLocalAllFix2(int *p, int * r) {return multiParmLocalAllFix2(std::span<int>(p, <# size #>), std::span<int>(r, <# size #>));}\n"
+
+
+// No fix emitted for any of the parameter since parameter `r` cannot be fixed
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ r++; // expected-note{{used in pointer arithmetic here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+// To show what if the `r` in `noneParmFix` can be fixed:
+void noneParmFix_control(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' and 'r' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'r' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p' and 'q' to 'std::span' to propagate bounds information between them}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ if (++r) {} // expected-note{{used in pointer arithmetic here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+}
+
+
+// No fix emitted for any of the parameter since local variable `l` cannot be fixed.
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmOrLocalFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+ // `l` and `r` must be fixed together while all parameters must be fixed together as well:
+ int * l; l = r; // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+ l++; // expected-note{{used in pointer arithmetic here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+// To show what if the `l` can be fixed in `noneParmOrLocalFix`:
+void noneParmOrLocalFix_control(int * p, int * q, int * r) {// \
+ expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q', 'r', and 'l' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', and 'l' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', and 'l' to 'std::span' to propagate bounds information between them}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+ int * l; // expected-warning{{'l' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', to 'std::span' to propagate bounds information between them}}
+ l = r;
+ if (++l){}; // expected-note{{used in pointer arithmetic here}}
+}
+
+
+// No fix emitted for any of the parameter since local variable `l` cannot be fixed.
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmOrLocalFix2(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+
+ int * a; a = r;
+ int * b; b = a;
+ int * l; l = b; // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+ // `a, b, l` and parameters must be fixed together but `l` can't be fixed:
+ l++; // expected-note{{used in pointer arithmetic here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+// To show what if the `l` can be fixed in `noneParmOrLocalFix2`:
+void noneParmOrLocalFix2_control(int * p, int * q, int * r) { // \
+ expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q', 'r', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+
+ int * a; a = r;
+ int * b; b = a;
+ int * l; // expected-warning{{'l' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'a', and 'b' to 'std::span' to propagate bounds information between them}}
+
+ l = b;
+ if(++l){} // expected-note{{used in pointer arithmetic here}}
+}
+
+// No fix emitted for any of the parameter since local variable `l` cannot be fixed
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmOrLocalFix3(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+
+ int * a; a = r;
+ int * b; b = a;
+ int * l; l = b; // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+ l++; // expected-note{{used in pointer arithmetic here}}
+
+ int * x; x = p; // expected-warning{{'x' is an unsafe pointer used for buffer access}}
+ tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
+
+void noneParmOrLocalFix3_control(int * p, int * q, int * r) { // \
+ expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q', 'r', 'x', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', 'x', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', 'x', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+
+ int * a; a = r;
+ int * b; b = a;
+ int * l; // expected-warning{{'l' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'x', 'a', and 'b' to 'std::span' to propagate bounds information between them}}
+
+ l = b;
+ if (++l){}; // expected-note{{used in pointer arithmetic here}}
+
+ int * x; // expected-warning{{'x' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'x' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}}
+ x = p;
+ tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+
+
+// No fix emitted for any of the parameter but some local variables will get fixed
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void noneParmSomeLocalFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'q' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'r' is an unsafe pointer used for buffer access}}
+ int tmp = p[5]; // expected-note{{used in buffer access here}}
+ tmp = q[5]; // expected-note{{used in buffer access here}}
+ tmp = r[5]; // expected-note{{used in buffer access here}}
+
+ int * a; a = r;
+ int * b; b = a;
+ int * l; l = b; // expected-warning{{'l' is an unsafe pointer used for buffer access}}
+
+ l++; // expected-note{{used in pointer arithmetic here}}
+
+ //`x` and `y` can be fixed
+ int * x = new int[10];
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> x"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+ // CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> y"
+ int * y; // expected-warning{{'y' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'y' to 'std::span' to preserve bounds information, and change 'x' to 'std::span' to propagate bounds information between them}}
+ y = x;
+ tmp = y[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1286,7 +1286,7 @@
const SourceManager &SM,
const LangOptions &LangOpts) {
bool Invalid = false;
- CharSourceRange CSR = CharSourceRange::getCharRange(SR.getBegin(), SR.getEnd());
+ CharSourceRange CSR = CharSourceRange::getCharRange(SR);
StringRef Text = Lexer::getSourceText(CSR, SM, LangOpts, &Invalid);
if (!Invalid)
@@ -1294,6 +1294,22 @@
return std::nullopt;
}
+// Returns the `SourceRange` of `D`. The reason why this function exists is
+// that `D->getSourceRange()` may return a range where the end location is the
+// starting location of the last token. The end location of the source range
+// returned by this function is the last location of the last token.
+static SourceRange getSourceRangeToTokenEnd(const Decl *D,
+ const SourceManager &SM,
+ LangOptions LangOpts) {
+ SourceLocation Begin = D->getBeginLoc();
+ SourceLocation
+ End = // `D->getEndLoc` should always return the starting location of the
+ // last token, so we should get the end of the token
+ Lexer::getLocForEndOfToken(D->getEndLoc(), 0, SM, LangOpts);
+
+ return SourceRange(Begin, End);
+}
+
// 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
@@ -1358,6 +1374,20 @@
return getRangeText(NameRange, SM, LangOpts);
}
+// Returns the text representing a `std::span` type where the element type is
+// represented by `EltTyText`. Note the optional parameter `Qualifiers`:
+// one needs to pass qualifiers explicitly if the element type needs to be
+// qualified.
+static std::string
+getSpanTypeText(StringRef EltTyText,
+ std::optional<Qualifiers> Quals = std::nullopt) {
+ const char *const SpanOpen = "std::span<";
+
+ if (Quals)
+ return SpanOpen + EltTyText.str() + ' ' + Quals->getAsString() + '>';
+ return SpanOpen + EltTyText.str() + '>';
+}
+
std::optional<FixItList>
DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
const VarDecl *VD = dyn_cast<VarDecl>(BaseDeclRefExpr->getDecl());
@@ -1703,11 +1733,11 @@
return SS.str();
}
-// For a `FunDecl`, one of whose `ParmVarDecl`s is being changed to have a new
-// type, this function produces fix-its to make the change self-contained. Let
-// 'F' be the entity defined by the original `FunDecl` and "NewF" be the entity
-// defined by the `FunDecl` after the change to the parameter. Fix-its produced
-// by this function are
+// For a `FunctionDecl`, whose `ParmVarDecl`s are being changed to have new
+// types, this function produces fix-its to make the change self-contained. Let
+// 'F' be the entity defined by the original `FunctionDecl` and "NewF" be the
+// entity defined by the `FunctionDecl` after the change to the parameters.
+// Fix-its produced by this function are
// 1. Add the `[[clang::unsafe_buffer_usage]]` attribute to each declaration
// of 'F';
// 2. Create a declaration of "NewF" next to each declaration of `F`;
@@ -1740,25 +1770,45 @@
// [[clang::unsafe_buffer_usage]]
// #endif
//
-// `NewTypeText` is the string representation of the new type, to which the
-// parameter indexed by `ParmIdx` is being changed.
+// `ParmsMask` is an array of size of `FD->getNumParams()` such
+// that `ParmsMask[i]` is true iff the `i`-th parameter will be fixed with some
+// strategy.
static std::optional<FixItList>
-createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
+createOverloadsForFixedParams(ArrayRef<bool> ParmsMask, const Strategy &S,
const FunctionDecl *FD, const ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler) {
+ const unsigned NumParms = FD->getNumParams();
+ assert(ParmsMask.size() == NumParms && "Incorrect parameter mask size");
// FIXME: need to make this conflict checking better:
if (hasConflictingOverload(FD))
return std::nullopt;
const SourceManager &SM = Ctx.getSourceManager();
const LangOptions &LangOpts = Ctx.getLangOpts();
+ std::vector<std::string> NewTysTexts(NumParms);
+
+ for (unsigned i = 0; i < NumParms; i++) {
+ if (!ParmsMask[i])
+ continue;
+
+ std::optional<Qualifiers> PteTyQuals = std::nullopt;
+ std::optional<std::string> PteTyText =
+ getPointeeTypeText(FD->getParamDecl(i), SM, LangOpts, &PteTyQuals);
+
+ if (!PteTyText)
+ // something wrong in obtaining the text of the pointee type, give up
+ return std::nullopt;
+ // FIXME: whether we should create std::span type depends on the Strategy.
+ NewTysTexts[i] = getSpanTypeText(*PteTyText, PteTyQuals);
+ }
// FIXME Respect indentation of the original code.
// A lambda that creates the text representation of a function declaration
- // with the new type signature:
+ // with the new type signatures:
const auto NewOverloadSignatureCreator =
- [&SM, &LangOpts](const FunctionDecl *FD, unsigned ParmIdx,
- StringRef NewTypeText) -> std::optional<std::string> {
+ [&SM, &LangOpts](
+ const FunctionDecl *FD, ArrayRef<bool> ParmsMask,
+ ArrayRef<std::string> NewTysTexts) -> std::optional<std::string> {
std::stringstream SS;
SS << ";";
@@ -1778,13 +1828,16 @@
if (Parm->isImplicit())
continue;
- if (i == ParmIdx) {
- SS << NewTypeText.str();
+ if (ParmsMask[i]) {
+ // This `i`-th parameter will be fixed with `NewTysTexts[i]` being its
+ // new type:
+ SS << NewTysTexts[i];
// print parameter name if provided:
- if (IdentifierInfo * II = Parm->getIdentifier())
- SS << " " << II->getName().str();
- } else if (auto ParmTypeText =
- getRangeText(Parm->getSourceRange(), SM, LangOpts)) {
+ if (auto II = Parm->getIdentifier())
+ SS << ' ' << II->getName().str();
+ } else if (auto ParmTypeText = getRangeText(
+ getSourceRangeToTokenEnd(Parm, SM, LangOpts),
+ SM, LangOpts)) {
// print the whole `Parm` without modification:
SS << ParmTypeText->str();
} else
@@ -1800,8 +1853,8 @@
// the original signature:
const auto OldOverloadDefCreator =
[&Handler, &SM,
- &LangOpts](const FunctionDecl *FD, unsigned ParmIdx,
- StringRef NewTypeText) -> std::optional<std::string> {
+ &LangOpts](const FunctionDecl *FD, ArrayRef<bool> ParmsMask,
+ ArrayRef<std::string> NewTysTexts) -> std::optional<std::string> {
std::stringstream SS;
SS << getEndOfLine().str();
@@ -1827,9 +1880,9 @@
continue;
assert(Parm->getIdentifier() &&
"A parameter of a function definition has no name");
- if (i == ParmIdx)
+ if (ParmsMask[i])
// This is our spanified paramter!
- SS << NewTypeText.str() << "(" << Parm->getIdentifier()->getName().str() << ", "
+ SS << NewTysTexts[i] << "(" << Parm->getIdentifier()->getName().str() << ", "
<< Handler.getUserFillPlaceHolder("size") << ")";
else
SS << Parm->getIdentifier()->getName().str();
@@ -1853,7 +1906,7 @@
// Inserts a definition with the old signature to the end of
// `FReDecl`:
if (auto OldOverloadDef =
- OldOverloadDefCreator(FReDecl, ParmIdx, NewTyText))
+ OldOverloadDefCreator(FReDecl, ParmsMask, NewTysTexts))
FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *OldOverloadDef));
else
return {}; // give up
@@ -1865,7 +1918,7 @@
}
// Inserts a declaration with the new signature to the end of `FReDecl`:
if (auto NewOverloadDecl =
- NewOverloadSignatureCreator(FReDecl, ParmIdx, NewTyText))
+ NewOverloadSignatureCreator(FReDecl, ParmsMask, NewTysTexts))
FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *NewOverloadDecl));
else
return {};
@@ -1874,67 +1927,42 @@
return FixIts;
}
-// To fix a `ParmVarDecl` to be of `std::span` type. In addition, creating a
-// new overload of the function so that the change is self-contained (see
-// `createOverloadsForFixedParams`).
-static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
- UnsafeBufferUsageHandler &Handler) {
+// To fix a `ParmVarDecl` to be of `std::span` type.
+static FixItList fixParamWithSpan(const ParmVarDecl *PVD,
+ const ASTContext &Ctx) {
+ // FIXME: can this code be shared with local variable fix?
+ assert(PVD->getType()->isPointerType() &&
+ "Unexpected non-pointer type parameter for fix");
if (PVD->hasDefaultArg())
// FIXME: generate fix-its for default values:
return {};
- assert(PVD->getType()->isPointerType());
- auto *FD = dyn_cast<FunctionDecl>(PVD->getDeclContext());
-
- if (!FD)
- return {};
std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
std::optional<std::string> PteTyText = getPointeeTypeText(
PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
if (!PteTyText)
- return {};
+ return {}; // somehow failed to obtain pointee type text
std::optional<StringRef> PVDNameText = PVD->getIdentifier()->getName();
if (!PVDNameText)
return {};
- std::string SpanOpen = "std::span<";
- std::string SpanClose = ">";
- std::string SpanTyText;
std::stringstream SS;
- SS << SpanOpen << *PteTyText;
- // Append qualifiers to span element type:
if (PteTyQualifiers)
- SS << " " << PteTyQualifiers->getAsString();
- SS << SpanClose;
- // Save the Span type text:
- SpanTyText = SS.str();
+ // Append qualifiers if they exist:
+ SS << getSpanTypeText(*PteTyText, PteTyQualifiers);
+ else
+ SS << getSpanTypeText(*PteTyText);
// Append qualifiers to the type of the parameter:
if (PVD->getType().hasQualifiers())
- SS << " " << PVD->getType().getQualifiers().getAsString();
+ SS << ' ' << PVD->getType().getQualifiers().getAsString();
// Append parameter's name:
- SS << " " << PVDNameText->str();
-
- FixItList Fixes;
- unsigned ParmIdx = 0;
-
- Fixes.push_back(
- FixItHint::CreateReplacement(PVD->getSourceRange(), SS.str()));
- for (auto *ParmIter : FD->parameters()) {
- if (PVD == ParmIter)
- break;
- ParmIdx++;
- }
- if (ParmIdx < FD->getNumParams())
- if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, SpanTyText,
- FD, Ctx, Handler)) {
- Fixes.append(*OverloadFix);
- return Fixes;
- }
- return {};
+ SS << ' ' << PVDNameText->str();
+ // Add replacement fix-it:
+ return {FixItHint::CreateReplacement(PVD->getSourceRange(), SS.str())};
}
static FixItList fixVariableWithSpan(const VarDecl *VD,
@@ -1989,7 +2017,7 @@
case Strategy::Kind::Span: {
if (VD->getType()->isPointerType()) {
if (const auto *PVD = dyn_cast<ParmVarDecl>(VD))
- return fixParamWithSpan(PVD, Ctx, Handler);
+ return fixParamWithSpan(PVD, Ctx);
if (VD->isLocalVarDecl())
return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
@@ -2020,25 +2048,32 @@
});
}
-static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForUnsafeVars,
- const Strategy &S,
- const VarDecl * Var) {
- for (const auto &F : FixablesForUnsafeVars.byVar.find(Var)->second) {
- std::optional<FixItList> Fixits = F->getFixits(S);
- if (!Fixits) {
- return true;
- }
- }
- return false;
+// Returns true if `VD` is a parameter declaration of `D` (a declaration of a
+// function, method, block, or lambda):
+static bool isParameterOf(const VarDecl *VD, const Decl *D) {
+ return isa<ParmVarDecl>(VD) &&
+ VD->getDeclContext() == dyn_cast<DeclContext>(D);
}
static std::map<const VarDecl *, FixItList>
getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
- ASTContext &Ctx,
+ ASTContext &Ctx,
/* The function decl under analysis */ const Decl *D,
- const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
- const DefMapTy &VarGrpMap) {
+ const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
+ const DefMapTy &VarGrpMap) {
+ // `FixItsForVariable` will map each variable to fix-its directly associated
+ // to the variable itself. `FixItsForVariable[x]` should be disjoint with
+ // `FixItsForVariable[y]` if `x` and `y` are two distinct variables:
std::map<const VarDecl *, FixItList> FixItsForVariable;
+ // Fix-its that are shared by parameters of `D` when parameters are fixed
+ // together:
+ FixItList FixItsSharedByParms;
+ // `FinalFixItsForVariable` will map each variable 'v' to the self-contained
+ // set of fix-its for 'v', including:
+ // 1) The union over `FixItsForVariable` on the variable group of 'v';
+ // 2) `FixItsSharedByParms`, if parameters are in the same group as 'v';
+ std::map<const VarDecl *, FixItList> FinalFixItsForVariable;
+
for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
FixItsForVariable[VD] =
fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler);
@@ -2048,84 +2083,122 @@
FixItsForVariable.erase(VD);
continue;
}
- bool ImpossibleToFix = false;
- llvm::SmallVector<FixItHint, 16> FixItsForVD;
for (const auto &F : Fixables) {
std::optional<FixItList> Fixits = F->getFixits(S);
- if (!Fixits) {
- ImpossibleToFix = true;
- break;
- } else {
- const FixItList CorrectFixes = Fixits.value();
- FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(),
- CorrectFixes.end());
+ if (Fixits) {
+ FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
+ Fixits->begin(), Fixits->end());
+ continue;
}
- }
-
- if (ImpossibleToFix) {
FixItsForVariable.erase(VD);
- continue;
+ break;
}
-
- const auto VarGroupForVD = VarGrpMap.find(VD);
- if (VarGroupForVD != VarGrpMap.end()) {
- for (const VarDecl * V : VarGroupForVD->second) {
- if (V == VD) {
- continue;
- }
- if (impossibleToFixForVar(FixablesForUnsafeVars, S, V)) {
- ImpossibleToFix = true;
- break;
- }
- }
+ }
+ // `FixItsForVariable` now contains only variables that can be
+ // fixed. A variable can be fixed if its' declaration and all Fixables
+ // associated to it can all be fixed.
+ {
+ // For each variable 'v' in `FixItsForVariable`, if a group member of 'v'
+ // CANNOT be fixed, we remove from `FixItsForVariable` the whole group:
+ SmallVector<const VarDecl *, 8> ToErase;
- if (ImpossibleToFix) {
- FixItsForVariable.erase(VD);
- for (const VarDecl * V : VarGroupForVD->second) {
- FixItsForVariable.erase(V);
- }
+ for (auto VD : FixItsForVariable) {
+ const auto VDGroupIter = VarGrpMap.find(VD.first);
+
+ if (VDGroupIter == VarGrpMap.end())
continue;
+ if (llvm::any_of(VDGroupIter->getSecond(),
+ [&FixItsForVariable](const VarDecl *GrpMember) -> bool {
+ return FixItsForVariable.find(GrpMember) ==
+ FixItsForVariable.end();
+ })) {
+ // At least a group member cannot be fixed, so we have to erase the
+ // whole group:
+ ToErase.insert(ToErase.end(), VDGroupIter->getSecond().begin(),
+ VDGroupIter->getSecond().end());
+ }
+ }
+ for (auto VarToErase : ToErase)
+ FixItsForVariable.erase(VarToErase);
+ }
+ // Now `FixItsForVariable` gets further reduced: a variable is in
+ // `FixItsForVariable` iff it can be fixed and all its group members can be
+ // fixed.
+
+ // Fix-its to parameter declarations of `D` are already in `FixItsForVariable`
+ // for parameters needing fix. Thus the type signature of `D` will be changed.
+ // To make the fix-its self-contained, we need to create an overload of `D`
+ // with its' original signature. The overload fix-it will be added to
+ // `FixItsSharedByParms`.
+ if (isa<FunctionDecl>(
+ D)) { // If `D` is not a `FunctionDecl`, no parameter will be fixed.
+ const FunctionDecl *FD = cast<FunctionDecl>(D);
+ const unsigned NumParms = FD->getNumParams();
+ SmallVector<bool> ParmsMask(NumParms);
+ const ParmVarDecl *FirstParmNeedsFix = nullptr;
+
+ for (unsigned i = 0; i < NumParms; i++) {
+ if (FixItsForVariable.find(FD->getParamDecl(i)) ==
+ FixItsForVariable.end()) {
+ ParmsMask[i] = false;
+ } else {
+ ParmsMask[i] = true;
+ if (!FirstParmNeedsFix)
+ FirstParmNeedsFix = FD->getParamDecl(i);
}
}
- FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
- FixItsForVD.begin(), FixItsForVD.end());
+ if (FirstParmNeedsFix) {
+ // In case at least one parameter needs to be fixed:
+ std::optional<FixItList> OverloadFixes =
+ createOverloadsForFixedParams(ParmsMask, S, FD, Ctx, Handler);
- // Fix-it shall not overlap with macros or/and templates:
- if (overlapWithMacro(FixItsForVariable[VD]) ||
- clang::internal::anyConflict(FixItsForVariable[VD],
- Ctx.getSourceManager())) {
- FixItsForVariable.erase(VD);
- continue;
+ if (OverloadFixes) {
+ FixItsSharedByParms.append(*OverloadFixes);
+ } else {
+ // Something wrong in generating `OverloadFixes`, need to remove the
+ // whole group, where parameters are in, from `FixItsForVariable` (Note
+ // that all parameters should be in the same group):
+ for (auto VarToErase : VarGrpMap.at(FirstParmNeedsFix))
+ FixItsForVariable.erase(VarToErase);
+ }
}
}
-
+ // To populate `FinalFixItsForVariable`:
for (auto VD : FixItsForVariable) {
+ FinalFixItsForVariable[VD.first].append(VD.second);
+
+ // Adding group members' fix-its to each variable's complete fix-it list:
const auto VarGroupForVD = VarGrpMap.find(VD.first);
- const Strategy::Kind ReplacementTypeForVD = S.lookup(VD.first);
+ bool GroupIncludesParms = isParameterOf(VD.first, D);
+
if (VarGroupForVD != VarGrpMap.end()) {
- for (const VarDecl * Var : VarGroupForVD->second) {
- if (Var == VD.first) {
+ for (const VarDecl *Var : VarGroupForVD->second) {
+ if (Var == VD.first)
continue;
- }
-
- FixItList GroupFix;
- if (FixItsForVariable.find(Var) == FixItsForVariable.end()) {
- GroupFix = fixVariable(Var, ReplacementTypeForVD, D,
- Tracker, Var->getASTContext(), Handler);
- } else {
- GroupFix = FixItsForVariable[Var];
- }
-
- for (auto Fix : GroupFix) {
- FixItsForVariable[VD.first].push_back(Fix);
- }
+ assert(FixItsForVariable.find(Var) != FixItsForVariable.end() &&
+ "It is unexpected that a group member cannot be fixed");
+ FinalFixItsForVariable[VD.first].append(FixItsForVariable[Var]);
+ // Test if `Var` is a parameter of `D`:
+ if (!GroupIncludesParms && isParameterOf(Var, D))
+ GroupIncludesParms = true;
}
}
- }
- return FixItsForVariable;
+ if (GroupIncludesParms)
+ FinalFixItsForVariable[VD.first].append(FixItsSharedByParms);
+ }
+ // Fix-its of one `VarDecl` shall not
+ // 1. overlap with macros or/and templates; or
+ // 2. conflicting each other;
+ for (auto Iter = FinalFixItsForVariable.begin();
+ Iter != FinalFixItsForVariable.end();)
+ if (overlapWithMacro(Iter->second) ||
+ clang::internal::anyConflict(Iter->second, Ctx.getSourceManager())) {
+ Iter = FinalFixItsForVariable.erase(Iter);
+ } else
+ Iter++;
+ return FinalFixItsForVariable;
}
-
static Strategy
getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
Strategy S;
@@ -2186,19 +2259,48 @@
// Fixpoint iteration for pointer assignments
using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>;
DepMapTy DependenciesMap{};
+ // A pointer variable `y` is in `PtrImplicationGraph[x]` if fixing pointer
+ // variable `x` implicates fixing `y`:
DepMapTy PtrAssignmentGraph{};
-
- for (auto it : FixablesForAllVars.byVar) {
- for (const FixableGadget *fixable : it.second) {
- std::optional<std::pair<const VarDecl *, const VarDecl *>> ImplPair =
- fixable->getStrategyImplications();
- if (ImplPair) {
- std::pair<const VarDecl *, const VarDecl *> Impl = ImplPair.value();
- PtrAssignmentGraph[Impl.first].insert(Impl.second);
+
+ {
+ // The set of parameters associated to UnsafeOps or involved in strategy
+ // implications of Fixables:
+ SmallSet<const VarDecl *, 16> ParmsNeedFix;
+
+ for (auto it : FixablesForAllVars.byVar) {
+ for (const FixableGadget *fixable : it.second) {
+ std::optional<std::pair<const VarDecl *, const VarDecl *>> ImplPair =
+ fixable->getStrategyImplications();
+ if (ImplPair) {
+ std::pair<const VarDecl *, const VarDecl *> Impl = ImplPair.value();
+ PtrAssignmentGraph[Impl.first].insert(Impl.second);
+ if (isParameterOf(Impl.first, D))
+ ParmsNeedFix.insert(Impl.first);
+ if (isParameterOf(Impl.second, D))
+ ParmsNeedFix.insert(Impl.second);
+ }
+ }
+ }
+ for (auto UnsafeVar : UnsafeOps.byVar)
+ if (isParameterOf(UnsafeVar.first, D))
+ ParmsNeedFix.insert(UnsafeVar.first);
+ // To force parameters in `ParmsNeedFix` to be in a ring in
+ // `PtrAssignmentGraph` so that
+ // 1) they are guaranteed to be in the same group;
+ // 2) no such parameter will be missed regardless of the starting node in
+ // search of connected components.
+ if (!ParmsNeedFix.empty()) {
+ auto First = ParmsNeedFix.begin(), Last = First;
+
+ for (auto I = ++First; I != ParmsNeedFix.end(); ++I) {
+ PtrAssignmentGraph[*Last].insert(*I);
+ Last = I;
}
+ if (First != Last)
+ PtrAssignmentGraph[*First].insert(*Last);
}
}
-
/*
The following code does a BFS traversal of the `PtrAssignmentGraph`
considering all unsafe vars as starting nodes and constructs an undirected
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
