ziqingluo-90 updated this revision to Diff 539720.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153059/new/
https://reviews.llvm.org/D153059
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/lib/Sema/AnalysisBasedWarnings.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,306 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fblocks -fsafe-buffer-usage-suggestions -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fblocks -fsafe-buffer-usage-suggestions \
+// RUN: %s 2>&1 | FileCheck %s
+
+// FIXME: what about possible diagnostic message non-determinism?
+
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]:
+void parmsNoFix(int *p, int *q) {
+ int * a = new int[10];
+ int * b = new int[10]; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'b' to 'std::span' to preserve bounds information}}
+
+ a = p;
+ a = q;
+ b[5] = 5; // expected-note{{used in buffer access here}}
+}
+
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:21-[[@LINE+2]]:27}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+14]]:2-[[@LINE+14]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid parmsSingleton(int *p) {return parmsSingleton(std::span<int>(p, <# size #>));}\n"
+void parmsSingleton(int *p) { //expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE+3]]:3-[[@LINE+3]]:12}:"std::span<int> a"
+ // CHECK: fix-it:{{.*}}:{[[@LINE+2]]:13-[[@LINE+2]]:13}:"{"
+ // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:24-[[@LINE+1]]:24}:", 10}"
+ int * a = new int[10];
+ // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> b"
+ int * b; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'b' to 'std::span' to preserve bounds information, and change 'a' to 'std::span' to propagate bounds information between them}}
+
+ b = a;
+ b[5] = 5; // expected-note{{used in buffer access here}}
+ p[5] = 5; // expected-note{{used in buffer access here}}
+}
+
+
+// Parameters other than `r` all will be fixed
+// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:24-[[@LINE+3]]:30}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:32-[[@LINE+2]]:39}:"std::span<int *> q"
+// CHECK: 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 safe types to make function 'multiParmAllFix' bounds-safe}} \
+ expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'a' to safe types to make function 'multiParmAllFix' bounds-safe}} \
+ expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p' and 'q' to safe types to make function 'multiParmAllFix' bounds-safe}}
+ 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: 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: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n"
+// CHECK: 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: fix-it:{{.*}}:{[[@LINE-1]]:28-[[@LINE-1]]:34}:"std::span<int> p"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:36-[[@LINE-2]]:43}:"std::span<int> r"
+ // CHECK: 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', 'z', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}}
+ // CHECK: 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', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}}
+ 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: 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: fix-it:{{.*}}:{[[@LINE+2]]:29-[[@LINE+2]]:35}:"std::span<int> p"
+// CHECK: 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 'x', 'r', and 'z' to safe types to make function 'multiParmLocalAllFix2' bounds-safe}} \
+ 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 safe types to make function 'multiParmLocalAllFix2' bounds-safe}}
+ int * x = new int[10];
+ // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> x"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+ int * z = new int[10];
+ // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> z"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK: 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: 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 safe types to make function 'noneParmFix_control' bounds-safe}} \
+ 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 safe types to make function 'noneParmFix_control' bounds-safe}} \
+ 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 safe types to make function 'noneParmFix_control' bounds-safe}}
+ 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 safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} \
+ 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 safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} \
+ 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 safe types to make function 'noneParmOrLocalFix_control' bounds-safe}}
+ 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', and 'r' to safe types to make function 'noneParmOrLocalFix_control' bounds-safe}}
+ 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}} \
+ 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}} \
+ 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}}
+ 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}}
+
+ 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 'x', 'q', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}} \
+ 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', 'x', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}} \
+ 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', 'q', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}
+ 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', 'x', 'q', 'r', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}
+
+ 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}}
+ 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: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> x"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+ // CHECK: 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]]
+
+// Test that other parameters of (lambdas and blocks) do not interfere
+// the grouping of variables of the function.
+// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:30-[[@LINE+3]]:37}:"std::span<int> p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:39-[[@LINE+2]]:46}:"std::span<int> q"
+// CHECK: fix-it:{{.*}}:{[[@LINE+20]]:2-[[@LINE+20]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid parmsFromLambdaAndBlock(int * p, int * q) {return parmsFromLambdaAndBlock(std::span<int>(p, <# size #>), std::span<int>(q, <# size #>));}\n"
+void parmsFromLambdaAndBlock(int * p, int * q) {
+ // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> a"
+ int * a; // expected-warning{{'a' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p', 'b', and 'q' to safe types to make function 'parmsFromLambdaAndBlock' bounds-safe}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> b"
+ int * b; // expected-warning{{'b' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'b' to 'std::span' to preserve bounds information, and change 'a', 'p', and 'q' to safe types to make function 'parmsFromLambdaAndBlock' bounds-safe}}
+ auto Lamb = [](int * x) -> void { // expected-warning{{'x' is an unsafe pointer used for buffer access}}
+ x[5] = 5; // expected-note{{used in buffer access here}}
+ };
+
+ void (^Blk)(int*) = ^(int * y) { // expected-warning{{'y' is an unsafe pointer used for buffer access}}
+ y[5] = 5; // expected-note{{used in buffer access here}}
+ };
+
+ a = p;
+ b = q;
+ a[5] = 5; // expected-note{{used in buffer access here}}
+ b[5] = 5; // expected-note{{used in buffer access here}}
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2163,6 +2163,41 @@
Sema &S;
bool SuggestSuggestions; // Recommend -fsafe-buffer-usage-suggestions?
+ // Lists as a string the names of variables in `VarGroupForVD` except for `VD`
+ // itself:
+ std::string listVariableGroupAsString(
+ const VarDecl *VD, const ArrayRef<const VarDecl *> &VarGroupForVD) const {
+ if (VarGroupForVD.size() <= 1)
+ return "";
+
+ std::vector<StringRef> VarNames;
+ auto PutInQuotes = [](StringRef S) -> std::string {
+ return "'" + S.str() + "'";
+ };
+
+ for (auto *V : VarGroupForVD) {
+ if (V == VD)
+ continue;
+ VarNames.push_back(V->getName());
+ }
+ if (VarNames.size() == 1) {
+ return PutInQuotes(VarNames[0]);
+ }
+ if (VarNames.size() == 2) {
+ return PutInQuotes(VarNames[0]) + " and " + PutInQuotes(VarNames[1]);
+ }
+ assert(VarGroupForVD.size() > 3);
+ const unsigned N = VarNames.size() -
+ 2; // need to print the last two names as "..., X, and Y"
+ std::string AllVars = "";
+
+ for (unsigned I = 0; I < N; ++I)
+ AllVars.append(PutInQuotes(VarNames[I]) + ", ");
+ AllVars.append(PutInQuotes(VarNames[N]) + ", and " +
+ PutInQuotes(VarNames[N + 1]));
+ return AllVars;
+ }
+
public:
UnsafeBufferUsageReporter(Sema &S, bool SuggestSuggestions)
: S(S), SuggestSuggestions(SuggestSuggestions) {}
@@ -2218,61 +2253,33 @@
}
}
- void handleUnsafeVariableGroup(const VarDecl *Variable,
- const DefMapTy &VarGrpMap,
- FixItList &&Fixes) override {
+ void handleUnsafeVariableGroup(const VarDecl *Variable, const VarGrpRef Group,
+ FixItList &&Fixes, const Decl *D,
+ bool BriefMsg) override {
assert(!SuggestSuggestions &&
"Unsafe buffer usage fixits displayed without suggestions!");
S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
<< Variable << (Variable->getType()->isPointerType() ? 0 : 1)
<< Variable->getSourceRange();
if (!Fixes.empty()) {
- const auto VarGroupForVD = VarGrpMap.find(Variable)->second;
unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy
- const auto &FD = S.Diag(Variable->getLocation(),
- diag::note_unsafe_buffer_variable_fixit_group);
+ const auto &FD =
+ S.Diag(Variable->getLocation(),
+ BriefMsg ? diag::note_unsafe_buffer_variable_fixit_together
+ : diag::note_unsafe_buffer_variable_fixit_group);
FD << Variable << FixItStrategy;
- std::string AllVars = "";
- if (VarGroupForVD.size() > 1) {
- if (VarGroupForVD.size() == 2) {
- if (VarGroupForVD[0] == Variable) {
- AllVars.append("'" + VarGroupForVD[1]->getName().str() + "'");
- } else {
- AllVars.append("'" + VarGroupForVD[0]->getName().str() + "'");
- }
- } else {
- bool first = false;
- if (VarGroupForVD.size() == 3) {
- for (const VarDecl * V : VarGroupForVD) {
- if (V == Variable) {
- continue;
- }
- if (!first) {
- first = true;
- AllVars.append("'" + V->getName().str() + "'" + " and ");
- } else {
- AllVars.append("'" + V->getName().str() + "'");
- }
- }
- } else {
- for (const VarDecl * V : VarGroupForVD) {
- if (V == Variable) {
- continue;
- }
- if (VarGroupForVD.back() != V) {
- AllVars.append("'" + V->getName().str() + "'" + ", ");
- } else {
- AllVars.append("and '" + V->getName().str() + "'");
- }
- }
- }
- }
- FD << AllVars << 1;
+ if (Group.size() > 1) {
+ FD << listVariableGroupAsString(Variable, Group)
+ << /* print the group */ 1;
} else {
- FD << "" << 0;
+ FD << "" << /* no group mate to print */ 0;
+ }
+ if (BriefMsg) {
+ assert(isa<FunctionDecl>(D) &&
+ "Fix for non-function declarations is not supported.");
+ FD << cast<FunctionDecl>(D)->getNameAsString();
}
-
for (const auto &F : Fixes)
FD << F;
}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -12,7 +12,7 @@
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
#include "clang/Lex/Preprocessor.h"
-#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/SetVector.h"
#include <memory>
#include <optional>
#include <sstream>
@@ -1148,7 +1148,11 @@
}
struct FixableGadgetSets {
- std::map<const VarDecl *, std::set<const FixableGadget *>> byVar;
+ std::map<const VarDecl *, std::set<const FixableGadget *>,
+ // To keep keys sorted by their locations in the map so that the
+ // order is deterministic:
+ CompareNode<VarDecl>>
+ byVar;
};
static FixableGadgetSets
@@ -1346,7 +1350,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)
@@ -1354,6 +1358,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
@@ -1418,6 +1438,21 @@
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());
@@ -1763,11 +1798,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`;
@@ -1800,25 +1835,43 @@
// [[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,
- const FunctionDecl *FD, const ASTContext &Ctx,
- UnsafeBufferUsageHandler &Handler) {
+createOverloadsForFixedParams(const std::vector<bool> &ParmsMask, const Strategy &S,
+ const FunctionDecl *FD, const ASTContext &Ctx,
+ UnsafeBufferUsageHandler &Handler) {
// FIXME: need to make this conflict checking better:
if (hasConflictingOverload(FD))
return std::nullopt;
const SourceManager &SM = Ctx.getSourceManager();
const LangOptions &LangOpts = Ctx.getLangOpts();
+ const unsigned NumParms = FD->getNumParams();
+ 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, &NewTysTexts,
+ &ParmsMask](const FunctionDecl *FD) -> std::optional<std::string> {
std::stringstream SS;
SS << ";";
@@ -1838,13 +1891,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
@@ -1859,9 +1915,8 @@
// A lambda that creates the text representation of a function definition with
// the original signature:
const auto OldOverloadDefCreator =
- [&Handler, &SM,
- &LangOpts](const FunctionDecl *FD, unsigned ParmIdx,
- StringRef NewTypeText) -> std::optional<std::string> {
+ [&Handler, &SM, &LangOpts, &NewTysTexts,
+ &ParmsMask](const FunctionDecl *FD) -> std::optional<std::string> {
std::stringstream SS;
SS << getEndOfLine().str();
@@ -1887,10 +1942,10 @@
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() << ", "
- << Handler.getUserFillPlaceHolder("size") << ")";
+ SS << NewTysTexts[i] << "(" << Parm->getIdentifier()->getName().str()
+ << ", " << Handler.getUserFillPlaceHolder("size") << ")";
else
SS << Parm->getIdentifier()->getName().str();
if (i != NumParms - 1)
@@ -1912,8 +1967,7 @@
assert(FReDecl == FD && "inconsistent function definition");
// Inserts a definition with the old signature to the end of
// `FReDecl`:
- if (auto OldOverloadDef =
- OldOverloadDefCreator(FReDecl, ParmIdx, NewTyText))
+ if (auto OldOverloadDef = OldOverloadDefCreator(FReDecl))
FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *OldOverloadDef));
else
return {}; // give up
@@ -1924,8 +1978,7 @@
FReDecl->getBeginLoc(), getUnsafeBufferUsageAttributeText()));
}
// Inserts a declaration with the new signature to the end of `FReDecl`:
- if (auto NewOverloadDecl =
- NewOverloadSignatureCreator(FReDecl, ParmIdx, NewTyText))
+ if (auto NewOverloadDecl = NewOverloadSignatureCreator(FReDecl))
FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *NewOverloadDecl));
else
return {};
@@ -1934,67 +1987,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,
@@ -2049,7 +2077,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);
@@ -2080,25 +2108,96 @@
});
}
-static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForAllVars,
- const Strategy &S,
- const VarDecl * Var) {
- for (const auto &F : FixablesForAllVars.byVar.find(Var)->second) {
- std::optional<FixItList> Fixits = F->getFixits(S);
- if (!Fixits) {
- return true;
+// Returns true iff `VD` is a parameter of the declaration `D`:
+static bool isParameterOf(const VarDecl *VD, const Decl *D) {
+ return isa<ParmVarDecl>(VD) &&
+ VD->getDeclContext() == dyn_cast<DeclContext>(D);
+}
+
+// Erasing variables in `FixItsForVariable`, if such a variable has an unfixable
+// group mate. A variable `v` is unfixable iff `FixItsForVariable` does not
+// contain `v`.
+static void eraseVarsForUnfixableGroupMates(
+ std::map<const VarDecl *, FixItList> &FixItsForVariable,
+ const VariableGroupsManager &VarGrpMgr) {
+ // Variables will be removed from `FixItsForVariable`:
+ SmallVector<const VarDecl *, 8> ToErase;
+
+ for (auto [VD, Ignore] : FixItsForVariable) {
+ VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD);
+ if (llvm::any_of(Grp,
+ [&FixItsForVariable](const VarDecl *GrpMember) -> bool {
+ return FixItsForVariable.find(GrpMember) ==
+ FixItsForVariable.end();
+ })) {
+ // At least one group member cannot be fixed, so we have to erase the
+ // whole group:
+ for (const VarDecl *Member : Grp)
+ ToErase.push_back(Member);
}
}
- return false;
+ for (auto *VarToErase : ToErase)
+ FixItsForVariable.erase(VarToErase);
+}
+
+// Returns the fix-its that create bounds-safe function overloads for the
+// function `D`, if `D`'s parameters will be changed to safe-types through
+// fix-its in `FixItsForVariable`.
+//
+// NOTE: In case `D`'s parameters will be changed but bounds-safe function
+// overloads cannot created, the whole group that contains the parameters will
+// be erased from `FixItsForVariable`.
+static FixItList createFunctionOverloadsForParms(
+ std::map<const VarDecl *, FixItList> &FixItsForVariable /* mutable */,
+ const VariableGroupsManager &VarGrpMgr, const Decl *D, const Strategy &S,
+ ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) {
+ FixItList FixItsSharedByParms{};
+
+ if (auto *FD = dyn_cast<FunctionDecl>(
+ D)) { // If `D` is not a `FunctionDecl`, no parameter will be fixed.
+ std::vector<bool> ParmsNeedFixMask(FD->getNumParams(), false);
+ const VarDecl *FirstParmNeedsFix = nullptr;
+
+ for (unsigned i = 0; i < FD->getNumParams(); i++)
+ if (FixItsForVariable.find(FD->getParamDecl(i)) !=
+ FixItsForVariable.end()) {
+ ParmsNeedFixMask[i] = true;
+ FirstParmNeedsFix = FD->getParamDecl(i);
+ }
+ if (FirstParmNeedsFix) {
+ // In case at least one parameter needs to be fixed:
+ std::optional<FixItList> OverloadFixes =
+ createOverloadsForFixedParams(ParmsNeedFixMask, S, FD, Ctx, Handler);
+
+ 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 *Member : VarGrpMgr.getGroupOfVar(FirstParmNeedsFix))
+ FixItsForVariable.erase(Member);
+ }
+ }
+ }
+ return FixItsSharedByParms;
}
+// Constructs self-contained fix-its for each variable in `FixablesForAllVars`.
static std::map<const VarDecl *, FixItList>
getFixIts(FixableGadgetSets &FixablesForAllVars, 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 VariableGroupsManager &VarGrpMgr) {
+ // `FixItsForVariable` will map each variable to a set of fix-its directly
+ // associated to the variable itself. Fix-its of distict variables in
+ // `FixItsForVariable` are disjoint.
std::map<const VarDecl *, FixItList> FixItsForVariable;
+
+ // Populate `FixItsForVariable` with fix-its directly associated with each
+ // variable. Fix-its directly associated to a variable 'v' are the ones
+ // produced by the `FixableGadget`s whose claimed variable is 'v'.
for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
FixItsForVariable[VD] =
fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler);
@@ -2108,84 +2207,68 @@
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 (ImpossibleToFix) {
- FixItsForVariable.erase(VD);
- continue;
- }
-
- const auto VarGroupForVD = VarGrpMap.find(VD);
- if (VarGroupForVD != VarGrpMap.end()) {
- for (const VarDecl * V : VarGroupForVD->second) {
- if (V == VD) {
- continue;
- }
- if (impossibleToFixForVar(FixablesForAllVars, S, V)) {
- ImpossibleToFix = true;
- break;
- }
- }
- if (ImpossibleToFix) {
- FixItsForVariable.erase(VD);
- for (const VarDecl * V : VarGroupForVD->second) {
- FixItsForVariable.erase(V);
- }
+ if (Fixits) {
+ FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
+ Fixits->begin(), Fixits->end());
continue;
}
- }
- FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
- FixItsForVD.begin(), FixItsForVD.end());
-
- // 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;
+ break;
}
}
- for (auto VD : FixItsForVariable) {
- const auto VarGroupForVD = VarGrpMap.find(VD.first);
- const Strategy::Kind ReplacementTypeForVD = S.lookup(VD.first);
- if (VarGroupForVD != VarGrpMap.end()) {
- 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);
- }
- }
+ // `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.
+
+ // To further remove from `FixItsForVariable` variables whose group mates
+ // cannot be fixed...
+ eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr);
+ // Now `FixItsForVariable` gets further reduced: a variable is in
+ // `FixItsForVariable` iff it can be fixed and all its group mates can be
+ // fixed.
+
+ // Fix-its of bounds-safe overloads of `D` are shared by parameters of `D`.
+ // That is, when fixing multiple parameters in one step, these fix-its will
+ // be applied only once (instead of being applied per parameter).
+ FixItList FixItsSharedByParms = createFunctionOverloadsForParms(
+ FixItsForVariable, VarGrpMgr, D, S, Ctx, Handler);
+
+ // `FinalFixItsForVariable` will map each variable 'v' to the self-contained
+ // set of fix-its for 'v', including:
+ // 1) fix-its directly associated to every group mate of 'v' (including 'v'
+ // itself); and
+ // 2) `FixItsSharedByParms`, if parameters are in the same group as 'v'.
+ std::map<const VarDecl *, FixItList> FinalFixItsForVariable;
+
+ for (auto [VD, Ignore] : FixItsForVariable) {
+ FixItList &FinalFixItsForVD = FinalFixItsForVariable[VD];
+ bool AnyParm = false;
+ VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD, &AnyParm);
+
+ for (auto *Member : Grp) {
+ FinalFixItsForVD.append(FixItsForVariable[Member]);
}
- }
- return FixItsForVariable;
+ if (AnyParm)
+ FinalFixItsForVD.append(FixItsSharedByParms);
+ }
+ // Fix-its that will be applied in one step shall NOT:
+ // 1. overlap with macros or/and templates; or
+ // 2. conflicting each other.
+ // Otherwise, the fix-its will be dropped.
+ 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;
@@ -2195,6 +2278,34 @@
return S;
}
+// Manages variable groups:
+class VariableGroupsManagerImpl : public VariableGroupsManager {
+ const std::vector<VarGrpTy> Groups;
+ const std::map<const VarDecl *, unsigned> &VarGrpMap;
+ const llvm::SetVector<const VarDecl *> &GrpsUnionForParms;
+
+public:
+ VariableGroupsManagerImpl(
+ const std::vector<VarGrpTy> &Groups,
+ const std::map<const VarDecl *, unsigned> &VarGrpMap,
+ const llvm::SetVector<const VarDecl *> &GrpsUnionForParms)
+ : Groups(Groups), VarGrpMap(VarGrpMap),
+ GrpsUnionForParms(GrpsUnionForParms) {}
+
+ VarGrpRef getGroupOfVar(const VarDecl *Var, bool *HasParm) const override {
+ if (GrpsUnionForParms.contains(Var)) {
+ if (HasParm)
+ *HasParm = true;
+ return GrpsUnionForParms.getArrayRef();
+ }
+ if (HasParm)
+ *HasParm = false;
+ if (VarGrpMap.find(Var) == VarGrpMap.end())
+ return std::nullopt;
+ return Groups[VarGrpMap.at(Var)];
+ }
+};
+
void clang::checkUnsafeBufferUsage(const Decl *D,
UnsafeBufferUsageHandler &Handler,
bool EmitSuggestions) {
@@ -2237,7 +2348,6 @@
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();
@@ -2256,7 +2366,7 @@
UnsafeVars.push_back(VD);
// Fixpoint iteration for pointer assignments
- using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>;
+ using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>;
DepMapTy DependenciesMap{};
DepMapTy PtrAssignmentGraph{};
@@ -2265,7 +2375,7 @@
std::optional<std::pair<const VarDecl *, const VarDecl *>> ImplPair =
fixable->getStrategyImplications();
if (ImplPair) {
- std::pair<const VarDecl *, const VarDecl *> Impl = ImplPair.value();
+ std::pair<const VarDecl *, const VarDecl *> Impl = std::move(*ImplPair);
PtrAssignmentGraph[Impl.first].insert(Impl.second);
}
}
@@ -2310,14 +2420,24 @@
}
}
+ // `Groups` stores the set of Connected Components in the graph.
+ std::vector<VarGrpTy> Groups;
+ // `VarGrpMap` maps variables that need fix to the groups (indexes) that the
+ // variables belong to. Group indexes refer to the elements in `Groups`.
+ // `VarGrpMap` is complete in that every variable that needs fix is in it.
+ std::map<const VarDecl *, unsigned> VarGrpMap;
+ // The union group over the ones in "Groups" that contain parameters of `D`:
+ llvm::SetVector<const VarDecl *>
+ GrpsUnionForParms; // these variables need to be fixed in one step
+
// Group Connected Components for Unsafe Vars
// (Dependencies based on pointer assignments)
std::set<const VarDecl *> VisitedVars{};
for (const auto &[Var, ignore] : UnsafeOps.byVar) {
if (VisitedVars.find(Var) == VisitedVars.end()) {
- std::vector<const VarDecl *> VarGroup{};
-
+ VarGrpTy &VarGroup = Groups.emplace_back();
std::queue<const VarDecl*> Queue{};
+
Queue.push(Var);
while(!Queue.empty()) {
const VarDecl* CurrentVar = Queue.front();
@@ -2331,21 +2451,26 @@
}
}
}
- for (const VarDecl * V : VarGroup) {
- if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) {
- VariableGroupsMap[V] = VarGroup;
- }
+
+ bool HasParm = false;
+ unsigned GrpIdx = Groups.size() - 1;
+
+ for (const VarDecl *V : VarGroup) {
+ VarGrpMap[V] = GrpIdx;
+ if (!HasParm && isParameterOf(V, D))
+ HasParm = true;
}
+ if (HasParm)
+ GrpsUnionForParms.insert(VarGroup.begin(), VarGroup.end());
}
}
Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
+ VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms);
FixItsForVariableGroup =
getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
- Tracker, Handler, VariableGroupsMap);
-
- // FIXME Detect overlapping FixIts.
+ Tracker, Handler, VarGrpMgr);
for (const auto &G : UnsafeOps.noVar) {
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
@@ -2353,10 +2478,16 @@
for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
auto FixItsIt = FixItsForVariableGroup.find(VD);
- Handler.handleUnsafeVariableGroup(VD, VariableGroupsMap,
+ bool BriefMsg = false; // Set to `true` if to NOT explain how variables are
+ // grouped together
+ // If `Group` contains parameters, make the diag message brief:
+ VarGrpRef Group = VarGrpMgr.getGroupOfVar(VD, &BriefMsg);
+
+ Handler.handleUnsafeVariableGroup(VD, Group,
FixItsIt != FixItsForVariableGroup.end()
- ? std::move(FixItsIt->second)
- : FixItList{});
+ ? std::move(FixItsIt->second)
+ : FixItList{},
+ D, BriefMsg);
for (const auto &G : WarningGadgets) {
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
}
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11841,6 +11841,9 @@
"used%select{| in pointer arithmetic| in buffer access}0 here">;
def note_unsafe_buffer_variable_fixit_group : Note<
"change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information%select{|, and change %2 to '%select{std::span|std::array|std::span::iterator}1' to propagate bounds information between them}3">;
+def note_unsafe_buffer_variable_fixit_together : Note<
+ "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information"
+ "%select{|, and change %2 to safe types to make function '%4' bounds-safe}3">;
def note_safe_buffer_usage_suggestions_disabled : Note<
"pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
def err_loongarch_builtin_requires_la32 : Error<
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
===================================================================
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -19,7 +19,22 @@
namespace clang {
-using DefMapTy = llvm::DenseMap<const VarDecl *, std::vector<const VarDecl *>>;
+using VarGrpTy = std::vector<const VarDecl *>;
+using VarGrpRef = ArrayRef<const VarDecl *>;
+
+class VariableGroupsManager {
+public:
+ VariableGroupsManager() = default;
+ virtual ~VariableGroupsManager() = default;
+ /// Returns the set of variables (including `Var`) that need to be fixed
+ /// together in one step.
+ ///
+ /// `Var` must be a variable that needs fix (so it must be in a group).
+ /// `HasParm` is an optional argument that will be set to true if the set of
+ /// variables, where `Var` is in, contains parameters.
+ virtual VarGrpRef getGroupOfVar(const VarDecl *Var,
+ bool *HasParm = nullptr) const;
+};
/// The interface that lets the caller handle unsafe buffer usage analysis
/// results by overriding this class's handle... methods.
@@ -37,11 +52,16 @@
bool IsRelatedToDecl) = 0;
/// Invoked when a fix is suggested against a variable. This function groups
- /// all variables that must be fixed together (i.e their types must be changed to the
- /// same target type to prevent type mismatches) into a single fixit.
+ /// all variables that must be fixed together (i.e their types must be changed
+ /// to the same target type to prevent type mismatches) into a single fixit.
+ ///
+ /// `D` is the declaration of the callable under analysis that owns `Variable`
+ /// and all the variables in `Group`.
+ /// `BriefMsg` is true if to NOT explain how variables are grouped together.
virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
- const DefMapTy &VarGrpMap,
- FixItList &&Fixes) = 0;
+ ArrayRef<const VarDecl *> Group,
+ FixItList &&Fixes, const Decl *D,
+ bool BriefMsg = false) = 0;
/// Returns a reference to the `Preprocessor`:
virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
