diff mbox series

Fix internal error on small array with negative lower bound

Message ID 4801877.GXAFRqVoOG@fomalhaut
State Accepted
Headers
Series Fix internal error on small array with negative lower bound |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Eric Botcazou May 18, 2023, 9:28 a.m. UTC 
  Hi,

Ada supports arrays with negative indices, although the internal index type is
sizetype like in other languages, which is unsigned.  This means that negative
values are represented by very large numbers, which works with a bit of care.
The attached test exposes a small loophole in output_constructor_bitfield.

Tested on x86-64/Linux, OK for the mainline?


2023-05-18  Eric Botcazou <ebotcazou@adacore.com>

	* varasm.cc (output_constructor_bitfield): Call tree_to_uhwi instead
	of tree_to_shwi on array indices.  Minor tweaks.


2023-05-18  Eric Botcazou <ebotcazou@adacore.com>

	* gnat.dg/specs/array6.ads: New test.

Comments

Richard Biener May 18, 2023, 12:02 p.m. UTC | #1 
On Thu, May 18, 2023 at 11:51 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> Ada supports arrays with negative indices, although the internal index type is
> sizetype like in other languages, which is unsigned.  This means that negative
> values are represented by very large numbers, which works with a bit of care.
> The attached test exposes a small loophole in output_constructor_bitfield.
>
> Tested on x86-64/Linux, OK for the mainline?

Would it be better to use

  wi::to_uhwi (wi::to_wide (local->index) - wi::to_wide (local->min_index))

to honor the actual sign of the indices?  I think nothing forbids frontends to
use a signed TYPE_DOMAIN here?  But the difference should be always
representable in an unsigned value of course.

>
> 2023-05-18  Eric Botcazou <ebotcazou@adacore.com>
>
>         * varasm.cc (output_constructor_bitfield): Call tree_to_uhwi instead
>         of tree_to_shwi on array indices.  Minor tweaks.
>
>
> 2023-05-18  Eric Botcazou <ebotcazou@adacore.com>
>
>         * gnat.dg/specs/array6.ads: New test.
>
> --
> Eric Botcazou
Eric Botcazou May 18, 2023, 5:44 p.m. UTC | #2 
> Would it be better to use
> 
>   wi::to_uhwi (wi::to_wide (local->index) - wi::to_wide (local->min_index))
> 
> to honor the actual sign of the indices?  I think nothing forbids frontends
> to use a signed TYPE_DOMAIN here?  But the difference should be always
> representable in an unsigned value of course.

We use tree_to_uhwi everywhere else though, see categorize_ctor_elements_1:

	  if (tree_fits_uhwi_p (lo_index) && tree_fits_uhwi_p (hi_index))
	    mult = (tree_to_uhwi (hi_index)
		    - tree_to_uhwi (lo_index) + 1);

or store_constructor

		    this_node_count = (tree_to_uhwi (hi_index)
			       - tree_to_uhwi (lo_index) + 1);

so the proposed form looks better for the sake of consistency.
Richard Biener May 18, 2023, 6:24 p.m. UTC | #3 
> Am 18.05.2023 um 19:44 schrieb Eric Botcazou <botcazou@adacore.com>:
> 
> 
>> 
>> Would it be better to use
>> 
>>  wi::to_uhwi (wi::to_wide (local->index) - wi::to_wide (local->min_index))
>> 
>> to honor the actual sign of the indices?  I think nothing forbids frontends
>> to use a signed TYPE_DOMAIN here?  But the difference should be always
>> representable in an unsigned value of course.
> 
> We use tree_to_uhwi everywhere else though, see categorize_ctor_elements_1:
> 
>      if (tree_fits_uhwi_p (lo_index) && tree_fits_uhwi_p (hi_index))
>        mult = (tree_to_uhwi (hi_index)
>            - tree_to_uhwi (lo_index) + 1);
> 
> or store_constructor
> 
>            this_node_count = (tree_to_uhwi (hi_index)
>                   - tree_to_uhwi (lo_index) + 1);
> 
> so the proposed form looks better for the sake of consistency.

Ok, thanks for checking.

Richard 

> -- 
> Eric Botcazou
> 
>
diff mbox series

Patch

diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index 2256194d934..478cbfe6736 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -5585,19 +5585,18 @@  output_constructor_bitfield (oc_local_state *local, unsigned int bit_offset)
 
   /* Relative index of this element if this is an array component.  */
   HOST_WIDE_INT relative_index
-    = (!local->field
-       ? (local->index
-	  ? (tree_to_shwi (local->index)
-	     - tree_to_shwi (local->min_index))
-	  : local->last_relative_index + 1)
-       : 0);
+    = (local->field
+       ? 0
+       : (local->index
+	  ? tree_to_uhwi (local->index) - tree_to_uhwi (local->min_index)
+	  : local->last_relative_index + 1));
 
   /* Bit position of this element from the start of the containing
      constructor.  */
   HOST_WIDE_INT constructor_relative_ebitpos
-      = (local->field
-	 ? int_bit_position (local->field)
-	 : ebitsize * relative_index);
+    = (local->field
+       ? int_bit_position (local->field)
+       : ebitsize * relative_index);
 
   /* Bit position of this element from the start of a possibly ongoing
      outer byte buffer.  */