[PATCH] adjust "partly out of bounds" warning (PR 98503)

[Jeff Law]

On 2/8/21 4:07 PM, Martin Sebor wrote:
> On 2/8/21 12:09 PM, Jeff Law wrote:
>>
>>
>> On 2/3/21 3:45 PM, Martin Sebor wrote:
>>> On 2/3/21 2:57 PM, Jeff Law wrote:
>>>>
>>>>
>>>> On 1/28/21 4:03 PM, Martin Sebor wrote:
>>>>> The GCC 11 -Warray-bounds enhancement to diagnose accesses whose
>>>>> leading offset is in bounds but whose trailing offset is not has
>>>>> been causing some confusion.  When the warning is issued for
>>>>> an access to an in-bounds member via a pointer to a struct that's
>>>>> larger than the pointed-to object, phrasing this strictly invalid
>>>>> access in terms of array subscripts can be misleading, especially
>>>>> when the source code doesn't involve any arrays or indexing.
>>>>>
>>>>> Since the problem boils down to an aliasing violation much more
>>>>> so than an actual out-of-bounds access, the attached patch adjusts
>>>>> the code to issue a -Wstrict-aliasing warning in these cases instead
>>>>> of -Warray-bounds.  In addition, since the aliasing assumptions in
>>>>> GCC can be disabled by -fno-strict-aliasing, the patch also makes
>>>>> these instances of the warning conditional on -fstrict-aliasing
>>>>> being in effect.
>>>>>
>>>>> Martin
>>>>>
>>>>> gcc-98503.diff
>>>>>
>>>>> PR middle-end/98503 -Warray-bounds when -Wstrict-aliasing would be
>>>>> more appropriate
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>>      PR middle-end/98503
>>>>>      * gimple-array-bounds.cc (array_bounds_checker::check_mem_ref):
>>>>>      Issue -Wstrict-aliasing for a subset of violations.
>>>>>      (array_bounds_checker::check_array_bounds):  Set new member.
>>>>>      * gimple-array-bounds.h (array_bounds_checker::cref_of_mref):
>>>>> New
>>>>>      data member.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>>      PR middle-end/98503
>>>>>      * g++.dg/warn/Warray-bounds-10.C: Adjust text of expected
>>>>> warnings.
>>>>>      * g++.dg/warn/Warray-bounds-11.C: Same.
>>>>>      * g++.dg/warn/Warray-bounds-12.C: Same.
>>>>>      * g++.dg/warn/Warray-bounds-13.C: Same.
>>>>>      * gcc.dg/Warray-bounds-63.c: Avoid -Wstrict-aliasing.  Adjust
>>>>> text
>>>>>      of expected warnings.
>>>>>      * gcc.dg/Warray-bounds-66.c: Adjust text of expected warnings.
>>>>>      * gcc.dg/Wstrict-aliasing-2.c: New test.
>>>>>      * gcc.dg/Wstrict-aliasing-3.c: New test.
>>>> What I don't like here is the strict-aliasing warnings inside the file
>>>> and analysis phase for array bounds checking.
>>>>
>>>> ISTM that catching this at cast time would be better.  So perhaps in
>>>> build_c_cast and the C++ equivalent?
>>>>
>>>> It would mean we're warning at the cast site rather than the
>>>> dereference, but that may ultimately be better for the warning anyway.
>>>> I'm not sure.
>>>
>>> I had actually experimented with a this (in the middle end, and only
>>> for accesses) but even that caused so many warnings that I abandoned
>>> it, at least for now.  -Warray-bounds has the benefit of flow analysis
>>> (and dead code elimination).  In the front end it would have neither
>>> and be both excessively noisy and ineffective at the same time.  For
>>> example:
>> I think we agree that this really is an aliasing issue and I would go
>> further and claim that it has nothing to do with array bounds checking.
>> Therefore warning for it within gimple-array-bounds just seems wrong.
>>
>> I realize that you like having DCE run and the ability to look at
>> offsets and such, but it really feels like the wrong place to do this.
>> Aliasing issues are also more of a front-end thing since the language
>> defines what is and what is not valid aliasing -- one might even argue
>> that any aliasing warning needs to be identified by the front-ends
>> exclusively (though possibly pruned away later).
>
> The aliasing restrictions are on accesses, which are [defined in
> C as] execution-time actions on objects.  Analyzing execution-time
> actions unavoidably depends on flow analysis which the front ends
> have only very limited support for (simple expressions only).
> I gave one example showing how the current -Wstrict-aliasing in
> the front end is ineffective against all but the most simplistic
> bugs, but there are many more.  For instance:
I'm aware of all this.  Even so I think trying to deal with strict
aliasing in the middle-end is fundamentally wrong.  The middle end is
not C and it can not assume C semantics and putting the warning in the
middle of the array bounds pass is, IMHO, just wrong.

That doesn't change the need to address the problems with the warning,
namely that it makes references to array bounds violations for accesses
which aren't arrays.

The details of *how* to address those problems are still quite fuzzy. 
The most obvious ideas to me are either rely on something like
__builtin_warning or to mark the problem expressions in the front-end,
but defer issuing the warning until we're done with the optimization
pipeline.

Jeff