Ada: fix layout of packed arrays

Ada: fix layout of packed arrays

[Arnaud Charlet]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

The problem pertains to the layout of packed arrays inside records on
big-endian targets and is twofold:
- when -fstrict-aliasing is specified, and the component is not aliased
  and does not have a representation clause, and the record is not packed,
  the packed array ends up being right-justified in the field, thus violating
  the documented layout of packed arrays,
- when the component has a representation clause that prescribes too great
  a size for the packed array, the packed array ends up being right-justified
  in the field too.
This is fixed by not removing the wrapper of the JM type in these cases.
The testcase p.adb uses UC constructs to check that the packed arrays are
still left-justified in both cases.

procedure p is
  type T is array (1..4) of Boolean; pragma Pack(T);

  type A is record field : T; end record;
  type B is record field : T; end record;
  for B use record field at 0 range 0..5; end record;

  function A_To_Byte is new Ada.Unchecked_Conversion (A, Unsigned_8);
  function B_To_Byte is new Ada.Unchecked_Conversion (B, Unsigned_8);

  procedure TtoA (qt : in T; qa : out A) is begin qa.field := qt; end;

  procedure TtoB (qt : in T; qb : out B) is begin qb.field := qt; end;

  my_t : T := (true, true, true, false);
  my_a : A;
  my_b : B;

