On Thu, Aug 1, 2019 at 7:11 PM Eric Botcazou <ebotca...@adacore.com> wrote: > > > So isn't this an issue with the code before when we have a RANGE_EXPR > > that covers the wrapping point? > > No, see the reduced testcase attached in the PR. > > > 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? > > Not really, everything is properly unsigned at this point, so the traversal > wraps around in an unsigned type. > > > 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. > > At least we need to take into account the wraparound. > > > 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? > > Integer types are limited to 64 bits in GNAT. > > > If that's not an issue then using signed compare unconditionally is > > a valid fix as well I guess. > > But all the types are unsigned here so this doesn't seem appealing. > > > 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 range is [-1, 1] converted to sizetype, which is unsigned, so the low > bound is UINT32_MAX and the high bound is 1.
Oh. OK, now I see. Still, if we have this domain and a CONSTRUCTOR
with a RANGE_EXPR UINT32_MAX, 1 and we are asking for access_index
zero then the old code did
/* Do we have match? */
if (wi::cmpu (access_index, index) >= 0
&& wi::cmpu (access_index, max_index) <= 0)
return cval;
and index == UINT32_MAX, max_index == 1. So we don't match it
but return NULL, assuming the constructor value is zero?
Not sure if the Ada FE ever creates RANGE_EXPRs for CONSTRUCTOR
entries, for single-valued entries the check of course works. But as it
was written above I assumed certain ordering conditions must hold...
(grepping shows no traces of RANGE_EXPR in gigi)
I'm hoping for a GCC C extension to specify array domains so the
GIMPLE FE can be used to play with these kind of things...
> > 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.
>
> No, we cannot break if the index wraps around, that's the bug.
>
> > So I'd appreciate more explanation on how the index "wraps".
>
> See above. I can rewind the entire chain of events if need be, but this is a
> long one starting a long time ago with the initial blunder with PLUS_EXPR and
> sizetype.
Yeah. Note that you can very well use signed TYPE_DOMAIN nowadays
(OK, no guarantees...).
I think we should kludge around similar to how we do in stor-layout.c
which does
/* ??? When it is obvious that the range is signed
represent it using ssizetype. */
if (TREE_CODE (lb) == INTEGER_CST
&& TREE_CODE (ub) == INTEGER_CST
&& TYPE_UNSIGNED (TREE_TYPE (lb))
&& tree_int_cst_lt (ub, lb))
{
lb = wide_int_to_tree (ssizetype,
offset_int::from (wi::to_wide (lb),
SIGNED));
ub = wide_int_to_tree (ssizetype,
offset_int::from (wi::to_wide (ub),
SIGNED));
}
So I am testing the attached.
Richard.
>
> --
> Eric Botcazou
fix-pr91169
Description: Binary data
