| Message ID | 9a52975c-c720-f42f-3564-7b69e361487d@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,tree-optimization/84047] missing -Warray-bounds on an out-of-bounds index | expand |
On 01/30/2018 03:11 PM, Aldy Hernandez wrote: > Hi! > > [Note: Jakub has mentioned that missing -Warray-bounds regressions > should be punted to GCC 9. I think this particular one is easy > pickings, but if this and/or the rest of the -Warray-bounds regressions > should be marked as GCC 9 material, please let me know so we can adjust > all relevant PRs.] Let me first say that I have no concern with adopting this patch for GCC 8 (and older) and deferring more comprehensive solutions for the remaining regressions until GCC 9. With that said, as we discussed last week in IRC, the patch I submitted for one of these regressions, bug 83776, also happens to fix one of the two test cases submitted for this bug: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg02144.html I put the patch together because it's a P2 bug and has a Target Milestone set to 6.5. I took that to mean that it's expected to be fixed in 8.1 in addition to 7.x and 6.x, but in light of Jakub's reservations and Ady's question above I'm not sure my understanding is correct, or if it is, that there necessarily is consensus to fix them in GCC 8. I also spent some time over the weekend extending my patch to handle the additional test case from comment #2 on bug 84047. That one is also due to the same underlying root cause. I'm close to done with it but I had to set it aside to handle a bug in my own code that was reported yesterday. I mention this because I too would find it helpful if it were made clear whether these regressions are expected to be fixed in GCC 8, so we can focus on the right bugs and avoid duplicating effort. If we want to fix all the reported -Warray-bounds regressions in GCC 8 then I'll finish my patch in progress and post it as a followup. Martin > > This is a -Warray-bounds regression that happens because the IL now has > an MEM_REF instead on ARRAY_REF. > > Previously we had an ARRAY_REF we could diagnose: > > D.2720_5 = "12345678"[1073741824]; > > But now this is represented as: > > _1 = MEM[(const char *)"12345678" + 1073741824B]; > > I think we can just allow check_array_bounds() to handle MEM_REF's and > everything should just work. > > The attached patch fixes both regressions mentioned in the PR. > > Tested on x86-64 Linux. > > OK?
On Tue, Jan 30, 2018 at 11:11 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > Hi! > > [Note: Jakub has mentioned that missing -Warray-bounds regressions should be > punted to GCC 9. I think this particular one is easy pickings, but if this > and/or the rest of the -Warray-bounds regressions should be marked as GCC 9 > material, please let me know so we can adjust all relevant PRs.] > > This is a -Warray-bounds regression that happens because the IL now has an > MEM_REF instead on ARRAY_REF. > > Previously we had an ARRAY_REF we could diagnose: > > D.2720_5 = "12345678"[1073741824]; > > But now this is represented as: > > _1 = MEM[(const char *)"12345678" + 1073741824B]; > > I think we can just allow check_array_bounds() to handle MEM_REF's and > everything should just work. > > The attached patch fixes both regressions mentioned in the PR. > > Tested on x86-64 Linux. > > OK? This doesn't look correct. You lump MEM_REF handling together with ADDR_EXPR handling but for the above case you want to diagnose _dereferences_ not address-taking. For the dereference case you need to amend the ARRAY_REF case, for example via Index: gcc/tree-vrp.c =================================================================== --- gcc/tree-vrp.c (revision 257181) +++ gcc/tree-vrp.c (working copy) @@ -5012,6 +5012,13 @@ check_array_bounds (tree *tp, int *walk_ if (TREE_CODE (t) == ARRAY_REF) vrp_prop->check_array_ref (location, t, false /*ignore_off_by_one*/); + else if (TREE_CODE (t) == MEM_REF + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR + && TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) == STRING_CST) + { + call factored part of check_array_ref passing in STRING_CST and offset + } + else if (TREE_CODE (t) == ADDR_EXPR) { vrp_prop->search_for_addr_array (t, location); note your patch will fail to warn for "1"[1] because taking that address is valid but not dereferencing it. Richard.
Since my patch isn't the easy one liner I wanted it to be, perhaps we should concentrate on Martin's patch, which is more robust, and has testcases to boot! His patch from last week also fixes a couple other PRs. Richard, would this be acceptable? That is, could you or Jakub review Martin's all-encompassing patch? If so, I'll drop mine. Also, could someone pontificate on whether we want to fix -Warray-bounds regressions for this release cycle? Thanks. On Wed, Jan 31, 2018 at 6:05 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Tue, Jan 30, 2018 at 11:11 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >> Hi! >> >> [Note: Jakub has mentioned that missing -Warray-bounds regressions should be >> punted to GCC 9. I think this particular one is easy pickings, but if this >> and/or the rest of the -Warray-bounds regressions should be marked as GCC 9 >> material, please let me know so we can adjust all relevant PRs.] >> >> This is a -Warray-bounds regression that happens because the IL now has an >> MEM_REF instead on ARRAY_REF. >> >> Previously we had an ARRAY_REF we could diagnose: >> >> D.2720_5 = "12345678"[1073741824]; >> >> But now this is represented as: >> >> _1 = MEM[(const char *)"12345678" + 1073741824B]; >> >> I think we can just allow check_array_bounds() to handle MEM_REF's and >> everything should just work. >> >> The attached patch fixes both regressions mentioned in the PR. >> >> Tested on x86-64 Linux. >> >> OK? > > This doesn't look correct. You lump MEM_REF handling together with > ADDR_EXPR handling but for the above case you want to diagnose > _dereferences_ not address-taking. > > For the dereference case you need to amend the ARRAY_REF case, for example > via > > Index: gcc/tree-vrp.c > =================================================================== > --- gcc/tree-vrp.c (revision 257181) > +++ gcc/tree-vrp.c (working copy) > @@ -5012,6 +5012,13 @@ check_array_bounds (tree *tp, int *walk_ > if (TREE_CODE (t) == ARRAY_REF) > vrp_prop->check_array_ref (location, t, false /*ignore_off_by_one*/); > > + else if (TREE_CODE (t) == MEM_REF > + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR > + && TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) == STRING_CST) > + { > + call factored part of check_array_ref passing in STRING_CST and offset > + } > + > else if (TREE_CODE (t) == ADDR_EXPR) > { > vrp_prop->search_for_addr_array (t, location); > > note your patch will fail to warn for "1"[1] because taking that > address is valid but not > dereferencing it. > > Richard.
On Thu, Feb 1, 2018 at 6:42 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > Since my patch isn't the easy one liner I wanted it to be, perhaps we > should concentrate on Martin's patch, which is more robust, and has > testcases to boot! His patch from last week also fixes a couple other > PRs. > > Richard, would this be acceptable? That is, could you or Jakub review > Martin's all-encompassing patch? If so, I'll drop mine. Sorry, no - this one looks way too complicated. > Also, could someone pontificate on whether we want to fix > -Warray-bounds regressions for this release cycle? Remove bogus ones? Yes. Add "missing ones"? No. Richard. > Thanks. > > On Wed, Jan 31, 2018 at 6:05 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Tue, Jan 30, 2018 at 11:11 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >>> Hi! >>> >>> [Note: Jakub has mentioned that missing -Warray-bounds regressions should be >>> punted to GCC 9. I think this particular one is easy pickings, but if this >>> and/or the rest of the -Warray-bounds regressions should be marked as GCC 9 >>> material, please let me know so we can adjust all relevant PRs.] >>> >>> This is a -Warray-bounds regression that happens because the IL now has an >>> MEM_REF instead on ARRAY_REF. >>> >>> Previously we had an ARRAY_REF we could diagnose: >>> >>> D.2720_5 = "12345678"[1073741824]; >>> >>> But now this is represented as: >>> >>> _1 = MEM[(const char *)"12345678" + 1073741824B]; >>> >>> I think we can just allow check_array_bounds() to handle MEM_REF's and >>> everything should just work. >>> >>> The attached patch fixes both regressions mentioned in the PR. >>> >>> Tested on x86-64 Linux. >>> >>> OK? >> >> This doesn't look correct. You lump MEM_REF handling together with >> ADDR_EXPR handling but for the above case you want to diagnose >> _dereferences_ not address-taking. >> >> For the dereference case you need to amend the ARRAY_REF case, for example >> via >> >> Index: gcc/tree-vrp.c >> =================================================================== >> --- gcc/tree-vrp.c (revision 257181) >> +++ gcc/tree-vrp.c (working copy) >> @@ -5012,6 +5012,13 @@ check_array_bounds (tree *tp, int *walk_ >> if (TREE_CODE (t) == ARRAY_REF) >> vrp_prop->check_array_ref (location, t, false /*ignore_off_by_one*/); >> >> + else if (TREE_CODE (t) == MEM_REF >> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR >> + && TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) == STRING_CST) >> + { >> + call factored part of check_array_ref passing in STRING_CST and offset >> + } >> + >> else if (TREE_CODE (t) == ADDR_EXPR) >> { >> vrp_prop->search_for_addr_array (t, location); >> >> note your patch will fail to warn for "1"[1] because taking that >> address is valid but not >> dereferencing it. >> >> Richard.
On 02/08/2018 03:38 AM, Richard Biener wrote: > On Thu, Feb 1, 2018 at 6:42 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >> Since my patch isn't the easy one liner I wanted it to be, perhaps we >> should concentrate on Martin's patch, which is more robust, and has >> testcases to boot! His patch from last week also fixes a couple other >> PRs. >> >> Richard, would this be acceptable? That is, could you or Jakub review >> Martin's all-encompassing patch? If so, I'll drop mine. > > Sorry, no - this one looks way too complicated. Presumably the complication is in he loop that follows SSA_NAMEs and the offsets in: const char *s0 = "12"; const char *s1 = s0 + 1; const char *s2 = s1 + 1; const char *s3 = s2 + 1; int i = *s0 + *s1 + *s2 + *s3; ? I don't know if this used to be diagnosed and is also part of the regression. If it isn't it could be removed for GCC 8 and then added for GCC 9. If this isn't your concern can you be more specific about what is? I should note (again) that this patch doesn't fix the whole regression. There are remaining cases (involving arrays) that used to be handled but no longer are. It's tedious (and hacky) to limit the fix to just the subset of the regression while at the same time preserving the pre-existing limitations (or bugs, depending on one's point of view). >> Also, could someone pontificate on whether we want to fix >> -Warray-bounds regressions for this release cycle? > > Remove bogus ones? Yes. Add "missing ones"? No. Can you please explain how to interpret the Target Milestone then? Why is it set it to 6.5 when the bug is not meant to be fixed? If it's meant to be fixed in 6.5 (and presumably also 7.4) but not in 8.1, do we expect to fix it in 8.2? More to the point, how can we tell which it is? Thanks Martin > > Richard. > >> Thanks. >> >> On Wed, Jan 31, 2018 at 6:05 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Tue, Jan 30, 2018 at 11:11 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >>>> Hi! >>>> >>>> [Note: Jakub has mentioned that missing -Warray-bounds regressions should be >>>> punted to GCC 9. I think this particular one is easy pickings, but if this >>>> and/or the rest of the -Warray-bounds regressions should be marked as GCC 9 >>>> material, please let me know so we can adjust all relevant PRs.] >>>> >>>> This is a -Warray-bounds regression that happens because the IL now has an >>>> MEM_REF instead on ARRAY_REF. >>>> >>>> Previously we had an ARRAY_REF we could diagnose: >>>> >>>> D.2720_5 = "12345678"[1073741824]; >>>> >>>> But now this is represented as: >>>> >>>> _1 = MEM[(const char *)"12345678" + 1073741824B]; >>>> >>>> I think we can just allow check_array_bounds() to handle MEM_REF's and >>>> everything should just work. >>>> >>>> The attached patch fixes both regressions mentioned in the PR. >>>> >>>> Tested on x86-64 Linux. >>>> >>>> OK? >>> >>> This doesn't look correct. You lump MEM_REF handling together with >>> ADDR_EXPR handling but for the above case you want to diagnose >>> _dereferences_ not address-taking. >>> >>> For the dereference case you need to amend the ARRAY_REF case, for example >>> via >>> >>> Index: gcc/tree-vrp.c >>> =================================================================== >>> --- gcc/tree-vrp.c (revision 257181) >>> +++ gcc/tree-vrp.c (working copy) >>> @@ -5012,6 +5012,13 @@ check_array_bounds (tree *tp, int *walk_ >>> if (TREE_CODE (t) == ARRAY_REF) >>> vrp_prop->check_array_ref (location, t, false /*ignore_off_by_one*/); >>> >>> + else if (TREE_CODE (t) == MEM_REF >>> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR >>> + && TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) == STRING_CST) >>> + { >>> + call factored part of check_array_ref passing in STRING_CST and offset >>> + } >>> + >>> else if (TREE_CODE (t) == ADDR_EXPR) >>> { >>> vrp_prop->search_for_addr_array (t, location); >>> >>> note your patch will fail to warn for "1"[1] because taking that >>> address is valid but not >>> dereferencing it. >>> >>> Richard.
On 02/08/2018 03:38 AM, Richard Biener wrote: > On Thu, Feb 1, 2018 at 6:42 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >> Since my patch isn't the easy one liner I wanted it to be, perhaps we >> should concentrate on Martin's patch, which is more robust, and has >> testcases to boot! His patch from last week also fixes a couple other >> PRs. >> >> Richard, would this be acceptable? That is, could you or Jakub review >> Martin's all-encompassing patch? If so, I'll drop mine. > > Sorry, no - this one looks way too complicated. > >> Also, could someone pontificate on whether we want to fix >> -Warray-bounds regressions for this release cycle? > > Remove bogus ones? Yes. Add "missing ones"? No. Seems reasonable. I'll retarget the missed warning stuff for gcc-9 and we'll consider those out-of-scope for gcc-8. Still in scope would be bogus warnings. Jeff
On Thu, Feb 8, 2018 at 9:45 PM, Martin Sebor <msebor@gmail.com> wrote: > On 02/08/2018 03:38 AM, Richard Biener wrote: >> >> On Thu, Feb 1, 2018 at 6:42 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >>> >>> Since my patch isn't the easy one liner I wanted it to be, perhaps we >>> should concentrate on Martin's patch, which is more robust, and has >>> testcases to boot! His patch from last week also fixes a couple other >>> PRs. >>> >>> Richard, would this be acceptable? That is, could you or Jakub review >>> Martin's all-encompassing patch? If so, I'll drop mine. >> >> >> Sorry, no - this one looks way too complicated. > > > Presumably the complication is in he loop that follows SSA_NAMEs > and the offsets in: > > const char *s0 = "12"; > const char *s1 = s0 + 1; > const char *s2 = s1 + 1; > const char *s3 = s2 + 1; > > int i = *s0 + *s1 + *s2 + *s3; > > ? > > I don't know if this used to be diagnosed and is also part of > the regression. If it isn't it could be removed for GCC 8 and > then added for GCC 9. If this isn't your concern can you be > more specific about what is? > > I should note (again) that this patch doesn't fix the whole > regression. There are remaining cases (involving arrays) that > used to be handled but no longer are. It's tedious (and hacky) > to limit the fix to just the subset of the regression while at > the same time preserving the pre-existing limitations (or bugs, > depending on one's point of view). > >>> Also, could someone pontificate on whether we want to fix >>> -Warray-bounds regressions for this release cycle? >> >> >> Remove bogus ones? Yes. Add "missing ones"? No. > > > Can you please explain how to interpret the Target Milestone > then? Why is it set it to 6.5 when the bug is not meant to > be fixed? > > If it's meant to be fixed in 6.5 (and presumably also 7.4) but > not in 8.1, do we expect to fix it in 8.2? More to the point, > how can we tell which it is? The target milestone is set by formal criteria. We have leeway with the priority and only P1 bugs must be fixed before the release. What release we fix something ultimatively ends up depending on how the patch looks like. And most bugs are about multiple issues so having "one" target milestone or even priority is difficult if it is supposed to be a hard one. Richard. > Thanks > Martin > > >> >> Richard. >> >>> Thanks. >>> >>> On Wed, Jan 31, 2018 at 6:05 AM, Richard Biener >>> <richard.guenther@gmail.com> wrote: >>>> >>>> On Tue, Jan 30, 2018 at 11:11 PM, Aldy Hernandez <aldyh@redhat.com> >>>> wrote: >>>>> >>>>> Hi! >>>>> >>>>> [Note: Jakub has mentioned that missing -Warray-bounds regressions >>>>> should be >>>>> punted to GCC 9. I think this particular one is easy pickings, but if >>>>> this >>>>> and/or the rest of the -Warray-bounds regressions should be marked as >>>>> GCC 9 >>>>> material, please let me know so we can adjust all relevant PRs.] >>>>> >>>>> This is a -Warray-bounds regression that happens because the IL now has >>>>> an >>>>> MEM_REF instead on ARRAY_REF. >>>>> >>>>> Previously we had an ARRAY_REF we could diagnose: >>>>> >>>>> D.2720_5 = "12345678"[1073741824]; >>>>> >>>>> But now this is represented as: >>>>> >>>>> _1 = MEM[(const char *)"12345678" + 1073741824B]; >>>>> >>>>> I think we can just allow check_array_bounds() to handle MEM_REF's and >>>>> everything should just work. >>>>> >>>>> The attached patch fixes both regressions mentioned in the PR. >>>>> >>>>> Tested on x86-64 Linux. >>>>> >>>>> OK? >>>> >>>> >>>> This doesn't look correct. You lump MEM_REF handling together with >>>> ADDR_EXPR handling but for the above case you want to diagnose >>>> _dereferences_ not address-taking. >>>> >>>> For the dereference case you need to amend the ARRAY_REF case, for >>>> example >>>> via >>>> >>>> Index: gcc/tree-vrp.c >>>> =================================================================== >>>> --- gcc/tree-vrp.c (revision 257181) >>>> +++ gcc/tree-vrp.c (working copy) >>>> @@ -5012,6 +5012,13 @@ check_array_bounds (tree *tp, int *walk_ >>>> if (TREE_CODE (t) == ARRAY_REF) >>>> vrp_prop->check_array_ref (location, t, false >>>> /*ignore_off_by_one*/); >>>> >>>> + else if (TREE_CODE (t) == MEM_REF >>>> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR >>>> + && TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) == >>>> STRING_CST) >>>> + { >>>> + call factored part of check_array_ref passing in STRING_CST and >>>> offset >>>> + } >>>> + >>>> else if (TREE_CODE (t) == ADDR_EXPR) >>>> { >>>> vrp_prop->search_for_addr_array (t, location); >>>> >>>> note your patch will fail to warn for "1"[1] because taking that >>>> address is valid but not >>>> dereferencing it. >>>> >>>> Richard. > >
gcc/ PR tree-optimization/84047 * tree-vrp.c (search_for_addr_array): Handle ADDR_EXPRs. (check_array_bounds): Same. diff --git a/gcc/testsuite/gcc.dg/pr84047.c b/gcc/testsuite/gcc.dg/pr84047.c new file mode 100644 index 00000000000..9f9c6ca4a9e --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr84047.c @@ -0,0 +1,18 @@ +/* { dg-compile } */ +/* { dg-options "-O2 -Warray-bounds" } */ + +int f (void) +{ + const char *s = "12345678"; + int i = 1U << 30; + return s[i]; /* { dg-warning "array bounds" } */ +} + +char a[8]; + +int g (void) +{ + const char *s = a + 4; + int i = 8; + return s[i]; /* { dg-warning "array bounds" } */ +} diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index 3af81f70872..07d39bab8a4 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -4922,14 +4922,15 @@ void vrp_prop::search_for_addr_array (tree t, location_t location) { /* Check each ARRAY_REFs in the reference chain. */ - do - { - if (TREE_CODE (t) == ARRAY_REF) - check_array_ref (location, t, true /*ignore_off_by_one*/); + if (TREE_CODE (t) == ADDR_EXPR) + do + { + if (TREE_CODE (t) == ARRAY_REF) + check_array_ref (location, t, true /*ignore_off_by_one*/); - t = TREE_OPERAND (t, 0); - } - while (handled_component_p (t)); + t = TREE_OPERAND (t, 0); + } + while (handled_component_p (t)); if (TREE_CODE (t) == MEM_REF && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR @@ -5012,7 +5013,9 @@ check_array_bounds (tree *tp, int *walk_subtree, void *data) if (TREE_CODE (t) == ARRAY_REF) vrp_prop->check_array_ref (location, t, false /*ignore_off_by_one*/); - else if (TREE_CODE (t) == ADDR_EXPR) + else if (TREE_CODE (t) == ADDR_EXPR + /* Handle (MEM_REF (ADDR_EXPR STRING_CST) INTEGER_CST). */ + || TREE_CODE (t) == MEM_REF) { vrp_prop->search_for_addr_array (t, location); *walk_subtree = FALSE;