[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 484156.
ziqingluo-90 added a comment.

did a rebase


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.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
@@ -67,21 +67,21 @@
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
 
-  auto ap1 = a;
+  auto ap1 = a; // expected-warning{{'ap1' participates in unchecked buffer operations}}
 
-  foo(ap1[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap1[1]);
 
-  auto ap2 = p;
+  auto ap2 = p; // expected-warning{{'ap2' participates in unchecked buffer operations}}
 
-  foo(ap2[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap2[1]);
 
-  auto ap3 = pp;
+  auto ap3 = pp; // expected-warning{{'ap3' participates in unchecked buffer operations}}
 
-  foo(ap3[1][1]); // expected-warning2{{unchecked operation on raw buffer in expression}}
+  foo(ap3[1][1]); // expected-warning{{unchecked operation on raw buffer in expression}}
 
-  auto ap4 = *pp;
+  auto ap4 = *pp; // expected-warning{{'ap4' participates in unchecked buffer operations}}
 
-  foo(ap4[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap4[1]);
 }
 
 void testUnevaluatedContext(int * p) {
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
@@ -0,0 +1,73 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+
+void foo(...);
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+// CHECK: std::span p{new int [10], 10};
+// CHECK: p[5] = 5;
+  int *p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  p[5] = 5;
+
+// CHECK: std::span q{new int [10], 10};
+// CHECK: std::span x{new int [10], 10};
+// CHECK: std::span y{new int, 1};
+// CHECK: std::span z{new int [10], 10};
+// CHECK: std::span w{new Int_t [10], 10};
+// CHECK: foo(q[5], x[5], y[5], z[5], w[5]);
+  const int *q = new int[10]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  Int_ptr_t x = new int[10];  // expected-warning{{variable 'x' participates in unchecked buffer operations}}
+  Int_ptr_t y = new int;  // expected-warning{{variable 'y' participates in unchecked buffer operations}}
+  Int_t * z = new int[10];// expected-warning{{variable 'z' participates in unchecked buffer operations}}
+  Int_t * w = new Int_t[10];  // expected-warning{{variable 'w' participates in unchecked buffer operations}}
+  foo(q[5], x[5], y[5], z[5], w[5]);
+  // y[5] will crash after being span
+}
+
+void local_array_subscript_auto() {
+// CHECK: std::span p{new int [10], 10};
+// CHECK: p[5] = 5;
+  auto p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  p[5] = 5;
+}
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+// CHECK: std::span p{new int [n], n};
+// CHECK: std::span q{new int [n++], ...};
+// CHECK: foo(p[5], q[5]);
+  int *p = new int[n]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  foo(p[5], q[5]);
+}
+
+
+void local_ptr_to_array() {
+  int n = 10;
+  int a[10];
+  int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+// CHECK: std::span p{a, 10};
+// CHECK: std::span q{b, ...};
+// CHECK: foo(p[5], q[5]);
+  int *p = a; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  int *q = b; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  foo(p[5], q[5]);
+}
+
+void local_ptr_addrof_init() {
+  int a[10];
+  int var;
+// CHECK: std::span p{&a, 1};
+// CHECK: std::span q{&var, 1};
+// CHECK: foo(p[5], q[5]);
+  int (*p)[10] = &a; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  int * q = &var; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  // These two expressions involve unsafe buffer accesses, which will
+  // crash at runtime after applying the fix-it,

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thanks for the rebase!

Nit: I'd just replace `std::function` with `auto` as it saves us repeating the 
parameter types (and `#include `).




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:617
+  // Printers that print extent into OS and sets ExtKnown to true:
+  std::function PrintExpr = [&ExtKnown, &OS, &PP](const Expr *Size) {
+Size->printPretty(OS, nullptr, PP);





Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:621
+  };
+  std::function PrintAPInt = [&ExtKnown, &OS](const APInt &Size) {
+Size.print(OS, false);





Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:625
+  };
+  std::function PrintOne = [&ExtKnown, &OS](void) {
+OS << "1";




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 484412.
ziqingluo-90 added a comment.

To follow LLVM's convention that global variables better have types that do NOT 
require construction, I change the type of the global variable from 
`std::string` to `constexpr const char * const`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.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
@@ -67,21 +67,21 @@
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
 
-  auto ap1 = a;
+  auto ap1 = a; // expected-warning{{'ap1' participates in unchecked buffer operations}}
 
-  foo(ap1[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap1[1]);
 
-  auto ap2 = p;
+  auto ap2 = p; // expected-warning{{'ap2' participates in unchecked buffer operations}}
 
-  foo(ap2[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap2[1]);
 
-  auto ap3 = pp;
+  auto ap3 = pp; // expected-warning{{'ap3' participates in unchecked buffer operations}}
 
-  foo(ap3[1][1]); // expected-warning2{{unchecked operation on raw buffer in expression}}
+  foo(ap3[1][1]); // expected-warning{{unchecked operation on raw buffer in expression}}
 
-  auto ap4 = *pp;
+  auto ap4 = *pp; // expected-warning{{'ap4' participates in unchecked buffer operations}}
 
-  foo(ap4[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap4[1]);
 }
 
 void testUnevaluatedContext(int * p) {
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
@@ -0,0 +1,73 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+
+void foo(...);
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+// CHECK: std::span p{new int [10], 10};
+// CHECK: p[5] = 5;
+  int *p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  p[5] = 5;
+
+// CHECK: std::span q{new int [10], 10};
+// CHECK: std::span x{new int [10], 10};
+// CHECK: std::span y{new int, 1};
+// CHECK: std::span z{new int [10], 10};
+// CHECK: std::span w{new Int_t [10], 10};
+// CHECK: foo(q[5], x[5], y[5], z[5], w[5]);
+  const int *q = new int[10]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  Int_ptr_t x = new int[10];  // expected-warning{{variable 'x' participates in unchecked buffer operations}}
+  Int_ptr_t y = new int;  // expected-warning{{variable 'y' participates in unchecked buffer operations}}
+  Int_t * z = new int[10];// expected-warning{{variable 'z' participates in unchecked buffer operations}}
+  Int_t * w = new Int_t[10];  // expected-warning{{variable 'w' participates in unchecked buffer operations}}
+  foo(q[5], x[5], y[5], z[5], w[5]);
+  // y[5] will crash after being span
+}
+
+void local_array_subscript_auto() {
+// CHECK: std::span p{new int [10], 10};
+// CHECK: p[5] = 5;
+  auto p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  p[5] = 5;
+}
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+// CHECK: std::span p{new int [n], n};
+// CHECK: std::span q{new int [n++], ...};
+// CHECK: foo(p[5], q[5]);
+  int *p = new int[n]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  foo(p[5], q[5]);
+}
+
+
+void local_ptr_to_array() {
+  int n = 10;
+  int a[10];
+  int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+// CHECK: std::span p{a, 10};
+// CHECK: std::span q{b, ...};
+// CHECK: foo(p[5], q[5]);
+  int *p = a; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  int *q = b; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  foo(p[5], q[5]);
+}
+
+void local_ptr_addrof_init() {
+  int a[10];
+  int var;
+// CHECK: std::span p{&a, 1};
+// CHECK: std::span q{&var, 1};
+// CHECK: foo(p[5], q[5]);
+  int (*p)[10] = &a; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  int * q = &var; // expected-wa

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 487527.
ziqingluo-90 added a comment.

Rebase the patch with respect to the re-architecture: 
https://reviews.llvm.org/D140062.

Since we do not have any `FixableGadget` to trigger fix-its at this point,  I 
let fix-its of local variable declarations always be emitted for the purpose of 
testing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.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
@@ -67,21 +67,21 @@
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
 
-  auto ap1 = a;
+  auto ap1 = a; // expected-warning{{'ap1' participates in unchecked buffer operations}}
 
   foo(ap1[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
 
-  auto ap2 = p;
+  auto ap2 = p; // expected-warning{{'ap2' participates in unchecked buffer operations}}
 
   foo(ap2[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
 
-  auto ap3 = pp;
+  auto ap3 = pp; // expected-warning{{'ap3' participates in unchecked buffer operations}}
 
   foo(ap3[1][1]); // expected-warning2{{unchecked operation on raw buffer in expression}}
 
-  auto ap4 = *pp;
+  auto ap4 = *pp; // expected-warning{{'ap4' participates in unchecked buffer operations}}
 
-  foo(ap4[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap4[1]);   // expected-warning{{unchecked operation on raw buffer in expression}}
 }
 
 void testUnevaluatedContext(int * p) {
@@ -176,7 +176,7 @@
 template
 void testPointerArithmetic(int * p, const int **q, T * x) {
   int a[10];
-  auto y = &a[0];
+  auto y = &a[0];   // expected-warning{{variable 'y' participates in unchecked buffer operations}}
 
   foo(p + 1, 1 + p, p - 1,  // expected-warning3{{unchecked operation on raw buffer in expression}}
   *q + 1, 1 + *q, *q - 1,   // expected-warning3{{unchecked operation on raw buffer in expression}}
@@ -227,23 +227,23 @@
 
 void innerInner(int * p) {
   auto Lam = [p]() {
-int * q = p;
-q++;   // expected-warning{{unchecked operation on raw buffer in expression}}
+int * q = p;   // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+q++;   // expected-warning{{unchecked operation on raw buffer in expression}}
 return *q;
   };
 }
   };
 
   auto Lam = [p]() {
-int * q = p;
-q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
+int * q = p;  // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
 return *q;
   };
 
   auto LamLam = [p]() {
 auto Lam = [p]() {
-  int * q = p;
-  q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
+  int * q = p;  // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
   return *q;
 };
   };
@@ -260,8 +260,8 @@
   };
 
   // lambda and block as call arguments...
-  foo( [p]() { int * q = p;
-  q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo( [p]() { int * q = p;  // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  q++;   // expected-warning{{unchecked operation on raw buffer in expression}}
   return *q;
},
^(int *p) { p++;   // expected-warning{{unchecked operation on raw buffer in expression}}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
@@ -0,0 +1,73 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+
+void foo(...);
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+// CHECK: std::span p{new int [10], 10};
+// CHECK: p[5] = 5;
+  int *p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  p[5] = 5; // expected-warning{{unchecked operation on raw buffer in expression}}
+
+// CHECK: std::span q{new int [10], 10};
+// CHECK: std::span x{new int [10], 10};
+// CHECK: std::span y{new int, 1};
+// CHECK: std::span z{new int [10], 1

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:46
+/// The text indicating that the user needs to provide input there:
+constexpr static const char *const UserFillPlaceHolder = "...";
 } // end namespace clang

Should we rather pick something that is syntactically incorrect in C++ in order 
to prevent accidental silent corruption of the sources?
FWIW Xcode uses `<#placeholder#>` syntax.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp:1
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: cp %s %t.cpp

I am starting to think we should split up our tests to allow for less conflicts 
between patches.
Could we please rename the file to a more specific name e. g. 
`warn-unsafe-buffer-usage-fixits-local-var-span.cpp`?



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp:1
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: cp %s %t.cpp

jkorous wrote:
> I am starting to think we should split up our tests to allow for less 
> conflicts between patches.
> Could we please rename the file to a more specific name e. g. 
> `warn-unsafe-buffer-usage-fixits-local-var-span.cpp`?
Also, let's check only correctness of the FixIts in this test.
I would remove `RUN` lines with `-verify` and `expected-warnings`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D139737#4037455 , @ziqingluo-90 
wrote:

> Since we do not have any `FixableGadget` to trigger fix-its at this point,  I 
> let fix-its of local variable declarations always be emitted for the purpose 
> of testing.

It sounds to me as if by landing the patch we'll temporarily make the compiler 
emit incorrect fixes. I think we should avoid doing that. Is it possible to 
wait until our first proof-of-concept `FixableGadget` lands before landing this 
patch? I think there shouldn't be too much conflict between such patches.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D139737#4042093 , @NoQ wrote:

> It sounds to me as if by landing the patch we'll temporarily make the 
> compiler emit incorrect fixes. I think we should avoid doing that. Is it 
> possible to wait until our first proof-of-concept `FixableGadget` lands 
> before landing this patch? I think there shouldn't be too much conflict 
> between such patches.

Any patch with `FixableGadget` will face the same problem - i. e. we still 
won't emit the fixit until this patch has landed.
We decided to include a simple `Fixable` in this patch to unlock testing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:46
+/// The text indicating that the user needs to provide input there:
+constexpr static const char *const UserFillPlaceHolder = "...";
 } // end namespace clang

jkorous wrote:
> Should we rather pick something that is syntactically incorrect in C++ in 
> order to prevent accidental silent corruption of the sources?
> FWIW Xcode uses `<#placeholder#>` syntax.
Correction - it is not Xcode, it is clang itself.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CodeCompleteConsumer.cpp#L323


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:46
+/// The text indicating that the user needs to provide input there:
+constexpr static const char *const UserFillPlaceHolder = "...";
 } // end namespace clang

jkorous wrote:
> jkorous wrote:
> > Should we rather pick something that is syntactically incorrect in C++ in 
> > order to prevent accidental silent corruption of the sources?
> > FWIW Xcode uses `<#placeholder#>` syntax.
> Correction - it is not Xcode, it is clang itself.
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CodeCompleteConsumer.cpp#L323
It looks like we could put different messages in between `<#` and `#>` to hint 
users in different situations.   I'll make this member a function then.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: jkorous, NoQ, t-rasmud, malavikasamak, 
aaron.ballman, xazax.hun, gribozavr.
Herald added a subscriber: rnkovacs.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Use clang fix-its to transform declarations of local variables, which are used 
for buffer access , to be of `std::span` type.

We placed a few limitations to keep the solution simple:

1. it only transforms local variable declarations (no parameter declaration)
2. it only considers single level pointers, i.e., pointers of type `T *` 
regardless of whether `T` is again a pointer.
3. it only transforms to `std::span` types (no `std::array`, or 
`std::span::iterator`, or ...)
4. it can only transform a `VarDecl` that belongs to a `DeclStmt` whose has a 
single child.

One of the purposes of keeping this patch simple enough is to first evaluate if 
fix-it is an appropriate approach to do the transformation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.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
@@ -69,19 +69,19 @@
 
   auto ap1 = a; // expected-warning{{variable 'ap1' participates in unchecked buffer operations}}
 
-  foo(ap1[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap1[1]);
 
   auto ap2 = p; // expected-warning{{variable 'ap2' participates in unchecked buffer operations}}
 
-  foo(ap2[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap2[1]);
 
-  auto ap3 = pp;
+  auto ap3 = pp; // expected-warning{{variable 'ap3' participates in unchecked buffer operations}}
 
-  foo(ap3[1][1]); // expected-warning2{{unchecked operation on raw buffer in expression}}
+  foo(ap3[1][1]); // expected-warning{{unchecked operation on raw buffer in expression}}
 
   auto ap4 = *pp; // expected-warning{{variable 'ap4' participates in unchecked buffer operations}}
 
-  foo(ap4[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(ap4[1]);
 }
 
 void testQualifiedParameters(const int * p, const int * const q,
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
@@ -3,16 +3,71 @@
 // RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
 // RUN: grep -v CHECK %t.cpp | FileCheck %s
 
+void foo(...);
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
 void local_array_subscript_simple() {
-// CHECK: std::span p = new int[10];
+// CHECK: std::span p{new int [10], 10};
 // CHECK: p[5] = 5;
   int *p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
   p[5] = 5;
+
+// CHECK: std::span q{new int [10], 10};
+// CHECK: std::span x{new int [10], 10};
+// CHECK: std::span y{new int, 1};
+// CHECK: std::span z{new int [10], 10};
+// CHECK: std::span w{new Int_t [10], 10};
+// CHECK: foo(q[5], x[5], y[5], z[5], w[5]);
+  const int *q = new int[10]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  Int_ptr_t x = new int[10];  // expected-warning{{variable 'x' participates in unchecked buffer operations}}
+  Int_ptr_t y = new int;  // expected-warning{{variable 'y' participates in unchecked buffer operations}}
+  Int_t * z = new int[10];// expected-warning{{variable 'z' participates in unchecked buffer operations}}
+  Int_t * w = new Int_t[10];  // expected-warning{{variable 'w' participates in unchecked buffer operations}}
+  foo(q[5], x[5], y[5], z[5], w[5]);
+  // y[5] will crash after being span
 }
 
 void local_array_subscript_auto() {
-// CHECK: std::span p = new int[10];
+// CHECK: std::span p{new int [10], 10};
 // CHECK: p[5] = 5;
   auto p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
   p[5] = 5;
 }
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+// CHECK: std::span p{new int [n], n};
+// CHECK: std::span q{new int [n++], ...};
+// CHECK: foo(p[5], q[5]);
+  int *p = new int[n]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+  foo(p[5], q[5]);
+}
+
+
+void local_ptr_to_array() {
+  int n = 10;
+  int 

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ok so at the conference @ymandel suggested us to use libTooling's Transformers 
API to make our lives easier 
(https://clang.llvm.org/docs/ClangTransformerTutorial.html). I'm fairly certain 
we should at least consider using them, but I haven't looked into it deeply 
enough yet. "Changing the type of a variable" is a problem that ideally should 
be solved only once, because it's a problem that's easy to formulate but 
difficult to "get right". Transformers appear to be a collection of solutions 
to problems of this kind. We should see if they already provide an 
out-of-the-box solution, or if they have bits and pieces of machinery we can 
reuse in our solution.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D139737#3989927 , @NoQ wrote:

> Ok so at the conference @ymandel suggested us to use libTooling's 
> Transformers API to make our lives easier 
> (https://clang.llvm.org/docs/ClangTransformerTutorial.html). I'm fairly 
> certain we should at least consider using them, but I haven't looked into it 
> deeply enough yet. "Changing the type of a variable" is a problem that 
> ideally should be solved only once, because it's a problem that's easy to 
> formulate but difficult to "get right". Transformers appear to be a 
> collection of solutions to problems of this kind. We should see if they 
> already provide an out-of-the-box solution, or if they have bits and pieces 
> of machinery we can reuse in our solution.



1. Transformer is good when you are basing your changes around AST matchers.  
It is a collection of combinators over the `MatchResult` type. From what I'm 
seeing here, though, you don't seem to be basing it on AST matchers, in which 
case the libraries are not as useful.
2. We (google) have a tool for changing the type of a variable which takes into 
account all cascading changes to other declarations that result from that type 
change. It is built on Transformer and another library that builds a graph of 
relationships in the AST, relevant to the type-rewriting problem.  
Unfortunately, that's all internal -- not for IP reasons but simply because we 
never had reason to upstream it (nor an obvioius place to put it). So, I'd say 
that this advanced tooling is useful here if you want to _selectively_ change 
type T to type S, so you need tooling to tell you specifically which S's to 
update. If you're changing all T to S, then the problem is simpler.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D139737#3991699 , @ymandel wrote:

> 2. We (google) have a tool for changing the type of a variable which takes 
> into account all cascading changes to other declarations that result from 
> that type change. It is built on Transformer and another library that builds 
> a graph of relationships in the AST, relevant to the type-rewriting problem.  
> Unfortunately, that's all internal -- not for IP reasons but simply because 
> we never had reason to upstream it (nor an obvioius place to put it). So, I'd 
> say that this advanced tooling is useful here if you want to _selectively_ 
> change type T to type S, so you need tooling to tell you specifically which 
> S's to update. If you're changing all T to S, then the problem is simpler.

Yeah we want to be selective, so in our case there's an extra analysis step 
between noticing that the variable is of a certain type and realizing that we 
want to change it to a different type.

I guess I'm curious if there are smaller building blocks of your machine that 
we could reuse. Like, for instance, the procedure of updating the type of the 
variable (without even updating all uses): it's a somewhat self-contained 
problem, and it's difficult enough on its own (you need to split up 
multi-variable `DeclStmt`s in two or three, find the right source locations for 
everything, and so on). I wonder if we could avoid reinventing the wheel there 
somehow.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D139737#3993813 , @NoQ wrote:

> In D139737#3991699 , @ymandel wrote:
>
>> 2. We (google) have a tool for changing the type of a variable which takes 
>> into account all cascading changes to other declarations that result from 
>> that type change. It is built on Transformer and another library that builds 
>> a graph of relationships in the AST, relevant to the type-rewriting problem. 
>>  Unfortunately, that's all internal -- not for IP reasons but simply because 
>> we never had reason to upstream it (nor an obvioius place to put it). So, 
>> I'd say that this advanced tooling is useful here if you want to 
>> _selectively_ change type T to type S, so you need tooling to tell you 
>> specifically which S's to update. If you're changing all T to S, then the 
>> problem is simpler.
>
> Yeah we want to be selective, so in our case there's an extra analysis step 
> between noticing that the variable is of a certain type and realizing that we 
> want to change it to a different type.
>
> I guess I'm curious if there are smaller building blocks of your machine that 
> we could reuse. Like, for instance, the procedure of updating the type of the 
> variable (without even updating all uses): it's a somewhat self-contained 
> problem, and it's difficult enough on its own (you need to split up 
> multi-variable `DeclStmt`s in two or three, find the right source locations 
> for everything, and so on). I wonder if we could avoid reinventing the wheel 
> there somehow.

Sure. I'll try to get you some sample code later today.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:127
+static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) 
{
+  auto isLvalueToRvalueCast = [](internal::Matcher M) {
+return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),

Nit: I think we could simplify this to just

```
static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) {
  return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
  castSubExpr(innerMatcher));
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 marked 8 inline comments as done.
ziqingluo-90 added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:637
 
-static Strategy
-getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) {

NoQ wrote:
> Hmm, did this need to be moved? I don't think you're calling this function 
> from the new code.
it does look like I moved it.  Will change it back.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:780-781
+  } else {
+// In cases `Init` is of the form `&Var` after stripping of implicit
+// casts, where `&` is the built-in operator, the extent is 1.
+if (auto AddrOfExpr = dyn_cast(Init->IgnoreImpCasts()))

NoQ wrote:
> ```lang=c
> int x = 1;
> char *ptr = &x; // std::span ptr { &x, 4 };
> ```
> This is valid code. I suspect we want to check types as well, to see that 
> type sizes match.
> 
> Most of the time code like this violates strict aliasing, but `char` is 
> exceptional, and even if it did violate strict aliasing, people can compile 
> with `-fno-strict-aliasing` to define away the UB, so we have to respect that.
This code is not valid in C++.  An explicit cast is needed in front of `&x`.  I 
will add a test to show that 

```
int x = 1;
char * ptr = (char *)&x;
```
will have a place holder for the span size.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 492882.
ziqingluo-90 added a comment.

Addressing comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,149 @@
+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+// CHECK: std::span p {new int[10], 10};
+// CHECK: std::span q {new int[10], 10};
+// CHECK: tmp = p[5];
+// CHECK: tmp = q[5];
+  int *p = new int[10];
+  const int *q = new int[10];
+  tmp = p[5];
+  tmp = q[5];
+
+// CHECK: std::span x {new int[10], 10};
+// CHECK: std::span y {new int, 1};
+// CHECK: std::span z {new int[10], 10};
+// CHECK: std::span w {new Int_t[10], 10};
+
+  Int_ptr_t x = new int[10];
+  Int_ptr_t y = new int;
+  Int_t * z = new int[10];
+  Int_t * w = new Int_t[10];
+
+  // CHECK: tmp = x[5];
+  tmp = x[5];
+  // CHECK: tmp = y[5];
+  tmp = y[5]; // y[5] will crash after being span
+  // CHECK: tmp = z[5];
+  tmp = z[5];
+  // CHECK: tmp = w[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  int tmp;
+// CHECK: std::span p {new int[10], 10};
+// CHECK: tmp = p[5];
+  auto p = new int[10];
+  tmp = p[5];
+}
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+  int tmp;
+
+  // CHECK: std::span p {new int[n], n};
+  // CHECK: std::span q {new int[n++], <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = new int[n];
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++];
+  tmp = p[5];
+  tmp = q[5];
+}
+
+
+void local_ptr_to_array() {
+  int tmp;
+  int n = 10;
+  int a[10];
+  int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  // CHECK: std::span p {a, 10};
+  // CHECK: std::span q {b, <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = a;
+  int *q = b;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+  int var;
+// CHECK: std::span q {&var, 1};
+// CHECK: var = q[5];
+  int * q = &var;
+  // This expression involves unsafe buffer accesses, which will crash
+  // at runtime after applying the fix-it,
+  var = q[5];
+}
+
+void decl_without_init() {
+  int tmp;
+  // CHECK: std::span p;
+  int * p;
+  // CHECK: std::span q;
+  Int_ptr_t q;
+
+  tmp = p[5];
+  tmp = q[5];
+}
+
+// Explicit casts are required in the following cases. No way to
+// figure out span extent for them automatically.
+void explict_cast() {
+  int tmp;
+  // CHECK: std::span p {(int*) new int[10][10], <# placeholder #>};
+  int * p = (int*) new int[10][10];
+  tmp = p[5];
+
+  int a;
+  // CHECK: std::span q {(char *)&a, <# placeholder #>};
+  char * q = (char *)&a;
+  tmp = (int) q[5];
+
+  void * r = &a;
+  // CHECK: std::span s {(char *) r, <# placeholder #>};
+  char * s = (char *) r;
+  tmp = (int) s[5];
+}
+
+void unsupported_multi_decl(int * x) {
+  // CHECK: int * p = x, * q = new int[10];
+  int * p = x, * q = new int[10];
+  *p = q[5];
+}
+
+void unsupported_fixit_overlapping_macro(int * x) {
+  int tmp;
+  // In the case below, a tentative fix-it replaces `MY_INT * p =` with `std::span p `.
+  // It overlaps with the use of the macro `MY_INT`.  The fix-it is
+  // discarded then.
+#define MY_INT int
+  // CHECK: MY_INT * p = new int[10];
+  MY_INT * p = new int[10];
+  tmp = p[5];
+
+#define MY_VAR(name) int * name
+  // CHECK: MY_VAR(q) = new int[10];
+  MY_VAR(q) = new int[10];
+  tmp = q[5];
+
+  // In cases where fix-its do not change the original code where
+  // macros are used, those fix-its will be emitted.  For example,
+  // fixits are inserted before and after `new MY_INT[MY_TEN]` below.
+#define MY_TEN 10
+  // CHECK: std::span g {new MY_INT[MY_TEN], <# placeholder #>};
+  int * g = new MY_INT[MY_TEN];
+  tmp = g[5];
+
+#undef MY_INT
+#undef MY_VAR
+#undef MY_TEN
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -9,6 +9,7 @@
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallVector.h"
 #include 
 #include 
@@ -115,6 +116

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thank you!! I think we're almost ready to commit but this concern is still 
hanging:

> I see that the patch doesn't touch `handleFixableVariable()`. Do we still 
> attach fixits to the warning, or did we already change it to be attached to a 
> note associated with the warning? Because fixits with placeholders aren't 
> allowed on warnings, we should make sure it's attached to note before landing 
> this patch.




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:780-781
+  } else {
+// In cases `Init` is of the form `&Var` after stripping of implicit
+// casts, where `&` is the built-in operator, the extent is 1.
+if (auto AddrOfExpr = dyn_cast(Init->IgnoreImpCasts()))

ziqingluo-90 wrote:
> NoQ wrote:
> > ```lang=c
> > int x = 1;
> > char *ptr = &x; // std::span ptr { &x, 4 };
> > ```
> > This is valid code. I suspect we want to check types as well, to see that 
> > type sizes match.
> > 
> > Most of the time code like this violates strict aliasing, but `char` is 
> > exceptional, and even if it did violate strict aliasing, people can compile 
> > with `-fno-strict-aliasing` to define away the UB, so we have to respect 
> > that.
> This code is not valid in C++.  An explicit cast is needed in front of `&x`.  
> I will add a test to show that 
> 
> ```
> int x = 1;
> char * ptr = (char *)&x;
> ```
> will have a place holder for the span size.
Yes you're right! It's only valid in C where these fixits don't apply.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

I am sorry I haven't notice this earlier - let's fix this before we land the 
patch.




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:690
+  Val.toString(Txt, 10, true);
+  return Txt.data();
+}

We either need a zero to terminate the string or pass the size of `Txt` to the 
`std::string` constructor here. (While `toString`'s name might sound like it'll 
take care of that it does not.)

Simplified testcase:
```
void local_ptr_to_array() {
  int tmp;
  int a[10];
  int *p = a;
  tmp = p[5];
}
```
what I get is (something like this):
```
void local_ptr_to_array() {
  int tmp;
  int a[10];
  std::span p {a, 10�o
};
  tmp = p[5];
}
```
The problem is that `APInt::toString` stores '1' and '0' to `Txt` but is 
missing the terminating `\0` character that `std::string` constructor expects.




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:690
+  Val.toString(Txt, 10, true);
+  return Txt.data();
+}

jkorous wrote:
> We either need a zero to terminate the string or pass the size of `Txt` to 
> the `std::string` constructor here. (While `toString`'s name might sound like 
> it'll take care of that it does not.)
> 
> Simplified testcase:
> ```
> void local_ptr_to_array() {
>   int tmp;
>   int a[10];
>   int *p = a;
>   tmp = p[5];
> }
> ```
> what I get is (something like this):
> ```
> void local_ptr_to_array() {
>   int tmp;
>   int a[10];
>   std::span p {a, 10�o
};
>   tmp = p[5];
> }
> ```
> The problem is that `APInt::toString` stores '1' and '0' to `Txt` but is 
> missing the terminating `\0` character that `std::string` constructor expects.
> 



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:694
+// Return text representation of an `Expr`.  In case unable to get the text
+// representatiion of `E`, return `Default`.
+static StringRef getExprTextOr(const Expr *E, const SourceManager &SM,




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 494053.
ziqingluo-90 marked 2 inline comments as done.
ziqingluo-90 added a comment.

To attach fix-its to notes instead of warnings.
Fix the ending '\0' issue raised by @jkorous


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-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
@@ -80,20 +80,24 @@
 
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
-  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
-  foo(ap1[1]);// expected-note{{used in buffer access here}}
+  foo(ap1[1]);// expected-note{{used in buffer access here}} 
 
-  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}}
+  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
 
   foo(ap2[1]);// expected-note{{used in buffer access here}}
 
-  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}}
+  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
 
   foo(ap3[1][1]); // expected-note{{used in buffer access here}}
   // expected-warning@-1{{unsafe buffer access}}
 
-  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}}
+  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
 
   foo(ap4[1]);// expected-note{{used in buffer access here}}
 }
@@ -355,7 +359,8 @@
   auto
 
 
-  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ 	   expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
   foo(ap1[1]);  // expected-note{{used in buffer access here}}
 }
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,168 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  const int *q = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span x"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+  Int_ptr_t y = new int;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span y"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
+  Int_t * z = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span z"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+  Int_t * w = new Int_t[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span w"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+
+  tmp = x[5];
+  tmp = y[5]; // y[5] will crash after being span
+  tmp = z[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  i

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 494075.
ziqingluo-90 added a comment.

rebase


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-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
@@ -80,20 +80,24 @@
 
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
-  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
-  foo(ap1[1]);// expected-note{{used in buffer access here}}
+  foo(ap1[1]);// expected-note{{used in buffer access here}} 
 
-  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}}
+  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
 
   foo(ap2[1]);// expected-note{{used in buffer access here}}
 
-  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}}
+  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
 
   foo(ap3[1][1]); // expected-note{{used in buffer access here}}
   // expected-warning@-1{{unsafe buffer access}}
 
-  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}}
+  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
 
   foo(ap4[1]);// expected-note{{used in buffer access here}}
 }
@@ -355,7 +359,8 @@
   auto
 
 
-  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ 	   expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
   foo(ap1[1]);  // expected-note{{used in buffer access here}}
 }
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,168 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  const int *q = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span x"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+  Int_ptr_t y = new int;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span y"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
+  Int_t * z = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span z"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+  Int_t * w = new Int_t[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span w"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+
+  tmp = x[5];
+  tmp = y[5]; // y[5] will crash after being span
+  tmp = z[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  int tmp;
+  auto p = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK: fix-it:"{{.*}

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

I got a test failure in 
`SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp` which I believe is 
caused solely by the fact that we emit the diagnostics in different order.
I am not sure it matters and since the Fix-Its clearly specify what source 
lines they apply to I suggest we simply replace every `// CHECK:` with `// 
CHECK-DAG:`.
That fixed the problem for me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:894
+if (VD->getType()->isPointerType())
+  return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
+return {};

I believe we should add another condition here: `VD->isLocalVarDecl()` as we 
don't support globals (yet?).
We run the matcher with `any_ds` tag only on function bodies so we won't 
discover globals anyway and the `assert(It != Defs.end() && "Definition never 
discovered!");` would fail.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:703
+  case Strategy::Kind::Span:
+return FixItList{};
+  case Strategy::Kind::Wontfix:

I am afraid I might have found one more problem :(
I believe that for `span` strategy we have to make sure the index is > 0. 
Otherwise 
That means either an unsigned integer or signed or unsigned literal that is 
greater than 0.
For the literal you can take inspiration here:
https://reviews.llvm.org/D142795



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 494689.
ziqingluo-90 added a comment.

address comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-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
@@ -80,20 +80,24 @@
 
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
-  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
-  foo(ap1[1]);// expected-note{{used in buffer access here}}
+  foo(ap1[1]);// expected-note{{used in buffer access here}} 
 
-  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}}
+  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
 
   foo(ap2[1]);// expected-note{{used in buffer access here}}
 
-  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}}
+  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
 
   foo(ap3[1][1]); // expected-note{{used in buffer access here}}
   // expected-warning@-1{{unsafe buffer access}}
 
-  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}}
+  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
 
   foo(ap4[1]);// expected-note{{used in buffer access here}}
 }
@@ -355,7 +359,8 @@
   auto
 
 
-  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ 	   expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
   foo(ap1[1]);  // expected-note{{used in buffer access here}}
 }
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,175 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  const int *q = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span x"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+  Int_ptr_t y = new int;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span y"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
+  Int_t * z = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span z"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+  Int_t * w = new Int_t[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span w"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+
+  tmp = x[5];
+  tmp = y[5]; // y[5] will crash after being span
+  tmp = z[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  int tmp;
+  auto p = new int[10];
+  // CHECK-DAG: f

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

As far as I'm concerned, I think this is great for the initial implementation! 
Let's commit as soon as Jan confirms that his problem is addressed.




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:894
+if (VD->getType()->isPointerType())
+  return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
+return {};

jkorous wrote:
> I believe we should add another condition here: `VD->isLocalVarDecl()` as we 
> don't support globals (yet?).
> We run the matcher with `any_ds` tag only on function bodies so we won't 
> discover globals anyway and the `assert(It != Defs.end() && "Definition never 
> discovered!");` would fail.
I think this check should happen much earlier. Like, we shouldn't define a 
strategy for globals, and we shouldn't build fixables out of them. And then 
`assert()` here, just to double-check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:703
+  case Strategy::Kind::Span:
+return FixItList{};
+  case Strategy::Kind::Wontfix:

jkorous wrote:
> I am afraid I might have found one more problem :(
> I believe that for `span` strategy we have to make sure the index is > 0. 
> Otherwise 
> That means either an unsigned integer or signed or unsigned literal that is 
> greater than 0.
> For the literal you can take inspiration here:
> https://reviews.llvm.org/D142795
> 
@ziqingluo-90 Sorry, looks like I wasn't clear here.
One case (that you've already addressed) is `ptr[-5]` - for that we can't use 
`std::span::operator[]` as it would immediately trap.
But there's the other case of:
```
uint8_t foo(uint8_t *ptr, int idx) {
  return ptr[idx]
}
```
If anyone uses a value that's both signed and not a compile-time constant then 
our compile-time analysis can not prove that the index is always >= 0 and 
consequently we can't use `std::span::operator[]` as a replacement.
That's why I think we really need to make sure that the index is ether a) 
positive literal or b) unsigned.
WDYT?




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:703
+  case Strategy::Kind::Span:
+return FixItList{};
+  case Strategy::Kind::Wontfix:

jkorous wrote:
> jkorous wrote:
> > I am afraid I might have found one more problem :(
> > I believe that for `span` strategy we have to make sure the index is > 0. 
> > Otherwise 
> > That means either an unsigned integer or signed or unsigned literal that is 
> > greater than 0.
> > For the literal you can take inspiration here:
> > https://reviews.llvm.org/D142795
> > 
> @ziqingluo-90 Sorry, looks like I wasn't clear here.
> One case (that you've already addressed) is `ptr[-5]` - for that we can't use 
> `std::span::operator[]` as it would immediately trap.
> But there's the other case of:
> ```
> uint8_t foo(uint8_t *ptr, int idx) {
>   return ptr[idx]
> }
> ```
> If anyone uses a value that's both signed and not a compile-time constant 
> then our compile-time analysis can not prove that the index is always >= 0 
> and consequently we can't use `std::span::operator[]` as a replacement.
> That's why I think we really need to make sure that the index is ether a) 
> positive literal or b) unsigned.
> WDYT?
> 
> 
And yes ... I was wrong - literal `0` is totally fine. Thanks for spotting that!



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:894
+if (VD->getType()->isPointerType())
+  return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
+return {};

NoQ wrote:
> jkorous wrote:
> > I believe we should add another condition here: `VD->isLocalVarDecl()` as 
> > we don't support globals (yet?).
> > We run the matcher with `any_ds` tag only on function bodies so we won't 
> > discover globals anyway and the `assert(It != Defs.end() && "Definition 
> > never discovered!");` would fail.
> I think this check should happen much earlier. Like, we shouldn't define a 
> strategy for globals, and we shouldn't build fixables out of them. And then 
> `assert()` here, just to double-check.
Fair enough, we can do that. Maybe a follow-up patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 495250.
ziqingluo-90 added a comment.

Fixed a bug in manipulating source locations:

Using `SourceLocation::getLocWithOffset` to get a new source location `L` with 
a relative offset is convenient. But it requires extra care to make sure that 
`L` has the same file ID as the location where `L` is originated.  Otherwise, 
`L` may be associated with a wrong file ID (or malloc ID).  Feeding such an `L` 
to a `Lexer` could cause failures.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-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
@@ -80,20 +80,24 @@
 
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
-  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
   foo(ap1[1]);// expected-note{{used in buffer access here}}
 
-  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}}
+  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
 
   foo(ap2[1]);// expected-note{{used in buffer access here}}
 
-  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}}
+  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
 
   foo(ap3[1][1]); // expected-note{{used in buffer access here}}
   // expected-warning@-1{{unsafe buffer access}}
 
-  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}}
+  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
 
   foo(ap4[1]);// expected-note{{used in buffer access here}}
 }
@@ -355,7 +359,8 @@
   auto
 
 
-  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ 	   expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
   foo(ap1[1]);  // expected-note{{used in buffer access here}}
 }
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,175 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  const int *q = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span x"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+  Int_ptr_t y = new int;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span y"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
+  Int_t * z = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span z"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+  Int_t * w = new Int_t[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span w"
+  // CHECK-DAG: fix

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 495263.
ziqingluo-90 added a comment.

To avoid emitting incorrect fix-its for array subscripts on `span` objects:

- For unsafe operations of the form `s[e]` where `s` is being transformed to be 
a `span`,  we only emit fix-its for `s[e]` when `e` is a non-negative integer 
literal or `e` has an unsigned integer type.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-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
@@ -80,20 +80,24 @@
 
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
-  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
-  foo(ap1[1]);// expected-note{{used in buffer access here}}
+  foo(ap1[1]);// expected-note{{used in buffer access here}} 
 
-  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}}
+  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
 
   foo(ap2[1]);// expected-note{{used in buffer access here}}
 
-  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}}
+  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
 
   foo(ap3[1][1]); // expected-note{{used in buffer access here}}
   // expected-warning@-1{{unsafe buffer access}}
 
-  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}}
+  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
 
   foo(ap4[1]);// expected-note{{used in buffer access here}}
 }
