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 :-)
Thanks,
jeff
