On Fri, Aug 2, 2019 at 10:24 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> 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.
Testing went OK but it looks like acats doesn't honor
RUNTESTFLAGS so I got no multilib testing for it :/
And the PR didn't contain sth I could plug into gnat.dg so I checked
with visual inspection of dumps on the reduced testcase.
I notice pretty-printing is also confused about the wrapping ;)
static struct p__intarray___PAD intarray = {.F={-100, [0]=0, 100}};
the [0]= is not necessary.
Anyway, I can see bogus IL before and still after the proposed patch :/
This is because I forgot to adjust cfield handling of setting index.
So I'm testing the adjusted patch attached which fixes the IL
and for testing have patched gcc/testsuite/ada/acats/run_all.sh
to add -m32.
Richard.
> Richard.
>
> >
> > --
> > Eric Botcazou
fix-pr91169
Description: Binary data
