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?
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.
