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 10:27 AM Eric Botcazou <ebotcazou@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  <ebotcazou@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


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