Re: [PATCH] MS Bitfields for Interix

[Geoff Keating <geoffk at geoffk dot org>]

"Douglas B Rupp" <rupp@gnat.com> writes:

> This patch turns on MS style bitfields for Interix, adds some new tests, and
> fixes some bugs.

This patch adds new functionality and so can't be committed while we're in phase 3.  You might want to post the bug-fixes separately if
you want to put them in right now.

> Bootstrapped and tested on Interix.
> Needs testing on SuperH.

> 2002-09-04  Douglas Rupp  <rupp@gnat.com>
> 	    Donn Terry  <donnte@microsoft.com>
> 
> 	* stor-layout.c (place_field): Handle alignment of whole
> 	structures when MSVC compatible bitfields are involved.
> 	Change method of computing location of MS bitfields to also
> 	be compatible with #pragma pack(n).
> 
> 	* toplev.c (parse_options_and_default_flags): Initialize
> 	target_flags before init_options.
> 
> 	* tree.h (record_layout_info): Add new field
> 	remaining_in_alignment.
> 
> 	* config/i386/i386-interix.h (MASK_MS_BITFIELD_LAYOUT,
> 	TARGET_USE_MS_BITFIELD_LAYOUT, SUBTARGET_SWITCHES,
> 	TARGET_SUBTARGET_DEFAULT): Define approriately to enable MSVC
> 	compatible bitfield layout.
> 	(PCC_BIT_FIELD_TYPE_TEST, GROUP_BITFIELDS_BY_ALIGN): Remove
> 	defines.
> 
> 	* config/i386/i386.c (ix86_ms_bitfield_layout): New function.
> 	(TARGET_MS_BITFIELD_LAYOUT_P): Define to above function.
> 	(TARGET_USE_MS_BITFIELD_LAYOUT): Define.
> 
> 	* objc/objc-lang.c (objc_init_options): Turn off MS bitfield layout.
> 	(objc_post_options): Warn that MS bitfield layout is not supported.
> 	(LANG_HOOKS_POST_OPTIONS): Define to above function.

> 	* testsuite/gcc.dg/bf-ms-layout.c: New test case.
> 	* testsuite/gcc.dg/bf-no-ms-layout.c: New test case.
> 	* testsuite/gcc.dg/i386-bitfield1.c (dg-options): Add appropriate
> 	flags for interix.
> 
> Index: gcc/stor-layout.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/stor-layout.c,v
> retrieving revision 1.128
> diff -u -p -r1.128 stor-layout.c
> --- gcc/stor-layout.c	7 Aug 2002 23:20:34 -0000	1.128
> +++ gcc/stor-layout.c	4 Sep 2002 18:42:15 -0000
> @@ -801,15 +801,26 @@ place_field (rli, field)
>    if ((* targetm.ms_bitfield_layout_p) (rli->t)
>        && type != error_mark_node
>        && DECL_BIT_FIELD_TYPE (field)
> -      && ! integer_zerop (TYPE_SIZE (type))
> -      && integer_zerop (DECL_SIZE (field)))
> +      && ! integer_zerop (TYPE_SIZE (type)))
>      {
> -      if (rli->prev_field
> -	  && DECL_BIT_FIELD_TYPE (rli->prev_field)
> -	  && ! integer_zerop (DECL_SIZE (rli->prev_field)))
> +      /* Here, the alignment of the underlying type of a bitfield can
> +	 affect the alignment of a record; even a zero-sized field
> +	 can do this.  The alignment should be to the alignment of
> +	 the type, except that for zero-size bitfields this only
> +	 applies if there was an immediately prior, non-zero-size
> +	 bitfield.  (That's the way it is, experimentally.) */
> +      if (! integer_zerop (DECL_SIZE (field))
> +	  || (rli->prev_field
> +	      && DECL_BIT_FIELD_TYPE (rli->prev_field)
> +	      && ! integer_zerop (DECL_SIZE (rli->prev_field))))
>  	{
> -	  rli->record_align = MAX (rli->record_align, desired_align);
> +	  unsigned int type_align = TYPE_ALIGN (type);
> +	  type_align = MAX (type_align, desired_align);
> +	  if (maximum_field_alignment != 0)
> +	    type_align = MIN (type_align, maximum_field_alignment);
> +	  rli->record_align = MAX (rli->record_align, type_align);
>  	  rli->unpacked_align = MAX (rli->unpacked_align, TYPE_ALIGN (type));
> +	  rli->unpadded_align = MAX (rli->unpadded_align, DECL_ALIGN (field));
>  	}
>        else
>  	desired_align = 1;
> @@ -991,48 +1002,140 @@ place_field (rli, field)
>      }
>  #endif
>  
> -  /* See the docs for TARGET_MS_BITFIELD_LAYOUT_P for details.  */
> +  /* See the docs for TARGET_MS_BITFIELD_LAYOUT_P for details.
> +     A subtlety:
> +	When a bit field is inserted into a packed record, the whole
> +	size of the underlying type is used by one or more same-size
> +	adjacent bitfields.  (That is, if its long:3, 32 bits is 
> +	used in the record, and any additional adjacent long bitfields are
> +	packed into the same chunk of 32 bits. However, if the size
> +	changes, a new field of that size is allocated.)  In an unpacked
> +	record, this is the same as using alignment, but not eqivalent
> +	when packing. 
> +
> +     Note: for compatability, we use the type size, not the type alignment
> +     to determine alignment, since that matches the documentation */