@@ -355,7 +359,8 @@
   auto
 
 
-  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ 	   expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
   foo(ap1[1]);  // expected-note{{used in buffer access here}}
 }
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,192 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  const int *q = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span x"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+  Int_ptr_t y = new int;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span y"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
+  Int_t * z = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span z"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+  Int_t * w = new Int_t[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span w"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:703
+  case Strategy::Kind::Span:
+return FixItList{};
+  case Strategy::Kind::Wontfix:

jkorous wrote:
> jkorous wrote:
> > jkorous wrote:
> > > I am afraid I might have found one more problem :(
> > > I believe that for `span` strategy we have to make sure the index is > 0. 
> > > Otherwise 
> > > That means either an unsigned integer or signed or unsigned literal that 
> > > is greater than 0.
> > > For the literal you can take inspiration here:
> > > https://reviews.llvm.org/D142795
> > > 
> > @ziqingluo-90 Sorry, looks like I wasn't clear here.
> > One case (that you've already addressed) is `ptr[-5]` - for that we can't 
> > use `std::span::operator[]` as it would immediately trap.
> > But there's the other case of:
> > ```
> > uint8_t foo(uint8_t *ptr, int idx) {
> >   return ptr[idx]
> > }
> > ```
> > If anyone uses a value that's both signed and not a compile-time constant 
> > then our compile-time analysis can not prove that the index is always >= 0 
> > and consequently we can't use `std::span::operator[]` as a replacement.
> > That's why I think we really need to make sure that the index is ether a) 
> > positive literal or b) unsigned.
> > WDYT?
> > 
> > 
> And yes ... I was wrong - literal `0` is totally fine. Thanks for spotting 
> that!
I think you are right.  Fixed it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous accepted this revision.
jkorous added a comment.

LGTM
Thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa29e67614c3b: [-Wunsafe-buffer-usage] Generate fix-it for 
local variable declarations (authored by ziqingluo-90).

Changed prior to commit:
  https://reviews.llvm.org/D139737?vs=495263&id=495630#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-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
@@ -80,20 +80,24 @@
 
 void testArraySubscriptsWithAuto(int *p, int **pp) {
   int a[10];
-  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  auto ap1 = a;   // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
-  foo(ap1[1]);// expected-note{{used in buffer access here}}
+  foo(ap1[1]);// expected-note{{used in buffer access here}} 
 
-  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}}
+  auto ap2 = p;   // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
 
   foo(ap2[1]);// expected-note{{used in buffer access here}}
 
