This is the mail archive of the
mailing list for the GCC project.
Re: [patch] Fix tree-optimization/91169
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Eric Botcazou <ebotcazou at adacore dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 1 Aug 2019 11:20:45 +0200
- Subject: Re: [patch] Fix tree-optimization/91169
- References: <1764855.0oU1eeNi03@arcturus.home>
On Thu, Aug 1, 2019 at 10:27 AM Eric Botcazou <email@example.com> wrote:
> 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)
? 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
--- 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;
- 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;
@@ -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)
*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. */
> 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".
> Tested on x86_64-suse-linux, OK for the mainline?
> 2019-08-01 Eric Botcazou <firstname.lastname@example.org>
> 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