[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