On Thu, Aug 1, 2019 at 10:27 AM Eric Botcazou <ebotca...@adacore.com> wrote:
>
> Hi,
>
> this fixes the cd2a31a regression in the ACATS testsuite on 32-bit targets
> introduced by the recent change to get_array_ctor_element_at_index:
>
> * fold-const.h (get_array_ctor_element_at_index): Adjust.
> * fold-const.c (get_array_ctor_element_at_index): Add
> ctor_idx output parameter informing the caller where in
> the constructor the element was (not) found. Add early exit
> for when the ctor is sorted.
>
> This change overlooks that the index can wrap around during the traversal of
> the CONSTRUCTOR and therefore causes the function to return bogus values as
> soon as this happens. Moreover, given this chunk of added code:
So isn't this an issue with the code before when we have a RANGE_EXPR
that covers the wrapping point? Then index > max_index and may not
catch the found element with
/* Do we have match? */
if (wi::cmpu (access_index, index) >= 0
&& wi::cmpu (access_index, max_index) <= 0)
return cval;
? We seem to be careful to convert the indices to the index type but
above use unsigned compares - isn't that the underlying issue? So
isn't
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c (revision 273968)
+++ gcc/fold-const.c (working copy)
@@ -11864,9 +11864,13 @@ get_array_ctor_element_at_index (tree ct
}
}
+ signop index_sgn = UNSIGNED;
if (index_type)
- access_index = wi::ext (access_index, TYPE_PRECISION (index_type),
- TYPE_SIGN (index_type));
+ {
+ index_sgn = TYPE_SIGN (index_type);
+ access_index = wi::ext (access_index, TYPE_PRECISION (index_type),
+ TYPE_SIGN (index_type));
+ }
offset_int index = low_bound - 1;
if (index_type)
@@ -11903,9 +11907,9 @@ get_array_ctor_element_at_index (tree ct
}
/* Do we have match? */
- if (wi::cmpu (access_index, index) >= 0)
+ if (wi::cmp (access_index, index, index_sgn) >= 0)
{
- if (wi::cmpu (access_index, max_index) <= 0)
+ if (wi::cmp (access_index, max_index, index_sgn) <= 0)
{
if (ctor_idx)
*ctor_idx = cnt;
> else if (in_gimple_form)
> /* We're past the element we search for. Note during parsing
> the elements might not be sorted.
> ??? We should use a binary search and a flag on the
> CONSTRUCTOR as to whether elements are sorted in declaration
> order. */
> break;
>
> I would respectfully suggest that the author thinks about redoing things from
> scratch here. In the meantime, the attached patch kludges around the issue.
I'm not sure how "doing from scratch" would fix things - we simply rely
on reliably detecting an element match.
I wonder how much we can rely on the array domain type to be in sync
with the constructor field entries.
Also we're using offset_int here - doesn't that break when with
Ada we have an array type with domain [uint128_t_max - 1, uint128_t_max + 2]
in a type larger than int128_t? Or, since we're interpreting it
inconsistently signed/unsigned, a 128bit integer typed domain wraps
around the sign?
If that's not an issue then using signed compare unconditionally is
a valid fix as well I guess.
But in reality 'index' should never really wrap, that is, if the domain
is of 'int' type and starts with INT_MAX then the array shall not have
more than 1 element. No?
The patch you posted isn't a real fix btw, since iff the break was
hit we either missed an index that is present or we found a gap
but then we can break.
So I'd appreciate more explanation on how the index "wraps".
Richard.
> Tested on x86_64-suse-linux, OK for the mainline?
>
>
> 2019-08-01 Eric Botcazou <ebotca...@adacore.com>
>
> PR tree-optimization/91169
> * fold-const.c (get_array_ctor_element_at_index): Remove early exit
> and
> do not return a bogus ctor_idx when the index wraps around.
>
> --
> Eric Botcazou
