| Message ID | 5d5a15c5-0066-6f60-da11-7752f0a29940@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | adjust "partly out of bounds" warning (PR 98503) | expand |
On Thu, Jan 28, 2021 at 6:04 PM Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> 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 -Wstrict-aliasing is a warning with numerical levels; can you clarify which of them this is active at? (also I have always found it confusing how -Wstrict-aliasing's numerical levels are in reverse order from most other warnings with numerical levels; i.e., how higher numbers supposedly mean fewer warnings for it (according to documentation) rather than more, as is the case with most other warnings with numerical levels)
On 1/30/21 12:36 AM, Eric Gallager wrote: > On Thu, Jan 28, 2021 at 6:04 PM Martin Sebor via Gcc-patches > <gcc-patches@gcc.gnu.org> 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 > > -Wstrict-aliasing is a warning with numerical levels; can you clarify > which of them this is active at? It runs at the default level 3. > (also I have always found it confusing how -Wstrict-aliasing's > numerical levels are in reverse order from most other warnings with > numerical levels; i.e., how higher numbers supposedly mean fewer > warnings for it (according to documentation) rather than more, as is > the case with most other warnings with numerical levels) > The description does seem a little counter-intuitive. The warning runs in the front ends and the analysis functions it calls at level 3 have the same complexity as at level 1 so I'm not sure I understand the point of the levels. I'd be surprised if anyone used anything but the default. Martin
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. Jeff
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: struct A { int i; }; struct B { int i; }; struct A a; B *p = (B*)&a; // FE would warn here (even if there's no access) That's also why the current -Warray-bounds (and the proposed -Wstrict-aliasing refinement) only triggers for accesses via larger types, as in: struct C { int i, j; }; void *p = &a; // FE can't warn here C *q = (C*)p; // or here p->j = 0; // middle end warns here Martin PS The existing -Wsrtrict-aliasing in the front end is superficial at best. It only detects problems in simple accesses that don't involve any data flow. For instance it warns only if f() below but not in g(): int a; void f (void) { *(float*)&a = 0; // -Wstrict-aliasing } void g (void) { float *p = (float*)&a; *p = 0; // silence } I'd like to improve it at some point but to catch the problem in g() the warning will certainly have to move into the middle end.
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). Jeff
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). 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
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
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
On 2/9/21 12:41 AM, Richard Biener wrote: > 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. Presumably you're referring to using placement new? The warning would have to be aware of it and either run before placement new is inlined. Alternatively, the inlining could add some annotation into the IL that the warning would then use to differentiate valid code from invalid. Likewise if there are other such constructs (are there?) they would need be marked up somehow by the front end. I speculate that's what Jeff was suggesting by having the FE mark up the code. Martin > >> 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
On 2/8/21 4:26 PM, Jeff Law wrote: > > > 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. All the existing access warnings in the middle end are designed around C/C++ (and other language) rules, just as much as all middle end optimizations, including aliasing, rely on them. I don't see how this instance is any different. -Warray-bounds is issued in the VRP pass (there is no dedicated array bounds pass). It just happens to live in a file recently named gimple-array-bounds.cc. The name was chosen only because that was the only warning that happened to be implemented there at the time. I have no problem with keeping separate things separate but I also see nothing wrong with issuing two or more warnings under different options from the same code/pass, especially when they're related or when both employ the same analysis. Some bugs are the results of violating multiple rules and which one is chosen in each instance as the most representative is a matter of deciding which is more important or relevant. For example, in: struct A { int a[2]; char *p; }; void f (struct A *p) { p->a[2] = 123; } the store to p->a[2] is both an out of bounds access and an aliasing violation. If -Wstrict-aliasing is enhanced to run in the middle end but independently from -Warray-bounds we will end up with two warnings for this bug and have to work extra hard to suppress one or the other. This is already the case with -Warray-bounds and -Wstringop-overflow. There are benefits to keeping related warnings together. > 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 patch we are discussing does that for the -Wstrict-aliasing subset of the warning. More still can be done. -Warray-bounds refers to arrays even for singleton objects purely for simplicity -- there's no difference between a singleton object and an array of one element. This has been so for a few releases now but can be changed. > 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. The purpose of the __builtin_warning I designed and implemented in 2019 is to let DCE eliminate instances issued early on about problems in conditional code that's later discovered to be unreachable. The built-in itself carries no semantic information, just the text of the warning. In straight line code like in the example I gave: int i; void *p = &i; // valid float *q = p; // valid *q = 0; // aliasing violation there isn't anything to annotate with __builtin_warning until we know that the *q is used to access i, which we can't tell in the FE. A -Wstrict-aliasing implementation early on in the middle end (when the type-morphing placement new calls are still in the IL so that the invalid *q = 0 can be distinguished from the valid new (q) int()) could emit __builtin_warning at that point and rely on later passes to eliminate it. But I'm open to suggestions for other annotations/approaches. Martin PS For reference here's the __builtin_warning patch: https://gcc.gnu.org/legacy-ml/gcc-patches/2019-10/msg01015.html
On Tue, Feb 9, 2021 at 4:37 PM Martin Sebor <msebor@gmail.com> wrote: > > On 2/9/21 12:41 AM, Richard Biener wrote: > > 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. > > Presumably you're referring to using placement new? No, I'm refering to the rules the GIMPLE IL follows. GIMPLE is no longer C or C++ and thus what is invalid in C or C++ doesn't necessarily have to be invalid in GIMPLE. > The warning > would have to be aware of it and either run before placement new > is inlined. Alternatively, the inlining could add some annotation > into the IL that the warning would then use to differentiate valid > code from invalid. At least in old versions of the C++ standard a simple "re-use" of storage starts lifetime of a new object (for certain classes of types, 'int' for example), so no placement new is needed here. > Likewise if there are other such constructs (are there?) they would > need be marked up somehow by the front end. If the frontend requires that a store does not change the memorys dynamic type (for diagnostic purposes) then it would need to mark it in a special way. By default any store in the GIMPLE IL alters the dynamic type of the destination. > I speculate that's what Jeff was suggesting by having the FE mark > up the code. > > Martin > > > > >> 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 >
On 2/10/21 3:39 AM, Richard Biener wrote: > On Tue, Feb 9, 2021 at 4:37 PM Martin Sebor <msebor@gmail.com> wrote: >> >> On 2/9/21 12:41 AM, Richard Biener wrote: >>> 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. >> >> Presumably you're referring to using placement new? > > No, I'm refering to the rules the GIMPLE IL follows. GIMPLE is no > longer C or C++ and thus what is invalid in C or C++ doesn't > necessarily have to be invalid in GIMPLE. > >> The warning >> would have to be aware of it and either run before placement new >> is inlined. Alternatively, the inlining could add some annotation >> into the IL that the warning would then use to differentiate valid >> code from invalid. > > At least in old versions of the C++ standard a simple "re-use" of > storage starts lifetime of a new object (for certain classes of types, > 'int' for example), so no placement new is needed here. I'm not familiar with this rule. Can you point me to the section of the C++ that describes it? AFAIK, C and C++ share the same aliasing restrictions with the exception of placement operator new. The intent, made explicit in Footnote 37 in C++ 98, is for the memory models of the two languages to be the same. (The same footnote is in all published revisions of C++). > >> Likewise if there are other such constructs (are there?) they would >> need be marked up somehow by the front end. > > If the frontend requires that a store does not change the memorys > dynamic type (for diagnostic purposes) then it would need to mark > it in a special way. By default any store in the GIMPLE IL alters > the dynamic type of the destination. What sort of a markup would you suggest to use and on what trees? Would a bit on INDIRECT_REF do? But unless there is some more straightforward way to change the type of a declared object than placement new, why would INDIRECT_REF alone, with no markup, not be a sufficient indication that the access doesn't modify the type of the accessed object? Martin > >> I speculate that's what Jeff was suggesting by having the FE mark >> up the code. >> >> Martin >> >>> >>>> 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 >>
On Wed, Feb 10, 2021 at 7:03 PM Martin Sebor <msebor@gmail.com> wrote: > > On 2/10/21 3:39 AM, Richard Biener wrote: > > On Tue, Feb 9, 2021 at 4:37 PM Martin Sebor <msebor@gmail.com> wrote: > >> > >> On 2/9/21 12:41 AM, Richard Biener wrote: > >>> 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. > >> > >> Presumably you're referring to using placement new? > > > > No, I'm refering to the rules the GIMPLE IL follows. GIMPLE is no > > longer C or C++ and thus what is invalid in C or C++ doesn't > > necessarily have to be invalid in GIMPLE. > > > >> The warning > >> would have to be aware of it and either run before placement new > >> is inlined. Alternatively, the inlining could add some annotation > >> into the IL that the warning would then use to differentiate valid > >> code from invalid. > > > > At least in old versions of the C++ standard a simple "re-use" of > > storage starts lifetime of a new object (for certain classes of types, > > 'int' for example), so no placement new is needed here. > > I'm not familiar with this rule. Can you point me to the section > of the C++ that describes it? For example C++14 3.8 Object lifetime. I can read 1) as int i; // lifetime starts - storage is obtained i = 1; foo (i); float *p = (float *)&i; // lifetime of *p starts - storage is obtained *p = 1.; // lifetime of i ends, storage is re-used the lifetime start of an object upon obtaining storage is a bit vague which is why GIMPLE starts the lifetime only upon the first _store_. Now I didn't find any restriction on how "storage with the proper alignment and size for type T is obtained". But of course restrictions to the above may be scattered throughout the 1000+ pages of the standard. For C similar behavior is required if you ever implement a memory allocator that re-uses storage. (yeah, I know that there's the argument that C doesn't allow to implement a memory allocator ... but that's not practical, not for GIMPLE at least) Richard. > AFAIK, C and C++ share the same aliasing restrictions with > the exception of placement operator new. The intent, made explicit > in Footnote 37 in C++ 98, is for the memory models of the two > languages to be the same. (The same footnote is in all published > revisions of C++). > > > > >> Likewise if there are other such constructs (are there?) they would > >> need be marked up somehow by the front end. > > > > If the frontend requires that a store does not change the memorys > > dynamic type (for diagnostic purposes) then it would need to mark > > it in a special way. By default any store in the GIMPLE IL alters > > the dynamic type of the destination. > > What sort of a markup would you suggest to use and on what trees? > Would a bit on INDIRECT_REF do? > > But unless there is some more straightforward way to change the type > of a declared object than placement new, why would INDIRECT_REF alone, > with no markup, not be a sufficient indication that the access doesn't > modify the type of the accessed object? > > Martin > > > > >> I speculate that's what Jeff was suggesting by having the FE mark > >> up the code. > >> > >> Martin > >> > >>> > >>>> 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 > >> >
On 2/11/21 1:09 AM, Richard Biener wrote: > On Wed, Feb 10, 2021 at 7:03 PM Martin Sebor <msebor@gmail.com> wrote: >> >> On 2/10/21 3:39 AM, Richard Biener wrote: >>> On Tue, Feb 9, 2021 at 4:37 PM Martin Sebor <msebor@gmail.com> wrote: >>>> >>>> On 2/9/21 12:41 AM, Richard Biener wrote: >>>>> 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. >>>> >>>> Presumably you're referring to using placement new? >>> >>> No, I'm refering to the rules the GIMPLE IL follows. GIMPLE is no >>> longer C or C++ and thus what is invalid in C or C++ doesn't >>> necessarily have to be invalid in GIMPLE. >>> >>>> The warning >>>> would have to be aware of it and either run before placement new >>>> is inlined. Alternatively, the inlining could add some annotation >>>> into the IL that the warning would then use to differentiate valid >>>> code from invalid. >>> >>> At least in old versions of the C++ standard a simple "re-use" of >>> storage starts lifetime of a new object (for certain classes of types, >>> 'int' for example), so no placement new is needed here. >> >> I'm not familiar with this rule. Can you point me to the section >> of the C++ that describes it? > > For example C++14 3.8 Object lifetime. I can read 1) as > > int i; // lifetime starts - storage is obtained > i = 1; > foo (i); > float *p = (float *)&i; // lifetime of *p starts - storage is obtained At this point p is just a pointer that points to the int i. The only new object here is the pointer p. > *p = 1.; // lifetime of i ends, storage is re-used I don't think that's the intended reading. The storage of an object can be reused by constructing another object in it, and the only way to do that in C++ is by placement new. Even if the store to *p was valid, it doesn't change i's type (only placement new can do that), so there is no way to access that value. GCC relies on that by assuming that a store through a pointer to one type doesn't change the value stored in declared objects of another type. It's a common bug to disregard this rule and that's I'm interested in detecting. I.e., aliasing violations. > the lifetime start of an object upon obtaining storage is a bit vague > which is why GIMPLE starts the lifetime only upon the first _store_. > > Now I didn't find any restriction on how "storage with the proper > alignment and size for type T is obtained". But of course restrictions > to the above may be scattered throughout the 1000+ pages of > the standard. Storage for an object is obtained either by its declaration or by an allocation call. > > For C similar behavior is required if you ever implement a > memory allocator that re-uses storage. (yeah, I know that > there's the argument that C doesn't allow to implement a > memory allocator ... but that's not practical, not for GIMPLE > at least) Allocated storage is subject to slightly different rules that those that apply to declared objects and I'm focusing only on the latter. (The argument that C doesn't make it possible to implement a memory allocator is about the strictly conforming subset of C, i.e., the subset that excludes unspecified and implementation-defined behavior.) But I'm still interested in: What sort of a markup would you suggest to use and on what trees? Would a bit on INDIRECT_REF do? and ...unless there is some more straightforward way to change the type of a declared object than placement new (I don't know of one), would INDIRECT_REF alone, with no markup, be a sufficient indication that the access doesn't modify the type of the accessed object? Thanks Martin > > Richard. > >> AFAIK, C and C++ share the same aliasing restrictions with >> the exception of placement operator new. The intent, made explicit >> in Footnote 37 in C++ 98, is for the memory models of the two >> languages to be the same. (The same footnote is in all published >> revisions of C++). >> >>> >>>> Likewise if there are other such constructs (are there?) they would >>>> need be marked up somehow by the front end. >>> >>> If the frontend requires that a store does not change the memorys >>> dynamic type (for diagnostic purposes) then it would need to mark >>> it in a special way. By default any store in the GIMPLE IL alters >>> the dynamic type of the destination. >> >> What sort of a markup would you suggest to use and on what trees? >> Would a bit on INDIRECT_REF do? >> >> But unless there is some more straightforward way to change the type >> of a declared object than placement new, why would INDIRECT_REF alone, >> with no markup, not be a sufficient indication that the access doesn't >> modify the type of the accessed object? >> >> Martin >> >>> >>>> I speculate that's what Jeff was suggesting by having the FE mark >>>> up the code. >>>> >>>> Martin >>>> >>>>> >>>>>> 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 >>>> >>
On Thu, Feb 11, 2021 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote: > > On 2/11/21 1:09 AM, Richard Biener wrote: > > On Wed, Feb 10, 2021 at 7:03 PM Martin Sebor <msebor@gmail.com> wrote: > >> > >> On 2/10/21 3:39 AM, Richard Biener wrote: > >>> On Tue, Feb 9, 2021 at 4:37 PM Martin Sebor <msebor@gmail.com> wrote: > >>>> > >>>> On 2/9/21 12:41 AM, Richard Biener wrote: > >>>>> 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. > >>>> > >>>> Presumably you're referring to using placement new? > >>> > >>> No, I'm refering to the rules the GIMPLE IL follows. GIMPLE is no > >>> longer C or C++ and thus what is invalid in C or C++ doesn't > >>> necessarily have to be invalid in GIMPLE. > >>> > >>>> The warning > >>>> would have to be aware of it and either run before placement new > >>>> is inlined. Alternatively, the inlining could add some annotation > >>>> into the IL that the warning would then use to differentiate valid > >>>> code from invalid. > >>> > >>> At least in old versions of the C++ standard a simple "re-use" of > >>> storage starts lifetime of a new object (for certain classes of types, > >>> 'int' for example), so no placement new is needed here. > >> > >> I'm not familiar with this rule. Can you point me to the section > >> of the C++ that describes it? > > > > For example C++14 3.8 Object lifetime. I can read 1) as > > > > int i; // lifetime starts - storage is obtained > > i = 1; > > foo (i); > > float *p = (float *)&i; // lifetime of *p starts - storage is obtained > > At this point p is just a pointer that points to the int i. > The only new object here is the pointer p. > > > *p = 1.; // lifetime of i ends, storage is re-used > > I don't think that's the intended reading. The storage of an object > can be reused by constructing another object in it, and the only way > to do that in C++ is by placement new. > > Even if the store to *p was valid, it doesn't change i's type (only > placement new can do that), so there is no way to access that value. > GCC relies on that by assuming that a store through a pointer to one > type doesn't change the value stored in declared objects of another > type. It's a common bug to disregard this rule and that's I'm > interested in detecting. I.e., aliasing violations. > > > the lifetime start of an object upon obtaining storage is a bit vague > > which is why GIMPLE starts the lifetime only upon the first _store_. > > > > Now I didn't find any restriction on how "storage with the proper > > alignment and size for type T is obtained". But of course restrictions > > to the above may be scattered throughout the 1000+ pages of > > the standard. > > Storage for an object is obtained either by its declaration or by > an allocation call. > > > > > For C similar behavior is required if you ever implement a > > memory allocator that re-uses storage. (yeah, I know that > > there's the argument that C doesn't allow to implement a > > memory allocator ... but that's not practical, not for GIMPLE > > at least) > > Allocated storage is subject to slightly different rules that those > that apply to declared objects and I'm focusing only on the latter. > > (The argument that C doesn't make it possible to implement a memory > allocator is about the strictly conforming subset of C, i.e., > the subset that excludes unspecified and implementation-defined > behavior.) > > But I'm still interested in: > > What sort of a markup would you suggest to use and on what trees? > Would a bit on INDIRECT_REF do? > > and > > ...unless there is some more straightforward way to change the type > of a declared object than placement new (I don't know of one), would > INDIRECT_REF alone, with no markup, be a sufficient indication that > the access doesn't modify the type of the accessed object? I do not have definite answers here. But it seems you're after the fact that some languages do not allow changing the dynamic type of declared objects (C++ does, kind-of, for "storage" portions!). So to me it seems that not the accesses but instead the objects need to be marked (thus the DECL node in this case). But again, beware of the funny ideas of the C++ commitee, making struct { int n; char c[4]; } x, y; new (x.c) int)(); *(int *)x.c = 1; y = x; assert (*(int *)y.c == 1); valid. So what you design today will break tomorrow :P Richard. > Thanks > Martin > > > > > Richard. > > > >> AFAIK, C and C++ share the same aliasing restrictions with > >> the exception of placement operator new. The intent, made explicit > >> in Footnote 37 in C++ 98, is for the memory models of the two > >> languages to be the same. (The same footnote is in all published > >> revisions of C++). > >> > >>> > >>>> Likewise if there are other such constructs (are there?) they would > >>>> need be marked up somehow by the front end. > >>> > >>> If the frontend requires that a store does not change the memorys > >>> dynamic type (for diagnostic purposes) then it would need to mark > >>> it in a special way. By default any store in the GIMPLE IL alters > >>> the dynamic type of the destination. > >> > >> What sort of a markup would you suggest to use and on what trees? > >> Would a bit on INDIRECT_REF do? > >> > >> But unless there is some more straightforward way to change the type > >> of a declared object than placement new, why would INDIRECT_REF alone, > >> with no markup, not be a sufficient indication that the access doesn't > >> modify the type of the accessed object? > >> > >> Martin > >> > >>> > >>>> I speculate that's what Jeff was suggesting by having the FE mark > >>>> up the code. > >>>> > >>>> Martin > >>>> > >>>>> > >>>>>> 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 > >>>> > >> >
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564483.html The review of this patch digressed into a design discussion of a new, more capable implementation of -Wstrict-aliasing, but the proposed patch turning just this one instance of -Warray-bounds into -Wstrict-aliasing and making it subject to -fstrict-aliasing wasn't decided. PR 98503 was raised by someone working with the kernel which uses -fno-strict-aliasing, and so to them the warning isn't useful. But since the warning does find potential bugs when strict aliasing is in effect, I'd still like to consider this patch for GCC 11 so that the kernel (and other such projects) doesn't have to deal with the false positives. If/when we add a new, dedicated solution for -Wstrict-aliasing I'll move this instance from gimple-array-bounds.cc there. Martin
On 3/18/2021 4:18 PM, Martin Sebor via Gcc-patches wrote: > Ping: > https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564483.html > > The review of this patch digressed into a design discussion of a new, > more capable implementation of -Wstrict-aliasing, but the proposed > patch turning just this one instance of -Warray-bounds into > -Wstrict-aliasing and making it subject to -fstrict-aliasing wasn't > decided. PR 98503 was raised by someone working with the kernel > which uses -fno-strict-aliasing, and so to them the warning isn't > useful. But since the warning does find potential bugs when strict > aliasing is in effect, I'd still like to consider this patch for > GCC 11 so that the kernel (and other such projects) doesn't have > to deal with the false positives. > > If/when we add a new, dedicated solution for -Wstrict-aliasing I'll > move this instance from gimple-array-bounds.cc there. I'm still not comfortable with the bleeding of strict aliasing bits into the gimple array-bounds checking bits. I think that needs to be fixed in a cleaner manner before this can go forward. jeff
On 3/20/21 10:48 AM, Jeff Law via Gcc-patches wrote: > > On 3/18/2021 4:18 PM, Martin Sebor via Gcc-patches wrote: >> Ping: >> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564483.html >> >> The review of this patch digressed into a design discussion of a new, >> more capable implementation of -Wstrict-aliasing, but the proposed >> patch turning just this one instance of -Warray-bounds into >> -Wstrict-aliasing and making it subject to -fstrict-aliasing wasn't >> decided. PR 98503 was raised by someone working with the kernel >> which uses -fno-strict-aliasing, and so to them the warning isn't >> useful. But since the warning does find potential bugs when strict >> aliasing is in effect, I'd still like to consider this patch for >> GCC 11 so that the kernel (and other such projects) doesn't have >> to deal with the false positives. >> >> If/when we add a new, dedicated solution for -Wstrict-aliasing I'll >> move this instance from gimple-array-bounds.cc there. > > I'm still not comfortable with the bleeding of strict aliasing bits into > the gimple array-bounds checking bits. I think that needs to be fixed > in a cleaner manner before this can go forward. I can't really think of any other way to resolve PR 98503 for GCC 11 than something like this. I suppose I could make the existing -Warra-bounds conditional on -fstrict-aliasing (without actually emitting -Wstrict-aliasing). That way the kernel would be able to disable it without giving up the aspects of the warning that are relevant to them. Would that be acceptable to you? If not, what would be? Martin
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. diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array-bounds.cc index 2576556f76b..f6b2af0d681 100644 --- a/gcc/gimple-array-bounds.cc +++ b/gcc/gimple-array-bounds.cc @@ -670,6 +670,8 @@ array_bounds_checker::check_mem_ref (location_t location, tree ref, axssize = wi::to_offset (access_size); const bool uboob = !lboob && offrange[0] + axssize > ubound; + /* Set to OFFRANGE converted to index range. */ + offset_int idxrange[2] = { offrange[0], offrange[1] }; if (lboob || uboob) { /* Treat a reference to a non-array object as one to an array @@ -681,43 +683,84 @@ array_bounds_checker::check_mem_ref (location_t location, tree ref, to compute the index to print in the diagnostic; arrays in MEM_REF don't mean anything. A type with no size like void is as good as having a size of 1. */ - tree type = TREE_TYPE (ref); - while (TREE_CODE (type) == ARRAY_TYPE) - type = TREE_TYPE (type); + tree type = strip_array_types (TREE_TYPE (ref)); if (tree size = TYPE_SIZE_UNIT (type)) { - offrange[0] = offrange[0] / wi::to_offset (size); - offrange[1] = offrange[1] / wi::to_offset (size); + idxrange[0] = offrange[0] / wi::to_offset (size); + idxrange[1] = offrange[1] / wi::to_offset (size); } } if (lboob) { - if (offrange[0] == offrange[1]) + if (idxrange[0] == idxrange[1]) warned = warning_at (location, OPT_Warray_bounds, "array subscript %wi is outside array bounds " "of %qT", - offrange[0].to_shwi (), reftype); + idxrange[0].to_shwi (), reftype); else warned = warning_at (location, OPT_Warray_bounds, "array subscript [%wi, %wi] is outside " "array bounds of %qT", - offrange[0].to_shwi (), - offrange[1].to_shwi (), reftype); + idxrange[0].to_shwi (), + idxrange[1].to_shwi (), reftype); } else if (uboob && !ignore_off_by_one) { + bool done = false; + tree backtype = reftype; if (alloc_stmt) /* If the memory was dynamically allocated refer to it as if it were an untyped array of bytes. */ backtype = build_array_type_nelts (unsigned_char_type_node, arrbounds[1].to_uhwi ()); + else if (cref_of_mref) + { + /* For a COMPONENT_REF (struct S, MEM_REF (T, ...), fld) see + if the offset of fld's last byte is in bounds of struct S, + and if so, issue -Wstrict-aliasing instead. */ + tree fld = TREE_OPERAND (cref_of_mref, 1); + offset_int fldoff = offrange[0] + wi::to_offset (byte_position (fld)); + if (tree fldsiz = DECL_SIZE_UNIT (fld)) + if (tree_fits_uhwi_p (fldsiz)) + fldoff += wi::to_offset (fldsiz); + + if (fldoff <= arrbounds[1]) + { + if (flag_strict_aliasing) + { + /* Only warn when -fstrict-aliasing is enabled (as per + the manual). */ + if (offrange[0] == 0) + warned = warning_at (location, OPT_Wstrict_aliasing, + "access to %qT by an lvalue of %qT " + "violates strict-aliasing rules", + TREE_TYPE (reftype), axstype); + else + warned = warning_at (location, OPT_Wstrict_aliasing, + "access to %qT by %<%T[%wi]%> " + "violates strict-aliasing rules", + TREE_TYPE (reftype), axstype, + offrange[0].to_shwi ()); + } + done = true; + } + } - warned = warning_at (location, OPT_Warray_bounds, - "array subscript %<%T[%wi]%> is partly " - "outside array bounds of %qT", - axstype, offrange[0].to_shwi (), backtype); + if (!done) + { + if (offrange[0] == 0) + warned = warning_at (location, OPT_Warray_bounds, + "access by %qT is partly outside array " + "bounds of %qT", + axstype, backtype); + else + warned = warning_at (location, OPT_Warray_bounds, + "array subscript %<%T[%wi]%> is partly " + "outside array bounds of %qT", + axstype, idxrange[0].to_shwi (), backtype); + } } if (warned) @@ -906,13 +949,21 @@ array_bounds_checker::check_array_bounds (tree *tp, int *walk_subtree, warned = checker->check_array_ref (location, t, false/*ignore_off_by_one*/); else if (TREE_CODE (t) == MEM_REF) - warned = checker->check_mem_ref (location, t, - false /*ignore_off_by_one*/); + { + warned = checker->check_mem_ref (location, t, + false /*ignore_off_by_one*/); + checker->cref_of_mref = NULL_TREE; + } else if (TREE_CODE (t) == ADDR_EXPR) { checker->check_addr_expr (location, t); *walk_subtree = FALSE; } + else if (TREE_CODE (t) == COMPONENT_REF + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF) + /* Remember T for the subsequent MEM_REF handler. */ + checker->cref_of_mref = t; + /* Propagate the no-warning bit to the outer expression. */ if (warned) TREE_NO_WARNING (t) = true; diff --git a/gcc/gimple-array-bounds.h b/gcc/gimple-array-bounds.h index 1bfa2d45870..02d5bacec8f 100644 --- a/gcc/gimple-array-bounds.h +++ b/gcc/gimple-array-bounds.h @@ -26,7 +26,7 @@ class array_bounds_checker public: array_bounds_checker (struct function *fun, class vr_values *v) - : fun (fun), ranges (v) { } + : fun (fun), ranges (v), cref_of_mref () { } void check (); private: @@ -36,8 +36,12 @@ private: void check_addr_expr (location_t, tree); const value_range *get_value_range (const_tree op); + /* The current function being processed. */ struct function *fun; + /* Pointer to an instance of the range analyzer. */ class vr_values *ranges; + /* A COMPONENT_REF with a MEM_REF operand. */ + tree cref_of_mref; }; #endif // GCC_GIMPLE_ARRAY_BOUNDS_H diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-10.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-10.C index 22466977b68..ea8691531c2 100644 --- a/gcc/testsuite/g++.dg/warn/Warray-bounds-10.C +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-10.C @@ -19,9 +19,9 @@ void warn_op_new () { T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new\\\(\(long \)?unsigned int\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); @@ -45,9 +45,9 @@ void warn_op_array_new () T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new \\\[]\\\(\(long \)?unsigned int\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-11.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-11.C index 9875e29085d..d934b800c73 100644 --- a/gcc/testsuite/g++.dg/warn/Warray-bounds-11.C +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-11.C @@ -21,9 +21,9 @@ void warn_op_new () { T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new\\\(std::size_t, const std::nothrow_t.\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); @@ -47,9 +47,9 @@ void warn_op_array_new () T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new \\\[]\\\(std::size_t, const std::nothrow_t&\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-12.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-12.C index 9e8b6048944..a8d1099e041 100644 --- a/gcc/testsuite/g++.dg/warn/Warray-bounds-12.C +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-12.C @@ -21,9 +21,9 @@ void warn_new () { T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new\\\(\(long \)?unsigned int\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); @@ -47,9 +47,9 @@ void warn_array_new () T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new \\\[]\\\(\(long \)?unsigned int\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-13.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-13.C index 42fb809de3c..705029f2718 100644 --- a/gcc/testsuite/g++.dg/warn/Warray-bounds-13.C +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-13.C @@ -25,9 +25,9 @@ void warn_nothrow_new () { T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new\\\(std::size_t, const std::nothrow_t.\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); @@ -51,9 +51,9 @@ void warn_nothrow_array_new () T (int32_t, 0, 0); // { dg-warning "array subscript 0 is outside array bounds of 'int32_t \\\[0]'" } // { dg-message "referencing an object of size \\d allocated by 'void\\\* operator new \\\[]\\\(std::size_t, const std::nothrow_t&\\\)'" "note" { target *-*-* } .-1 } - T (int32_t, 1, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[1]'" } - T (int32_t, 2, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[2]'" } - T (int32_t, 3, 0); // { dg-warning "array subscript 'int32_t {aka int}\\\[0]' is partly outside array bounds of 'unsigned char \\\[3]'" } + T (int32_t, 1, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[1]'" } + T (int32_t, 2, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[2]'" } + T (int32_t, 3, 0); // { dg-warning "access by 'int32_t' {aka 'int'} is partly outside array bounds of 'unsigned char \\\[3]'" } T (int32_t, 4, 0); diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-63.c b/gcc/testsuite/gcc.dg/Warray-bounds-63.c index a3fc9188211..0ef4a469f01 100644 --- a/gcc/testsuite/gcc.dg/Warray-bounds-63.c +++ b/gcc/testsuite/gcc.dg/Warray-bounds-63.c @@ -31,10 +31,11 @@ void word_store_to_decl (void) char *p = (char*)&s; - int16_t *q = (int16_t*)(p + 1); + typedef __attribute__ ((may_alias)) int16_t alias16_t; + alias16_t *q = (alias16_t*)(p + 1); q[0] = 0; q[1] = 1; - q[2] = 2; // { dg-warning "array subscript 'int16_t {aka short int}\\\[2]' is partly outside array bounds of 'struct S6\\\[1]'" } + q[2] = 2; // { dg-warning "array subscript 'alias16_t {aka short int}\\\[2]' is partly outside array bounds of 'struct S6\\\[1]'" } sink (&s); } diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-66.c b/gcc/testsuite/gcc.dg/Warray-bounds-66.c index c61891f5c07..85906fcf7b7 100644 --- a/gcc/testsuite/gcc.dg/Warray-bounds-66.c +++ b/gcc/testsuite/gcc.dg/Warray-bounds-66.c @@ -119,7 +119,7 @@ void test_alloca_int16_range (unsigned n) { p = alloca (UR (0, 1)); // { dg-message "object of size between 0 and 1 allocated by '__builtin_alloca'" } sink (p); - T (p[0]); // { dg-warning "subscript 'int16_t {aka short int}\\\[0\\\]' is partly outside array bounds of 'unsigned char\\\[1]'" } + T (p[0]); // { dg-warning "access by 'int16_t' {aka 'short int'} is partly outside array bounds of 'unsigned char\\\[1]'" } T (p[1]); // { dg-warning "subscript 1 is outside array bounds of 'int16_t\\\[0]'" } } diff --git a/gcc/testsuite/gcc.dg/Wstrict-aliasing-2.c b/gcc/testsuite/gcc.dg/Wstrict-aliasing-2.c new file mode 100644 index 00000000000..cc1d548239f --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstrict-aliasing-2.c @@ -0,0 +1,85 @@ +/* PR middle-end/98503 -Warray-bounds when -Wstrict-aliasing would be more + appropriate + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +void* malloc (__SIZE_TYPE__); + + +struct A { int i1, i2; }; +struct B { int i1, i2, i3; }; + +extern struct A a; +extern struct A a2[2]; + + +void warn_strict_alias_B_i1_A (void) +{ + struct B *p = (struct B*)&a; + p->i1 = 0; // { dg-warning "\\\[-Wstrict-aliasing" } +} + +void warn_strict_alias_B_i2_A (void) +{ + struct B *p = (struct B*)&a; + p->i2 = 0; // { dg-warning "access to 'struct A' by an lvalue of 'struct B' violates strict-aliasing rules" } +} + +void nowarn_strict_alias_B_i3_A (void) +{ + struct B *p = (struct B*)&a; + p->i3 = 0; // { dg-warning "\\\[-Warray-bounds" } +} + + +void warn_strict_alias_B_A2 (void) +{ + struct B *p = (struct B*)&a2; + /* Of the aliasing violations below only last one is diagnosed because + it's the only one where the member access exceeds the boundary of + the destination object. */ + p[0].i1 = 0; // { dg-warning "\\\[-Wstrict-aliasing" "pr?????" { xfail *-*-* } } + p[0].i2 = 0; // { dg-warning "\\\[-Wstrict-aliasing" "pr?????" { xfail *-*-* } } + p[0].i3 = 0; // { dg-warning "\\\[-Wstrict-aliasing" "pr?????" { xfail *-*-* } } + p[1].i1 = 0; // { dg-warning "\\\[-Wstrict-aliasing" } + p[1].i2 = 0; // { dg-warning "\\\[-Warray-bounds" } + p[1].i3 = 0; // { dg-warning "\\\[-Warray-bounds" } +} + +void* nowarn_access_A_i1_malloc_A (void) +{ + struct B *p = (struct B*)malloc (sizeof (struct A)); + struct A *q = (struct A*)p; + q->i1 = 0; + return p; +} + +void* nowarn_access_A_i2_malloc_A (void) +{ + struct B *p = (struct B*)malloc (sizeof (struct A)); + struct A *q = (struct A*)p; + q->i2 = 0; + return p; +} + + +struct B* warn_array_bounds_access_B_i1_malloc_A (void) +{ + struct B *p = malloc (sizeof (struct A)); + p->i1 = 0; // { dg-warning "\\\[-Warray-bounds" } + return p; +} + +struct B* warn_array_bounds_access_B_i2_malloc_A (void) +{ + struct B *p = malloc (sizeof (struct A)); + p->i2 = 0; // { dg-warning "\\\[-Warray-bounds" } + return p; +} + +struct B* warn_array_bounds_access_B_i3_malloc_A (void) +{ + struct B *p = malloc (sizeof (struct A)); + p->i3 = 0; // { dg-warning "\\\[-Warray-bounds" } + return p; +} diff --git a/gcc/testsuite/gcc.dg/Wstrict-aliasing-3.c b/gcc/testsuite/gcc.dg/Wstrict-aliasing-3.c new file mode 100644 index 00000000000..5fe17ef22dc --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstrict-aliasing-3.c @@ -0,0 +1,71 @@ +/* PR middle-end/98503 -Warray-bounds when -Wstrict-aliasing would be more + appropriate + { dg-do compile } + { dg-options "-O2 -Wall -fno-strict-aliasing" } */ + +void* malloc (__SIZE_TYPE__); + + +struct A { int i1, i2; }; +struct B { int i1, i2, i3; }; + +extern struct A a; +extern struct A a2[2]; + + +void nowarn_strict_alias_B_i1_A (void) +{ + struct B *p = (struct B*)&a; + p->i1 = 0; // only warn with -fstrict-aliasing +} + +void nowarn_strict_alias_B_i2_A (void) +{ + struct B *p = (struct B*)&a; + p->i2 = 0; // only warn with -fstrict-aliasing +} + +void nowarn_strict_alias_B_i3_A (void) +{ + struct B *p = (struct B*)&a; + p->i3 = 0; // { dg-warning "\\\[-Warray-bounds" } +} + + +void* nowarn_access_A_i1_malloc_A (void) +{ + struct B *p = (struct B*)malloc (sizeof (struct A)); + struct A *q = (struct A*)p; + q->i1 = 0; + return p; +} + +void* nowarn_access_A_i2_malloc_A (void) +{ + struct B *p = (struct B*)malloc (sizeof (struct A)); + struct A *q = (struct A*)p; + q->i2 = 0; + return p; +} + + +struct B* warn_array_bounds_access_B_i1_malloc_A (void) +{ + struct B *p = malloc (sizeof (struct A)); + p->i1 = 0; // { dg-warning "\\\[-Warray-bounds" } + return p; +} + +struct B* warn_array_bounds_access_B_i2_malloc_A (void) +{ + struct B *p = malloc (sizeof (struct A)); + p->i2 = 0; // { dg-warning "\\\[-Warray-bounds" } + return p; +} + +struct B* warn_array_bounds_access_B_i3_malloc_A (void) +{ + struct B *p = malloc (sizeof (struct A)); + p->i3 = 0; // { dg-warning "\\\[-Warray-bounds" } + return p; +}