[PATCH] D107051: [clang][analyzer] Improve bug report in alpha.security.ReturnPtrRange

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added reviewers: steakhal, NoQ, martong, vsavchenko.
Szelethus added a comment.
Herald added a subscriber: rnkovacs.

One of the test files needs fixing: https://reviews.llvm.org/harbormaster/unit/view/931615/

Love the patch, though I agree that the note message needs some improvement. How about we change from this:

  warning: Returned pointer value with index 11 points outside the original object with size of 10 'Data' objects (potential buffer overflow)

To this:

  warning: Returned pointer points outside the original object (potential buffer overflow)
  note: The original object 'Data' is an int array of size 10, the returned pointer points at the 11th element

or this:

  warning: Returned pointer points to the 11th element of 'Data', but that is an int array of size 10 (potential buffer overflow)



================
Comment at: clang/test/Analysis/return-ptr-range.cpp:1
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+// RUN1: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -analyzer-output text -verify=notes %s
----------------
steakhal wrote:
> Is `RUN1` intentional? If so, what does it do?
We could just delete it. I guess that was the intent, to make this RUN line non-functional.


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:2
+// RUN1: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -analyzer-output text -verify=notes %s
 
----------------
steakhal wrote:
> You can specify multiple match prefixes.
> If you were passing `-verify=expected,notes`, you would not have to duplicate each warning.
Don't forget `core`!


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:11
+int arr3[10]; // notes-note{{Memory object declared here}}
 int *ptr;
 
----------------
steakhal wrote:
> This is not used in every test case. I would recommend moving this to the function parameter where it's actually being used.
If this is a cpp file, just use namespaces, and redeclare everything inside them, like this: https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/track-control-dependency-conditions.cpp


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:19-20
+    ptr = arr1 + x; // notes-note{{Value assigned to 'ptr'}}
+    if (x != 20) // notes-note{{Assuming 'x' is equal to 20}}
+                 // notes-note at -1{{Taking false branch}}
+      return arr1; // no-warning
----------------
steakhal wrote:
> This is probably more of a taste.
> I would prefer fewer indentations.
> The same applies everywhere.
I disagree, and prefer it as it is written now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107051