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@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