Can you document this in the user documentation as well as the source?
Users might expect that 'packed' means 'packed'.  It'd need to be
documented in two places, one in the description of 'packed' and one in
the place where the MS bitfield support is documented.

Also, somewhere in the release notes (perhaps the NEWS file?) there
should be mention that the ABI has changed under these circumstances.

>    if ((* targetm.ms_bitfield_layout_p) (rli->t)
> -      && TREE_CODE (field) == FIELD_DECL
> -      && type != error_mark_node
> -      && ! DECL_PACKED (field)
> -      && rli->prev_field
> -      && DECL_SIZE (field)
> -      && host_integerp (DECL_SIZE (field), 1)
> -      && DECL_SIZE (rli->prev_field)
> -      && host_integerp (DECL_SIZE (rli->prev_field), 1)
> -      && host_integerp (rli->offset, 1)
> -      && host_integerp (TYPE_SIZE (type), 1)
> -      && host_integerp (TYPE_SIZE (TREE_TYPE (rli->prev_field)), 1)
> -      && ((DECL_BIT_FIELD_TYPE (rli->prev_field)
> -	   && ! integer_zerop (DECL_SIZE (rli->prev_field)))
> -	  || (DECL_BIT_FIELD_TYPE (field)
> -	      && ! integer_zerop (DECL_SIZE (field))))
> -      && (! simple_cst_equal (TYPE_SIZE (type),
> -			      TYPE_SIZE (TREE_TYPE (rli->prev_field)))
> -	  /* If the previous field was a zero-sized bit-field, either
> -	     it was ignored, in which case we must ensure the proper
> -	     alignment of this field here, or it already forced the
> -	     alignment of this field, in which case forcing the
> -	     alignment again is harmless.  So, do it in both cases.  */
> -	  || (DECL_BIT_FIELD_TYPE (rli->prev_field)
> -	      && integer_zerop (DECL_SIZE (rli->prev_field)))))
> +      && (DECL_BIT_FIELD_TYPE(field)
> +	 || rli->prev_field))
>      {
> -      unsigned int type_align = TYPE_ALIGN (type);
> +      /* At this point, either the prior or current are bitfields,
> +	 (possibly both), and we're dealing with MS packing. */
> +      tree prev_saved = rli->prev_field;
> +
> +      /* Is the prior field a bitfield?  If so, handle "runs" of same
> +	 type size fields. */
> +      if (rli->prev_field /* necessarily a bitfield if it exists. */) 
> +	{
> +	  /* If both are bitfields, nonzero, and the same size, this is
> +	     the middle of a run.  Zero declared size fields are special
> +	     and handled as "end of run". (Note: it's nonzero declared
> +	     size, but equal type sizes!) */
> +	  if (DECL_BIT_FIELD_TYPE (field)
> +	      && !integer_zerop (DECL_SIZE (field))
> +	      && !integer_zerop (DECL_SIZE (rli->prev_field))
> +	      && simple_cst_equal (TYPE_SIZE (type),
> +		   TYPE_SIZE (TREE_TYPE (rli->prev_field))) )
> +	    {
> +	      /* We're in the middle of a run of equal type size fields; make
> +		 sure we realign if we run out of bits.  (Not decl size,
> +		 type size!) */
> +	      int bitsize = TREE_INT_CST_LOW (DECL_SIZE (field));
> +	      tree type_size = TYPE_SIZE(TREE_TYPE(rli->prev_field));
> +
> +	      if (rli->remaining_in_alignment < bitsize)
> +		{
> +		  /* out of bits; bump up to next 'word'. */
> +		  rli->bitpos = size_binop (PLUS_EXPR,
> +				      type_size,
> +				      DECL_FIELD_BIT_OFFSET(rli->prev_field));
> +		  rli->prev_field = field;
> +		  rli->remaining_in_alignment = TREE_INT_CST_LOW (type_size);
> +		}
> +	      rli->remaining_in_alignment -= bitsize;
> +	    }
> +	  else
> +	    {
> +	      /* End of a run: if leaving a run of bitfields of the same type 
> +		 size, we have to "use up" the rest of the bits of the type 
> +		 size.
> +
> +		 Compute the new position as the sum of the size for the prior
> +		 type and where we first started working on that type.
> +		 Note: since the beginning of the field was aligned then
> +		 of course the end will be too.  No round needed.  */
> +
> +	      if (!integer_zerop (DECL_SIZE (rli->prev_field)))
> +		{
> +		  tree type_size = TYPE_SIZE(TREE_TYPE(rli->prev_field));
> +		  rli->bitpos = size_binop (PLUS_EXPR,
> +				      type_size,
> +				      DECL_FIELD_BIT_OFFSET(rli->prev_field));
> +		}
> +	      else
> +		{
> +		  /* We "use up" size zero fields; the code below should behave
> +		     as if the prior field was not a bitfield. */
> +		  prev_saved = NULL;
> +		}
> +
> +	      /* Cause a new bitfield to be captured, either this time (if 
> +		 currently a bitfield) or next time we see one. */
> +	      if (!DECL_BIT_FIELD_TYPE(field)
> +		 || integer_zerop (DECL_SIZE (field)))
> +		{
> +		  rli->prev_field = NULL;
> +		}
> +	    }
> +	  normalize_rli (rli);
> +        }
>  
> -      if (rli->prev_field
> -	  && DECL_BIT_FIELD_TYPE (rli->prev_field)
> -	  /* If the previous bit-field is zero-sized, we've already
> -	     accounted for its alignment needs (or ignored it, if
> -	     appropriate) while placing it.  */
> -	  && ! integer_zerop (DECL_SIZE (rli->prev_field)))
> -	type_align = MAX (type_align,
> -			  TYPE_ALIGN (TREE_TYPE (rli->prev_field)));
> +      /* If we're starting a new run of same size type bitfields
> +	 (or a run of non-bitfields), set up the "first of the run"
> +	 fields. 
> +
> +	 That is, if the current field is not a bitfield, or if there
> +	 was a prior bitfield the type sizes differ, or if there wasn't
> +	 a prior bitfield the size of the current field is nonzero.
> +
> +	 Note: we must be sure to test ONLY the type size if there was
> +	 a prior bitfield and ONLY for the current field being zero if
> +	 there wasn't.  */
> +
> +      if (!DECL_BIT_FIELD_TYPE (field)
> +	  || ( prev_saved != NULL 
> +	       ? !simple_cst_equal (TYPE_SIZE (type),
> +	              TYPE_SIZE (TREE_TYPE (prev_saved)))
> +	       : !integer_zerop (DECL_SIZE (field)) ))
> +	{
> +	  unsigned int type_align = 8;  /* Never below 8 for compatability */
>  
> -      if (maximum_field_alignment != 0)
> -	type_align = MIN (type_align, maximum_field_alignment);
> +	  rli->remaining_in_alignment 
> +	      = TREE_INT_CST_LOW (TYPE_SIZE(TREE_TYPE(field)))
> +	        - TREE_INT_CST_LOW (DECL_SIZE (field));
> +
> +	  /* Now align (conventionally) for the new type. */
> +	  if (!DECL_PACKED(field))
> +	      type_align = MAX(TYPE_ALIGN (type), type_align);
> +
> +	  if (prev_saved
> +	      && DECL_BIT_FIELD_TYPE (prev_saved)
> +	      /* If the previous bit-field is zero-sized, we've already
> +		 accounted for its alignment needs (or ignored it, if
> +		 appropriate) while placing it.  */
> +	      && ! integer_zerop (DECL_SIZE (prev_saved)))
> +	    type_align = MAX (type_align,
> +			      TYPE_ALIGN (TREE_TYPE (prev_saved)));
> +
> +	  if (maximum_field_alignment != 0)
> +	    type_align = MIN (type_align, maximum_field_alignment);
>  
> -      rli->bitpos = round_up (rli->bitpos, type_align);
> +	  rli->bitpos = round_up (rli->bitpos, type_align);
> +          /* If we really aligned, don't allow subsequent bitfields
> +	     to undo that. */
> +	  rli->prev_field = NULL;
> +	}
>      }
>  
>    /* Offset so far becomes the position of this field after normalizing.  */
> @@ -1061,7 +1164,9 @@ place_field (rli, field)
>    if (known_align != actual_align)
>      layout_decl (field, actual_align);
>  
> -  rli->prev_field = field;
> +  /* Only the MS bitfields use this. */
> +  if (rli->prev_field == NULL && DECL_BIT_FIELD_TYPE(field))
> +      rli->prev_field = field;
>  
>    /* Now add size of this field to the size of the record.  If the size is
>       not constant, treat the field as being a multiple of bytes and just
> Index: gcc/toplev.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/toplev.c,v
> retrieving revision 1.670
> diff -u -p -r1.670 toplev.c
> --- gcc/toplev.c	29 Aug 2002 23:32:49 -0000	1.670
> +++ gcc/toplev.c	4 Sep 2002 18:42:26 -0000
> @@ -4754,6 +4754,11 @@ parse_options_and_default_flags (argc, a
>    /* Register the language-independent parameters.  */
>    add_params (lang_independent_params, LAST_PARAM);
>  
> +  /* Initialize target_flags before init_options and OPTIMIZATION_OPTIONS so 
> +     they can modify it.  */
> +  target_flags = 0;
> +  set_target_switch ("");
> +
>    /* Perform language-specific options initialization.  */
>    (*lang_hooks.init_options) ();
>  
> @@ -4866,11 +4871,6 @@ parse_options_and_default_flags (argc, a
>    /* Initialize how much space enums occupy, by default.  */
>    flag_short_enums = DEFAULT_SHORT_ENUMS;
>  #endif
> -
> -  /* Initialize target_flags before OPTIMIZATION_OPTIONS so the latter can
> -     modify it.  */
> -  target_flags = 0;
> -  set_target_switch ("");
>  
>    /* Unwind tables are always present in an ABI-conformant IA-64
>       object file, so the default should be ON.  */

See the comment for the change to objc-lang.c.  This change should not
be needed.

> Index: gcc/tree.h
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/tree.h,v
> retrieving revision 1.349
> diff -u -p -r1.349 tree.h
> --- gcc/tree.h	14 Aug 2002 02:15:40 -0000	1.349
> +++ gcc/tree.h	4 Sep 2002 18:42:34 -0000
> @@ -2399,6 +2399,8 @@ typedef struct record_layout_info_s
>    /* The static variables (i.e., class variables, as opposed to
>       instance variables) encountered in T.  */
>    tree pending_statics;
> +  /* Bits remaining in the current alignment group */
> +  int remaining_in_alignment;
>    int packed_maybe_necessary;
>  } *record_layout_info;
>  
> Index: gcc/config/i386/i386-interix.h
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/config/i386/i386-interix.h,v
> retrieving revision 1.37
> diff -u -p -r1.37 i386-interix.h
> --- gcc/config/i386/i386-interix.h	29 Aug 2002 21:40:11 -0000	1.37
> +++ gcc/config/i386/i386-interix.h	4 Sep 2002 18:42:39 -0000
> @@ -33,13 +33,28 @@ Boston, MA 02111-1307, USA.  */
>  #define HANDLE_SYSV_PRAGMA
>  #undef HANDLE_PRAGMA_WEAK  /* until the link format can handle it */
>  
> +/* Masks for subtarget switches used by other files.  */
> +#define MASK_MS_BITFIELD_LAYOUT 0x10000000 /* Use native (MS) bitfield layout */
> +
> +/* Tell i386.c to put a target-specific specialization of
> +   ms_bitfield_layout_p in struct gcc_target targetm.  */
> +#define TARGET_USE_MS_BITFIELD_LAYOUT  \
> +  (target_flags & MASK_MS_BITFIELD_LAYOUT)
> +
> +#undef  SUBTARGET_SWITCHES
> +#define SUBTARGET_SWITCHES \
> +{ "ms-bitfields", MASK_MS_BITFIELD_LAYOUT, N_("Use native (MS) bitfield layout") }, \
> +{ "no-ms-bitfields", -MASK_MS_BITFIELD_LAYOUT, N_("Use gcc default bitfield layout") },
> +
> +
>  /* By default, target has a 80387, uses IEEE compatible arithmetic,
>     and returns float values in the 387 and needs stack probes
> -   We also align doubles to 64-bits for MSVC default compatibility */
> +   We also align doubles to 64-bits for MSVC default compatibility
> +   We do bitfields MSVC-compatably by default, too. */
>  #undef TARGET_SUBTARGET_DEFAULT
>  #define TARGET_SUBTARGET_DEFAULT \
>     (MASK_80387 | MASK_IEEE_FP | MASK_FLOAT_RETURNS | MASK_STACK_PROBE | \
> -    MASK_ALIGN_DOUBLE)
> +    MASK_ALIGN_DOUBLE | MASK_MS_BITFIELD_LAYOUT)
>  
>  #undef TARGET_CPU_DEFAULT
>  #define TARGET_CPU_DEFAULT 2 /* 486 */
> @@ -273,8 +288,6 @@ while (0)
>  #define HOST_PTR_AS_INT unsigned long
>  
>  #define PCC_BITFIELD_TYPE_MATTERS 1
> -#define PCC_BITFIELD_TYPE_TEST TYPE_NATIVE(rec)
> -#define GROUP_BITFIELDS_BY_ALIGN TYPE_NATIVE(rec)
>  
>  /* The following two flags are usually "off" for i386, because some non-gnu
>     tools (for the i386) don't handle them.  However, we don't have that

Are MS bitfields different under Interix (compared to, say, cygwin)?
If they are not, it would be better to put the flags in
TARGET_SWITCHES and TARGET_DEFAULT and so on, not special-case
Interix.

> Index: gcc/objc/objc-lang.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/objc/objc-lang.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 objc-lang.c
> --- gcc/objc/objc-lang.c	10 Aug 2002 02:18:28 -0000	1.26
> +++ gcc/objc/objc-lang.c	4 Sep 2002 18:43:07 -0000
> @@ -30,6 +30,7 @@ Boston, MA 02111-1307, USA.  */
>  #include "langhooks-def.h"
>  
>  static void objc_init_options                   PARAMS ((void));
> +static bool objc_post_options                   PARAMS ((void));
>  
>  #undef LANG_HOOKS_NAME
>  #define LANG_HOOKS_NAME "GNU Objective-C"
> @@ -42,7 +43,7 @@ static void objc_init_options           
>  #undef LANG_HOOKS_DECODE_OPTION
>  #define LANG_HOOKS_DECODE_OPTION c_common_decode_option
>  #undef LANG_HOOKS_POST_OPTIONS
> -#define LANG_HOOKS_POST_OPTIONS c_common_post_options
> +#define LANG_HOOKS_POST_OPTIONS objc_post_options
>  #undef LANG_HOOKS_GET_ALIAS_SET
>  #define LANG_HOOKS_GET_ALIAS_SET c_common_get_alias_set
>  #undef LANG_HOOKS_SAFE_FROM_P
> @@ -164,4 +165,23 @@ objc_init_options ()
>  {
>    flag_objc = 1;
>    c_common_init_options (clk_c);
> +#ifdef MASK_MS_BITFIELD_LAYOUT
> +  /* Objc tries to parallel the code in stor-layout.c at runtime
> +     (see libobjc/encoding.c).  This packing info isn't available, 
> +     so it's hopeless to try. */
> +  target_flags &= ~MASK_MS_BITFIELD_LAYOUT;
> +#endif
> +} 
> +
> +static bool
> +objc_post_options ()
> +{
> +#ifdef MASK_MS_BITFIELD_LAYOUT
> +  /* And if the user tries to set the flag, don't allow it. */
> +  if ((target_flags & MASK_MS_BITFIELD_LAYOUT) != 0)
> +    {
> +      error ("ms-bitfields not supported for objc");
> +    }
> +#endif
> +  return c_common_post_options ();
>  }

Please re-design the patch so that the objc frontend interacts with
the backends in a less coupled manner.  In particular, do not use
target_flags in the frontends.  I would suggest either removing this
code or renaming the flag to '-fms-bitfield-layout' and giving it its
own variable.

-- 
- Geoffrey Keating <geoffk@geoffk.org>