On Tue, Feb 9, 2021 at 1:04 AM Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> 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: > > int i; > void *p = &i; // valid > float *q = p; // valid > *q = 0; // aliasing violation > > This bug is easily detectable in the middle end but impossible > to do in the front end (same as all other invalid accesses).
But the code is valid in GIMPLE which allows to re-use the 'int i' storage with storing using a different type. > Whether this is done in gimple-array-bounds or some other pass seems > to me like a minor implementation detail. It naturally came out of > an enhancement I implemented there (which would detect the above > with float replaced by any larger type as an out-of-bounds access) > but I have no problem with moving this subset to some other pass > (or duplicating it there). In fact, as I said, I'd like to enhance > -Wstrict-aliasing to detect more bugs at some point, so that might > be a good time to move this instance of the warning there as well. > But the enhancement I'm thinking of is in the middle end, not in > the front end. > > In any event, the warning is valid, just the phrasing is misleading > since there in the case of the struct member there isn't necessarily > any subscripting involved or even an access to members beyond the end > of the accessed object. Issuing it under -Warray-bounds and with > -fno-strict-aliasing compounds the problem. I put together this > patch in response to the feedback I got from you and from the reporter > in PR 98503 where you both made this point, so I'm not sure why > improving it as both of you suggested is an issue. > > Martin
