[PATCH] handle conditionals in -Wstringop-overflow et al. (PR 92936)

Martin Sebor msebor@gmail.com
Tue Dec 1 00:58:06 GMT 2020


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


More information about the Gcc-patches mailing list