begin
  TtoA (my_t, my_a);
  Put ("my_a =" & Unsigned_8'Image(Shift_Right(A_To_Byte(my_a), 4)));
  New_Line;
  TtoB (my_t, my_b);
  Put ("my_b =" & Unsigned_8'Image(Shift_Right(B_To_Byte(my_b), 4)));
end;

$ gnatmake -O2 p

2004-10-26  Eric Botcazou  <ebotcazou@act-europe.fr>

	* decl.c (gnat_to_gnu_field): Use the type of the inner object for a
	JM type only if its size matches that of the wrapper.  When a size is
	prescribed and the field is not aliased, remove the wrapper of a JM
	type only if the size is not greater than that of the packed array.
	(gnat_to_gnu_entity): Change the extension of packed array wrappers
	from LJM to JM.


[-- Attachment #2: difs.10 --]
[-- Type: text/plain, Size: 3531 bytes --]

Index: decl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ada/decl.c,v
retrieving revision 1.59
diff -u -p -r1.59 decl.c
--- decl.c	4 Oct 2004 14:56:03 -0000	1.59
+++ decl.c	27 Oct 2004 09:32:43 -0000
@@ -521,7 +521,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 
 	/* If an alignment is specified, use it if valid.   Note that
 	   exceptions are objects but don't have alignments.  We must do this
-	   before we validate the size, since the alignment can affect the 
+	   before we validate the size, since the alignment can affect the
 	   size.  */
 	if (kind != E_Exception && Known_Alignment (gnat_entity))
 	  {
@@ -1342,7 +1342,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 	  TYPE_RM_SIZE_NUM (gnu_field_type)
 	    = UI_To_gnu (RM_Size (gnat_entity), bitsizetype);
 	  gnu_type = make_node (RECORD_TYPE);
-	  TYPE_NAME (gnu_type) = create_concat_name (gnat_entity, "LJM");
+	  TYPE_NAME (gnu_type) = create_concat_name (gnat_entity, "JM");
 	  TYPE_ALIGN (gnu_type) = TYPE_ALIGN (gnu_field_type);
 	  TYPE_PACKED (gnu_type) = 1;
 
@@ -5019,15 +5019,15 @@ gnat_to_gnu_field (Entity_Id gnat_field,
     gnu_size = validate_size (Esize (gnat_field), gnu_field_type,
 			      gnat_field, FIELD_DECL, false, true);
 
-  /* If the field's type is justified modular, the wrapper can prevent
-     packing so we make the field the type of the inner object unless the
-     situation forbids it. We may not do that when the field is addressable_p,
-     typically because in that case this field may later be passed by-ref for
-     a formal argument expecting the justification.  The condition below
-     is then matching the addressable_p code for COMPONENT_REF.  */
-  if (!Is_Aliased (gnat_field) && flag_strict_aliasing
-      && TREE_CODE (gnu_field_type) == RECORD_TYPE
-      && TYPE_JUSTIFIED_MODULAR_P (gnu_field_type))
+  /* If the field's type is justified modular and the size of the packed
+     array it wraps is the same as that of the field, we can make the field
+     the type of the inner object.  Note that we may need to do so if the
+     record is packed or the field has a component clause, but these cases
+     are handled later.  */
+  if (TREE_CODE (gnu_field_type) == RECORD_TYPE
+      && TYPE_JUSTIFIED_MODULAR_P (gnu_field_type)
+      && tree_int_cst_equal (TYPE_SIZE (gnu_field_type),
+			     TYPE_ADA_SIZE (gnu_field_type)))
     gnu_field_type = TREE_TYPE (TYPE_FIELDS (gnu_field_type));
 
   /* If we are packing this record, have a specified size that's smaller than
@@ -5173,12 +5173,16 @@ gnat_to_gnu_field (Entity_Id gnat_field,
     gnu_pos = NULL_TREE;
   else
     {
-      /* Unless this field is aliased, we can remove any justified
-	 modular type since it's only needed in the unchecked conversion
-	 case, which doesn't apply here.  */
+      /* If the field's type is justified modular, we would need to remove
+	 the wrapper to (better) meet the layout requirements.  However we
+	 can do so only if the field is not aliased to preserve the unique
+	 layout and if the prescribed size is not greater than that of the
+	 packed array to preserve the justification.  */
       if (!needs_strict_alignment
 	  && TREE_CODE (gnu_field_type) == RECORD_TYPE
-	  && TYPE_JUSTIFIED_MODULAR_P (gnu_field_type))
+	  && TYPE_JUSTIFIED_MODULAR_P (gnu_field_type)
+	  && tree_int_cst_compare (gnu_size, TYPE_ADA_SIZE (gnu_field_type))
+	       <= 0)
 	gnu_field_type = TREE_TYPE (TYPE_FIELDS (gnu_field_type));
 
       gnu_field_type

Re: Ada: fix layout of packed arrays

[Arnaud Charlet]

Tested on x86-linux and committed on mainline.

> The problem pertains to the layout of packed arrays inside records on
> big-endian targets and is twofold:
> [...]

Re: Ada: fix layout of packed arrays

[James A. Morrison]

Arnaud Charlet <charlet@ACT-Europe.FR> writes:

> procedure p is
>   type T is array (1..4) of Boolean; pragma Pack(T);
> 
>   type A is record field : T; end record;
>   type B is record field : T; end record;
>   for B use record field at 0 range 0..5; end record;
> 
>   function A_To_Byte is new Ada.Unchecked_Conversion (A, Unsigned_8);
>   function B_To_Byte is new Ada.Unchecked_Conversion (B, Unsigned_8);
> 
>   procedure TtoA (qt : in T; qa : out A) is begin qa.field := qt; end;
> 
>   procedure TtoB (qt : in T; qb : out B) is begin qb.field := qt; end;
> 
>   my_t : T := (true, true, true, false);
>   my_a : A;
>   my_b : B;
> 
> begin
>   TtoA (my_t, my_a);
>   Put ("my_a =" & Unsigned_8'Image(Shift_Right(A_To_Byte(my_a), 4)));
>   New_Line;
>   TtoB (my_t, my_b);
>   Put ("my_b =" & Unsigned_8'Image(Shift_Right(B_To_Byte(my_b), 4)));
> end;
> 
> $ gnatmake -O2 p

 To get this to compile I've had to add:

with Interfaces;
use interfaces;
with Ada.Text_IO;
use Ada.Text_IO;

 This seems to get rid of most of the problems.  It doesn't define anything
for A_To_Byte or B_To_Byte.  Where are these supposed to be defined?

 I'm using gnat 3.3 .



-- 
Thanks, Jim

http://www.student.cs.uwaterloo.ca/~ja2morri/
http://phython.blogspot.com
http://open.nit.ca/wiki/?page=jim

Re: Ada: fix layout of packed arrays

[Florian Weimer]

* James A. Morrison:

>  To get this to compile I've had to add:
>
> with Interfaces;
> use interfaces;
> with Ada.Text_IO;
> use Ada.Text_IO;
>
>  This seems to get rid of most of the problems.  It doesn't define anything
> for A_To_Byte or B_To_Byte.

Add "with Ada.Unchecked_Conversion".

> Where are these supposed to be defined?

These are generic instantiations:

>   function A_To_Byte is new Ada.Unchecked_Conversion (A, Unsigned_8);
>   function B_To_Byte is new Ada.Unchecked_Conversion (B, Unsigned_8);

Re: Ada: fix layout of packed arrays

[James A. Morrison]

Florian Weimer <fw@deneb.enyo.de> writes:

> * James A. Morrison:
> 
> >  To get this to compile I've had to add:
> >
> > with Interfaces;
> > use interfaces;
> > with Ada.Text_IO;
> > use Ada.Text_IO;
> >
> >  This seems to get rid of most of the problems.  It doesn't define anything
> > for A_To_Byte or B_To_Byte.
> 
> Add "with Ada.Unchecked_Conversion".
> 
> > Where are these supposed to be defined?
> 
> These are generic instantiations:
> 
> >   function A_To_Byte is new Ada.Unchecked_Conversion (A, Unsigned_8);
> >   function B_To_Byte is new Ada.Unchecked_Conversion (B, Unsigned_8);
> 

 That did it.

-- 
Thanks,
Jim

http://www.student.cs.uwaterloo.ca/~ja2morri/
http://phython.blogspot.com
http://open.nit.ca/wiki/?page=jim

Re: Ada: fix layout of packed arrays

[Eric Botcazou]

>  To get this to compile I've had to add:
>
> with Interfaces;
> use interfaces;
> with Ada.Text_IO;
> use Ada.Text_IO;

Sure, the original testcase starts with:

with Ada.Unchecked_Conversion;
with Interfaces; use Interfaces;
with Ada.Text_IO; use Ada.Text_IO;

Note that the testcase is only meant to be run on big-endian targets, it 
produces garbage on little-endian targets because packed arrays are 
right-justified there.

-- 
Eric Botcazou

Re: Ada: fix layout of packed arrays

[James A. Morrison]

Eric Botcazou <ebotcazou@act-europe.fr> writes:

> Note that the testcase is only meant to be run on big-endian targets, it 
> produces garbage on little-endian targets because packed arrays are 
> right-justified there.
> 
> -- 
> Eric Botcazou

 Is there an easy way to do that in ADA?  Both Shift_Right(A_To_Byte(my_a), 4)
and  Shift_Right(B_To_Byte(my_b), 4) should be 3, right? 


-- 
Thanks,
Jim

http://www.student.cs.uwaterloo.ca/~ja2morri/
http://phython.blogspot.com
http://open.nit.ca/wiki/?page=jim

Re: Ada: fix layout of packed arrays

[Eric Botcazou]

>  Is there an easy way to do that in ADA?  Both Shift_Right(A_To_Byte(my_a),
> 4) and  Shift_Right(B_To_Byte(my_b), 4) should be 3, right?

To do what exactly?  The correct output is 14 IIRC.

-- 
Eric Botcazou

Re: Ada: fix layout of packed arrays

[James A. Morrison]

Eric Botcazou <ebotcazou@act-europe.fr> writes:

> >  Is there an easy way to do that in ADA?  Both Shift_Right(A_To_Byte(my_a),
> > 4) and  Shift_Right(B_To_Byte(my_b), 4) should be 3, right?
> 
> To do what exactly?

 Test to see if a system is big endian or not.

>  The correct output is 14 IIRC.

 That's useful to know.

> -- 
> Eric Botcazou
> 


-- 
Thanks,
Jim

http://www.student.cs.uwaterloo.ca/~ja2morri/
http://phython.blogspot.com
http://open.nit.ca/wiki/?page=jim

Re: Ada: fix layout of packed arrays

[Eric Botcazou]

> > To do what exactly?
>
>  Test to see if a system is big endian or not.

Oh!  The testsuite harness takes care of that.

-- 
Eric Botcazou

Re: Ada: fix layout of packed arrays

[James A. Morrison]

Eric Botcazou <ebotcazou@act-europe.fr> writes:

> > > To do what exactly?
> >
> >  Test to see if a system is big endian or not.
> 
> Oh!  The testsuite harness takes care of that.
> 
> -- 
> Eric Botcazou


 Which testsuite harness?  I'm trying to put together a dejagnu testsuite for
Ada. 


-- 
Thanks,
Jim

http://www.student.cs.uwaterloo.ca/~ja2morri/
http://phython.blogspot.com
http://open.nit.ca/wiki/?page=jim

Re: Ada: fix layout of packed arrays

[Eric Botcazou]

>  Which testsuite harness?  I'm trying to put together a dejagnu testsuite
> for Ada.

The harness of the internal AdaCore testsuite (which is not dejagnu-based).

-- 
Eric Botcazou

Re: Ada: fix layout of packed arrays

[Arnaud Charlet]

>  Test to see if a system is big endian or not.

You could probably use System.Default_Bit_Order

Arno