On 11/30/20 1:49 PM, Jeff Law wrote:
On 11/29/20 3:27 PM, Martin Sebor wrote:
On 11/13/20 2:34 PM, Jeff Law wrote:
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.
I've reused the same approach/class as in tree-ssa-strlen.c and
after adjusting things that need to be adjusted and retesting
with Binutils/GDB and Glibc committed the attached patch in
r11-5523.
That said, although the recursion hasn't been a problem (there's
still code elsewhere that does this sort of traversal of the use-
def chains that doesn't use the parameter), the subsequent patch
that adds the cache makes it possible to reduce it to just trees
(when the function is called in a GIMPLE pass to cache each
pointer assignment).
Martin
gcc-92936.diff
OK. I didn't necessarily expect that the recursion is causing problems
in our code base or in the testsuite. It's not terribly unusual for
tests which cause these kinds of problems to be too big to include in
the testsuite. It's also the case that these problems typically aren't
reported until after a release gets out into the wild and someone tries
there machine generated torturous code on the latest bits :-)
What I meant was that the recursion in compute_objsize has been
there in various forms since at least GCC 8 (it's still in other
parts of GCC today), and it hasn't been a source of reported
problems.
I also never did understand how following a use-def chain of
SSA_NAMEs could have any but linear complexity. For everything
except PHIs the traversal is strictly linear; for PHIs, setting
and testing the visited bitmap breaks cycles, again guaranteeing
linear complexity. For reference, here are the numbers I put
together last year (this was for get_range_strlen_dynamic, not
compute_objsize):
https://gcc.gnu.org/pipermail/gcc-patches/2019-July/525000.html
Anyway, whether it's actually needed or not, the limit's there
now (in compute_objsize), and the caching should only make
reaching it that much less likely.
Martin
