* [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) @ 2021-01-20 0:56 Martin Sebor 2021-01-29 17:22 ` PING " Martin Sebor 2021-02-08 19:59 ` Jeff Law 0 siblings, 2 replies; 24+ messages in thread From: Martin Sebor @ 2021-01-20 0:56 UTC (permalink / raw) To: gcc-patches [-- Attachment #1: Type: text/plain, Size: 505 bytes --] Similar to the problem reported for -Wstringop-overflow in pr98266 and already fixed, -Warray-bounds is also susceptible to false positives in assignments and copies involving virtual inheritance. Because the two warnings don't share code yet (hopefully in GCC 12) the attached patch adds its own workaround for this problem to gimple-array-bounds.cc, this one slightly more crude because of the limited insight the array bounds checking has into the checked expressions. Tested on x86_64-linux. Martin [-- Attachment #2: gcc-98266.diff --] [-- Type: text/x-patch, Size: 1997 bytes --] PR middle-end/98266 - bogus array subscript is partly outside array bounds on virtual inheritance gcc/ChangeLog: PR middle-end/98266 * gimple-array-bounds.cc (array_bounds_checker::check_array_bounds): Avoid checking references involving artificial members. gcc/testsuite/ChangeLog: PR middle-end/98266 * g++.dg/warn/Warray-bounds-15.C: New test. diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array-bounds.cc index 2576556f76b..413deacece4 100644 --- a/gcc/gimple-array-bounds.cc +++ b/gcc/gimple-array-bounds.cc @@ -911,8 +911,16 @@ array_bounds_checker::check_array_bounds (tree *tp, int *walk_subtree, else if (TREE_CODE (t) == ADDR_EXPR) { checker->check_addr_expr (location, t); - *walk_subtree = FALSE; + *walk_subtree = false; } + else if (TREE_CODE (t) == COMPONENT_REF + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF + && DECL_ARTIFICIAL (TREE_OPERAND (t, 1))) + /* Hack: Skip MEM_REF checking for artificial members to avoid false + positives for C++ classes with virtual bases. See pr98266 and + pr97595. */ + *walk_subtree = false; + /* Propagate the no-warning bit to the outer expression. */ if (warned) TREE_NO_WARNING (t) = true; diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C new file mode 100644 index 00000000000..eb75527dc3d --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C @@ -0,0 +1,30 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +#if __cplusplus < 201103L +# define noexcept throw () +#endif + +struct A +{ + virtual ~A () noexcept; + const char* s; +}; + +struct B: virtual A { }; +struct C: virtual B { }; +struct D: virtual A { }; // { dg-bogus "\\\[-Warray-bounds" } + +struct E: virtual B, virtual D +{ + E (const char*); +}; + +void f (E); + +void g () +{ + f (E ("")); +} ^ permalink raw reply [flat|nested] 24+ messages in thread
* PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-01-20 0:56 [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) Martin Sebor @ 2021-01-29 17:22 ` Martin Sebor 2021-02-06 17:12 ` PING 2 " Martin Sebor 2021-02-08 19:59 ` Jeff Law 1 sibling, 1 reply; 24+ messages in thread From: Martin Sebor @ 2021-01-29 17:22 UTC (permalink / raw) To: gcc-patches Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563894.html On 1/19/21 5:56 PM, Martin Sebor wrote: > Similar to the problem reported for -Wstringop-overflow in pr98266 > and already fixed, -Warray-bounds is also susceptible to false > positives in assignments and copies involving virtual inheritance. > Because the two warnings don't share code yet (hopefully in GCC 12) > the attached patch adds its own workaround for this problem to > gimple-array-bounds.cc, this one slightly more crude because of > the limited insight the array bounds checking has into the checked > expressions. > > Tested on x86_64-linux. > > Martin ^ permalink raw reply [flat|nested] 24+ messages in thread
* PING 2 [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-01-29 17:22 ` PING " Martin Sebor @ 2021-02-06 17:12 ` Martin Sebor 0 siblings, 0 replies; 24+ messages in thread From: Martin Sebor @ 2021-02-06 17:12 UTC (permalink / raw) To: gcc-patches Ping 2: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563894.html On 1/29/21 10:22 AM, Martin Sebor wrote: > Ping: > https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563894.html > > On 1/19/21 5:56 PM, Martin Sebor wrote: >> Similar to the problem reported for -Wstringop-overflow in pr98266 >> and already fixed, -Warray-bounds is also susceptible to false >> positives in assignments and copies involving virtual inheritance. >> Because the two warnings don't share code yet (hopefully in GCC 12) >> the attached patch adds its own workaround for this problem to >> gimple-array-bounds.cc, this one slightly more crude because of >> the limited insight the array bounds checking has into the checked >> expressions. >> >> Tested on x86_64-linux. >> >> Martin > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-01-20 0:56 [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) Martin Sebor 2021-01-29 17:22 ` PING " Martin Sebor @ 2021-02-08 19:59 ` Jeff Law 2021-02-08 21:56 ` Martin Sebor 1 sibling, 1 reply; 24+ messages in thread From: Jeff Law @ 2021-02-08 19:59 UTC (permalink / raw) To: Martin Sebor, gcc-patches On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: > Similar to the problem reported for -Wstringop-overflow in pr98266 > and already fixed, -Warray-bounds is also susceptible to false > positives in assignments and copies involving virtual inheritance. > Because the two warnings don't share code yet (hopefully in GCC 12) > the attached patch adds its own workaround for this problem to > gimple-array-bounds.cc, this one slightly more crude because of > the limited insight the array bounds checking has into the checked > expressions. > > Tested on x86_64-linux. > > Martin > > gcc-98266.diff > > PR middle-end/98266 - bogus array subscript is partly outside array bounds on virtual inheritance > > gcc/ChangeLog: > > PR middle-end/98266 > * gimple-array-bounds.cc (array_bounds_checker::check_array_bounds): > Avoid checking references involving artificial members. > > gcc/testsuite/ChangeLog: > > PR middle-end/98266 > * g++.dg/warn/Warray-bounds-15.C: New test. It seems to me that we've got the full statement at some point and thus the full expression so at some point couldn't we detect when TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using TYPE_SIZE_UNIT rather than DECL_SIZE_UNIT in gimple-array-bounds.cc Am I missing something? jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-08 19:59 ` Jeff Law @ 2021-02-08 21:56 ` Martin Sebor 2021-02-08 22:26 ` Jeff Law 2021-02-25 23:15 ` Jeff Law 0 siblings, 2 replies; 24+ messages in thread From: Martin Sebor @ 2021-02-08 21:56 UTC (permalink / raw) To: Jeff Law, gcc-patches On 2/8/21 12:59 PM, Jeff Law wrote: > > > On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >> Similar to the problem reported for -Wstringop-overflow in pr98266 >> and already fixed, -Warray-bounds is also susceptible to false >> positives in assignments and copies involving virtual inheritance. >> Because the two warnings don't share code yet (hopefully in GCC 12) >> the attached patch adds its own workaround for this problem to >> gimple-array-bounds.cc, this one slightly more crude because of >> the limited insight the array bounds checking has into the checked >> expressions. >> >> Tested on x86_64-linux. >> >> Martin >> >> gcc-98266.diff >> >> PR middle-end/98266 - bogus array subscript is partly outside array bounds on virtual inheritance >> >> gcc/ChangeLog: >> >> PR middle-end/98266 >> * gimple-array-bounds.cc (array_bounds_checker::check_array_bounds): >> Avoid checking references involving artificial members. >> >> gcc/testsuite/ChangeLog: >> >> PR middle-end/98266 >> * g++.dg/warn/Warray-bounds-15.C: New test. > It seems to me that we've got the full statement at some point and thus > the full expression so at some point couldn't we detect when > TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using TYPE_SIZE_UNIT > rather than DECL_SIZE_UNIT in gimple-array-bounds.cc > > Am I missing something? The expression we're looking at when the false positive is issued is the MEM_REF in the LHS of: MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void *)&_ZTC1E24_1D + 24B]; TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between DECL_SIZE and TYPE_SIZE. Martin ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-08 21:56 ` Martin Sebor @ 2021-02-08 22:26 ` Jeff Law 2021-02-08 22:44 ` Martin Sebor 2021-02-25 23:15 ` Jeff Law 1 sibling, 1 reply; 24+ messages in thread From: Jeff Law @ 2021-02-08 22:26 UTC (permalink / raw) To: Martin Sebor, gcc-patches On 2/8/21 2:56 PM, Martin Sebor wrote: > On 2/8/21 12:59 PM, Jeff Law wrote: >> >> >> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>> and already fixed, -Warray-bounds is also susceptible to false >>> positives in assignments and copies involving virtual inheritance. >>> Because the two warnings don't share code yet (hopefully in GCC 12) >>> the attached patch adds its own workaround for this problem to >>> gimple-array-bounds.cc, this one slightly more crude because of >>> the limited insight the array bounds checking has into the checked >>> expressions. >>> >>> Tested on x86_64-linux. >>> >>> Martin >>> >>> gcc-98266.diff >>> >>> PR middle-end/98266 - bogus array subscript is partly outside array >>> bounds on virtual inheritance >>> >>> gcc/ChangeLog: >>> >>> PR middle-end/98266 >>> * gimple-array-bounds.cc >>> (array_bounds_checker::check_array_bounds): >>> Avoid checking references involving artificial members. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR middle-end/98266 >>> * g++.dg/warn/Warray-bounds-15.C: New test. >> It seems to me that we've got the full statement at some point and thus >> the full expression so at some point couldn't we detect when >> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using TYPE_SIZE_UNIT >> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >> >> Am I missing something? > > The expression we're looking at when the false positive is issued > is the MEM_REF in the LHS of: > > MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void > *)&_ZTC1E24_1D + 24B]; > > TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and > TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between > DECL_SIZE and TYPE_SIZE. So that seems like it's a different issue then, unrelated to 97595. Right? Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-08 22:26 ` Jeff Law @ 2021-02-08 22:44 ` Martin Sebor 2021-02-08 23:11 ` Jeff Law 2021-02-25 23:40 ` Jeff Law 0 siblings, 2 replies; 24+ messages in thread From: Martin Sebor @ 2021-02-08 22:44 UTC (permalink / raw) To: Jeff Law, gcc-patches On 2/8/21 3:26 PM, Jeff Law wrote: > > > On 2/8/21 2:56 PM, Martin Sebor wrote: >> On 2/8/21 12:59 PM, Jeff Law wrote: >>> >>> >>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>>> and already fixed, -Warray-bounds is also susceptible to false >>>> positives in assignments and copies involving virtual inheritance. >>>> Because the two warnings don't share code yet (hopefully in GCC 12) >>>> the attached patch adds its own workaround for this problem to >>>> gimple-array-bounds.cc, this one slightly more crude because of >>>> the limited insight the array bounds checking has into the checked >>>> expressions. >>>> >>>> Tested on x86_64-linux. >>>> >>>> Martin >>>> >>>> gcc-98266.diff >>>> >>>> PR middle-end/98266 - bogus array subscript is partly outside array >>>> bounds on virtual inheritance >>>> >>>> gcc/ChangeLog: >>>> >>>> PR middle-end/98266 >>>> * gimple-array-bounds.cc >>>> (array_bounds_checker::check_array_bounds): >>>> Avoid checking references involving artificial members. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR middle-end/98266 >>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>> It seems to me that we've got the full statement at some point and thus >>> the full expression so at some point couldn't we detect when >>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using TYPE_SIZE_UNIT >>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>> >>> Am I missing something? >> >> The expression we're looking at when the false positive is issued >> is the MEM_REF in the LHS of: >> >> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >> *)&_ZTC1E24_1D + 24B]; >> >> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >> DECL_SIZE and TYPE_SIZE. > So that seems like it's a different issue then, unrelated to 97595. Right? I think the underlying problem is the same. We're getting a size that doesn't correspond to what's actually being accessed, and it happens because of the virtual inheritance. In pr97595 Jason suggested to use the decl/type size inequality to identify this case but I think we could have just as well used DECL_ARTIFICIAL instead. At least the test cases from pr97595 both pass with this change. Martin > > Jeff > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-08 22:44 ` Martin Sebor @ 2021-02-08 23:11 ` Jeff Law 2021-02-09 17:49 ` Martin Sebor 2021-02-25 23:40 ` Jeff Law 1 sibling, 1 reply; 24+ messages in thread From: Jeff Law @ 2021-02-08 23:11 UTC (permalink / raw) To: Martin Sebor, gcc-patches On 2/8/21 3:44 PM, Martin Sebor wrote: > On 2/8/21 3:26 PM, Jeff Law wrote: >> >> >> On 2/8/21 2:56 PM, Martin Sebor wrote: >>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>> >>>> >>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>> positives in assignments and copies involving virtual inheritance. >>>>> Because the two warnings don't share code yet (hopefully in GCC 12) >>>>> the attached patch adds its own workaround for this problem to >>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>> the limited insight the array bounds checking has into the checked >>>>> expressions. >>>>> >>>>> Tested on x86_64-linux. >>>>> >>>>> Martin >>>>> >>>>> gcc-98266.diff >>>>> >>>>> PR middle-end/98266 - bogus array subscript is partly outside array >>>>> bounds on virtual inheritance >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> PR middle-end/98266 >>>>> * gimple-array-bounds.cc >>>>> (array_bounds_checker::check_array_bounds): >>>>> Avoid checking references involving artificial members. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> PR middle-end/98266 >>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>> It seems to me that we've got the full statement at some point and >>>> thus >>>> the full expression so at some point couldn't we detect when >>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using TYPE_SIZE_UNIT >>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>> >>>> Am I missing something? >>> >>> The expression we're looking at when the false positive is issued >>> is the MEM_REF in the LHS of: >>> >>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>> *)&_ZTC1E24_1D + 24B]; >>> >>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>> DECL_SIZE and TYPE_SIZE. >> So that seems like it's a different issue then, unrelated to 97595. >> Right? > > I think the underlying problem is the same. We're getting a size > that doesn't correspond to what's actually being accessed, and it > happens because of the virtual inheritance. In pr97595 Jason > suggested to use the decl/type size inequality to identify this > case but I think we could have just as well used DECL_ARTIFICIAL > instead. At least the test cases from pr97595 both pass with > this change. But in the 98266 case the type and decl sizes are the same. So to be true that would mean that the underlying type we're using to access memory differs from its actual type. Is that the case in the IL? And does this have wider implications for diagnostics or optimizations that rely on accurate type sizing? I'm just trying to make sure I understand, not accepting or rejecting the patch yet. jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-08 23:11 ` Jeff Law @ 2021-02-09 17:49 ` Martin Sebor 2021-02-23 16:02 ` PING " Martin Sebor 0 siblings, 1 reply; 24+ messages in thread From: Martin Sebor @ 2021-02-09 17:49 UTC (permalink / raw) To: Jeff Law, gcc-patches On 2/8/21 4:11 PM, Jeff Law wrote: > > > On 2/8/21 3:44 PM, Martin Sebor wrote: >> On 2/8/21 3:26 PM, Jeff Law wrote: >>> >>> >>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>> >>>>> >>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>> positives in assignments and copies involving virtual inheritance. >>>>>> Because the two warnings don't share code yet (hopefully in GCC 12) >>>>>> the attached patch adds its own workaround for this problem to >>>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>>> the limited insight the array bounds checking has into the checked >>>>>> expressions. >>>>>> >>>>>> Tested on x86_64-linux. >>>>>> >>>>>> Martin >>>>>> >>>>>> gcc-98266.diff >>>>>> >>>>>> PR middle-end/98266 - bogus array subscript is partly outside array >>>>>> bounds on virtual inheritance >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> PR middle-end/98266 >>>>>> * gimple-array-bounds.cc >>>>>> (array_bounds_checker::check_array_bounds): >>>>>> Avoid checking references involving artificial members. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> PR middle-end/98266 >>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>> It seems to me that we've got the full statement at some point and >>>>> thus >>>>> the full expression so at some point couldn't we detect when >>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using TYPE_SIZE_UNIT >>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>> >>>>> Am I missing something? >>>> >>>> The expression we're looking at when the false positive is issued >>>> is the MEM_REF in the LHS of: >>>> >>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>> *)&_ZTC1E24_1D + 24B]; >>>> >>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>> DECL_SIZE and TYPE_SIZE. >>> So that seems like it's a different issue then, unrelated to 97595. >>> Right? >> >> I think the underlying problem is the same. We're getting a size >> that doesn't correspond to what's actually being accessed, and it >> happens because of the virtual inheritance. In pr97595 Jason >> suggested to use the decl/type size inequality to identify this >> case but I think we could have just as well used DECL_ARTIFICIAL >> instead. At least the test cases from pr97595 both pass with >> this change. > But in the 98266 case the type and decl sizes are the same. So to be > true that would mean that the underlying type we're using to access > memory differs from its actual type. Is that the case in the IL? And > does this have wider implications for diagnostics or optimizations that > rely on accurate type sizing? > > I'm just trying to make sure I understand, not accepting or rejecting > the patch yet. The part of the IL with the MEM_REF is this: void g () { void * D.2789; struct E D.2652; <bb 2> [local count: 1073741824]: E::E (&D.2652, ""); f (&D.2652); <bb 3> [local count: 1073741824]: MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void *)&_ZTC1E24_1D + 24B]; ... The access here is to the _vptr.D pointer member of D.2652 which is just past the end of the parent object (as reflected by its SIZE): it sets sets up the virtual table pointer. The access in pr97595 is to the member subobject, which, as Jason explained (and I accordingly documented under DECL_SIZE in tree.h), is also laid out separately from the parent object. These cases aren't exactly the same (which is also why the test I added for -Warray-bounds in pr97595 didn't expose this bug) but they are closely related. The one here can be distinguished by DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member inequality. Might this impact other warnings? I'd say so if they don't take these things into account. I just learned about this in pr97595 which was a -Wstringop-overflow false positive but I also saw a similar instance of -Warray-bounds with my patch to improve caching and enhance array bounds checking. I dealt with that instance of the warning in that patch but proactively added a test case to the fix for pr97595. But the test case is focused on the subobject access and not on one to the virtual table so (as I said above) it didn't expose this bug. Might this also impact optimizations? I can imagine someone unaware of this "gotcha" making the same "naive" assumption I did, but I'd also expect such an invalid assumption to be found either in code review or quickly cause problems in testing. Martin ^ permalink raw reply [flat|nested] 24+ messages in thread
* PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-09 17:49 ` Martin Sebor @ 2021-02-23 16:02 ` Martin Sebor 2021-02-23 21:52 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Martin Sebor @ 2021-02-23 16:02 UTC (permalink / raw) To: Jeff Law, gcc-patches [CC Jason for any further comments/clarification] On 2/9/21 10:49 AM, Martin Sebor wrote: > On 2/8/21 4:11 PM, Jeff Law wrote: >> >> >> On 2/8/21 3:44 PM, Martin Sebor wrote: >>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>> >>>> >>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>> >>>>>> >>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>>> positives in assignments and copies involving virtual inheritance. >>>>>>> Because the two warnings don't share code yet (hopefully in GCC 12) >>>>>>> the attached patch adds its own workaround for this problem to >>>>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>>>> the limited insight the array bounds checking has into the checked >>>>>>> expressions. >>>>>>> >>>>>>> Tested on x86_64-linux. >>>>>>> >>>>>>> Martin >>>>>>> >>>>>>> gcc-98266.diff >>>>>>> >>>>>>> PR middle-end/98266 - bogus array subscript is partly outside array >>>>>>> bounds on virtual inheritance >>>>>>> >>>>>>> gcc/ChangeLog: >>>>>>> >>>>>>> PR middle-end/98266 >>>>>>> * gimple-array-bounds.cc >>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>> Avoid checking references involving artificial members. >>>>>>> >>>>>>> gcc/testsuite/ChangeLog: >>>>>>> >>>>>>> PR middle-end/98266 >>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>> It seems to me that we've got the full statement at some point and >>>>>> thus >>>>>> the full expression so at some point couldn't we detect when >>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>> TYPE_SIZE_UNIT >>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>> >>>>>> Am I missing something? >>>>> >>>>> The expression we're looking at when the false positive is issued >>>>> is the MEM_REF in the LHS of: >>>>> >>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>>> *)&_ZTC1E24_1D + 24B]; >>>>> >>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>> DECL_SIZE and TYPE_SIZE. >>>> So that seems like it's a different issue then, unrelated to 97595. >>>> Right? >>> >>> I think the underlying problem is the same. We're getting a size >>> that doesn't correspond to what's actually being accessed, and it >>> happens because of the virtual inheritance. In pr97595 Jason >>> suggested to use the decl/type size inequality to identify this >>> case but I think we could have just as well used DECL_ARTIFICIAL >>> instead. At least the test cases from pr97595 both pass with >>> this change. >> But in the 98266 case the type and decl sizes are the same. So to be >> true that would mean that the underlying type we're using to access >> memory differs from its actual type. Is that the case in the IL? And >> does this have wider implications for diagnostics or optimizations that >> rely on accurate type sizing? >> >> I'm just trying to make sure I understand, not accepting or rejecting >> the patch yet. > > The part of the IL with the MEM_REF is this: > > void g () > { > void * D.2789; > struct E D.2652; > > <bb 2> [local count: 1073741824]: > E::E (&D.2652, ""); > f (&D.2652); > > <bb 3> [local count: 1073741824]: > MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void > *)&_ZTC1E24_1D + 24B]; > ... > > The access here is to the _vptr.D pointer member of D.2652 which is > just past the end of the parent object (as reflected by its SIZE): > it sets sets up the virtual table pointer. > > The access in pr97595 is to the member subobject, which, as Jason > explained (and I accordingly documented under DECL_SIZE in tree.h), > is also laid out separately from the parent object. > > These cases aren't exactly the same (which is also why the test > I added for -Warray-bounds in pr97595 didn't expose this bug) but > they are closely related. The one here can be distinguished by > DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member > inequality. > > Might this impact other warnings? I'd say so if they don't take > these things into account. I just learned about this in pr97595 > which was a -Wstringop-overflow false positive but I also saw > a similar instance of -Warray-bounds with my patch to improve > caching and enhance array bounds checking. I dealt with that > instance of the warning in that patch but proactively added > a test case to the fix for pr97595. But the test case is focused > on the subobject access and not on one to the virtual table so > (as I said above) it didn't expose this bug. > > Might this also impact optimizations? I can imagine someone > unaware of this "gotcha" making the same "naive" assumption > I did, but I'd also expect such an invalid assumption to be > found either in code review or quickly cause problems in > testing. Jeff, does this answer your question? > > Martin ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-23 16:02 ` PING " Martin Sebor @ 2021-02-23 21:52 ` Jason Merrill 2021-02-23 23:07 ` Martin Sebor 2021-02-25 23:47 ` Jeff Law 0 siblings, 2 replies; 24+ messages in thread From: Jason Merrill @ 2021-02-23 21:52 UTC (permalink / raw) To: Martin Sebor, Jeff Law, gcc-patches On 2/23/21 11:02 AM, Martin Sebor wrote: > [CC Jason for any further comments/clarification] > > On 2/9/21 10:49 AM, Martin Sebor wrote: >> On 2/8/21 4:11 PM, Jeff Law wrote: >>> >>> >>> On 2/8/21 3:44 PM, Martin Sebor wrote: >>>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>>> >>>>> >>>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>>> >>>>>>> >>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>>>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>>>> positives in assignments and copies involving virtual inheritance. >>>>>>>> Because the two warnings don't share code yet (hopefully in GCC 12) >>>>>>>> the attached patch adds its own workaround for this problem to >>>>>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>>>>> the limited insight the array bounds checking has into the checked >>>>>>>> expressions. >>>>>>>> >>>>>>>> Tested on x86_64-linux. >>>>>>>> >>>>>>>> Martin >>>>>>>> >>>>>>>> gcc-98266.diff >>>>>>>> >>>>>>>> PR middle-end/98266 - bogus array subscript is partly outside array >>>>>>>> bounds on virtual inheritance >>>>>>>> >>>>>>>> gcc/ChangeLog: >>>>>>>> >>>>>>>> PR middle-end/98266 >>>>>>>> * gimple-array-bounds.cc >>>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>>> Avoid checking references involving artificial members. >>>>>>>> >>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>> >>>>>>>> PR middle-end/98266 >>>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>>> It seems to me that we've got the full statement at some point and >>>>>>> thus >>>>>>> the full expression so at some point couldn't we detect when >>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>>> TYPE_SIZE_UNIT >>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>>> >>>>>>> Am I missing something? >>>>>> >>>>>> The expression we're looking at when the false positive is issued >>>>>> is the MEM_REF in the LHS of: >>>>>> >>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>>>> *)&_ZTC1E24_1D + 24B]; >>>>>> >>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>>> DECL_SIZE and TYPE_SIZE. >>>>> So that seems like it's a different issue then, unrelated to 97595. >>>>> Right? >>>> >>>> I think the underlying problem is the same. We're getting a size >>>> that doesn't correspond to what's actually being accessed, and it >>>> happens because of the virtual inheritance. In pr97595 Jason >>>> suggested to use the decl/type size inequality to identify this >>>> case but I think we could have just as well used DECL_ARTIFICIAL >>>> instead. At least the test cases from pr97595 both pass with >>>> this change. >>> But in the 98266 case the type and decl sizes are the same. So to be >>> true that would mean that the underlying type we're using to access >>> memory differs from its actual type. Is that the case in the IL? And >>> does this have wider implications for diagnostics or optimizations that >>> rely on accurate type sizing? >>> >>> I'm just trying to make sure I understand, not accepting or rejecting >>> the patch yet. >> >> The part of the IL with the MEM_REF is this: >> >> void g () >> { >> void * D.2789; >> struct E D.2652; >> >> <bb 2> [local count: 1073741824]: >> E::E (&D.2652, ""); >> f (&D.2652); >> >> <bb 3> [local count: 1073741824]: >> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >> *)&_ZTC1E24_1D + 24B]; >> ... >> >> The access here is to the _vptr.D pointer member of D.2652 which is >> just past the end of the parent object (as reflected by its SIZE): >> it sets sets up the virtual table pointer. >> >> The access in pr97595 is to the member subobject, which, as Jason >> explained (and I accordingly documented under DECL_SIZE in tree.h), >> is also laid out separately from the parent object. >> >> These cases aren't exactly the same (which is also why the test >> I added for -Warray-bounds in pr97595 didn't expose this bug) but >> they are closely related. The one here can be distinguished by >> DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member >> inequality. >> >> Might this impact other warnings? I'd say so if they don't take >> these things into account. I just learned about this in pr97595 >> which was a -Wstringop-overflow false positive but I also saw >> a similar instance of -Warray-bounds with my patch to improve >> caching and enhance array bounds checking. I dealt with that >> instance of the warning in that patch but proactively added >> a test case to the fix for pr97595. But the test case is focused >> on the subobject access and not on one to the virtual table so >> (as I said above) it didn't expose this bug. >> >> Might this also impact optimizations? I can imagine someone >> unaware of this "gotcha" making the same "naive" assumption >> I did, but I'd also expect such an invalid assumption to be >> found either in code review or quickly cause problems in >> testing. > > Jeff, does this answer your question? I don't see how the issue here depends on the artificiality of the vptr; I'd expect to see the same problem with a data member. The problem is that a D base subobject is smaller than a complete D object, and in this case the base subobject is allocated such that if it were a full D object, it would overlap the end of E. And we're checking the MEM_REF as though accessing a full D object, so we get a warning. The general issue about the confusion between complete and as-base types is PR 22488; I have various thoughts about a proper fix, but there have always been higher-priority things to work on. One possible approach to this PR is that we don't need to check an intermediate _REF; in MEM[(struct D *)&D.2651 + 24B]._vptr.D we aren't loading a whole D, we're only looking at the _vptr, which is within the bounds of E. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-23 21:52 ` Jason Merrill @ 2021-02-23 23:07 ` Martin Sebor 2021-02-25 0:35 ` Jason Merrill 2021-02-25 23:47 ` Jeff Law 1 sibling, 1 reply; 24+ messages in thread From: Martin Sebor @ 2021-02-23 23:07 UTC (permalink / raw) To: Jason Merrill, Jeff Law, gcc-patches On 2/23/21 2:52 PM, Jason Merrill wrote: > On 2/23/21 11:02 AM, Martin Sebor wrote: >> [CC Jason for any further comments/clarification] >> >> On 2/9/21 10:49 AM, Martin Sebor wrote: >>> On 2/8/21 4:11 PM, Jeff Law wrote: >>>> >>>> >>>> On 2/8/21 3:44 PM, Martin Sebor wrote: >>>>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>>>> >>>>>> >>>>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>>>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>>>>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>>>>> positives in assignments and copies involving virtual inheritance. >>>>>>>>> Because the two warnings don't share code yet (hopefully in GCC >>>>>>>>> 12) >>>>>>>>> the attached patch adds its own workaround for this problem to >>>>>>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>>>>>> the limited insight the array bounds checking has into the checked >>>>>>>>> expressions. >>>>>>>>> >>>>>>>>> Tested on x86_64-linux. >>>>>>>>> >>>>>>>>> Martin >>>>>>>>> >>>>>>>>> gcc-98266.diff >>>>>>>>> >>>>>>>>> PR middle-end/98266 - bogus array subscript is partly outside >>>>>>>>> array >>>>>>>>> bounds on virtual inheritance >>>>>>>>> >>>>>>>>> gcc/ChangeLog: >>>>>>>>> >>>>>>>>> PR middle-end/98266 >>>>>>>>> * gimple-array-bounds.cc >>>>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>>>> Avoid checking references involving artificial members. >>>>>>>>> >>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>> >>>>>>>>> PR middle-end/98266 >>>>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>>>> It seems to me that we've got the full statement at some point and >>>>>>>> thus >>>>>>>> the full expression so at some point couldn't we detect when >>>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>>>> TYPE_SIZE_UNIT >>>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>>>> >>>>>>>> Am I missing something? >>>>>>> >>>>>>> The expression we're looking at when the false positive is issued >>>>>>> is the MEM_REF in the LHS of: >>>>>>> >>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>>>>> *)&_ZTC1E24_1D + 24B]; >>>>>>> >>>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>>>> DECL_SIZE and TYPE_SIZE. >>>>>> So that seems like it's a different issue then, unrelated to 97595. >>>>>> Right? >>>>> >>>>> I think the underlying problem is the same. We're getting a size >>>>> that doesn't correspond to what's actually being accessed, and it >>>>> happens because of the virtual inheritance. In pr97595 Jason >>>>> suggested to use the decl/type size inequality to identify this >>>>> case but I think we could have just as well used DECL_ARTIFICIAL >>>>> instead. At least the test cases from pr97595 both pass with >>>>> this change. >>>> But in the 98266 case the type and decl sizes are the same. So to be >>>> true that would mean that the underlying type we're using to access >>>> memory differs from its actual type. Is that the case in the IL? And >>>> does this have wider implications for diagnostics or optimizations that >>>> rely on accurate type sizing? >>>> >>>> I'm just trying to make sure I understand, not accepting or rejecting >>>> the patch yet. >>> >>> The part of the IL with the MEM_REF is this: >>> >>> void g () >>> { >>> void * D.2789; >>> struct E D.2652; >>> >>> <bb 2> [local count: 1073741824]: >>> E::E (&D.2652, ""); >>> f (&D.2652); >>> >>> <bb 3> [local count: 1073741824]: >>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>> *)&_ZTC1E24_1D + 24B]; >>> ... >>> >>> The access here is to the _vptr.D pointer member of D.2652 which is >>> just past the end of the parent object (as reflected by its SIZE): >>> it sets sets up the virtual table pointer. >>> >>> The access in pr97595 is to the member subobject, which, as Jason >>> explained (and I accordingly documented under DECL_SIZE in tree.h), >>> is also laid out separately from the parent object. >>> >>> These cases aren't exactly the same (which is also why the test >>> I added for -Warray-bounds in pr97595 didn't expose this bug) but >>> they are closely related. The one here can be distinguished by >>> DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member >>> inequality. >>> >>> Might this impact other warnings? I'd say so if they don't take >>> these things into account. I just learned about this in pr97595 >>> which was a -Wstringop-overflow false positive but I also saw >>> a similar instance of -Warray-bounds with my patch to improve >>> caching and enhance array bounds checking. I dealt with that >>> instance of the warning in that patch but proactively added >>> a test case to the fix for pr97595. But the test case is focused >>> on the subobject access and not on one to the virtual table so >>> (as I said above) it didn't expose this bug. >>> >>> Might this also impact optimizations? I can imagine someone >>> unaware of this "gotcha" making the same "naive" assumption >>> I did, but I'd also expect such an invalid assumption to be >>> found either in code review or quickly cause problems in >>> testing. >> >> Jeff, does this answer your question? > > I don't see how the issue here depends on the artificiality of the vptr; > I'd expect to see the same problem with a data member. The problem is > that a D base subobject is smaller than a complete D object, and in this > case the base subobject is allocated such that if it were a full D > object, it would overlap the end of E. And we're checking the MEM_REF > as though accessing a full D object, so we get a warning. Thanks for chiming in! There is no warning for accesses to data members defined in the virtual bases because their offset isn't constant. For example, given the following modified version of the test case from the patch: struct A { virtual ~A (); int a; }; struct B: virtual A { int b; }; struct C: virtual A { int c; }; struct D: virtual B, virtual C { int d; }; D d; void g () { D *p = &d; p->a = p->b = p->c = p->d = 1; } we end up with: void g () { ... <bb 2> [local count: 1073741824]: d.d = 1; <<< okay _1 = d._vptr.D; <<< DECL_ARTIFICIAL _2 = MEM[(long int *)_1 + -40B]; <<< not checked _3 = (sizetype) _2; <<< not constant... _4 = &d + _3; MEM[(struct C *)_4].c = 1; <<< ...no warning _5 = MEM[(long int *)_1 + -24B]; _6 = (sizetype) _5; _7 = &d + _6; MEM[(struct B *)_7].b = 1; _8 = MEM[(long int *)_1 + -32B]; _9 = (sizetype) _8; _10 = &d + _9; MEM[(struct A *)_10].a = 1; return; } > > The general issue about the confusion between complete and as-base types > is PR 22488; I have various thoughts about a proper fix, but there have > always been higher-priority things to work on. Thanks for the reference! > > One possible approach to this PR is that we don't need to check an > intermediate _REF; in > > MEM[(struct D *)&D.2651 + 24B]._vptr.D > > we aren't loading a whole D, we're only looking at the _vptr, which is > within the bounds of E. This is also what the patch does: it keeps us from reaching the MEM_REF because _vptr.D is artificial. -Warray-bounds still of course checks other MEM_REFs in order to detect bugs like: struct A { int i, j; }; A* f () { A *p = (A*)new char[4]; p->i = 1; p->j = 2; return p; } Martin > > Jason > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-23 23:07 ` Martin Sebor @ 2021-02-25 0:35 ` Jason Merrill 2021-03-01 23:11 ` Martin Sebor 0 siblings, 1 reply; 24+ messages in thread From: Jason Merrill @ 2021-02-25 0:35 UTC (permalink / raw) To: Martin Sebor, Jeff Law, gcc-patches On 2/23/21 6:07 PM, Martin Sebor wrote: > On 2/23/21 2:52 PM, Jason Merrill wrote: >> On 2/23/21 11:02 AM, Martin Sebor wrote: >>> [CC Jason for any further comments/clarification] >>> >>> On 2/9/21 10:49 AM, Martin Sebor wrote: >>>> On 2/8/21 4:11 PM, Jeff Law wrote: >>>>> >>>>> >>>>> On 2/8/21 3:44 PM, Martin Sebor wrote: >>>>>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>>>>> >>>>>>> >>>>>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>>>>> Similar to the problem reported for -Wstringop-overflow in >>>>>>>>>> pr98266 >>>>>>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>>>>>> positives in assignments and copies involving virtual >>>>>>>>>> inheritance. >>>>>>>>>> Because the two warnings don't share code yet (hopefully in >>>>>>>>>> GCC 12) >>>>>>>>>> the attached patch adds its own workaround for this problem to >>>>>>>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>>>>>>> the limited insight the array bounds checking has into the >>>>>>>>>> checked >>>>>>>>>> expressions. >>>>>>>>>> >>>>>>>>>> Tested on x86_64-linux. >>>>>>>>>> >>>>>>>>>> Martin >>>>>>>>>> >>>>>>>>>> gcc-98266.diff >>>>>>>>>> >>>>>>>>>> PR middle-end/98266 - bogus array subscript is partly outside >>>>>>>>>> array >>>>>>>>>> bounds on virtual inheritance >>>>>>>>>> >>>>>>>>>> gcc/ChangeLog: >>>>>>>>>> >>>>>>>>>> PR middle-end/98266 >>>>>>>>>> * gimple-array-bounds.cc >>>>>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>>>>> Avoid checking references involving artificial members. >>>>>>>>>> >>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>> >>>>>>>>>> PR middle-end/98266 >>>>>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>>>>> It seems to me that we've got the full statement at some point >>>>>>>>> and >>>>>>>>> thus >>>>>>>>> the full expression so at some point couldn't we detect when >>>>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>>>>> TYPE_SIZE_UNIT >>>>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>>>>> >>>>>>>>> Am I missing something? >>>>>>>> >>>>>>>> The expression we're looking at when the false positive is issued >>>>>>>> is the MEM_REF in the LHS of: >>>>>>>> >>>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>>>>>> *)&_ZTC1E24_1D + 24B]; >>>>>>>> >>>>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>>>>> DECL_SIZE and TYPE_SIZE. >>>>>>> So that seems like it's a different issue then, unrelated to 97595. >>>>>>> Right? >>>>>> >>>>>> I think the underlying problem is the same. We're getting a size >>>>>> that doesn't correspond to what's actually being accessed, and it >>>>>> happens because of the virtual inheritance. In pr97595 Jason >>>>>> suggested to use the decl/type size inequality to identify this >>>>>> case but I think we could have just as well used DECL_ARTIFICIAL >>>>>> instead. At least the test cases from pr97595 both pass with >>>>>> this change. >>>>> But in the 98266 case the type and decl sizes are the same. So to be >>>>> true that would mean that the underlying type we're using to access >>>>> memory differs from its actual type. Is that the case in the IL? And >>>>> does this have wider implications for diagnostics or optimizations >>>>> that >>>>> rely on accurate type sizing? >>>>> >>>>> I'm just trying to make sure I understand, not accepting or rejecting >>>>> the patch yet. >>>> >>>> The part of the IL with the MEM_REF is this: >>>> >>>> void g () >>>> { >>>> void * D.2789; >>>> struct E D.2652; >>>> >>>> <bb 2> [local count: 1073741824]: >>>> E::E (&D.2652, ""); >>>> f (&D.2652); >>>> >>>> <bb 3> [local count: 1073741824]: >>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>> *)&_ZTC1E24_1D + 24B]; >>>> ... >>>> >>>> The access here is to the _vptr.D pointer member of D.2652 which is >>>> just past the end of the parent object (as reflected by its SIZE): >>>> it sets sets up the virtual table pointer. >>>> >>>> The access in pr97595 is to the member subobject, which, as Jason >>>> explained (and I accordingly documented under DECL_SIZE in tree.h), >>>> is also laid out separately from the parent object. >>>> >>>> These cases aren't exactly the same (which is also why the test >>>> I added for -Warray-bounds in pr97595 didn't expose this bug) but >>>> they are closely related. The one here can be distinguished by >>>> DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member >>>> inequality. >>>> >>>> Might this impact other warnings? I'd say so if they don't take >>>> these things into account. I just learned about this in pr97595 >>>> which was a -Wstringop-overflow false positive but I also saw >>>> a similar instance of -Warray-bounds with my patch to improve >>>> caching and enhance array bounds checking. I dealt with that >>>> instance of the warning in that patch but proactively added >>>> a test case to the fix for pr97595. But the test case is focused >>>> on the subobject access and not on one to the virtual table so >>>> (as I said above) it didn't expose this bug. >>>> >>>> Might this also impact optimizations? I can imagine someone >>>> unaware of this "gotcha" making the same "naive" assumption >>>> I did, but I'd also expect such an invalid assumption to be >>>> found either in code review or quickly cause problems in >>>> testing. >>> >>> Jeff, does this answer your question? >> >> I don't see how the issue here depends on the artificiality of the >> vptr; I'd expect to see the same problem with a data member. The >> problem is that a D base subobject is smaller than a complete D >> object, and in this case the base subobject is allocated such that if >> it were a full D object, it would overlap the end of E. And we're >> checking the MEM_REF as though accessing a full D object, so we get a >> warning. > > Thanks for chiming in! > > There is no warning for accesses to data members defined in > the virtual bases because their offset isn't constant. For example, > given the following modified version of the test case from the patch: > > struct A > { > virtual ~A (); > int a; > }; > > struct B: virtual A { int b; }; > struct C: virtual A { int c; }; > struct D: virtual B, virtual C > { > int d; > }; > > D d; > > void g () > { > D *p = &d; > p->a = p->b = p->c = p->d = 1; > } > > we end up with: > > void g () > { > ... > <bb 2> [local count: 1073741824]: > d.d = 1; <<< okay > _1 = d._vptr.D; <<< DECL_ARTIFICIAL > _2 = MEM[(long int *)_1 + -40B]; <<< not checked > _3 = (sizetype) _2; <<< not constant... > _4 = &d + _3; > MEM[(struct C *)_4].c = 1; <<< ...no warning Interesting. I do get a warning if I embed the access in the destructor: struct A { virtual ~A (); int a; }; struct B: virtual A { }; struct C: virtual A { int c; ~C() { c = 0; } // warns twice, for _vptr and c accesses }; struct D: virtual B, virtual C { D(); }; void g() { D d; } > _5 = MEM[(long int *)_1 + -24B]; > _6 = (sizetype) _5; > _7 = &d + _6; > MEM[(struct B *)_7].b = 1; > _8 = MEM[(long int *)_1 + -32B]; > _9 = (sizetype) _8; > _10 = &d + _9; > MEM[(struct A *)_10].a = 1; > return; > > } > >> The general issue about the confusion between complete and as-base >> types is PR 22488; I have various thoughts about a proper fix, but >> there have always been higher-priority things to work on. > > Thanks for the reference! > >> One possible approach to this PR is that we don't need to check an >> intermediate _REF; in >> >> MEM[(struct D *)&D.2651 + 24B]._vptr.D >> >> we aren't loading a whole D, we're only looking at the _vptr, which is >> within the bounds of E. > > This is also what the patch does: it keeps us from reaching > the MEM_REF because _vptr.D is artificial. My point applies equally to the access to c in my testcase above: we shouldn't check lvalue refs as though we're loading or storing the whole aggregate object. The full access is (*((D*)&e))._vptr.D. The MEM_REF represents the *, which we shouldn't check by itself, because it doesn't imply a memory access of its own. We should only check the COMPONENT_REF around it that is an actual memory access. > -Warray-bounds still of course checks other MEM_REFs in order to > detect bugs like: > > struct A { int i, j; }; > > A* f () > { > A *p = (A*)new char[4]; > p->i = 1; > p->j = 2; > return p; > } Of course. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-25 0:35 ` Jason Merrill @ 2021-03-01 23:11 ` Martin Sebor 2021-03-02 14:11 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Martin Sebor @ 2021-03-01 23:11 UTC (permalink / raw) To: Jason Merrill, Jeff Law, gcc-patches [-- Attachment #1: Type: text/plain, Size: 10859 bytes --] On 2/24/21 5:35 PM, Jason Merrill wrote: > On 2/23/21 6:07 PM, Martin Sebor wrote: >> On 2/23/21 2:52 PM, Jason Merrill wrote: >>> On 2/23/21 11:02 AM, Martin Sebor wrote: >>>> [CC Jason for any further comments/clarification] >>>> >>>> On 2/9/21 10:49 AM, Martin Sebor wrote: >>>>> On 2/8/21 4:11 PM, Jeff Law wrote: >>>>>> >>>>>> >>>>>> On 2/8/21 3:44 PM, Martin Sebor wrote: >>>>>>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>>>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>>>>>> Similar to the problem reported for -Wstringop-overflow in >>>>>>>>>>> pr98266 >>>>>>>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>>>>>>> positives in assignments and copies involving virtual >>>>>>>>>>> inheritance. >>>>>>>>>>> Because the two warnings don't share code yet (hopefully in >>>>>>>>>>> GCC 12) >>>>>>>>>>> the attached patch adds its own workaround for this problem to >>>>>>>>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>>>>>>>> the limited insight the array bounds checking has into the >>>>>>>>>>> checked >>>>>>>>>>> expressions. >>>>>>>>>>> >>>>>>>>>>> Tested on x86_64-linux. >>>>>>>>>>> >>>>>>>>>>> Martin >>>>>>>>>>> >>>>>>>>>>> gcc-98266.diff >>>>>>>>>>> >>>>>>>>>>> PR middle-end/98266 - bogus array subscript is partly outside >>>>>>>>>>> array >>>>>>>>>>> bounds on virtual inheritance >>>>>>>>>>> >>>>>>>>>>> gcc/ChangeLog: >>>>>>>>>>> >>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>> * gimple-array-bounds.cc >>>>>>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>>>>>> Avoid checking references involving artificial members. >>>>>>>>>>> >>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>> >>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>>>>>> It seems to me that we've got the full statement at some point >>>>>>>>>> and >>>>>>>>>> thus >>>>>>>>>> the full expression so at some point couldn't we detect when >>>>>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>>>>>> TYPE_SIZE_UNIT >>>>>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>>>>>> >>>>>>>>>> Am I missing something? >>>>>>>>> >>>>>>>>> The expression we're looking at when the false positive is issued >>>>>>>>> is the MEM_REF in the LHS of: >>>>>>>>> >>>>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>>>>>>> *)&_ZTC1E24_1D + 24B]; >>>>>>>>> >>>>>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>>>>>> DECL_SIZE and TYPE_SIZE. >>>>>>>> So that seems like it's a different issue then, unrelated to 97595. >>>>>>>> Right? >>>>>>> >>>>>>> I think the underlying problem is the same. We're getting a size >>>>>>> that doesn't correspond to what's actually being accessed, and it >>>>>>> happens because of the virtual inheritance. In pr97595 Jason >>>>>>> suggested to use the decl/type size inequality to identify this >>>>>>> case but I think we could have just as well used DECL_ARTIFICIAL >>>>>>> instead. At least the test cases from pr97595 both pass with >>>>>>> this change. >>>>>> But in the 98266 case the type and decl sizes are the same. So to be >>>>>> true that would mean that the underlying type we're using to access >>>>>> memory differs from its actual type. Is that the case in the IL? >>>>>> And >>>>>> does this have wider implications for diagnostics or optimizations >>>>>> that >>>>>> rely on accurate type sizing? >>>>>> >>>>>> I'm just trying to make sure I understand, not accepting or rejecting >>>>>> the patch yet. >>>>> >>>>> The part of the IL with the MEM_REF is this: >>>>> >>>>> void g () >>>>> { >>>>> void * D.2789; >>>>> struct E D.2652; >>>>> >>>>> <bb 2> [local count: 1073741824]: >>>>> E::E (&D.2652, ""); >>>>> f (&D.2652); >>>>> >>>>> <bb 3> [local count: 1073741824]: >>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> >>>>> [(void *)&_ZTC1E24_1D + 24B]; >>>>> ... >>>>> >>>>> The access here is to the _vptr.D pointer member of D.2652 which is >>>>> just past the end of the parent object (as reflected by its SIZE): >>>>> it sets sets up the virtual table pointer. >>>>> >>>>> The access in pr97595 is to the member subobject, which, as Jason >>>>> explained (and I accordingly documented under DECL_SIZE in tree.h), >>>>> is also laid out separately from the parent object. >>>>> >>>>> These cases aren't exactly the same (which is also why the test >>>>> I added for -Warray-bounds in pr97595 didn't expose this bug) but >>>>> they are closely related. The one here can be distinguished by >>>>> DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member >>>>> inequality. >>>>> >>>>> Might this impact other warnings? I'd say so if they don't take >>>>> these things into account. I just learned about this in pr97595 >>>>> which was a -Wstringop-overflow false positive but I also saw >>>>> a similar instance of -Warray-bounds with my patch to improve >>>>> caching and enhance array bounds checking. I dealt with that >>>>> instance of the warning in that patch but proactively added >>>>> a test case to the fix for pr97595. But the test case is focused >>>>> on the subobject access and not on one to the virtual table so >>>>> (as I said above) it didn't expose this bug. >>>>> >>>>> Might this also impact optimizations? I can imagine someone >>>>> unaware of this "gotcha" making the same "naive" assumption >>>>> I did, but I'd also expect such an invalid assumption to be >>>>> found either in code review or quickly cause problems in >>>>> testing. >>>> >>>> Jeff, does this answer your question? >>> >>> I don't see how the issue here depends on the artificiality of the >>> vptr; I'd expect to see the same problem with a data member. The >>> problem is that a D base subobject is smaller than a complete D >>> object, and in this case the base subobject is allocated such that if >>> it were a full D object, it would overlap the end of E. And we're >>> checking the MEM_REF as though accessing a full D object, so we get a >>> warning. >> >> Thanks for chiming in! >> >> There is no warning for accesses to data members defined in >> the virtual bases because their offset isn't constant. For example, >> given the following modified version of the test case from the patch: >> >> struct A >> { >> virtual ~A (); >> int a; >> }; >> >> struct B: virtual A { int b; }; >> struct C: virtual A { int c; }; >> struct D: virtual B, virtual C >> { >> int d; >> }; >> >> D d; >> >> void g () >> { >> D *p = &d; >> p->a = p->b = p->c = p->d = 1; >> } >> >> we end up with: >> >> void g () >> { >> ... >> <bb 2> [local count: 1073741824]: >> d.d = 1; <<< okay >> _1 = d._vptr.D; <<< DECL_ARTIFICIAL >> _2 = MEM[(long int *)_1 + -40B]; <<< not checked >> _3 = (sizetype) _2; <<< not constant... >> _4 = &d + _3; >> MEM[(struct C *)_4].c = 1; <<< ...no warning > > Interesting. I do get a warning if I embed the access in the destructor: > > struct A > { > virtual ~A (); > int a; > }; > > struct B: virtual A { }; > struct C: virtual A { > int c; > ~C() { c = 0; } // warns twice, for _vptr and c accesses > }; > struct D: virtual B, virtual C > { > D(); > }; > > void g() > { > D d; > } Ah, right. Unlike in member functions, in a dtor (or in the ctors of the derived class) the offsets are known/constant so accesses involving those are checked and we do get a false positive. Thanks for the test case! > >> _5 = MEM[(long int *)_1 + -24B]; >> _6 = (sizetype) _5; >> _7 = &d + _6; >> MEM[(struct B *)_7].b = 1; >> _8 = MEM[(long int *)_1 + -32B]; >> _9 = (sizetype) _8; >> _10 = &d + _9; >> MEM[(struct A *)_10].a = 1; >> return; >> >> } >> >>> The general issue about the confusion between complete and as-base >>> types is PR 22488; I have various thoughts about a proper fix, but >>> there have always been higher-priority things to work on. >> >> Thanks for the reference! >> >>> One possible approach to this PR is that we don't need to check an >>> intermediate _REF; in >>> >>> MEM[(struct D *)&D.2651 + 24B]._vptr.D >>> >>> we aren't loading a whole D, we're only looking at the _vptr, which >>> is within the bounds of E. >> >> This is also what the patch does: it keeps us from reaching >> the MEM_REF because _vptr.D is artificial. > > My point applies equally to the access to c in my testcase above: we > shouldn't check lvalue refs as though we're loading or storing the whole > aggregate object. > > The full access is (*((D*)&e))._vptr.D. The MEM_REF represents the *, > which we shouldn't check by itself, because it doesn't imply a memory > access of its own. We should only check the COMPONENT_REF around it > that is an actual memory access. The -Warray-bounds implementation checks every ARRAY_REF and MEM_REF, regardless of their context(*). It doesn't check COMPONENT_REF except while checking the other REFs (while walking up the use-def chains), and the checks don't know the context of the REF. The hack this patch put in was to circumvent that when the walk saw a COMPONENT_REF to an artificial member (which as you showed isn't enough). I'm not sure if what you suggest (IIUC) is feasible without reworking how the checking works in general. What is feasible is working harder and detecting if a) the type of the accessed object has virtual bases and b) if the offset of the accessed field is within the bounds of the enclosing object. In that case, skipping the MEM_REF check avoids the warnings for all the cases we have discussed (artificial members, ctors and dtors). It still seems hacky but sufficiently narrow in scope for GCC 11(**). Please see the attached revision. Martin [*] The warning uses walk_gimple_op whose documentation suggests is the wrong API to use for code in an SSA form. SSA form code is supposed to use the standard gimple operand APIs. I think those would make it easier consider the context. [**] In the future I'd like to restructure the whole checker to avoid this walk altogether (besides also using Ranger and the pointer_query class). > >> -Warray-bounds still of course checks other MEM_REFs in order to >> detect bugs like: >> >> struct A { int i, j; }; >> >> A* f () >> { >> A *p = (A*)new char[4]; >> p->i = 1; >> p->j = 2; >> return p; >> } > > Of course. > [-- Attachment #2: gcc-98266.diff --] [-- Type: text/x-patch, Size: 9692 bytes --] PR middle-end/98266 - bogus array subscript is partly outside array bounds on virtual inheritance gcc/ChangeLog: PR middle-end/98266 * gimple-array-bounds.cc (array_bounds_checker::check_array_bounds): Avoid checking references involving members of virtual bases that are in bounds of the enclosing object. gcc/testsuite/ChangeLog: PR middle-end/98266 * g++.dg/warn/Warray-bounds-15.C: New test. * g++.dg/warn/Warray-bounds-18.C: New test. diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array-bounds.cc index d7fd2c74111..bf96c61465d 100644 --- a/gcc/gimple-array-bounds.cc +++ b/gcc/gimple-array-bounds.cc @@ -919,8 +919,31 @@ array_bounds_checker::check_array_bounds (tree *tp, int *walk_subtree, else if (TREE_CODE (t) == ADDR_EXPR) { checker->check_addr_expr (location, t); - *walk_subtree = FALSE; + *walk_subtree = false; } + else if (TREE_CODE (t) == COMPONENT_REF + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF) + { + /* Hack: Skip MEM_REF checks in accesses to a member of a type + with virtual bases at an offset that's in bounds of the size + of the enclosing object. See pr98266 and pr97595. */ + tree ref = TREE_OPERAND (TREE_OPERAND (t, 0), 0); + if (TREE_CODE (ref) == ADDR_EXPR) + ref = TREE_OPERAND (ref, 0); + tree reftype = TREE_TYPE (ref); + if (DECL_P (ref) + && RECORD_OR_UNION_TYPE_P (reftype) + && TYPE_BINFO (reftype)) + { + tree fld = TREE_OPERAND (t, 1); + tree fldpos = byte_position (fld); + if (tree refsize = DECL_SIZE_UNIT (ref)) + if (TREE_CODE (refsize) == INTEGER_CST + && tree_int_cst_lt (fldpos, refsize)) + *walk_subtree = false; + } + } + /* Propagate the no-warning bit to the outer expression. */ if (warned) TREE_NO_WARNING (t) = true; diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C new file mode 100644 index 00000000000..3aba508ce11 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C @@ -0,0 +1,174 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +struct A +{ + virtual ~A (); + const char *s; +}; + +struct B: virtual A { }; +struct C: virtual A { }; // { dg-bogus "\\\[-Warray-bounds" } + +struct D1: virtual B, virtual C +{ + D1 (const char*); +}; + +void sink (void*); +void sink (D1); + + +// Verify that accesses to the virtual table aren't diagnosed. +void test_vtbl () +{ + sink (D1 ("")); +} + + +struct C2: virtual A { + C2 (); + int i, j, a[2]; + ~C2 () + { + i = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + j = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[0] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[1] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +struct D2: virtual B, virtual C2 +{ + D2 (); +}; + +void sink (void*); + +/* Verify that only out of bounds accesses to members of a virtual base + class are diagnosed. Use direct array accesses. */ +void test_vmem_base_ctor_arryaccess () +{ + D2 d2; + sink (&d2); +} + + +struct C3: virtual A { + C3 (); + int a[2]; + ~C3 () + { + int *p = a; + *p++ = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + *p++ = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +struct D3: virtual B, virtual C3 +{ + D3 (); +}; + +/* Verify that only out of bounds accesses to members of a virtual base + class are diagnosed. Use pointer accesses. */ +void test_vmem_base_dtor_ptraccess () +{ + D3 d3; + sink (&d3); +} + + +struct C4: virtual A { + C4 (); + int i, j, a[2]; +}; + +struct D4: virtual B, virtual C4 +{ + D4 () + { + i = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + j = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[0] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[1] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of a virtual base + class made in the ctor of a derived class are diagnosed. Use direct + array accesses. */ +void test_vmem_derived_ctor_arryaccess () +{ + D4 d4; + sink (&d4); +} + + +struct D5: virtual B, virtual C4 +{ + D5 () + { + int *p = a; + *p++ = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + *p++ = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of a virtual base + class made in the ctor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_ctor_ptraccess () +{ + D5 d5; + sink (&d5); +} + + +struct D6: virtual B, virtual C4 +{ + ~D6 () + { + i = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + j = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[0] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[1] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of a virtual base + class made in the dtor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_dtor_arryaccess () +{ + D6 d6; + sink (&d6); +} + + +struct D7: virtual B, virtual C4 +{ + ~D7 () + { + int *p = a; + *p++ = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + *p++ = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of a virtual base + class made in the dtor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_dtor_ptraccess () +{ + D7 d7; + sink (&d7); +} diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-18.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-18.C new file mode 100644 index 00000000000..01d0f04a476 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-18.C @@ -0,0 +1,175 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + Same as Warray-bounds-15.C but with ordinary (nonvirtual) bases. + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +struct A +{ + virtual ~A (); + const char *s; +}; + +struct B: A { }; +struct C: A { }; + +struct D1: B, C +{ + D1 (const char*); +}; + +void sink (void*); +void sink (D1); + + +// Verify that accesses to the table aren't diagnosed. +void test_vtbl () +{ + sink (D1 ("")); +} + + +struct C2: A { + C2 (); + int i, j, a[2]; + ~C2 () + { + i = __LINE__; + j = __LINE__; + a[0] = __LINE__; + a[1] = __LINE__; + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +struct D2: B, C2 +{ + D2 (); +}; + +void sink (void*); + +/* Verify that only out of bounds accesses to members of an ordinary base + class are diagnosed. Use direct array accesses. */ +void test_vmem_base_ctor_arryaccess () +{ + D2 d2; + sink (&d2); +} + + +struct C3: A { + C3 (); + int a[2]; + ~C3 () + { + int *p = a; + *p++ = __LINE__; + *p++ = __LINE__; + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +struct D3: B, C3 +{ + D3 (); +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class are diagnosed. Use pointer accesses. */ +void test_vmem_base_dtor_ptraccess () +{ + D3 d3; + sink (&d3); +} + + +struct C4: A { + C4 (); + int i, j, a[2]; +}; + +struct D4: B, C4 +{ + D4 () + { + i = __LINE__; + j = __LINE__; + a[0] = __LINE__; + a[1] = __LINE__; + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the ctor of a derived class are diagnosed. Use direct + array accesses. */ +void test_vmem_derived_ctor_arryaccess () +{ + D4 d4; + sink (&d4); +} + + +struct D5: B, C4 +{ + D5 () + { + int *p = a; + *p++ = __LINE__; + *p++ = __LINE__; + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the ctor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_ctor_ptraccess () +{ + D5 d5; + sink (&d5); +} + + +struct D6: B, C4 +{ + ~D6 () + { + i = __LINE__; + j = __LINE__; + a[0] = __LINE__; + a[1] = __LINE__; + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the dtor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_dtor_arryaccess () +{ + D6 d6; + sink (&d6); +} + + +struct D7: B, C4 +{ + ~D7 () + { + int *p = a; + *p++ = __LINE__; + *p++ = __LINE__; + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the dtor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_dtor_ptraccess () +{ + D7 d7; + sink (&d7); +} ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-03-01 23:11 ` Martin Sebor @ 2021-03-02 14:11 ` Jason Merrill 2021-03-03 23:20 ` Martin Sebor 0 siblings, 1 reply; 24+ messages in thread From: Jason Merrill @ 2021-03-02 14:11 UTC (permalink / raw) To: Martin Sebor, Jeff Law, gcc-patches On 3/1/21 6:11 PM, Martin Sebor wrote: > On 2/24/21 5:35 PM, Jason Merrill wrote: >> On 2/23/21 6:07 PM, Martin Sebor wrote: >>> On 2/23/21 2:52 PM, Jason Merrill wrote: >>>> On 2/23/21 11:02 AM, Martin Sebor wrote: >>>>> [CC Jason for any further comments/clarification] >>>>> >>>>> On 2/9/21 10:49 AM, Martin Sebor wrote: >>>>>> On 2/8/21 4:11 PM, Jeff Law wrote: >>>>>>> >>>>>>> >>>>>>> On 2/8/21 3:44 PM, Martin Sebor wrote: >>>>>>>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>>>>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>>>>>>> Similar to the problem reported for -Wstringop-overflow in >>>>>>>>>>>> pr98266 >>>>>>>>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>>>>>>>> positives in assignments and copies involving virtual >>>>>>>>>>>> inheritance. >>>>>>>>>>>> Because the two warnings don't share code yet (hopefully in >>>>>>>>>>>> GCC 12) >>>>>>>>>>>> the attached patch adds its own workaround for this problem to >>>>>>>>>>>> gimple-array-bounds.cc, this one slightly more crude because of >>>>>>>>>>>> the limited insight the array bounds checking has into the >>>>>>>>>>>> checked >>>>>>>>>>>> expressions. >>>>>>>>>>>> >>>>>>>>>>>> Tested on x86_64-linux. >>>>>>>>>>>> >>>>>>>>>>>> Martin >>>>>>>>>>>> >>>>>>>>>>>> gcc-98266.diff >>>>>>>>>>>> >>>>>>>>>>>> PR middle-end/98266 - bogus array subscript is partly >>>>>>>>>>>> outside array >>>>>>>>>>>> bounds on virtual inheritance >>>>>>>>>>>> >>>>>>>>>>>> gcc/ChangeLog: >>>>>>>>>>>> >>>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>>> * gimple-array-bounds.cc >>>>>>>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>>>>>>> Avoid checking references involving artificial members. >>>>>>>>>>>> >>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>> >>>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>>>>>>> It seems to me that we've got the full statement at some >>>>>>>>>>> point and >>>>>>>>>>> thus >>>>>>>>>>> the full expression so at some point couldn't we detect when >>>>>>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>>>>>>> TYPE_SIZE_UNIT >>>>>>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>>>>>>> >>>>>>>>>>> Am I missing something? >>>>>>>>>> >>>>>>>>>> The expression we're looking at when the false positive is issued >>>>>>>>>> is the MEM_REF in the LHS of: >>>>>>>>>> >>>>>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void >>>>>>>>>> *)&_ZTC1E24_1D + 24B]; >>>>>>>>>> >>>>>>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>>>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>>>>>>> DECL_SIZE and TYPE_SIZE. >>>>>>>>> So that seems like it's a different issue then, unrelated to >>>>>>>>> 97595. >>>>>>>>> Right? >>>>>>>> >>>>>>>> I think the underlying problem is the same. We're getting a size >>>>>>>> that doesn't correspond to what's actually being accessed, and it >>>>>>>> happens because of the virtual inheritance. In pr97595 Jason >>>>>>>> suggested to use the decl/type size inequality to identify this >>>>>>>> case but I think we could have just as well used DECL_ARTIFICIAL >>>>>>>> instead. At least the test cases from pr97595 both pass with >>>>>>>> this change. >>>>>>> But in the 98266 case the type and decl sizes are the same. So >>>>>>> to be >>>>>>> true that would mean that the underlying type we're using to access >>>>>>> memory differs from its actual type. Is that the case in the IL? >>>>>>> And >>>>>>> does this have wider implications for diagnostics or >>>>>>> optimizations that >>>>>>> rely on accurate type sizing? >>>>>>> >>>>>>> I'm just trying to make sure I understand, not accepting or >>>>>>> rejecting >>>>>>> the patch yet. >>>>>> >>>>>> The part of the IL with the MEM_REF is this: >>>>>> >>>>>> void g () >>>>>> { >>>>>> void * D.2789; >>>>>> struct E D.2652; >>>>>> >>>>>> <bb 2> [local count: 1073741824]: >>>>>> E::E (&D.2652, ""); >>>>>> f (&D.2652); >>>>>> >>>>>> <bb 3> [local count: 1073741824]: >>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> >>>>>> [(void *)&_ZTC1E24_1D + 24B]; >>>>>> ... >>>>>> >>>>>> The access here is to the _vptr.D pointer member of D.2652 which is >>>>>> just past the end of the parent object (as reflected by its SIZE): >>>>>> it sets sets up the virtual table pointer. >>>>>> >>>>>> The access in pr97595 is to the member subobject, which, as Jason >>>>>> explained (and I accordingly documented under DECL_SIZE in tree.h), >>>>>> is also laid out separately from the parent object. >>>>>> >>>>>> These cases aren't exactly the same (which is also why the test >>>>>> I added for -Warray-bounds in pr97595 didn't expose this bug) but >>>>>> they are closely related. The one here can be distinguished by >>>>>> DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member >>>>>> inequality. >>>>>> >>>>>> Might this impact other warnings? I'd say so if they don't take >>>>>> these things into account. I just learned about this in pr97595 >>>>>> which was a -Wstringop-overflow false positive but I also saw >>>>>> a similar instance of -Warray-bounds with my patch to improve >>>>>> caching and enhance array bounds checking. I dealt with that >>>>>> instance of the warning in that patch but proactively added >>>>>> a test case to the fix for pr97595. But the test case is focused >>>>>> on the subobject access and not on one to the virtual table so >>>>>> (as I said above) it didn't expose this bug. >>>>>> >>>>>> Might this also impact optimizations? I can imagine someone >>>>>> unaware of this "gotcha" making the same "naive" assumption >>>>>> I did, but I'd also expect such an invalid assumption to be >>>>>> found either in code review or quickly cause problems in >>>>>> testing. >>>>> >>>>> Jeff, does this answer your question? >>>> >>>> I don't see how the issue here depends on the artificiality of the >>>> vptr; I'd expect to see the same problem with a data member. The >>>> problem is that a D base subobject is smaller than a complete D >>>> object, and in this case the base subobject is allocated such that >>>> if it were a full D object, it would overlap the end of E. And >>>> we're checking the MEM_REF as though accessing a full D object, so >>>> we get a warning. >>> >>> Thanks for chiming in! >>> >>> There is no warning for accesses to data members defined in >>> the virtual bases because their offset isn't constant. For example, >>> given the following modified version of the test case from the patch: >>> >>> struct A >>> { >>> virtual ~A (); >>> int a; >>> }; >>> >>> struct B: virtual A { int b; }; >>> struct C: virtual A { int c; }; >>> struct D: virtual B, virtual C >>> { >>> int d; >>> }; >>> >>> D d; >>> >>> void g () >>> { >>> D *p = &d; >>> p->a = p->b = p->c = p->d = 1; >>> } >>> >>> we end up with: >>> >>> void g () >>> { >>> ... >>> <bb 2> [local count: 1073741824]: >>> d.d = 1; <<< okay >>> _1 = d._vptr.D; <<< DECL_ARTIFICIAL >>> _2 = MEM[(long int *)_1 + -40B]; <<< not checked >>> _3 = (sizetype) _2; <<< not constant... >>> _4 = &d + _3; >>> MEM[(struct C *)_4].c = 1; <<< ...no warning >> >> Interesting. I do get a warning if I embed the access in the destructor: >> >> struct A >> { >> virtual ~A (); >> int a; >> }; >> >> struct B: virtual A { }; >> struct C: virtual A { >> int c; >> ~C() { c = 0; } // warns twice, for _vptr and c accesses >> }; >> struct D: virtual B, virtual C >> { >> D(); >> }; >> >> void g() >> { >> D d; >> } > > Ah, right. Unlike in member functions, in a dtor (or in the ctors > of the derived class) the offsets are known/constant so accesses > involving those are checked and we do get a false positive. Thanks > for the test case! > >> >>> _5 = MEM[(long int *)_1 + -24B]; >>> _6 = (sizetype) _5; >>> _7 = &d + _6; >>> MEM[(struct B *)_7].b = 1; >>> _8 = MEM[(long int *)_1 + -32B]; >>> _9 = (sizetype) _8; >>> _10 = &d + _9; >>> MEM[(struct A *)_10].a = 1; >>> return; >>> >>> } >>> >>>> The general issue about the confusion between complete and as-base >>>> types is PR 22488; I have various thoughts about a proper fix, but >>>> there have always been higher-priority things to work on. >>> >>> Thanks for the reference! >>> >>>> One possible approach to this PR is that we don't need to check an >>>> intermediate _REF; in >>>> >>>> MEM[(struct D *)&D.2651 + 24B]._vptr.D >>>> >>>> we aren't loading a whole D, we're only looking at the _vptr, which >>>> is within the bounds of E. >>> >>> This is also what the patch does: it keeps us from reaching >>> the MEM_REF because _vptr.D is artificial. >> >> My point applies equally to the access to c in my testcase above: we >> shouldn't check lvalue refs as though we're loading or storing the >> whole aggregate object. >> >> The full access is (*((D*)&e))._vptr.D. The MEM_REF represents the *, >> which we shouldn't check by itself, because it doesn't imply a memory >> access of its own. We should only check the COMPONENT_REF around it >> that is an actual memory access. > > The -Warray-bounds implementation checks every ARRAY_REF and MEM_REF, > regardless of their context(*). Aha. That seems like a flaw, but as you say, not one that can be addressed easily. > It doesn't check COMPONENT_REF except > while checking the other REFs (while walking up the use-def chains), > and the checks don't know the context of the REF. The hack this patch > put in was to circumvent that when the walk saw a COMPONENT_REF to > an artificial member (which as you showed isn't enough). > > I'm not sure if what you suggest (IIUC) is feasible without reworking > how the checking works in general. > What is feasible is working harder > and detecting if a) the type of the accessed object has virtual bases > and b) if the offset of the accessed field is within the bounds of > the enclosing object. In that case, skipping the MEM_REF check avoids > the warnings for all the cases we have discussed (artificial members, > ctors and dtors). It still seems hacky but sufficiently narrow in > scope for GCC 11(**). > > Please see the attached revision. > + else if (TREE_CODE (t) == COMPONENT_REF > + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF) > + { > + /* Hack: Skip MEM_REF checks in accesses to a member of a type > + with virtual bases at an offset that's in bounds of the size > + of the enclosing object. See pr98266 and pr97595. */ > + tree ref = TREE_OPERAND (TREE_OPERAND (t, 0), 0); > + if (TREE_CODE (ref) == ADDR_EXPR) > + ref = TREE_OPERAND (ref, 0); > + tree reftype = TREE_TYPE (ref); > + if (DECL_P (ref) > + && RECORD_OR_UNION_TYPE_P (reftype) > + && TYPE_BINFO (reftype)) > + { > + tree fld = TREE_OPERAND (t, 1); > + tree fldpos = byte_position (fld); > + if (tree refsize = DECL_SIZE_UNIT (ref)) > + if (TREE_CODE (refsize) == INTEGER_CST > + && tree_int_cst_lt (fldpos, refsize)) > + *walk_subtree = false; > + } > + } Here fld F is a member of a base class B, and ref is a variable of a derived class D, so you're comparing the offset of F in B to the size of D without considering the offset of B in D. I think you just need to add the MEM_REF offset to fldpos before comparing. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-03-02 14:11 ` Jason Merrill @ 2021-03-03 23:20 ` Martin Sebor 2021-03-04 5:33 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Martin Sebor @ 2021-03-03 23:20 UTC (permalink / raw) To: Jason Merrill, Jeff Law, gcc-patches [-- Attachment #1: Type: text/plain, Size: 13253 bytes --] On 3/2/21 7:11 AM, Jason Merrill wrote: > On 3/1/21 6:11 PM, Martin Sebor wrote: >> On 2/24/21 5:35 PM, Jason Merrill wrote: >>> On 2/23/21 6:07 PM, Martin Sebor wrote: >>>> On 2/23/21 2:52 PM, Jason Merrill wrote: >>>>> On 2/23/21 11:02 AM, Martin Sebor wrote: >>>>>> [CC Jason for any further comments/clarification] >>>>>> >>>>>> On 2/9/21 10:49 AM, Martin Sebor wrote: >>>>>>> On 2/8/21 4:11 PM, Jeff Law wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 2/8/21 3:44 PM, Martin Sebor wrote: >>>>>>>>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>>>>>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>>>>>>>> Similar to the problem reported for -Wstringop-overflow in >>>>>>>>>>>>> pr98266 >>>>>>>>>>>>> and already fixed, -Warray-bounds is also susceptible to false >>>>>>>>>>>>> positives in assignments and copies involving virtual >>>>>>>>>>>>> inheritance. >>>>>>>>>>>>> Because the two warnings don't share code yet (hopefully in >>>>>>>>>>>>> GCC 12) >>>>>>>>>>>>> the attached patch adds its own workaround for this problem to >>>>>>>>>>>>> gimple-array-bounds.cc, this one slightly more crude >>>>>>>>>>>>> because of >>>>>>>>>>>>> the limited insight the array bounds checking has into the >>>>>>>>>>>>> checked >>>>>>>>>>>>> expressions. >>>>>>>>>>>>> >>>>>>>>>>>>> Tested on x86_64-linux. >>>>>>>>>>>>> >>>>>>>>>>>>> Martin >>>>>>>>>>>>> >>>>>>>>>>>>> gcc-98266.diff >>>>>>>>>>>>> >>>>>>>>>>>>> PR middle-end/98266 - bogus array subscript is partly >>>>>>>>>>>>> outside array >>>>>>>>>>>>> bounds on virtual inheritance >>>>>>>>>>>>> >>>>>>>>>>>>> gcc/ChangeLog: >>>>>>>>>>>>> >>>>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>>>> * gimple-array-bounds.cc >>>>>>>>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>>>>>>>> Avoid checking references involving artificial members. >>>>>>>>>>>>> >>>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>>> >>>>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>>>>>>>> It seems to me that we've got the full statement at some >>>>>>>>>>>> point and >>>>>>>>>>>> thus >>>>>>>>>>>> the full expression so at some point couldn't we detect when >>>>>>>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>>>>>>>> TYPE_SIZE_UNIT >>>>>>>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>>>>>>>> >>>>>>>>>>>> Am I missing something? >>>>>>>>>>> >>>>>>>>>>> The expression we're looking at when the false positive is >>>>>>>>>>> issued >>>>>>>>>>> is the MEM_REF in the LHS of: >>>>>>>>>>> >>>>>>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> >>>>>>>>>>> [(void >>>>>>>>>>> *)&_ZTC1E24_1D + 24B]; >>>>>>>>>>> >>>>>>>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>>>>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>>>>>>>> DECL_SIZE and TYPE_SIZE. >>>>>>>>>> So that seems like it's a different issue then, unrelated to >>>>>>>>>> 97595. >>>>>>>>>> Right? >>>>>>>>> >>>>>>>>> I think the underlying problem is the same. We're getting a size >>>>>>>>> that doesn't correspond to what's actually being accessed, and it >>>>>>>>> happens because of the virtual inheritance. In pr97595 Jason >>>>>>>>> suggested to use the decl/type size inequality to identify this >>>>>>>>> case but I think we could have just as well used DECL_ARTIFICIAL >>>>>>>>> instead. At least the test cases from pr97595 both pass with >>>>>>>>> this change. >>>>>>>> But in the 98266 case the type and decl sizes are the same. So >>>>>>>> to be >>>>>>>> true that would mean that the underlying type we're using to access >>>>>>>> memory differs from its actual type. Is that the case in the >>>>>>>> IL? And >>>>>>>> does this have wider implications for diagnostics or >>>>>>>> optimizations that >>>>>>>> rely on accurate type sizing? >>>>>>>> >>>>>>>> I'm just trying to make sure I understand, not accepting or >>>>>>>> rejecting >>>>>>>> the patch yet. >>>>>>> >>>>>>> The part of the IL with the MEM_REF is this: >>>>>>> >>>>>>> void g () >>>>>>> { >>>>>>> void * D.2789; >>>>>>> struct E D.2652; >>>>>>> >>>>>>> <bb 2> [local count: 1073741824]: >>>>>>> E::E (&D.2652, ""); >>>>>>> f (&D.2652); >>>>>>> >>>>>>> <bb 3> [local count: 1073741824]: >>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> >>>>>>> [(void *)&_ZTC1E24_1D + 24B]; >>>>>>> ... >>>>>>> >>>>>>> The access here is to the _vptr.D pointer member of D.2652 which is >>>>>>> just past the end of the parent object (as reflected by its SIZE): >>>>>>> it sets sets up the virtual table pointer. >>>>>>> >>>>>>> The access in pr97595 is to the member subobject, which, as Jason >>>>>>> explained (and I accordingly documented under DECL_SIZE in tree.h), >>>>>>> is also laid out separately from the parent object. >>>>>>> >>>>>>> These cases aren't exactly the same (which is also why the test >>>>>>> I added for -Warray-bounds in pr97595 didn't expose this bug) but >>>>>>> they are closely related. The one here can be distinguished by >>>>>>> DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member >>>>>>> inequality. >>>>>>> >>>>>>> Might this impact other warnings? I'd say so if they don't take >>>>>>> these things into account. I just learned about this in pr97595 >>>>>>> which was a -Wstringop-overflow false positive but I also saw >>>>>>> a similar instance of -Warray-bounds with my patch to improve >>>>>>> caching and enhance array bounds checking. I dealt with that >>>>>>> instance of the warning in that patch but proactively added >>>>>>> a test case to the fix for pr97595. But the test case is focused >>>>>>> on the subobject access and not on one to the virtual table so >>>>>>> (as I said above) it didn't expose this bug. >>>>>>> >>>>>>> Might this also impact optimizations? I can imagine someone >>>>>>> unaware of this "gotcha" making the same "naive" assumption >>>>>>> I did, but I'd also expect such an invalid assumption to be >>>>>>> found either in code review or quickly cause problems in >>>>>>> testing. >>>>>> >>>>>> Jeff, does this answer your question? >>>>> >>>>> I don't see how the issue here depends on the artificiality of the >>>>> vptr; I'd expect to see the same problem with a data member. The >>>>> problem is that a D base subobject is smaller than a complete D >>>>> object, and in this case the base subobject is allocated such that >>>>> if it were a full D object, it would overlap the end of E. And >>>>> we're checking the MEM_REF as though accessing a full D object, so >>>>> we get a warning. >>>> >>>> Thanks for chiming in! >>>> >>>> There is no warning for accesses to data members defined in >>>> the virtual bases because their offset isn't constant. For example, >>>> given the following modified version of the test case from the patch: >>>> >>>> struct A >>>> { >>>> virtual ~A (); >>>> int a; >>>> }; >>>> >>>> struct B: virtual A { int b; }; >>>> struct C: virtual A { int c; }; >>>> struct D: virtual B, virtual C >>>> { >>>> int d; >>>> }; >>>> >>>> D d; >>>> >>>> void g () >>>> { >>>> D *p = &d; >>>> p->a = p->b = p->c = p->d = 1; >>>> } >>>> >>>> we end up with: >>>> >>>> void g () >>>> { >>>> ... >>>> <bb 2> [local count: 1073741824]: >>>> d.d = 1; <<< okay >>>> _1 = d._vptr.D; <<< DECL_ARTIFICIAL >>>> _2 = MEM[(long int *)_1 + -40B]; <<< not checked >>>> _3 = (sizetype) _2; <<< not constant... >>>> _4 = &d + _3; >>>> MEM[(struct C *)_4].c = 1; <<< ...no warning >>> >>> Interesting. I do get a warning if I embed the access in the >>> destructor: >>> >>> struct A >>> { >>> virtual ~A (); >>> int a; >>> }; >>> >>> struct B: virtual A { }; >>> struct C: virtual A { >>> int c; >>> ~C() { c = 0; } // warns twice, for _vptr and c accesses >>> }; >>> struct D: virtual B, virtual C >>> { >>> D(); >>> }; >>> >>> void g() >>> { >>> D d; >>> } >> >> Ah, right. Unlike in member functions, in a dtor (or in the ctors >> of the derived class) the offsets are known/constant so accesses >> involving those are checked and we do get a false positive. Thanks >> for the test case! >> >>> >>>> _5 = MEM[(long int *)_1 + -24B]; >>>> _6 = (sizetype) _5; >>>> _7 = &d + _6; >>>> MEM[(struct B *)_7].b = 1; >>>> _8 = MEM[(long int *)_1 + -32B]; >>>> _9 = (sizetype) _8; >>>> _10 = &d + _9; >>>> MEM[(struct A *)_10].a = 1; >>>> return; >>>> >>>> } >>>> >>>>> The general issue about the confusion between complete and as-base >>>>> types is PR 22488; I have various thoughts about a proper fix, but >>>>> there have always been higher-priority things to work on. >>>> >>>> Thanks for the reference! >>>> >>>>> One possible approach to this PR is that we don't need to check an >>>>> intermediate _REF; in >>>>> >>>>> MEM[(struct D *)&D.2651 + 24B]._vptr.D >>>>> >>>>> we aren't loading a whole D, we're only looking at the _vptr, which >>>>> is within the bounds of E. >>>> >>>> This is also what the patch does: it keeps us from reaching >>>> the MEM_REF because _vptr.D is artificial. >>> >>> My point applies equally to the access to c in my testcase above: we >>> shouldn't check lvalue refs as though we're loading or storing the >>> whole aggregate object. >>> >>> The full access is (*((D*)&e))._vptr.D. The MEM_REF represents the >>> *, which we shouldn't check by itself, because it doesn't imply a >>> memory access of its own. We should only check the COMPONENT_REF >>> around it that is an actual memory access. >> >> The -Warray-bounds implementation checks every ARRAY_REF and MEM_REF, >> regardless of their context(*). > > Aha. That seems like a flaw, but as you say, not one that can be > addressed easily. > >> It doesn't check COMPONENT_REF except >> while checking the other REFs (while walking up the use-def chains), >> and the checks don't know the context of the REF. The hack this patch >> put in was to circumvent that when the walk saw a COMPONENT_REF to >> an artificial member (which as you showed isn't enough). >> >> I'm not sure if what you suggest (IIUC) is feasible without reworking >> how the checking works in general. > >> What is feasible is working harder >> and detecting if a) the type of the accessed object has virtual bases >> and b) if the offset of the accessed field is within the bounds of >> the enclosing object. In that case, skipping the MEM_REF check avoids >> the warnings for all the cases we have discussed (artificial members, >> ctors and dtors). It still seems hacky but sufficiently narrow in >> scope for GCC 11(**). >> >> Please see the attached revision. > >> + else if (TREE_CODE (t) == COMPONENT_REF >> + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF) >> + { >> + /* Hack: Skip MEM_REF checks in accesses to a member of a type >> + with virtual bases at an offset that's in bounds of the size >> + of the enclosing object. See pr98266 and pr97595. */ >> + tree ref = TREE_OPERAND (TREE_OPERAND (t, 0), 0); >> + if (TREE_CODE (ref) == ADDR_EXPR) >> + ref = TREE_OPERAND (ref, 0); >> + tree reftype = TREE_TYPE (ref); >> + if (DECL_P (ref) >> + && RECORD_OR_UNION_TYPE_P (reftype) >> + && TYPE_BINFO (reftype)) >> + { >> + tree fld = TREE_OPERAND (t, 1); >> + tree fldpos = byte_position (fld); >> + if (tree refsize = DECL_SIZE_UNIT (ref)) >> + if (TREE_CODE (refsize) == INTEGER_CST >> + && tree_int_cst_lt (fldpos, refsize)) >> + *walk_subtree = false; >> + } >> + } > > Here fld F is a member of a base class B, and ref is a variable of a > derived class D, so you're comparing the offset of F in B to the size of > D without considering the offset of B in D. I think you just need to > add the MEM_REF offset to fldpos before comparing. I see what you mean, thanks, but I can't think of a test case to trigger a false negative due to the smaller offset. Any suggestions? With more testing I also realized that focusing solely on an underlying DECL like in the above doesn't prevent the warning when an object is created in dynamically allocated memory or in a backing buffer. So the attached revision both adjusts the offset computation upward and handles all kinds of backing store and moves the logic into a helper function for better readability. I've also added more tests. Retested on x86_64-linux. Thanks again for your help! Martin PS The TYPE_BINFO test isn't as quite as restrictive as I had hoped. It means we consider all derived class, not just those with virtual bases. I tried also requiring BINFO_VIRTUAL_P() to be true but that doesn't work. Using BINFO_VTABLE() does work but it still isn't the same. > > Jason > [-- Attachment #2: gcc-98266.diff --] [-- Type: text/x-patch, Size: 14297 bytes --] PR middle-end/98266 - bogus array subscript is partly outside array bounds on virtual inheritance gcc/ChangeLog: PR middle-end/98266 * gimple-array-bounds.cc (inbounds_vbase_memaccess_p): New function. (array_bounds_checker::check_array_bounds): Call it. gcc/testsuite/ChangeLog: PR middle-end/98266 * g++.dg/warn/Warray-bounds-15.C: New test. * g++.dg/warn/Warray-bounds-18.C: New test. * g++.dg/warn/Warray-bounds-19.C: New test. * g++.dg/warn/Warray-bounds-20.C: New test. * g++.dg/warn/Warray-bounds-21.C: New test. diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array-bounds.cc index d7fd2c74111..54f32051199 100644 --- a/gcc/gimple-array-bounds.cc +++ b/gcc/gimple-array-bounds.cc @@ -890,6 +890,50 @@ array_bounds_checker::check_addr_expr (location_t location, tree t) } } +/* Return true if T is a reference to a member of a base class that's within + the bounds of the enclosing complete object. The function "hacks" around + problems discussed in pr98266 and pr97595. */ + +static bool +inbounds_vbase_memaccess_p (tree t) +{ + if (TREE_CODE (t) != COMPONENT_REF) + return false; + + tree mref = TREE_OPERAND (t, 0); + if (TREE_CODE (mref) != MEM_REF) + return false; + + /* Consider the access if its type is a derived class. */ + tree mreftype = TREE_TYPE (mref); + if (!RECORD_OR_UNION_TYPE_P (mreftype) + || !TYPE_BINFO (mreftype)) + return false; + + /* Compute the size of the referenced object (it could be dynamically + allocated). */ + access_ref aref; // unused + tree refop = TREE_OPERAND (mref, 0); + tree refsize = compute_objsize (refop, 1, &aref); + if (!refsize || TREE_CODE (refsize) != INTEGER_CST) + return false; + + /* Compute the byte offset of the member within its enclosing class. */ + tree fld = TREE_OPERAND (t, 1); + tree fldpos = byte_position (fld); + if (TREE_CODE (fldpos) != INTEGER_CST) + return false; + + /* Compute the byte offset of the member with the outermost complete + object by adding its offset computed above to the MEM_REF offset. */ + tree refoff = TREE_OPERAND (mref, 1); + tree fldoff = int_const_binop (PLUS_EXPR, fldpos, refoff); + + /* Return true if the member offset is less than the size of the complete + object. */ + return tree_int_cst_lt (fldoff, refsize); +} + /* Callback for walk_tree to check a tree for out of bounds array accesses. The array_bounds_checker class is passed in DATA. */ @@ -919,8 +963,14 @@ array_bounds_checker::check_array_bounds (tree *tp, int *walk_subtree, else if (TREE_CODE (t) == ADDR_EXPR) { checker->check_addr_expr (location, t); - *walk_subtree = FALSE; + *walk_subtree = false; } + else if (inbounds_vbase_memaccess_p (t)) + /* Hack: Skip MEM_REF checks in accesses to a member of a base class + at an offset that's within the bounds of the enclosing object. + See pr98266 and pr97595. */ + *walk_subtree = false; + /* Propagate the no-warning bit to the outer expression. */ if (warned) TREE_NO_WARNING (t) = true; diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C new file mode 100644 index 00000000000..455f3a0c300 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-15.C @@ -0,0 +1,33 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +#if __cplusplus < 201103L +// This matters for the test case. +# define noexcept throw () +#endif + +struct A +{ + virtual ~A () noexcept; + const char *s; +}; + +struct B: virtual A { }; +struct C: virtual A { }; // { dg-bogus "\\\[-Warray-bounds" } + +struct D: virtual B, virtual C +{ + D (const char*); +}; + +void sink (void*); +void sink (D); + + +// Verify that accesses to the table aren't diagnosed. +void test_vtbl () +{ + sink (D ("")); +} diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-18.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-18.C new file mode 100644 index 00000000000..53d93cf83e7 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-18.C @@ -0,0 +1,167 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +struct A +{ + int ai, aj, aa[2]; + + virtual ~A (); +}; + +struct B: virtual A { }; +struct C: virtual A { }; + +void sink (void*); + +struct C1: virtual A +{ + int c2i, c2j, c2a[2]; + + C1 (); + ~C1 () + { // { dg-bogus "\\\[-Warray-bounds" } + c2i = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + c2j = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + c2a[0] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + c2a[1] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + c2a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +struct D1: virtual B, virtual C1 +{ + D1 (); +}; + +void sink (void*); + +/* Verify that only out of bounds accesses to members of an ordinary base + class are diagnosed. Use direct array accesses. */ +void test_vmem_base_ctor_arryaccess () +{ + D1 d2; + sink (&d2); +} + + +struct C2: virtual A +{ + int c3a[2]; + + C2 (); + ~C2 () + { // { dg-bogus "\\\[-Warray-bounds" } + int *p = c3a; + *p++ = __LINE__; + *p++ = __LINE__; + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +struct D2: virtual B, virtual C2 +{ + D2 (); +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class are diagnosed. Use pointer accesses. */ +void test_vmem_base_dtor_ptraccess () +{ + D2 d3; + sink (&d3); +} + + +struct C3: virtual A // { dg-bogus "\\\[-Warray-bounds" } +{ + int i, j, a[2]; + + C3 (); +}; + +struct D3: virtual B, virtual C3 +{ + D3 () + { // { dg-bogus "\\\[-Warray-bounds" } + i = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + j = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[0] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[1] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the ctor of a derived class are diagnosed. Use direct + array accesses. */ +void test_vmem_derived_ctor_arryaccess () +{ + D3 d4; + sink (&d4); +} + + +struct D4: virtual B, virtual C3 +{ + D4 () + { // { dg-bogus "\\\[-Warray-bounds" } + int *p = a; + *p++ = __LINE__; + *p++ = __LINE__; + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the ctor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_ctor_ptraccess () +{ + D4 d5; + sink (&d5); +} + + +struct D5: virtual B, virtual C3 // { dg-bogus "\\\[-Warray-bounds" } +{ + ~D5 () + { + i = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + j = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[0] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[1] = __LINE__; // { dg-bogus "\\\[-Warray-bounds" } + a[2] = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the dtor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_dtor_arryaccess () +{ + D5 d6; + sink (&d6); +} + + +struct D6: virtual B, virtual C3 // { dg-bogus "\\\[-Warray-bounds" } +{ + ~D6 () + { + int *p = a; + *p++ = __LINE__; + *p++ = __LINE__; + *p++ = __LINE__; // { dg-warning "\\\[-Warray-bounds" } + } +}; + +/* Verify that only out of bounds accesses to members of an ordinary base + class made in the dtor of a derived class are diagnosed. Use pointer + accesses. */ +void test_vmem_derived_dtor_ptraccess () +{ + D6 d7; + sink (&d7); +} diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-19.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-19.C new file mode 100644 index 00000000000..e5fabe955ea --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-19.C @@ -0,0 +1,110 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +void* operator new (__SIZE_TYPE__, void *p) { return p; } +void* operator new[] (__SIZE_TYPE__, void *p) { return p; } + + +struct A +{ + virtual ~A (); + int ai; +}; + +struct B: virtual A { }; + + +// Exercise access to base members by ctor of the most derived class. + +struct C1: virtual A // { dg-bogus "\\\[-Warray-bounds" } +{ + int c1i; + C1 (); +}; + +struct D1: virtual B, virtual C1 +{ + D1 () { ai = 0; c1i = 1; }; +}; + +void sink (void*); + +void nowarn_derived_ctor_access_decl () +{ + D1 d1; + sink (&d1); +} + +void nowarn_derived_ctor_access_new () +{ + D1 *p = new D1; + sink (p); +} + +void nowarn_derived_ctor_access_placement_new () +{ + char a[sizeof (D1)]; + D1 *p = new (a) D1; + sink (p); +} + +void nowarn_derived_ctor_access_new_array () +{ + D1 *p = new D1[2]; + sink (p); +} + +void nowarn_derived_ctor_access_placement_new_array () +{ + char a[sizeof (D1) * 2]; + D1 *p = new (a) D1[2]; + sink (p); +} + + +// Exercise access to base members by ctor of the second most derived class. + +struct C2: virtual A +{ + int c2i; + ~C2 () { ai = 0; c2i = 1; } // { dg-bogus "\\\[-Warray-bounds" +}; + +struct D2: virtual B, virtual C2 +{ + D2 (); +}; + +void nowarn_base_dtor_access_decl () +{ + D2 d2; + sink (&d2); +} + +void nowarn_base_dtor_access_new () +{ + D2 *p = new D2; + sink (p); +} + +void nowarn_base_dtor_access_placement_new () +{ + char a[sizeof (D2)]; + D2 *p = new (a) D2; + sink (p); +} + +void nowarn_base_dtor_access_new_array () +{ + D2 *p = new D2[2]; + sink (p); +} + +void nowarn_base_dtor_access_placement_new_array () +{ + char a[sizeof (D2) * 2]; + D2 *p = new (a) D2[2]; + sink (p); +} diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-20.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-20.C new file mode 100644 index 00000000000..e142ea16787 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-20.C @@ -0,0 +1,68 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +void* operator new (__SIZE_TYPE__, void *p) { return p; } +void* operator new[] (__SIZE_TYPE__, void *p) { return p; } + + +struct A +{ + int ai; + virtual ~A (); +}; + +struct B: virtual A { }; + +struct C: virtual A +{ + int ci; + C (); +}; + +struct D1: virtual B, virtual C +{ + /* The warning would ideally point to the assignment but instead points + to the opening brace. */ + D1 () + { // { dg-warning "\\\[-Warray-bounds" "brace" } + ci = 0; // { dg-warning "\\\[-Warray-bounds" "assign" { xfail *-*-* } } + } +}; + +void sink (void*); + +void warn_derived_ctor_access_new_decl () +{ + char a[sizeof (D1)]; // { dg-message "referencing 'a'" "note" } + char *p = a; + ++p; + D1 *q = new (p) D1; + sink (q); +} + +void warn_derived_ctor_access_new_alloc () +{ + char *p = (char*)operator new (sizeof (D1)); // { dg-message "referencing an object of size \\d+ allocated by 'void\\\* operator new\\\(" "note" } + ++p; + D1 *q = new (p) D1; + sink (q); +} + +void warn_derived_ctor_access_new_array_decl () +{ + char b[sizeof (D1) * 2]; // { dg-message "referencing 'b'" "note" } + char *p = b; + ++p; + D1 *q = new (p) D1[2]; + sink (q); +} + +void warn_derived_ctor_access_new_array_alloc () +{ + char *p = new char[sizeof (D1) * 2]; // { dg-message "referencing an object of size \\d+ allocated by 'void\\\* operator new \\\[]\\\(" "note" } + ++p; + D1 *q = new (p) D1[2]; + sink (q); +} diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds-21.C b/gcc/testsuite/g++.dg/warn/Warray-bounds-21.C new file mode 100644 index 00000000000..57bb98bf63f --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds-21.C @@ -0,0 +1,111 @@ +/* PR middle-end/98266 - bogus array subscript is partly outside array + bounds on virtual inheritance + Same as Warray-bounds-19.C with nonvirtual inheritance. + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +void* operator new (__SIZE_TYPE__, void *p) { return p; } +void* operator new[] (__SIZE_TYPE__, void *p) { return p; } + + +struct A +{ + virtual ~A (); + int ai; +}; + +struct B: A { }; + + +// Exercise access to base members by ctor of the most derived class. + +struct C1: A +{ + int c1i; + C1 (); +}; + +struct D1: B, C1 +{ + D1 () { B::ai = 0; C1::ai = 1; c1i = 2; }; +}; + +void sink (void*); + +void nowarn_derived_ctor_access_decl () +{ + D1 d1; + sink (&d1); +} + +void nowarn_derived_ctor_access_new () +{ + D1 *p = new D1; + sink (p); +} + +void nowarn_derived_ctor_access_placement_new () +{ + char a[sizeof (D1)]; + D1 *p = new (a) D1; + sink (p); +} + +void nowarn_derived_ctor_access_new_array () +{ + D1 *p = new D1[2]; + sink (p); +} + +void nowarn_derived_ctor_access_placement_new_array () +{ + char a[sizeof (D1) * 2]; + D1 *p = new (a) D1[2]; + sink (p); +} + + +// Exercise access to base members by ctor of the second most derived class. + +struct C2: A +{ + int c2i; + ~C2 () { ai = 0; c2i = 1; } +}; + +struct D2: B, C2 +{ + D2 (); +}; + +void nowarn_base_dtor_access_decl () +{ + D2 d2; + sink (&d2); +} + +void nowarn_base_dtor_access_new () +{ + D2 *p = new D2; + sink (p); +} + +void nowarn_base_dtor_access_placement_new () +{ + char a[sizeof (D2)]; + D2 *p = new (a) D2; + sink (p); +} + +void nowarn_base_dtor_access_new_array () +{ + D2 *p = new D2[2]; + sink (p); +} + +void nowarn_base_dtor_access_placement_new_array () +{ + char a[sizeof (D2) * 2]; + D2 *p = new (a) D2[2]; + sink (p); +} ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-03-03 23:20 ` Martin Sebor @ 2021-03-04 5:33 ` Jason Merrill 2021-03-04 17:44 ` Martin Sebor 0 siblings, 1 reply; 24+ messages in thread From: Jason Merrill @ 2021-03-04 5:33 UTC (permalink / raw) To: Martin Sebor, Jeff Law, gcc-patches On 3/3/21 6:20 PM, Martin Sebor wrote: > On 3/2/21 7:11 AM, Jason Merrill wrote: >> On 3/1/21 6:11 PM, Martin Sebor wrote: >>> On 2/24/21 5:35 PM, Jason Merrill wrote: >>>> On 2/23/21 6:07 PM, Martin Sebor wrote: >>>>> On 2/23/21 2:52 PM, Jason Merrill wrote: >>>>>> On 2/23/21 11:02 AM, Martin Sebor wrote: >>>>>>> [CC Jason for any further comments/clarification] >>>>>>> >>>>>>> On 2/9/21 10:49 AM, Martin Sebor wrote: >>>>>>>> On 2/8/21 4:11 PM, Jeff Law wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 2/8/21 3:44 PM, Martin Sebor wrote: >>>>>>>>>> On 2/8/21 3:26 PM, Jeff Law wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 2/8/21 2:56 PM, Martin Sebor wrote: >>>>>>>>>>>> On 2/8/21 12:59 PM, Jeff Law wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>>>>>>>>>>>>> Similar to the problem reported for -Wstringop-overflow in >>>>>>>>>>>>>> pr98266 >>>>>>>>>>>>>> and already fixed, -Warray-bounds is also susceptible to >>>>>>>>>>>>>> false >>>>>>>>>>>>>> positives in assignments and copies involving virtual >>>>>>>>>>>>>> inheritance. >>>>>>>>>>>>>> Because the two warnings don't share code yet (hopefully >>>>>>>>>>>>>> in GCC 12) >>>>>>>>>>>>>> the attached patch adds its own workaround for this >>>>>>>>>>>>>> problem to >>>>>>>>>>>>>> gimple-array-bounds.cc, this one slightly more crude >>>>>>>>>>>>>> because of >>>>>>>>>>>>>> the limited insight the array bounds checking has into the >>>>>>>>>>>>>> checked >>>>>>>>>>>>>> expressions. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Tested on x86_64-linux. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Martin >>>>>>>>>>>>>> >>>>>>>>>>>>>> gcc-98266.diff >>>>>>>>>>>>>> >>>>>>>>>>>>>> PR middle-end/98266 - bogus array subscript is partly >>>>>>>>>>>>>> outside array >>>>>>>>>>>>>> bounds on virtual inheritance >>>>>>>>>>>>>> >>>>>>>>>>>>>> gcc/ChangeLog: >>>>>>>>>>>>>> >>>>>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>>>>> * gimple-array-bounds.cc >>>>>>>>>>>>>> (array_bounds_checker::check_array_bounds): >>>>>>>>>>>>>> Avoid checking references involving artificial members. >>>>>>>>>>>>>> >>>>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>>>> >>>>>>>>>>>>>> PR middle-end/98266 >>>>>>>>>>>>>> * g++.dg/warn/Warray-bounds-15.C: New test. >>>>>>>>>>>>> It seems to me that we've got the full statement at some >>>>>>>>>>>>> point and >>>>>>>>>>>>> thus >>>>>>>>>>>>> the full expression so at some point couldn't we detect when >>>>>>>>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using >>>>>>>>>>>>> TYPE_SIZE_UNIT >>>>>>>>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >>>>>>>>>>>>> >>>>>>>>>>>>> Am I missing something? >>>>>>>>>>>> >>>>>>>>>>>> The expression we're looking at when the false positive is >>>>>>>>>>>> issued >>>>>>>>>>>> is the MEM_REF in the LHS of: >>>>>>>>>>>> >>>>>>>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> >>>>>>>>>>>> [(void >>>>>>>>>>>> *)&_ZTC1E24_1D + 24B]; >>>>>>>>>>>> >>>>>>>>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and >>>>>>>>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between >>>>>>>>>>>> DECL_SIZE and TYPE_SIZE. >>>>>>>>>>> So that seems like it's a different issue then, unrelated to >>>>>>>>>>> 97595. >>>>>>>>>>> Right? >>>>>>>>>> >>>>>>>>>> I think the underlying problem is the same. We're getting a size >>>>>>>>>> that doesn't correspond to what's actually being accessed, and it >>>>>>>>>> happens because of the virtual inheritance. In pr97595 Jason >>>>>>>>>> suggested to use the decl/type size inequality to identify this >>>>>>>>>> case but I think we could have just as well used DECL_ARTIFICIAL >>>>>>>>>> instead. At least the test cases from pr97595 both pass with >>>>>>>>>> this change. >>>>>>>>> But in the 98266 case the type and decl sizes are the same. So >>>>>>>>> to be >>>>>>>>> true that would mean that the underlying type we're using to >>>>>>>>> access >>>>>>>>> memory differs from its actual type. Is that the case in the >>>>>>>>> IL? And >>>>>>>>> does this have wider implications for diagnostics or >>>>>>>>> optimizations that >>>>>>>>> rely on accurate type sizing? >>>>>>>>> >>>>>>>>> I'm just trying to make sure I understand, not accepting or >>>>>>>>> rejecting >>>>>>>>> the patch yet. >>>>>>>> >>>>>>>> The part of the IL with the MEM_REF is this: >>>>>>>> >>>>>>>> void g () >>>>>>>> { >>>>>>>> void * D.2789; >>>>>>>> struct E D.2652; >>>>>>>> >>>>>>>> <bb 2> [local count: 1073741824]: >>>>>>>> E::E (&D.2652, ""); >>>>>>>> f (&D.2652); >>>>>>>> >>>>>>>> <bb 3> [local count: 1073741824]: >>>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> >>>>>>>> [(void *)&_ZTC1E24_1D + 24B]; >>>>>>>> ... >>>>>>>> >>>>>>>> The access here is to the _vptr.D pointer member of D.2652 which is >>>>>>>> just past the end of the parent object (as reflected by its SIZE): >>>>>>>> it sets sets up the virtual table pointer. >>>>>>>> >>>>>>>> The access in pr97595 is to the member subobject, which, as Jason >>>>>>>> explained (and I accordingly documented under DECL_SIZE in tree.h), >>>>>>>> is also laid out separately from the parent object. >>>>>>>> >>>>>>>> These cases aren't exactly the same (which is also why the test >>>>>>>> I added for -Warray-bounds in pr97595 didn't expose this bug) but >>>>>>>> they are closely related. The one here can be distinguished by >>>>>>>> DECL_ARTIFICAL. The other by the DECL_SIZE != TYPE_SIZE member >>>>>>>> inequality. >>>>>>>> >>>>>>>> Might this impact other warnings? I'd say so if they don't take >>>>>>>> these things into account. I just learned about this in pr97595 >>>>>>>> which was a -Wstringop-overflow false positive but I also saw >>>>>>>> a similar instance of -Warray-bounds with my patch to improve >>>>>>>> caching and enhance array bounds checking. I dealt with that >>>>>>>> instance of the warning in that patch but proactively added >>>>>>>> a test case to the fix for pr97595. But the test case is focused >>>>>>>> on the subobject access and not on one to the virtual table so >>>>>>>> (as I said above) it didn't expose this bug. >>>>>>>> >>>>>>>> Might this also impact optimizations? I can imagine someone >>>>>>>> unaware of this "gotcha" making the same "naive" assumption >>>>>>>> I did, but I'd also expect such an invalid assumption to be >>>>>>>> found either in code review or quickly cause problems in >>>>>>>> testing. >>>>>>> >>>>>>> Jeff, does this answer your question? >>>>>> >>>>>> I don't see how the issue here depends on the artificiality of the >>>>>> vptr; I'd expect to see the same problem with a data member. The >>>>>> problem is that a D base subobject is smaller than a complete D >>>>>> object, and in this case the base subobject is allocated such that >>>>>> if it were a full D object, it would overlap the end of E. And >>>>>> we're checking the MEM_REF as though accessing a full D object, so >>>>>> we get a warning. >>>>> >>>>> Thanks for chiming in! >>>>> >>>>> There is no warning for accesses to data members defined in >>>>> the virtual bases because their offset isn't constant. For example, >>>>> given the following modified version of the test case from the patch: >>>>> >>>>> struct A >>>>> { >>>>> virtual ~A (); >>>>> int a; >>>>> }; >>>>> >>>>> struct B: virtual A { int b; }; >>>>> struct C: virtual A { int c; }; >>>>> struct D: virtual B, virtual C >>>>> { >>>>> int d; >>>>> }; >>>>> >>>>> D d; >>>>> >>>>> void g () >>>>> { >>>>> D *p = &d; >>>>> p->a = p->b = p->c = p->d = 1; >>>>> } >>>>> >>>>> we end up with: >>>>> >>>>> void g () >>>>> { >>>>> ... >>>>> <bb 2> [local count: 1073741824]: >>>>> d.d = 1; <<< okay >>>>> _1 = d._vptr.D; <<< DECL_ARTIFICIAL >>>>> _2 = MEM[(long int *)_1 + -40B]; <<< not checked >>>>> _3 = (sizetype) _2; <<< not constant... >>>>> _4 = &d + _3; >>>>> MEM[(struct C *)_4].c = 1; <<< ...no warning >>>> >>>> Interesting. I do get a warning if I embed the access in the >>>> destructor: >>>> >>>> struct A >>>> { >>>> virtual ~A (); >>>> int a; >>>> }; >>>> >>>> struct B: virtual A { }; >>>> struct C: virtual A { >>>> int c; >>>> ~C() { c = 0; } // warns twice, for _vptr and c accesses >>>> }; >>>> struct D: virtual B, virtual C >>>> { >>>> D(); >>>> }; >>>> >>>> void g() >>>> { >>>> D d; >>>> } >>> >>> Ah, right. Unlike in member functions, in a dtor (or in the ctors >>> of the derived class) the offsets are known/constant so accesses >>> involving those are checked and we do get a false positive. Thanks >>> for the test case! >>> >>>> >>>>> _5 = MEM[(long int *)_1 + -24B]; >>>>> _6 = (sizetype) _5; >>>>> _7 = &d + _6; >>>>> MEM[(struct B *)_7].b = 1; >>>>> _8 = MEM[(long int *)_1 + -32B]; >>>>> _9 = (sizetype) _8; >>>>> _10 = &d + _9; >>>>> MEM[(struct A *)_10].a = 1; >>>>> return; >>>>> >>>>> } >>>>> >>>>>> The general issue about the confusion between complete and as-base >>>>>> types is PR 22488; I have various thoughts about a proper fix, but >>>>>> there have always been higher-priority things to work on. >>>>> >>>>> Thanks for the reference! >>>>> >>>>>> One possible approach to this PR is that we don't need to check an >>>>>> intermediate _REF; in >>>>>> >>>>>> MEM[(struct D *)&D.2651 + 24B]._vptr.D >>>>>> >>>>>> we aren't loading a whole D, we're only looking at the _vptr, >>>>>> which is within the bounds of E. >>>>> >>>>> This is also what the patch does: it keeps us from reaching >>>>> the MEM_REF because _vptr.D is artificial. >>>> >>>> My point applies equally to the access to c in my testcase above: we >>>> shouldn't check lvalue refs as though we're loading or storing the >>>> whole aggregate object. >>>> >>>> The full access is (*((D*)&e))._vptr.D. The MEM_REF represents the >>>> *, which we shouldn't check by itself, because it doesn't imply a >>>> memory access of its own. We should only check the COMPONENT_REF >>>> around it that is an actual memory access. >>> >>> The -Warray-bounds implementation checks every ARRAY_REF and MEM_REF, >>> regardless of their context(*). >> >> Aha. That seems like a flaw, but as you say, not one that can be >> addressed easily. >> >>> It doesn't check COMPONENT_REF except >>> while checking the other REFs (while walking up the use-def chains), >>> and the checks don't know the context of the REF. The hack this patch >>> put in was to circumvent that when the walk saw a COMPONENT_REF to >>> an artificial member (which as you showed isn't enough). >>> >>> I'm not sure if what you suggest (IIUC) is feasible without reworking >>> how the checking works in general. >> >>> What is feasible is working harder >>> and detecting if a) the type of the accessed object has virtual bases >>> and b) if the offset of the accessed field is within the bounds of >>> the enclosing object. In that case, skipping the MEM_REF check avoids >>> the warnings for all the cases we have discussed (artificial members, >>> ctors and dtors). It still seems hacky but sufficiently narrow in >>> scope for GCC 11(**). >>> >>> Please see the attached revision. >> >>> + else if (TREE_CODE (t) == COMPONENT_REF >>> + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF) >>> + { >>> + /* Hack: Skip MEM_REF checks in accesses to a member of a type >>> + with virtual bases at an offset that's in bounds of the size >>> + of the enclosing object. See pr98266 and pr97595. */ >>> + tree ref = TREE_OPERAND (TREE_OPERAND (t, 0), 0); >>> + if (TREE_CODE (ref) == ADDR_EXPR) >>> + ref = TREE_OPERAND (ref, 0); >>> + tree reftype = TREE_TYPE (ref); >>> + if (DECL_P (ref) >>> + && RECORD_OR_UNION_TYPE_P (reftype) >>> + && TYPE_BINFO (reftype)) >>> + { >>> + tree fld = TREE_OPERAND (t, 1); >>> + tree fldpos = byte_position (fld); >>> + if (tree refsize = DECL_SIZE_UNIT (ref)) >>> + if (TREE_CODE (refsize) == INTEGER_CST >>> + && tree_int_cst_lt (fldpos, refsize)) >>> + *walk_subtree = false; >>> + } >>> + } >> >> Here fld F is a member of a base class B, and ref is a variable of a >> derived class D, so you're comparing the offset of F in B to the size >> of D without considering the offset of B in D. I think you just need >> to add the MEM_REF offset to fldpos before comparing. > > I see what you mean, thanks, but I can't think of a test case to > trigger a false negative due to the smaller offset. Any suggestions? My only ideas involve undefined behavior, casting the address to a pointer to an unrelated too-large class. I don't know if that would show up as a MEM_REF of this pattern. > With more testing I also realized that focusing solely on an underlying > DECL like in the above doesn't prevent the warning when an object is > created in dynamically allocated memory or in a backing buffer. So > the attached revision both adjusts the offset computation upward and > handles all kinds of backing store and moves the logic into a helper > function for better readability. I've also added more tests. > > Retested on x86_64-linux. > > Thanks again for your help! > > Martin > > PS The TYPE_BINFO test isn't as quite as restrictive as I had hoped. > It means we consider all derived class, not just those with virtual > bases. It's even less restrictive than that: all C++ classes have TYPE_BINFO. > I tried also requiring BINFO_VIRTUAL_P() to be true but that > doesn't work. Right, BINFO_VIRTUAL_P is true for the binfo representing the virtual base in the inheritance hierarchy, not on the binfo for the derived class. > Using BINFO_VTABLE() does work but it still isn't the same. Indeed, virtual functions and virtual bases both cause a class to have a vtable. You could also check BINFO_N_BASE_BINFOS. Or not bother trying to restrict this to classes with virtual bases (i.e. leave the patch as it is), since the result is just as correct for other classes. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-03-04 5:33 ` Jason Merrill @ 2021-03-04 17:44 ` Martin Sebor 2021-03-04 20:00 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Martin Sebor @ 2021-03-04 17:44 UTC (permalink / raw) To: Jason Merrill, Jeff Law, gcc-patches On 3/3/21 10:33 PM, Jason Merrill wrote: > On 3/3/21 6:20 PM, Martin Sebor wrote: ... >> I see what you mean, thanks, but I can't think of a test case to >> trigger a false negative due to the smaller offset. Any suggestions? > > My only ideas involve undefined behavior, casting the address to a > pointer to an unrelated too-large class. I don't know if that would > show up as a MEM_REF of this pattern. Okay, let me see if I can come up with something based on that. > >> With more testing I also realized that focusing solely on an underlying >> DECL like in the above doesn't prevent the warning when an object is >> created in dynamically allocated memory or in a backing buffer. So >> the attached revision both adjusts the offset computation upward and >> handles all kinds of backing store and moves the logic into a helper >> function for better readability. I've also added more tests. >> >> Retested on x86_64-linux. >> >> Thanks again for your help! >> >> Martin >> >> PS The TYPE_BINFO test isn't as quite as restrictive as I had hoped. >> It means we consider all derived class, not just those with virtual >> bases. > > It's even less restrictive than that: all C++ classes have TYPE_BINFO. > >> I tried also requiring BINFO_VIRTUAL_P() to be true but that >> doesn't work. > > Right, BINFO_VIRTUAL_P is true for the binfo representing the virtual > base in the inheritance hierarchy, not on the binfo for the derived class. > >> Using BINFO_VTABLE() does work but it still isn't the same. > > Indeed, virtual functions and virtual bases both cause a class to have a > vtable. You could also check BINFO_N_BASE_BINFOS. > > Or not bother trying to restrict this to classes with virtual bases > (i.e. leave the patch as it is), since the result is just as correct for > other classes. > > Jason > I think I'll just go with this last version if you're okay with it as is. Okay to commit? Martin PS Let me propose a separate patch to add some text to the BINFO comments in tree.h to clarify the macros per the above. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-03-04 17:44 ` Martin Sebor @ 2021-03-04 20:00 ` Jason Merrill 0 siblings, 0 replies; 24+ messages in thread From: Jason Merrill @ 2021-03-04 20:00 UTC (permalink / raw) To: Martin Sebor, Jeff Law, gcc-patches On 3/4/21 12:44 PM, Martin Sebor wrote: > On 3/3/21 10:33 PM, Jason Merrill wrote: >> On 3/3/21 6:20 PM, Martin Sebor wrote: > ... >>> I see what you mean, thanks, but I can't think of a test case to >>> trigger a false negative due to the smaller offset. Any suggestions? >> >> My only ideas involve undefined behavior, casting the address to a >> pointer to an unrelated too-large class. I don't know if that would >> show up as a MEM_REF of this pattern. > > Okay, let me see if I can come up with something based on that. > >> >>> With more testing I also realized that focusing solely on an underlying >>> DECL like in the above doesn't prevent the warning when an object is >>> created in dynamically allocated memory or in a backing buffer. So >>> the attached revision both adjusts the offset computation upward and >>> handles all kinds of backing store and moves the logic into a helper >>> function for better readability. I've also added more tests. >>> >>> Retested on x86_64-linux. >>> >>> Thanks again for your help! >>> >>> Martin >>> >>> PS The TYPE_BINFO test isn't as quite as restrictive as I had hoped. >>> It means we consider all derived class, not just those with virtual >>> bases. >> >> It's even less restrictive than that: all C++ classes have TYPE_BINFO. >> >>> I tried also requiring BINFO_VIRTUAL_P() to be true but that >>> doesn't work. >> >> Right, BINFO_VIRTUAL_P is true for the binfo representing the virtual >> base in the inheritance hierarchy, not on the binfo for the derived >> class. >> >>> Using BINFO_VTABLE() does work but it still isn't the same. >> >> Indeed, virtual functions and virtual bases both cause a class to have >> a vtable. You could also check BINFO_N_BASE_BINFOS. >> >> Or not bother trying to restrict this to classes with virtual bases >> (i.e. leave the patch as it is), since the result is just as correct >> for other classes. >> >> Jason >> > > I think I'll just go with this last version if you're okay with it > as is. Okay to commit? OK. > Martin > > PS Let me propose a separate patch to add some text to the BINFO > comments in tree.h to clarify the macros per the above. > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-23 21:52 ` Jason Merrill 2021-02-23 23:07 ` Martin Sebor @ 2021-02-25 23:47 ` Jeff Law 2021-02-26 3:56 ` Jason Merrill 1 sibling, 1 reply; 24+ messages in thread From: Jeff Law @ 2021-02-25 23:47 UTC (permalink / raw) To: Jason Merrill, Martin Sebor, gcc-patches On 2/23/21 2:52 PM, Jason Merrill wrote: > > I don't see how the issue here depends on the artificiality of the vptr; That's what I was trying to get at -- is DECL_ARTIFICIAL really a good way to detect these kinds of cases. It sounds like it likely isn't, but it may also be the best we can do. I'm not sure yet. > I'd expect to see the same problem with a data member. The problem is > that a D base subobject is smaller than a complete D object, and in > this case the base subobject is allocated such that if it were a full > D object, it would overlap the end of E. And we're checking the > MEM_REF as though accessing a full D object, so we get a warning. > > The general issue about the confusion between complete and as-base > types is PR 22488; I have various thoughts about a proper fix, but > there have always been higher-priority things to work on. > > One possible approach to this PR is that we don't need to check an > intermediate _REF; in > > MEM[(struct D *)&D.2651 + 24B]._vptr.D > > we aren't loading a whole D, we're only looking at the _vptr, which is > within the bounds of E. That raises a really interesting question. What's the right bounds to use with these kinds of constructs. Should we be looking at D's type (struct E) or struct D? I'm not sure there's a one-size-fits-all answer here and I don't think we have any other context go help guide us. Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-25 23:47 ` Jeff Law @ 2021-02-26 3:56 ` Jason Merrill 0 siblings, 0 replies; 24+ messages in thread From: Jason Merrill @ 2021-02-26 3:56 UTC (permalink / raw) To: Jeff Law, Martin Sebor, gcc-patches On 2/25/21 6:47 PM, Jeff Law wrote: > > > On 2/23/21 2:52 PM, Jason Merrill wrote: >> >> I don't see how the issue here depends on the artificiality of the vptr; > That's what I was trying to get at -- is DECL_ARTIFICIAL really a good > way to detect these kinds of cases. It sounds like it likely isn't, but > it may also be the best we can do. I'm not sure yet. > > > > >> I'd expect to see the same problem with a data member. The problem is >> that a D base subobject is smaller than a complete D object, and in >> this case the base subobject is allocated such that if it were a full >> D object, it would overlap the end of E. And we're checking the >> MEM_REF as though accessing a full D object, so we get a warning. >> >> The general issue about the confusion between complete and as-base >> types is PR 22488; I have various thoughts about a proper fix, but >> there have always been higher-priority things to work on. >> >> One possible approach to this PR is that we don't need to check an >> intermediate _REF; in >> >> MEM[(struct D *)&D.2651 + 24B]._vptr.D >> >> we aren't loading a whole D, we're only looking at the _vptr, which is >> within the bounds of E. > That raises a really interesting question. What's the right bounds to > use with these kinds of constructs. Should we be looking at D's type > (struct E) or struct D? I'm not sure there's a one-size-fits-all answer > here and I don't think we have any other context go help guide us. struct E, definitely; the actual bounds of the struct D object are unknown here. Unless, I suppose, we were to work harder to find the D FIELD_DECL in E that corresponds to this offset and get its DECL_SIZE, but: We know the bounds of the E object, and we can determine that _vptr.D is within those bounds; that seems sufficient to decide not to warn. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-08 22:44 ` Martin Sebor 2021-02-08 23:11 ` Jeff Law @ 2021-02-25 23:40 ` Jeff Law 2021-03-01 23:16 ` Martin Sebor 1 sibling, 1 reply; 24+ messages in thread From: Jeff Law @ 2021-02-25 23:40 UTC (permalink / raw) To: Martin Sebor, gcc-patches On 2/8/21 3:44 PM, Martin Sebor wrote: > > I think the underlying problem is the same. We're getting a size > that doesn't correspond to what's actually being accessed, and it > happens because of the virtual inheritance. In pr97595 Jason > suggested to use the decl/type size inequality to identify this > case but I think we could have just as well used DECL_ARTIFICIAL > instead. At least the test cases from pr97595 both pass with > this change. OK. I guess it's a minor question of semantics between pr97595 and pr98266 being the same. But I think we can put that behind us now. I think the big question is whether or not DECL_ARTIFICIAL is the right way to detect these cases, or if there simply isn't a way as one message from Jason seems to imply. Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-25 23:40 ` Jeff Law @ 2021-03-01 23:16 ` Martin Sebor 0 siblings, 0 replies; 24+ messages in thread From: Martin Sebor @ 2021-03-01 23:16 UTC (permalink / raw) To: Jeff Law, gcc-patches On 2/25/21 4:40 PM, Jeff Law wrote: > > > On 2/8/21 3:44 PM, Martin Sebor wrote: >> >> I think the underlying problem is the same. We're getting a size >> that doesn't correspond to what's actually being accessed, and it >> happens because of the virtual inheritance. In pr97595 Jason >> suggested to use the decl/type size inequality to identify this >> case but I think we could have just as well used DECL_ARTIFICIAL >> instead. At least the test cases from pr97595 both pass with >> this change. > OK. I guess it's a minor question of semantics between pr97595 and > pr98266 being the same. But I think we can put that behind us now. > > I think the big question is whether or not DECL_ARTIFICIAL is the right > way to detect these cases, or if there simply isn't a way as one message > from Jason seems to imply. DECL_ARTIFICIAL happens to work for the vtbl case but it doesn't work for other accesses to members of virtual bases that don't involve artificial members but where the offset is constant. I didn't think those came up but as Jason showed, they do: in accesses in destructors (and also in some in constructors). So the DECL_ARTIFICIAL test isn't the right way to detect all those cases. I have posted an updated patch that handles those cases as well as direct vtbl accesses by detecting the COMPONENT_REF (MEM_REF (&d), m)) access to member m of object d with virtual bases, that doesn't depend on DECL_ARTIFICIAL. Martin ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) 2021-02-08 21:56 ` Martin Sebor 2021-02-08 22:26 ` Jeff Law @ 2021-02-25 23:15 ` Jeff Law 1 sibling, 0 replies; 24+ messages in thread From: Jeff Law @ 2021-02-25 23:15 UTC (permalink / raw) To: Martin Sebor, gcc-patches On 2/8/21 2:56 PM, Martin Sebor wrote: > On 2/8/21 12:59 PM, Jeff Law wrote: >> >> >> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote: >>> Similar to the problem reported for -Wstringop-overflow in pr98266 >>> and already fixed, -Warray-bounds is also susceptible to false >>> positives in assignments and copies involving virtual inheritance. >>> Because the two warnings don't share code yet (hopefully in GCC 12) >>> the attached patch adds its own workaround for this problem to >>> gimple-array-bounds.cc, this one slightly more crude because of >>> the limited insight the array bounds checking has into the checked >>> expressions. >>> >>> Tested on x86_64-linux. >>> >>> Martin >>> >>> gcc-98266.diff >>> >>> PR middle-end/98266 - bogus array subscript is partly outside array >>> bounds on virtual inheritance >>> >>> gcc/ChangeLog: >>> >>> PR middle-end/98266 >>> * gimple-array-bounds.cc >>> (array_bounds_checker::check_array_bounds): >>> Avoid checking references involving artificial members. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR middle-end/98266 >>> * g++.dg/warn/Warray-bounds-15.C: New test. >> It seems to me that we've got the full statement at some point and thus >> the full expression so at some point couldn't we detect when >> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT? Or should we be using TYPE_SIZE_UNIT >> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc >> >> Am I missing something? > > The expression we're looking at when the false positive is issued > is the MEM_REF in the LHS of: > > MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void > *)&_ZTC1E24_1D + 24B]; > > TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and > TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between > DECL_SIZE and TYPE_SIZE. So it's a related, but not necessary exactly the same as pr97595 where we do have the discrepancy between DECL_SIZE and TYPE_SIZE. I think Jason's comment in pr97595 is the key here. For a base subject with virtual bases the offset is bounded by the most-derived object size -- which runs a bit counter to some of the concepts we rely on. So I think that raises two key questions. First, can we detect when we've got an access to a base subobject with virtual bases. I think that's what you're trying to do with the DECL_ARTIFICIAL check. I don't know offhand if that's the best way to detect these kinds of accesses or not. That's probably the most important question here, particularly for gcc-11. Second, if we can detect those kinds of accesses, do we have any way to get the most derived object size? If so, obviously we'd want to use it (gcc-12 perhaps), if not, we'd punt like your patch does. Jeff ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-03-04 20:00 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-20 0:56 [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266) Martin Sebor 2021-01-29 17:22 ` PING " Martin Sebor 2021-02-06 17:12 ` PING 2 " Martin Sebor 2021-02-08 19:59 ` Jeff Law 2021-02-08 21:56 ` Martin Sebor 2021-02-08 22:26 ` Jeff Law 2021-02-08 22:44 ` Martin Sebor 2021-02-08 23:11 ` Jeff Law 2021-02-09 17:49 ` Martin Sebor 2021-02-23 16:02 ` PING " Martin Sebor 2021-02-23 21:52 ` Jason Merrill 2021-02-23 23:07 ` Martin Sebor 2021-02-25 0:35 ` Jason Merrill 2021-03-01 23:11 ` Martin Sebor 2021-03-02 14:11 ` Jason Merrill 2021-03-03 23:20 ` Martin Sebor 2021-03-04 5:33 ` Jason Merrill 2021-03-04 17:44 ` Martin Sebor 2021-03-04 20:00 ` Jason Merrill 2021-02-25 23:47 ` Jeff Law 2021-02-26 3:56 ` Jason Merrill 2021-02-25 23:40 ` Jeff Law 2021-03-01 23:16 ` Martin Sebor 2021-02-25 23:15 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).