This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Fix tree-optimization/91169


On Thu, Aug 1, 2019 at 7:11 PM Eric Botcazou <ebotcazou@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

Attachment: fix-pr91169
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]