I see following wrong warning: $ cat snippet.c char buf[128]; char *src = "HCSparta"; int main(int argc, char **argv) { char *dst = buf + sizeof(buf); if (argc) { dst -= argc; __builtin_memcpy(dst, src, argc + 0); } } $ gcc snippet.c -O2 -Wstringop-overflow=2 -fno-common -g snippet.c: In function ‘main’: snippet.c:11:7: warning: ‘__builtin_memcpy’ writing 1 or more bytes into a region of size 0 overflows the destination [-Wstringop-overflow=] 11 | __builtin_memcpy(dst, src, argc + 0); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ $ gcc snippet.c -O2 -Wstringop-overflow=2 -fno-common -g -fsanitize=address && ./a.out [OK] While doing s/0/1: $ cat snippet.c char buf[128]; char *src = "HCSparta"; int main(int argc, char **argv) { char *dst = buf + sizeof(buf); if (argc) { dst -= argc; __builtin_memcpy(dst, src, argc + 1); } } $ gcc snippet.c -O2 -Wstringop-overflow=2 -fno-common -g [OK] But: $ gcc snippet.c -O2 -Wstringop-overflow=2 -fno-common -g -fsanitize=address && ./a.out ================================================================= ==6195==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000404220 at pc 0x7ffff763a5d0 bp 0x7fffffffdb70 sp 0x7fffffffd320 WRITE of size 2 at 0x000000404220 thread T0 #0 0x7ffff763a5cf in __interceptor_memcpy /home/marxin/Programming/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:790 #1 0x4010b4 in main /tmp/snippet.c:11 #2 0x7ffff73b4b7a in __libc_start_main ../csu/libc-start.c:308 #3 0x401119 in _start (/tmp/a.out+0x401119) 0x000000404220 is located 0 bytes to the right of global variable 'buf' defined in 'snippet.c:1:6' (0x4041a0) of size 128 SUMMARY: AddressSanitizer: global-buffer-overflow /home/marxin/Programming/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:790 in __interceptor_memcpy ...
The -Wstringop-overflow warning is a consequence of pointer offsets being treated as unsigned integers, argc being signed, and the object size computation returning scalars rather than ranges. In the first test case (with argc + 0): <bb 3> [local count: 354334802]: _1 = (sizetype) argc_5(D); _2 = -_1; dst_7 = &MEM[(void *)&buf + 128B] + _2; src.0_3 = src; __builtin_memcpy (dst_7, src.0_3, _1); When computing the size of dst_7 the compute_objsize() function determines the offset _2 to be in the range [1, SIZE_MAX] and doesn't consider the fact that the upper half of the range represents negative values. As a result, it determines the size to be zero. The number of bytes to write (i.e., (size_t)(argc + 1) is [1, SIZE_MAX]). The test case makes the tacit assumption that argc is necessarily non-negative. That makes sense for the argc argument to main but not in other situations. Changing the if condition to make the assumption explicit 'if (argc > 0)' eliminates the warning. This makes range of the offset [-INT_MAX, -1], and because compute_objsize() doesn't handle negative offsets, it fails to determine the size. There's a comment indicating that is not a feature but a known limitation: /* Ignore negative offsets for now. For others, use the lower bound as the most optimistic estimate of the (remaining)size. */ If I recall I did that not because negative offsets cannot be handled better but to keep the code simple. Either way, with negative offsets handled the warning will not be issued. The missing -Wstringop-overflow in the second test case (with argc + 1): <bb 3> [local count: 354334802]: _1 = (sizetype) argc_7(D); _2 = -_1; dst_9 = &MEM[(void *)&buf + 128B] + _2; _3 = argc_7(D) + 1; _4 = (long unsigned int) _3; src.0_5 = src; __builtin_memcpy (dst_9, src.0_5, _4); is due to the number of bytes to write is [0, INT_MAX] so the warning doesn't trigger. The bogus warning can be avoided in the first case simply by punting on offsets that could be in the negative range, but almost certainly not without some false negatives. I'm not sure it's necessarily a good tradeoff (I don't know that it isn't either). Is this code representative of some wide-spread idiom? A more robust solution, one that gets rid of the false positives without as many false negatives, will involve changing compute_objsize() to return a range of sizes rather than a constant. But that will have to wait for GCC 10.
(In reply to Martin Sebor from comment #1) > > The test case makes the tacit assumption that argc is necessarily > non-negative. That makes sense for the argc argument to main but not in > other situations. Would it be possible to propose an update to the C standard that allows the argc argument to main to be unsigned? Could GCC start accepting unsigned argc as an extension so that -Wmain allows it?
Well, we could perhaps do something like: --- gcc/gimple-ssa-evrp.c.jj 2019-01-01 12:37:15.712998659 +0100 +++ gcc/gimple-ssa-evrp.c 2019-02-15 09:27:49.569441402 +0100 @@ -41,6 +41,7 @@ along with GCC; see the file COPYING3. #include "tree-cfgcleanup.h" #include "vr-values.h" #include "gimple-ssa-evrp-analyze.h" +#include "tree-dfa.h" class evrp_folder : public substitute_and_fold_engine { @@ -307,6 +308,21 @@ execute_early_vrp () scev_initialize (); calculate_dominance_info (CDI_DOMINATORS); + /* argc in main is never negative. */ + if (MAIN_NAME_P (DECL_NAME (current_function_decl)) + && DECL_ARGUMENTS (current_function_decl)) + { + tree argc = DECL_ARGUMENTS (current_function_decl); + if (TYPE_MAIN_VARIANT (TREE_TYPE (argc)) == integer_type_node) + { + argc = ssa_default_def (cfun, argc); + if (argc && SSA_NAME_RANGE_INFO (argc) == NULL) + set_range_info (argc, VR_RANGE, + wi::zero (TYPE_PRECISION (integer_type_node)), + wi::to_wide (TYPE_MAX_VALUE (integer_type_node))); + } + } + /* Walk stmts in dominance order and propagate VRP. */ evrp_dom_walker walker; walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun)); (not really sure if it will work fine with Ada, it does some games with main). That said, I bet the original package code didn't have the warning in main but somewhere else.
Note, e.g. C99 guarantees that: "If they are declared, the parameters to the main function shall obey the following constraints: — The value of argc shall be nonnegative." Similarly C++, http://eel.is/c++draft/basic.start.main "The value of argc shall be non-negative." So, we could do this, or perhaps add lang_GNU_C () || lang_GNU_CXX () || lang_GNU_OBJC () check too, evrp is pre-IPA pass and thus it can check the defining language easily.
> That said, I bet the original package code didn't have the warning in main > but somewhere else. Yes.
Created attachment 45727 [details] original test-case
Thanks for analysis. > > The bogus warning can be avoided in the first case simply by punting on > offsets that could be in the negative range, but almost certainly not > without some false negatives. Yes, I can confirm that adding a check for negative values can make the warning gone. > I'm not sure it's necessarily a good tradeoff > (I don't know that it isn't either). Is this code representative of some > wide-spread idiom? I see it just in one package.
Created attachment 45728 [details] gcc9-pr89350.patch Untested patch for the argc range stuff.
Let me put together a patch for the negative offsets. The argc patch is useful on its own, independent of this bug.
Patch: https://gcc.gnu.org/ml/gcc-patches/2019-02/msg02029.html
Author: msebor Date: Fri Mar 22 02:58:27 2019 New Revision: 269867 URL: https://gcc.gnu.org/viewcvs?rev=269867&root=gcc&view=rev Log: PR tree-optimization/89350 - Wrong -Wstringop-overflow= warning since r261518 gcc/ChangeLog: PR tree-optimization/89350 * builtins.c (compute_objsize): Also ignore offsets whose upper bound is negative. * gimple-ssa-warn-restrict.c (builtin_memref): Add new member. (builtin_memref::builtin_memref): Initialize new member. Allow EXPR to be null. (builtin_memref::extend_offset_range): Replace local with a member. Avoid assuming pointer offsets are unsigned. (builtin_memref::set_base_and_offset): Determine base object before computing offset range. (builtin_access::builtin_access): Handle memset. (builtin_access::generic_overlap): Replace local with a member. (builtin_access::strcat_overlap): Same. (builtin_access::overlap): Same. (maybe_diag_overlap): Same. (maybe_diag_access_bounds): Same. (wrestrict_dom_walker::check_call): Handle memset. (check_bounds_or_overlap): Same. gcc/testsuite/ChangeLog: PR tree-optimization/89350 * gcc.dg/Wstringop-overflow.c: Xfail overly ambitious tests. * gcc.dg/Wstringop-overflow-11.c: New test. * gcc.dg/Wstringop-overflow-12.c: New test. * gcc.dg/pr89350.c: New test. * gcc.dg/pr40340-1.c: Adjust expected warning. * gcc.dg/pr40340-2.c: Same. * gcc.dg/pr40340-4.c: Same. * gcc.dg/pr40340-5.c: Same. Added: trunk/gcc/testsuite/gcc.dg/Wstringop-overflow-11.c trunk/gcc/testsuite/gcc.dg/Wstringop-overflow-12.c trunk/gcc/testsuite/gcc.dg/pr89350.c Modified: trunk/gcc/ChangeLog trunk/gcc/builtins.c trunk/gcc/gimple-ssa-warn-restrict.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/Warray-bounds-40.c trunk/gcc/testsuite/gcc.dg/Wstringop-overflow.c trunk/gcc/testsuite/gcc.dg/pr40340-1.c trunk/gcc/testsuite/gcc.dg/pr40340-2.c trunk/gcc/testsuite/gcc.dg/pr40340-4.c trunk/gcc/testsuite/gcc.dg/pr40340-5.c
Patch committed in r269867.