[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:256
-  foo(f(p, &p, a, a)[1]); // expected-warning{{unsafe buffer access}}
-  // FIXME: expected note@-1{{in instantiation of 
function template specialization 'f' requested here}}
 

I don't think this FIXME was correct. The code we're warning about isn't 
expanded from a template.


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

https://reviews.llvm.org/D146773

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

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 552560.
NoQ added a comment.

Rebase.

Make warnings a bit more verbose than before. This plays nicely with our 
attempts to categorize unemitted fixits via `-mllvm -debug-only=SafeBuffers`, 
but this time it doesn't make sense to make the extra information debug-only; 
it makes perfect sense as part of the warning.

TODO: Some new code paths aren't covered by tests yet (captured variables, 
static member variables).


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

https://reviews.llvm.org/D146773

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-c-linkage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-main.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.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
@@ -41,34 +41,34 @@
 // expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
   foo(p[1], // expected-note{{used in buffer access here}}
   pp[1][1], // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer parameter variable 'pp'}}
   1[1[pp]], // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer parameter variable 'pp'}}
   1[pp][1]  // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer parameter variable 'pp'}}
   );
 
   if (p[3]) {   // expected-note{{used in buffer access here}}
 void * q = p;
 
-foo(((int*)q)[10]); // expected-warning{{unsafe buffer access}}
+foo(((int*)q)[10]); // expected-warning{{unsafe buffer access through raw pointer expression}}
   }
 
-  foo(((int*)voidPtrCall())[3], // expected-warning{{unsafe buffer access}}
-  3[(int*)voidPtrCall()],   // expected-warning{{unsafe buffer access}}
-  charPtrCall()[3], // expected-warning{{unsafe buffer access}}
-  3[charPtrCall()]  // expected-warning{{unsafe buffer access}}
+  foo(((int*)voidPtrCall())[3], // expected-warning{{unsafe buffer access through raw pointer expression}}
+  3[(int*)voidPtrCall()],   // expected-warning{{unsafe buffer access through raw pointer expression}}
+  charPtrCall()[3], // expected-warning{{unsafe buffer access through raw pointer return value of function 'charPtrCall'}}
+  3[charPtrCall()]  // expected-warning{{unsafe buffer access through raw pointer return value of function 'charPtrCall'}}
   );
 
 int a[10];  // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
 int b[10][10];  // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
 
   foo(a[1], 1[a],   // expected-note2{{used in buffer access here}}
-  b[3][4],  // expected-warning{{unsafe buffer access}}
+  b[3][4],  // expected-warning{{unsafe buffer access into raw array local variable 'b'}}
 // expected-note@-1{{used in buffer access here}}
-  4[b][3],  // expected-warning{{unsafe buffer access}}
+  4[b][3],  // expected-warning{{unsafe buffer access into raw array local variable 'b'}}
 // expected-note@-1{{used in buffer access here}}
-  4[3[b]]); // expected-warning{{unsafe buffer access}}
+  4[3[b]]); // expected-warning{{unsafe buffer access into raw array local variable 'b'}}
 // expected-note@-1{{used in buffer access here}}
 
   // Not to warn when index is zero
@@ -108,7 +108,7 @@
   q[1], 1[q], q[-1],// expected-note3{{used in buffer access here}}
   a[1], // expected-note{{used in buffer access here}} `a` is of pointer type
   b[1][2]   // expected-note{{used in buffer access here}} `b[1]` is of array type
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access into raw array parameter va

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11786
+  "%select{"
+"unsafe operation on raw pointer in expression|"
+"unsafe arithmetic on raw pointer|"

NoQ wrote:
> malavikasamak wrote:
> > NoQ wrote:
> > > The first mode doesn't show up in any tests and it's probably dead code 
> > > at this point.
> > What do you think of specifying the variable name in these warnings? It 
> > could be helpful when there are more than one DREs in a statement.  
> These warnings are emitted only when we *cannot* identify the variable; 
> either because it's not there at all, or we don't know how to find it. If we 
> could figure out the variable, we'd be emitting `warn_unsafe_buffer_variable` 
> instead. IIRC, in all of the provided test cases the operand isn't a DRE.
> 
> That said, I totally agree that it's a great idea to try a few other things 
> to point out the specific subexpression, even if we don't connect the warning 
> gadget to a variable. Say, we can try to blame `MemberExpr`s or `CallExpr`s 
> the same way we blame DREs, and include their name in the warning. I'll try a 
> few things real quick and see how bad it gets.
> 
> Note that these are also the things for which we may eventually start 
> providing constructive solutions. Say, an unsafe operation on `CallExpr` can 
> be a reason to autofix the callee to return a `std::span`. So hopefully these 
> warnings will eventually be replaced by better warnings in more and more 
> cases.
Wait nvm, the whole point of this patch is to warn when variable discovery is 
manually turned off! I'm an idiot, this absolutely deserves more effort.


Repository:
  rC Clang

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

https://reviews.llvm.org/D146773

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

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 accepted this revision.
ziqingluo-90 added a comment.
This revision is now accepted and ready to land.

LGTM,  Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D146773

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

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11786
+  "%select{"
+"unsafe operation on raw pointer in expression|"
+"unsafe arithmetic on raw pointer|"

