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]

[PATCH] Fix bug and pessimization for packed records in Ada


Hello,

The patch

2003-04-03  Jason Merrill  <jason@redhat.com>

	* stor-layout.c (do_type_align): New fn, split out from...
	(layout_decl): ...here.  Do all alignment calculations for
	FIELD_DECLs here.
	(update_alignment_for_field): Not here.
	(start_record_layout, debug_rli): Remove unpadded_align.
	* tree.h (struct record_layout_info_s): Remove unpadded_align.
	* c-decl.c (finish_enum): Don't set DECL_SIZE, DECL_ALIGN
	or DECL_MODE on the CONST_DECLs.
	(finish_struct): Don't mess with DECL_ALIGN.

posted at http://gcc.gnu.org/ml/gcc-patches/2003-04/msg00209.html introduced a 
bug and a pessimization for the alignment of certain fields inside packed 
records in Ada, on STRICT_ALIGNMENT targets.

The typical testcase is:

with Ada.Text_IO; use Ada.Text_IO;

procedure P is

   type Time_T is record
      Hour : Integer;
   end record;

   type Date_And_Time_T is record
      Date : Integer;
      Time : Time_T;
   end record;
   pragma Pack(Date_And_Time_T);

   procedure
     Assign_Hour_Of (T : out Time_T)
   is
   begin
      T.Hour := 44;
   end;

   procedure
     Clobber_Hour_Of (DT: out Date_And_Time_T)
   is
   begin
      Assign_Hour_Of (Dt.Time);
   end;

   DT : Date_And_Time_T;

begin
   DT.Time.Hour := 22;
   Clobber_Hour_Of (DT);
   Put_Line (Integer'Image(DT.Time.Hour));
end;

Before the patch, the Time field of the Date_And_Time_T record type was 
promoted to 32-bit alignment (in Ada, we try to promote fields in packed 
records to their natural alignment as much as possible) so the whole record 
was 32-bit aligned.  As a consequence, Clobber_Hour_Of was essentially 
reduced to a mere addition on the address.

After the patch, the field is not promoted anymore to 32-bit alignment, 
although it is given SImode.  The consequence is twofold: a temporary is 
created in Date_And_Time_T and the middle-end gets confused creating it 
because of the SImode of the field, leading to wrong code e.g. on SPARC.


What happens is that the alignment promotion in layout_decl:

  /* See if we can use an ordinary integer mode for a bit-field. 
     Conditions are: a fixed size that is correct for another mode
     and occupying a complete byte or bytes on proper boundary.  */
  if (code == FIELD_DECL && DECL_BIT_FIELD (decl)
      && TYPE_SIZE (type) != 0
      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
    {
      enum machine_mode xmode
	= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);

      if (xmode != BLKmode && known_align >= GET_MODE_ALIGNMENT (xmode))
	{
	  DECL_ALIGN (decl) = MAX (GET_MODE_ALIGNMENT (xmode),
				   DECL_ALIGN (decl));
	  DECL_MODE (decl) = xmode;
	  DECL_BIT_FIELD (decl) = 0;
	}
    }

may be overriden by:

     /* If the field is of variable size, we can't misalign it since we
	 have no way to make a temporary to align the result.  But this
	 isn't an issue if the decl is not addressable.  Likewise if it
	 is of unknown size.

	 Note that do_type_align may set DECL_USER_ALIGN, so we need to
	 check old_user_align instead.  */
      if (DECL_PACKED (decl)
	  && !old_user_align
	  && (DECL_NONADDRESSABLE_P (decl)
	      || DECL_SIZE_UNIT (decl) == 0
	      || TREE_CODE (DECL_SIZE_UNIT (decl)) == INTEGER_CST))
	DECL_ALIGN (decl) = MIN (DECL_ALIGN (decl), BITS_PER_UNIT);

They were originally (before the patch) in the reverse order.


The patch I'm proposing is also twofold:
- it swaps back the two blocks of code so that we cannot end up in a 
"semi-promoted" state anymore; and, in order not to change the default 
behaviour, it essentially adds an 'else' between them (that is, we don't 
promote if the alignment was reduced to the minimal one because of the 
packedness),
- it adds a language hook to let the front-end override the 'else' if it deems 
it profitable.

The patch also gets rid of the set_lang_adjust_rli hook that was added for Ada 
3 years ago and silenced (but not removed) by Jason in the same patch, with 
the comment (ada/misc.c:gnat_adjust_rli)

/* ??? This code seems to have no actual effect; record_align should already
     reflect the largest alignment desired by a field.  jason 2003-04-01  */

that I think is correct.


Bootstrapped/regtested on i586-mandrake-linux-gnu.  Both problems are 
regressions in Ada wrt before Jason's patch so I'm requesting approval for 
both mainline and 3.4.x branch.


2004-11-18 ?Eric Botcazou ?<ebotcazou@act-europe.fr>

	* langhooks.h (can_promote_packed_bit_field_p): New.
	* langhooks.c (lhd_can_promote_packed_bit_field_p): New function.
	* langhooks-def.h (lhd_can_promote_packed_bit_field_p): Declare it.
	(LANG_HOOKS_CAN_PROMOTE_PACKED_BIT_FIELD_P): Define to it.
	(LANG_HOOKS_INITIALIZER): Add LANG_HOOKS_CAN_PROMOTE_PACKED_BIT_FIELD_P.
	* stor-layout.c (layout_decl): Attempt to promote a bit-field only
	after the default alignment has been computed. ?Do not promote it
	if the alignment was forced to the minimal one because of packedness,
	unless the language specifically allows it.
	(lang_adjust_rli): Remove.
	(set_lang_adjust_rli): Likewise.
	(layout_type): Remove invocation of set_lang_adjust_rli.
	* tree.h (set_lang_adjust_rli): Remove.
	* ada/misc.c (gnat_adjust_rli): Remove.
	(gnat_init): Remove invocation of set_lang_adjust_rli.
	(gnat_can_promote_packed_bit_field_p): New function.
	(LANG_HOOKS_CAN_PROMOTE_PACKED_BIT_FIELD_P): Set it
	to gnat_can_promote_packed_bit_field_p.


-- 
Eric Botcazou


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