-  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}}
+  auto ap3 = pp;  // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
+		 expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
 
   foo(ap3[1][1]); // expected-note{{used in buffer access here}}
   // expected-warning@-1{{unsafe buffer access}}
 
-  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}}
+  auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
+  		 expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
 
   foo(ap4[1]);// expected-note{{used in buffer access here}}
 }
@@ -355,7 +359,8 @@
   auto
 
 
-  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+  ap1 = p;  // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ 	   expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
 
   foo(ap1[1]);  // expected-note{{used in buffer access here}}
 }
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,192 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  const int *q = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span x"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+  Int_ptr_t y = new int;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span y"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
+  Int_t * z = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span z"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+  Int_t * w = new Int_t[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span w"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 490648.
ziqingluo-90 added a comment.

Rebased the code w.r.t. a series of refactoring in [-Wunsafe-buffer-usage].
Added a `FixableGadget` for array subscripts of the form `DRE[*]` in the 
context of lvalue-to-rvalue casting.

Also did a refactoring at the place where matchers are applied:  Matchers of a 
`FixableGadget` and  of a `WarningGadget` cat match for the same AST node.  So 
they should not be put in an `anyOf` group, otherwise they can disable each 
other due to short-circuiting.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,86 @@
+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+// CHECK: std::span p{new int [10], 10};
+// CHECK: std::span q{new int [10], 10};
+// CHECK: tmp = p[5];
+// CHECK: tmp = q[5];
+  int *p = new int[10];
+  const int *q = new int[10];
+  tmp = p[5];
+  tmp = q[5];
+
+// CHECK: std::span x{new int [10], 10};
+// CHECK: std::span y{new int, 1};
+// CHECK: std::span z{new int [10], 10};
+// CHECK: std::span w{new Int_t [10], 10};
+
+  Int_ptr_t x = new int[10];
+  Int_ptr_t y = new int;
+  Int_t * z = new int[10];
+  Int_t * w = new Int_t[10];
+
+  // CHECK: tmp = x[5];
+  tmp = x[5];
+  // CHECK: tmp = y[5];
+  tmp = y[5]; // y[5] will crash after being span
+  // CHECK: tmp = z[5];
+  tmp = z[5];
+  // CHECK: tmp = w[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  int tmp;
+// CHECK: std::span p{new int [10], 10};
+// CHECK: tmp = p[5];
+  auto p = new int[10];
+  tmp = p[5];
+}
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+  int tmp;
+  //FIXME: need to think it twice about side effects in the expressions
+  //used to initialize span objects
+  // CHECK: std::span p{new int [n], n};
+  // CHECK: std::span q{new int [n++], <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = new int[n];
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++];
+  tmp = p[5];
+  tmp = q[5];
+}
+
+
+void local_ptr_to_array() {
+  int tmp;
+  int n = 10;
+  int a[10];
+  int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  // CHECK: std::span p{a, 10};
+  // CHECK: std::span q{b, <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = a;
+  int *q = b;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+  int var;
+// CHECK: std::span q{&var, 1};
+// CHECK: var = q[5];
+  int * q = &var;
+  // This expression involves unsafe buffer accesses, which will crash
+  // at runtime after applying the fix-it,
+  var = q[5];
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -115,6 +115,21 @@
   MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);  
   return Visitor.findMatch(DynTypedNode::create(Node));
 }
