On 11/2/20 7:24 PM, Martin Sebor wrote: > The attached patch extends compute_objsize() to handle conditional > expressions represented either as PHIs or MIN_EXPR and MAX_EXPR. > > To simplify the handling of the -Wstringop-overflow/-overread > warnings the change factors this code out of tree-ssa-strlen.c > and into inform_access() in builtins.c, making it a member of > access_ref. Besides eliminating a decent amount of code > duplication this also improves the consistency of the warnings. > > Finally, the change introduces a distinction between the definite > kinds of -Wstringop-overflow (and -Wstringop-overread) warnings > and the maybe kind. The latter are currently only being issued > for function array parameters but I expect to make use of them > more extensively in the future. > > Besides the usual GCC bootstrap/regtest I have tested the change > with Binutils/GDB and Glibc and verified that it doesn't introduce > any false positives. > > Martin > > gcc-92936.diff > > PR middle-end/92936 - missing warning on a past-the-end store to a PHI > PR middle-end/92940 - incorrect offset and size in -Wstringop-overflow for > out-of-bounds store into VLA and two offset ranges > PR middle-end/89428 - missing -Wstringop-overflow on a PHI with variable > offset > > gcc/ChangeLog: > > PR middle-end/92936 > PR middle-end/92940 > PR middle-end/89428 > * builtins.c (access_ref::access_ref): Initialize member. > (access_ref::phi): New function. > (access_ref::get_ref): New function. > (access_ref::add_offset): Remove duplicate assignment. > (maybe_warn_for_bound): Add "maybe" kind of warning messages. > (warn_for_access): Same. > (inform_access): Rename... > (access_ref::inform_access): ...to this. Print PHI arguments. Format > offset the same as size and simplify. Improve printing of allocation > functions and VLAs. > (check_access): Adjust to the above. > (gimple_parm_array_size): Change argument. > (handle_min_max_size): New function. > * builtins.h (struct access_ref): Declare new members. > (gimple_parm_array_size): Change argument. > * tree-ssa-strlen.c (maybe_warn_overflow): Use access_ref and simplify. > (handle_builtin_memcpy): Correct argument passed to maybe_warn_overflow. > (handle_builtin_memset): Same. > > gcc/testsuite/ChangeLog: > > PR middle-end/92936 > PR middle-end/92940 > PR middle-end/89428 > * c-c++-common/Wstringop-overflow-2.c: Adjust text of expected > informational notes. > * gcc.dg/Wstringop-overflow-11.c: Remove xfails. > * gcc.dg/Wstringop-overflow-12.c: Same. > * gcc.dg/Wstringop-overflow-17.c: Adjust text of expected messages. > * gcc.dg/Wstringop-overflow-27.c: Same. Remove xfails. > * gcc.dg/Wstringop-overflow-28.c: Adjust text of expected messages. > * gcc.dg/Wstringop-overflow-29.c: Same. > * gcc.dg/Wstringop-overflow-37.c: Same. > * gcc.dg/Wstringop-overflow-46.c: Same. > * gcc.dg/Wstringop-overflow-47.c: Same. > * gcc.dg/Wstringop-overflow-54.c: Same. > * gcc.dg/warn-strnlen-no-nul.c: Add expected warning. > * gcc.dg/Wstringop-overflow-58.c: New test. > * gcc.dg/Wstringop-overflow-59.c: New test. > * gcc.dg/Wstringop-overflow-60.c: New test. > * gcc.dg/Wstringop-overflow-61.c: New test. > * gcc.dg/Wstringop-overflow-62.c: New test.
So my only significant concern here is the recursive nature and the lack of a limiter for pathological cases. We certainly run into cases with thousands of PHI arguments and deep chains of PHIs feeding other PHIs. Can you put in a PARAM to limit the amount of recursion and and PHI arguments you look at? With that I think this is fine -- I consider it unlikely this patch is the root cause of the ICEs I sent you earlier today from the tester since those failures are in the array bounds checking bits. jeff
