On Thu, Feb 8, 2018 at 9:45 PM, Martin Sebor <mse...@gmail.com> wrote:
> On 02/08/2018 03:38 AM, Richard Biener wrote:
>>
>> On Thu, Feb 1, 2018 at 6:42 PM, Aldy Hernandez <al...@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.guent...@gmail.com> wrote:
>>>>
>>>> On Tue, Jan 30, 2018 at 11:11 PM, Aldy Hernandez <al...@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.
>
>