+
+AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher, innerMatcher) {
+  return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
+}
+
+// Returns a matcher that matches any expression 'e' such that `innerMatcher`
+// matches 'e' and 'e' is in an Unspecified Lvalue Context.
+static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) {
+  auto isLvalueToRvalueCast = [](internal::Matcher M) {
+return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
+castSubExpr(M));
+  };
+  //FIXME: add assignmentTo context...
+  return isLvalueToRvalueCast(innerMatcher);
+}
 } // namespace clang::ast_matchers
 
 namespace {
@@ -282,7 +297,7 @@
 /// Array subscript expressions on raw pointers as if they're arrays. Unsafe as
 /// it doesn't have any bounds checks for the array.
 class ArraySubscriptGadget : public WarningGadget {
-  static constexpr const char *const ArraySubscrTag = "arraySubscr";
+  static constexpr const char *const ArraySubscrTag = "ArraySubscript";
   const ArraySubscriptExpr *ASE;
 
 public:
@@ -366,6 +381,51 @@
   // FIXME: pointer adding zero should be fine
   //FIXME: this gadge will need a fix-it
 };
+
+class Strategy;
+
+// Represents ex

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:385
+
+class Strategy;
+

There's already a forward declaration on line 144!



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:412
+arraySubscriptExpr(BaseIsArrayOrPtrDRE,
+   unless(hasIndex(integerLiteral(equals(0)
+.bind(ULCArraySubscriptTag);

Subscripts `[0]` might be safe, but this doesn't mean we should avoid emitting 
fixits when we see them. I think this clause should be dropped here. (Might be 
worth adding a test).



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:723
+  OS << "{";
+  Init->printPretty(OS, nullptr, PP);
+  OS << ", ";

Uhh, I'm worried about this approach. I'm not sure these pretty-printers are 
even correct. They try to look similar to the underlying code, but I don't 
think they're required to. And even if they were, It only takes one forgotten 
override in `StmtPrinter` to produce incorrect pretty-prints that won't 
compile. So it's very unreliable technology.

I think the intended way to do these things is to simply avoid overwriting code 
that you want to preserve. Instead, modify code around it. Eg., if you want to 
change
```lang=c++
int *x = foo();
```
to
```lang=c++
std::span x { foo(), N };
```
then you add `std::span<` before `int`, then don't touch `int`, then replace ` 
*` with `> `, then replace `=` with ` {`, then don't touch `foo()`, then add `, 
N }` immediately after.

If you actually need to move code around (eg., make preserved chunks appear in 
a different order), I think this should go through a facility like 
`Lexer::getSourceText()` which would give you the exact source code as written.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 491581.
ziqingluo-90 added a comment.

Refactored the fix-it generation code to stop using the pretty-printer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,96 @@
+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+// CHECK: std::span p {new int[10], 10};
+// CHECK: std::span q {new int[10], 10};
+// CHECK: tmp = p[5];
+// CHECK: tmp = q[5];
+  int *p = new int[10];
+  const int *q = new int[10];
+  tmp = p[5];
+  tmp = q[5];
+
+// CHECK: std::span x {new int[10], 10};
+// CHECK: std::span y {new int, 1};
+// CHECK: std::span z {new int[10], 10};
+// CHECK: std::span w {new Int_t[10], 10};
+
+  Int_ptr_t x = new int[10];
+  Int_ptr_t y = new int;
+  Int_t * z = new int[10];
+  Int_t * w = new Int_t[10];
+
+  // CHECK: tmp = x[5];
+  tmp = x[5];
+  // CHECK: tmp = y[5];
+  tmp = y[5]; // y[5] will crash after being span
+  // CHECK: tmp = z[5];
+  tmp = z[5];
+  // CHECK: tmp = w[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  int tmp;
+// CHECK: std::span p {new int[10], 10};
+// CHECK: tmp = p[5];
+  auto p = new int[10];
+  tmp = p[5];
+}
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+  int tmp;
+
+  // CHECK: std::span p {new int[n], n};
+  // CHECK: std::span q {new int[n++], <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = new int[n];
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++];
+  tmp = p[5];
+  tmp = q[5];
+}
+
+
+void local_ptr_to_array() {
+  int tmp;
+  int n = 10;
+  int a[10];
+  int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  // CHECK: std::span p {a, 10};
+  // CHECK: std::span q {b, <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = a;
+  int *q = b;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+  int var;
+// CHECK: std::span q {&var, 1};
+// CHECK: var = q[5];
+  int * q = &var;
+  // This expression involves unsafe buffer accesses, which will crash
+  // at runtime after applying the fix-it,
+  var = q[5];
+}
+
+void decl_without_init() {
+  int tmp;
+  // CHECK: std::span p;
+  int * p;
+  // CHECK: std::span q;
+  Int_ptr_t q;
+
+  tmp = p[5];
+  tmp = q[5];
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -9,6 +9,7 @@
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallVector.h"
 #include 
 #include 
@@ -115,6 +116,21 @@
   MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);  
   return Visitor.findMatch(DynTypedNode::create(Node));
 }
+
+AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher, innerMatcher) {
+  return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
+}
+
+// Returns a matcher that matches any expression 'e' such that `innerMatcher`
+// matches 'e' and 'e' is in an Unspecified Lvalue Context.
+static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) {
+  auto isLvalueToRvalueCast = [](internal::Matcher M) {
+return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
+castSubExpr(M));
+  };
+  //FIXME: add assignmentTo context...
+  return isLvalueToRvalueCast(innerMatcher);
+}
 } // namespace clang::ast_matchers
 
 namespace {
@@ -282,7 +298,7 @@
 /// Array subscript expressions on raw pointers as if they're arrays. Unsafe as
 /// it doesn't have any bounds checks for the array.
 class ArraySubscriptGadget : public WarningGadget {
-  static constexpr const char *const ArraySubscrTag = "arraySubscr";
+  static constexpr const char *const ArraySubscrTag = "ArraySubscript";
   const ArraySubscriptExpr *ASE;
 
 public:
@@ -366,6 +382,51 @@
   // FIXME: pointer adding zero should be fine
   //FIXME: this gadge will need a fix-it
 };
+
+class Strategy;
+
+// Represents expressions of the form `DRE[*]` in the Unspecified Lvalue
+// Context (see `isInUn

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 491917.
ziqingluo-90 added a comment.

Addressed the minor comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139737/new/

https://reviews.llvm.org/D139737

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,96 @@
+// RUN: cp %s %t.cpp
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
+// RUN: grep -v CHECK %t.cpp | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+// CHECK: std::span p {new int[10], 10};
+// CHECK: std::span q {new int[10], 10};
+// CHECK: tmp = p[5];
+// CHECK: tmp = q[5];
+  int *p = new int[10];
+  const int *q = new int[10];
+  tmp = p[5];
+  tmp = q[5];
+
+// CHECK: std::span x {new int[10], 10};
+// CHECK: std::span y {new int, 1};
+// CHECK: std::span z {new int[10], 10};
+// CHECK: std::span w {new Int_t[10], 10};
+
+  Int_ptr_t x = new int[10];
+  Int_ptr_t y = new int;
+  Int_t * z = new int[10];
+  Int_t * w = new Int_t[10];
+
+  // CHECK: tmp = x[5];
+  tmp = x[5];
+  // CHECK: tmp = y[5];
+  tmp = y[5]; // y[5] will crash after being span
+  // CHECK: tmp = z[5];
+  tmp = z[5];
+  // CHECK: tmp = w[5];
+  tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+  int tmp;
+// CHECK: std::span p {new int[10], 10};
+// CHECK: tmp = p[5];
+  auto p = new int[10];
+  tmp = p[5];
+}
+
+void local_array_subscript_variable_extent() {
+  int n = 10;
+  int tmp;
+
+  // CHECK: std::span p {new int[n], n};
+  // CHECK: std::span q {new int[n++], <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = new int[n];
+  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  int *q = new int[n++];
+  tmp = p[5];
+  tmp = q[5];
+}
+
+
+void local_ptr_to_array() {
+  int tmp;
+  int n = 10;
+  int a[10];
+  int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
+  // CHECK: std::span p {a, 10};
+  // CHECK: std::span q {b, <# placeholder #>};
+  // CHECK: tmp = p[5];
+  // CHECK: tmp = q[5];
+  int *p = a;
+  int *q = b;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+  int var;
+// CHECK: std::span q {&var, 1};
+// CHECK: var = q[5];
+  int * q = &var;
+  // This expression involves unsafe buffer accesses, which will crash
+  // at runtime after applying the fix-it,
+  var = q[5];
+}
+
+void decl_without_init() {
+  int tmp;
+  // CHECK: std::span p;
+  int * p;
+  // CHECK: std::span q;
+  Int_ptr_t q;
+
+  tmp = p[5];
+  tmp = q[5];
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -9,6 +9,7 @@
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallVector.h"
 #include 
 #include 
@@ -115,6 +116,21 @@
   MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);  
   return Visitor.findMatch(DynTypedNode::create(Node));
 }
+
+AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher, innerMatcher) {
+  return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
+}
+
+// Returns a matcher that matches any expression 'e' such that `innerMatcher`
+// matches 'e' and 'e' is in an Unspecified Lvalue Context.
+static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) {
+  auto isLvalueToRvalueCast = [](internal::Matcher M) {
+return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
+castSubExpr(M));
+  };
+  //FIXME: add assignmentTo context...
+  return isLvalueToRvalueCast(innerMatcher);
+}
 } // namespace clang::ast_matchers
 
 namespace {
@@ -282,7 +298,7 @@
 /// Array subscript expressions on raw pointers as if they're arrays. Unsafe as
 /// it doesn't have any bounds checks for the array.
 class ArraySubscriptGadget : public WarningGadget {
-  static constexpr const char *const ArraySubscrTag = "arraySubscr";
+  static constexpr const char *const ArraySubscrTag = "ArraySubscript";
   const ArraySubscriptExpr *ASE;
 
 public:
@@ -366,6 +382,47 @@
   // FIXME: pointer adding zero should be fine
   //FIXME: this gadge will need a fix-it
 };
+
+// Represents expressions of the form `DRE[*]` in the Unspecified Lvalue
+// Context (see `isInUnspecifiedLvalueContext`).
+// Note here `[]` is the built-in s

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Awesome, we're getting closer to producing our first fix!

I think the most important thing right now is to add a lot of 
`SourceLocation::isMacroID()` checks everywhere, so that to *never* try to fix 
code that's fully or partially expanded from macros. I suspect that we can do 
that "after the fact": just scan the list of emitted fixits for any source 
ranges that start or end in macros, and if even one such range is found, 
abandon the fix. Might be worth double-checking that the compiler doesn't 
already do that automatically for all emitted fixits.

I see that the patch doesn't touch `handleFixableVariable()`. Do we still 
attach fixits to the warning, or did we already change it to be attached to a 
note associated with the warning? Because fixits with placeholders aren't 
allowed on warnings, we should make sure it's attached to note before landing 
this patch.




Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:42
+  /// Returns the text indicating that the user needs to provide input there:
+  static std::string
+  getUserFillPlaceHolder(const StringRef &HintTextToUser = "placeholder") {

Do we really want this method in the public interface? If the idea is to let 
people the user override it, maybe let's actually allow that by dropping 
`static` and adding `virtual`? I think this is actually a good idea, maybe if 
somebody uses our analysis outside of the `clang` binary, they may want a 
different placeholder.



Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:43
+  static std::string
+  getUserFillPlaceHolder(const StringRef &HintTextToUser = "placeholder") {
+std::string s = std::string("<# ");

`const StringRef &` seems redundant given that `StringRef` is already a "Ref". 
Just pass by value.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:408
+auto BaseIsArrayOrPtrDRE =
+hasBase(ignoringParenImpCasts(allOf(declRefExpr(), ArrayOrPtr)));
+auto Target =

A bit more concise.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:637
 
-static Strategy
-getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) {

Hmm, did this need to be moved? I don't think you're calling this function from 
the new code.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:726
+template 
+static std::optional getPassedLoc(const NodeTy *Node,
+  const SourceManager &SM,

(https://www.oxfordinternationalenglish.com/passed-vs-past-whats-the-difference/,
 looks like "Passed` is always a verb)



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:768
+if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) {
+  if (!Ext->HasSideEffects(Ctx))
+ExtentText =

Excellent observation!



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:771
+getExprTextOr(Ext, SM, LangOpts, StringRef(DefaultExtentTxt));
+} else if (!CxxNew->isArray())
+  ExtentText = One;

I wonder how do we end up in such situation. If it's not array new, this means 
it's also not a buffer of multiple elements. But I guess the pointer can be 
reassigned later. Yeah I think you're right, this is the correct solution. 
Maybe let's leave a comment explaining how this could happen?



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:773-774
+  ExtentText = One;
+  } else if (auto CArrTy = dyn_cast(
+ Init->IgnoreImpCasts()->getType().getTypePtr())) {
+// In cases `Init` is of an array type after stripping off implicit casts,

That's another case where `.getTypePtr()` is unnecessary: 
`dyn_cast(QT.getTypePtr())` is equivalent to`QT->getAs()` where `->` is 
`QualType`'s overloaded operator and `getAs` is `Type::getAs`.

Additionally, array types are special - see doxygen comments for 
`ASTContext::getAsArrayType()`. For some reason you're suppposed to consult 
ASTContext when casting them, which I did in the suggested fix. I'm not sure if 
it matters in this case when we just want to extract the size.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:780-781
+  } else {
+// In cases `Init` is of the form `&Var` after stripping of implicit
+// casts, where `&` is the built-in operator, the extent is 1.
+if (auto AddrOfExpr = dyn_cast(Init->IgnoreImpCasts()))

```lang=c
int x = 1;
char *ptr = &x; // std::span ptr { &x, 4 };
```
This is valid code. I suspect we want to check types as well, to see that type 
sizes match.

Most of the time code like this violates strict aliasing, but `char` is 
exceptional, and even if it did violate strict aliasing, people can compile 
with `-fno-strict-aliasing` to define away the UB, so we have to respect that.