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]

Re: C++ structure layout breakage


Jim --

  Thanks for the excellent detective work.

  I am sorry to have been the originator of this mess!
Double embarassing since I was actually trying to clean
up the code in the process...

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com


----- Original Message -----
Sent   : 10/24/00 6:30 PM
From   : Jim Wilson <wilson@cygnus.com>
To     : gcc-patches@gcc.gnu.org
Subject: Re: C++ structure layout breakage 

> I of course found yet another problem.  This one showed up while trying to
> build a ia64-linux GNU libc with my last proposed patch.  The simplified
> testcase is this:
> union foo
> {
>   long double ld;
>   double d;
> };
> sub ()
> {
>   union foo bar;
>   bar.d = 0.0;
> }
> This gets an abort in store_bit_field because we are trying to insert a
> DFmode value into an TFmode (or XFmode) object, and that just doesn't work,
> as we don't allow (subreg:DF (reg:TF)).  This is likely the original problem
> that Kenner tried to fix with his March 25 change to compute_record_mode.
> The real problem here is a bug in Mark Mitchell's March 14 change.
> 2000-03-14  Mark Mitchell  <mark@codesourcery.com>
> 	* stor-layout.c (layout_union): Remove.
> 	(layout_union_field): New function, split out from layout_union.
> 	(finish_union_layout): Likewise.
> 	(layout_field): Handle unions by calling layout_union_field.
> 	(finish_record_layout): Handle unions.
> 	(layout_type): Combine RECORD_TYPE, UNION_TYPE, and
> 	QUAL_UNION_TYPE handling.
> This commonized the handling of records and unions.  Unfortunately, Mark
> missed the fact that the code setting TYPE_MODE for records and unions was
> different, and this difference is important.  With gcc-2.8.1, the union
> above gets BLKmode.  After Mark's change, it got TFmode which doesn't work.
> Since changing the default mode of unions is an ABI change, this is undesirable
> and the best fix is to go back to the original BLKmode.
> I now have found problems with 5 separate patches, and have good explanations
> for all of them.  Richard Henderson broke store_bit_field for FP modes.  Mark
> Mitchell broke TYPE_MODE of unions.  Kenner's Mar 25 patch incorrectly tried
> to fix Mark's bug.  Kenner's Aug 18 patch incorrectly tried to fix Richard's
> bug.  Kenner's Oct 20 patch fixed a problem with the Mar 25 patch.  I have
> better fixes for the original two bugs, so none of Kenner's 3 changes are
> needed anymore.
> This patch has been tested with an ia32-linux make bootstrap and make check.
> Also, on ia64-linux, with a GNU libc build and make check, a linux kernel
> build, and I've got a toolchain bootstrap running but didn't want to wait for
> it as it will take a while.
> 2000-10-24  Jim Wilson  <wilson@cygnus.com>
> 	* expmed.c (store_bit_field): Move integer pun code down after
> 	code that calls emit_move_insn for entire register move.
> 	* stor-layout.c (compute_record_mode): Revert Mar 25, Aug 18, and
> 	Oct 20 changes.  Only store mode in TYPE_MODE if RECORD_TYPE.
>
> Index: expmed.c
> ===================================================================
> RCS file: /cvs/gcc/egcs/gcc/expmed.c,v
> retrieving revision 1.67
> diff -p -r1.67 expmed.c
> *** expmed.c	2000/10/19 15:48:22	1.67
> --- expmed.c	2000/10/24 22:58:03
> *************** store_bit_field (str_rtx, bitsize, bitnu
> *** 269,289 ****
>         op0 = SUBREG_REG (op0);
>       }
>
> -   /* Make sure we are playing with integral modes.  Pun with subregs
> -      if we aren't.  */
> -   {
> -     enum machine_mode imode = int_mode_for_mode (GET_MODE (op0));
> -     if (imode != GET_MODE (op0))
> -       {
> - 	if (GET_CODE (op0) == MEM)
> - 	  op0 = change_address (op0, imode, NULL_RTX);
> - 	else if (imode != BLKmode)
> - 	  op0 = gen_lowpart (imode, op0);
> - 	else
> - 	  abort ();
> -       }
> -   }
> - 
>     /* If OP0 is a register, BITPOS must count within a word.
>        But as we have it, it counts within whatever size OP0 now has.
>        On a bigendian machine, these are not the same, so convert.  */
> --- 269,274 ----
> *************** store_bit_field (str_rtx, bitsize, bitnu
> *** 336,341 ****
> --- 321,343 ----
>         emit_move_insn (op0, value);
>         return value;
>       }
> + 
> +   /* Make sure we are playing with integral modes.  Pun with subregs
> +      if we aren't.  This must come after the entire register case above,
> +      since that case is valid for any mode.  The following cases are only
> +      valid for integral modes.  */
> +   {
> +     enum machine_mode imode = int_mode_for_mode (GET_MODE (op0));
> +     if (imode != GET_MODE (op0))
> +       {
> + 	if (GET_CODE (op0) == MEM)
> + 	  op0 = change_address (op0, imode, NULL_RTX);
> + 	else if (imode != BLKmode)
> + 	  op0 = gen_lowpart (imode, op0);
> + 	else
> + 	  abort ();
> +       }
> +   }
>
>     /* Storing an lsb-aligned field in a register
>        can be done with a movestrict instruction.  */
> Index: stor-layout.c
> ===================================================================
> RCS file: /cvs/gcc/egcs/gcc/stor-layout.c,v
> retrieving revision 1.86
> diff -p -r1.86 stor-layout.c
> *** stor-layout.c	2000/10/20 19:42:40	1.86
> --- stor-layout.c	2000/10/24 22:58:04
> *************** compute_record_mode (type)
> *** 1065,1076 ****
>
>         /* If this field is the whole struct, remember its mode so
>   	 that, say, we can put a double in a class into a DF
> ! 	 register instead of forcing it to live in the stack.  However,
> ! 	 we don't support using such a mode if there is no integer mode
> ! 	 of the same size, so don't set it here.  */
> !       if (field == TYPE_FIELDS (type) && TREE_CHAIN (field) == 0
> ! 	  && int_mode_for_mode (DECL_MODE (field)) != BLKmode
> ! 	  && operand_equal_p (DECL_SIZE (field), TYPE_SIZE (type), 1))
>   	mode = DECL_MODE (field);
>
>   #ifdef STRUCT_FORCE_BLK
> --- 1065,1072 ----
>
>         /* If this field is the whole struct, remember its mode so
>   	 that, say, we can put a double in a class into a DF
> ! 	 register instead of forcing it to live in the stack.  */
> !       if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
>   	mode = DECL_MODE (field);
>
>   #ifdef STRUCT_FORCE_BLK
> *************** compute_record_mode (type)
> *** 1081,1088 ****
>   #endif /* STRUCT_FORCE_BLK  */
>       }
>
> !   if (mode != VOIDmode)
> !     /* We only have one real field; use its mode.  */
>       TYPE_MODE (type) = mode;
>     else
>       TYPE_MODE (type) = mode_for_size_tree (TYPE_SIZE (type), MODE_INT, 1);
> --- 1077,1085 ----
>   #endif /* STRUCT_FORCE_BLK  */
>       }
>
> !   /* If we only have one real field; use its mode.  This only applies to
> !      RECORD_TYPE.  This does not apply to unions.  */
> !   if (TREE_CODE (type) == RECORD_TYPE && mode != VOIDmode)
>       TYPE_MODE (type) = mode;
>     else
>       TYPE_MODE (type) = mode_for_size_tree (TYPE_SIZE (type), MODE_INT, 1);


____________________________________________________

Get MORE than just FREE e-mail. Get a private Web office.
Organize your team with FREE communication and
collaboration tools from HotOffice(R) at  www.hotoffice.com.

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