malavikasamak wrote:
> NoQ wrote:
> > The first mode doesn't show up in any tests and it's probably dead code at 
> > this point.
> What do you think of specifying the variable name in these warnings? It could 
> be helpful when there are more than one DREs in a statement.  
These warnings are emitted only when we *cannot* identify the variable; either 
because it's not there at all, or we don't know how to find it. If we could 
figure out the variable, we'd be emitting `warn_unsafe_buffer_variable` 
instead. IIRC, in all of the provided test cases the operand isn't a DRE.

That said, I totally agree that it's a great idea to try a few other things to 
point out the specific subexpression, even if we don't connect the warning 
gadget to a variable. Say, we can try to blame `MemberExpr`s or `CallExpr`s the 
same way we blame DREs, and include their name in the warning. I'll try a few 
things real quick and see how bad it gets.

Note that these are also the things for which we may eventually start providing 
constructive solutions. Say, an unsafe operation on `CallExpr` can be a reason 
to autofix the callee to return a `std::span`. So hopefully these warnings will 
eventually be replaced by better warnings in more and more cases.


Repository:
  rC Clang

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

https://reviews.llvm.org/D146773

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

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Malavika Samak via Phabricator via cfe-commits]

malavikasamak added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11786
+  "%select{"
+"unsafe operation on raw pointer in expression|"
+"unsafe arithmetic on raw pointer|"

NoQ wrote:
> The first mode doesn't show up in any tests and it's probably dead code at 
> this point.
What do you think of specifying the variable name in these warnings? It could 
be helpful when there are more than one DREs in a statement.  


Repository:
  rC Clang

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

https://reviews.llvm.org/D146773

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

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11786
+  "%select{"
+"unsafe operation on raw pointer in expression|"
+"unsafe arithmetic on raw pointer|"

The first mode doesn't show up in any tests and it's probably dead code at this 
point.


Repository:
  rC Clang

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

https://reviews.llvm.org/D146773

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

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: jkorous, t-rasmud, ziqingluo-90, malavikasamak, 
aaron.ballman, gribozavr, ymandel, sgatev.
Herald added subscribers: steakhal, martong.
Herald added a project: All.
NoQ requested review of this revision.

So far we didn't pay enough attention to them given that we spent a lot of time 
on grouped warnings, but D146669  will cause 
them to be displayed more often. Make them nicer and more understandable.


Repository:
  rC Clang

https://reviews.llvm.org/D146773

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.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
@@ -41,34 +41,34 @@
 // expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
   foo(p[1], // expected-note{{used in buffer access here}}
   pp[1][1], // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer}}
   1[1[pp]], // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer}}
   1[pp][1]  // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer}}
   );
 
   if (p[3]) {   // expected-note{{used in buffer access here}}
 void * q = p;
 
-foo(((int*)q)[10]); // expected-warning{{unsafe buffer access}}
+foo(((int*)q)[10]); // expected-warning{{unsafe buffer access through raw pointer}}
   }
 
-  foo(((int*)voidPtrCall())[3], // expected-warning{{unsafe buffer access}}
-  3[(int*)voidPtrCall()],   // expected-warning{{unsafe buffer access}}
-  charPtrCall()[3], // expected-warning{{unsafe buffer access}}
-  3[charPtrCall()]  // expected-warning{{unsafe buffer access}}
+  foo(((int*)voidPtrCall())[3], // expected-warning{{unsafe buffer access through raw pointer}}
+  3[(int*)voidPtrCall()],   // expected-warning{{unsafe buffer access through raw pointer}}
+  charPtrCall()[3], // expected-warning{{unsafe buffer access through raw pointer}}
+  3[charPtrCall()]  // expected-warning{{unsafe buffer access through raw pointer}}
   );
 
 int a[10];  // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
 int b[10][10];  // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
 
   foo(a[1], 1[a],   // expected-note2{{used in buffer access here}}
-  b[3][4],  // expected-warning{{unsafe buffer access}}
+  b[3][4],  // expected-warning{{unsafe buffer access through raw pointer}}
 // expected-note@-1{{used in buffer access here}}
-  4[b][3],  // expected-warning{{unsafe buffer access}}
+  4[b][3],  // expected-warning{{unsafe buffer access through raw pointer}}
 // expected-note@-1{{used in buffer access here}}
-  4[3[b]]); // expected-warning{{unsafe buffer access}}
+  4[3[b]]); // expected-warning{{unsafe buffer access through raw pointer}}
 // expected-note@-1{{used in buffer access here}}
 
   // Not to warn when index is zero
@@ -96,7 +96,7 @@
 		 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}}
+  // expected-warning@-1{{unsafe buffer access through raw pointer}}
 
   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}}
@@ -120,7 +120,7 @@
   q[1], 1[q], q[-1],// expected-note3{{used in buffer access here}}
   a[1], // expected-note{{used in buffer access here}} `a` is of pointer type
   b[1][2]   // expected-note{{used in buffer access here}} `b[1]` is of array type
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer}}
   );
 }
 
@@ -139,32 +139,32 @@
 T_t * funRetTStar();
 
 void testStructMembers(stru