Fix ms_struct/-mms-bitfields structure layout (PR target/52991)

Message ID 20180228002633.GD5866@tucnak
State New
Headers show
Series
  • Fix ms_struct/-mms-bitfields structure layout (PR target/52991)
Related show

Commit Message

Jakub Jelinek Feb. 28, 2018, 12:26 a.m.
Hi!

The following patch fixes the reported ms_struct/-mms-bitfields structure
layout issues from PR52991.

There are multiple issues, two of them introduced by the
https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html -mms-bitfields
revamp from Eric and follow-up fix r114552, the rest has been introduced
later when the known_align < desired_align case has been enabled for the ms
bitfield layout.

The first 2 hunks fix alignment of packed non-bitfield fields, we can't
ignore all the alignment updates for them, just should use only
desired_align which takes DECL_PACKED into account, rather than
MAX (type_align, desired_align).  Similarly, the last hunk in stor-layout.c
makes sure that for DECL_PACKED fields we use BITS_PER_UNIT alignment rather
than the type alignment.

The rest attempts to unbreak r184409 which enabled known_align < desired_align
case; doing that if rli->prev_field and ms layout is wrong, we first need to
deal with the bitfield packing and if we are within a bitfield word, we
shouldn't do any realignment, only in between them.

The patch reverts changes to bf-ms-layout{,-2}.c tests done in 2012, which
were done just to match the r184409 changes, and adds 2 new tests.  All of
these 4 I've tested (slightly tweaked, so that it compiles with VC) with
the online VC compiler http://rextester.com/l/c_online_compiler_visual .

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-02-27  Jakub Jelinek  <jakub@redhat.com>

	PR target/52991
	* stor-layout.c (update_alignment_for_field): For
	targetm.ms_bitfield_layout_p (rli->t), if !is_bitfield
	&& !DECL_PACKED (field), do the alignment update, just use
	only desired_align instead of MAX (type_align, desired_align)
	as the alignment.
	(place_field): Don't do known_align < desired_align handling
	early if targetm.ms_bitfield_layout_p (rli->t) and rli->prev_field
	is non-NULL, instead do it after rli->prev_field handling and
	only if not within a bitfield word.  For DECL_PACKED (field)
	use type_align of BITS_PER_UNIT.

	* gcc.dg/bf-ms-layout.c: Revert 2012-04-26 changes.
	* gcc.dg/bf-ms-layout-2.c: Revert 2012-02-23 changes.
	* gcc.dg/bf-ms-layout-4.c: New test.
	* gcc.dg/bf-ms-layout-5.c: New test.


	Jakub

Comments

Kai Tietz Feb. 28, 2018, 9:25 a.m. | #1
Hello Jakub,

I can't approve this patch, but I can confirm that changes are
sensible.  I verified the structure layout - as far as possible - also
with MS' compiler, and can confirm the adjustments.

Cheers,
Ka

2018-02-28 1:26 GMT+01:00 Jakub Jelinek <jakub@redhat.com>:
> Hi!

>

> The following patch fixes the reported ms_struct/-mms-bitfields structure

> layout issues from PR52991.

>

> There are multiple issues, two of them introduced by the

> https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html -mms-bitfields

> revamp from Eric and follow-up fix r114552, the rest has been introduced

> later when the known_align < desired_align case has been enabled for the ms

> bitfield layout.

>

> The first 2 hunks fix alignment of packed non-bitfield fields, we can't

> ignore all the alignment updates for them, just should use only

> desired_align which takes DECL_PACKED into account, rather than

> MAX (type_align, desired_align).  Similarly, the last hunk in stor-layout.c

> makes sure that for DECL_PACKED fields we use BITS_PER_UNIT alignment rather

> than the type alignment.

>

> The rest attempts to unbreak r184409 which enabled known_align < desired_align

> case; doing that if rli->prev_field and ms layout is wrong, we first need to

> deal with the bitfield packing and if we are within a bitfield word, we

> shouldn't do any realignment, only in between them.

>

> The patch reverts changes to bf-ms-layout{,-2}.c tests done in 2012, which

> were done just to match the r184409 changes, and adds 2 new tests.  All of

> these 4 I've tested (slightly tweaked, so that it compiles with VC) with

> the online VC compiler http://rextester.com/l/c_online_compiler_visual .

>

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

>

> 2018-02-27  Jakub Jelinek  <jakub@redhat.com>

>

>         PR target/52991

>         * stor-layout.c (update_alignment_for_field): For

>         targetm.ms_bitfield_layout_p (rli->t), if !is_bitfield

>         && !DECL_PACKED (field), do the alignment update, just use

>         only desired_align instead of MAX (type_align, desired_align)

>         as the alignment.

>         (place_field): Don't do known_align < desired_align handling

>         early if targetm.ms_bitfield_layout_p (rli->t) and rli->prev_field

>         is non-NULL, instead do it after rli->prev_field handling and

>         only if not within a bitfield word.  For DECL_PACKED (field)

>         use type_align of BITS_PER_UNIT.

>

>         * gcc.dg/bf-ms-layout.c: Revert 2012-04-26 changes.

>         * gcc.dg/bf-ms-layout-2.c: Revert 2012-02-23 changes.

>         * gcc.dg/bf-ms-layout-4.c: New test.

>         * gcc.dg/bf-ms-layout-5.c: New test.

>

> --- gcc/stor-layout.c.jj        2018-02-22 14:35:33.135216198 +0100

> +++ gcc/stor-layout.c   2018-02-27 18:56:26.906494801 +0100

> @@ -1038,7 +1038,7 @@ update_alignment_for_field (record_layou

>          the type, except that for zero-size bitfields this only

>          applies if there was an immediately prior, nonzero-size

>          bitfield.  (That's the way it is, experimentally.) */

> -      if ((!is_bitfield && !DECL_PACKED (field))

> +      if (!is_bitfield

>           || ((DECL_SIZE (field) == NULL_TREE

>                || !integer_zerop (DECL_SIZE (field)))

>               ? !DECL_PACKED (field)

> @@ -1047,7 +1047,10 @@ update_alignment_for_field (record_layou

>                  && ! integer_zerop (DECL_SIZE (rli->prev_field)))))

>         {

>           unsigned int type_align = TYPE_ALIGN (type);

> -         type_align = MAX (type_align, desired_align);

> +         if (!is_bitfield && DECL_PACKED (field))

> +           type_align = desired_align;

> +         else

> +           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);

> @@ -1303,7 +1306,9 @@ place_field (record_layout_info rli, tre

>

>    /* Does this field automatically have alignment it needs by virtue

>       of the fields that precede it and the record's own alignment?  */

> -  if (known_align < desired_align)

> +  if (known_align < desired_align

> +      && (! targetm.ms_bitfield_layout_p (rli->t)

> +         || rli->prev_field == NULL))

>      {

>        /* No, we need to skip space before this field.

>          Bump the cumulative size to multiple of field alignment.  */

> @@ -1331,8 +1336,6 @@ place_field (record_layout_info rli, tre

>

>        if (! TREE_CONSTANT (rli->offset))

>         rli->offset_align = desired_align;

> -      if (targetm.ms_bitfield_layout_p (rli->t))

> -       rli->prev_field = NULL;

>      }

>

>    /* Handle compatibility with PCC.  Note that if the record has any

> @@ -1448,6 +1451,8 @@ place_field (record_layout_info rli, tre

>        /* This is a bitfield if it exists.  */

>        if (rli->prev_field)

>         {

> +         bool realign_p = known_align < desired_align;

> +

>           /* 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

> @@ -1481,7 +1486,10 @@ place_field (record_layout_info rli, tre

>                     rli->remaining_in_alignment = typesize - bitsize;

>                 }

>               else

> -               rli->remaining_in_alignment -= bitsize;

> +               {

> +                 rli->remaining_in_alignment -= bitsize;

> +                 realign_p = false;

> +               }

>             }

>           else

>             {

> @@ -1512,6 +1520,31 @@ place_field (record_layout_info rli, tre

>                 rli->prev_field = NULL;

>             }

>

> +         /* Does this field automatically have alignment it needs by virtue

> +            of the fields that precede it and the record's own alignment?  */

> +         if (realign_p)

> +           {

> +             /* If the alignment is still within offset_align, just align

> +                the bit position.  */

> +             if (desired_align < rli->offset_align)

> +               rli->bitpos = round_up (rli->bitpos, desired_align);

> +             else

> +               {

> +                 /* First adjust OFFSET by the partial bits, then align.  */

> +                 tree d = size_binop (CEIL_DIV_EXPR, rli->bitpos,

> +                                      bitsize_unit_node);

> +                 rli->offset = size_binop (PLUS_EXPR, rli->offset,

> +                                           fold_convert (sizetype, d));

> +                 rli->bitpos = bitsize_zero_node;

> +

> +                 rli->offset = round_up (rli->offset,

> +                                         desired_align / BITS_PER_UNIT);

> +               }

> +

> +             if (! TREE_CONSTANT (rli->offset))

> +               rli->offset_align = desired_align;

> +           }

> +

>           normalize_rli (rli);

>          }

>

> @@ -1530,7 +1563,7 @@ place_field (record_layout_info rli, tre

>        if (!DECL_BIT_FIELD_TYPE (field)

>           || (prev_saved != NULL

>               ? !simple_cst_equal (TYPE_SIZE (type), TYPE_SIZE (prev_type))

> -             : !integer_zerop (DECL_SIZE (field)) ))

> +             : !integer_zerop (DECL_SIZE (field))))

>         {

>           /* Never smaller than a byte for compatibility.  */

>           unsigned int type_align = BITS_PER_UNIT;

> @@ -1555,7 +1588,8 @@ place_field (record_layout_info rli, tre

>             }

>

>           /* Now align (conventionally) for the new type.  */

> -         type_align = TYPE_ALIGN (TREE_TYPE (field));

> +         if (! DECL_PACKED (field))

> +           type_align = TYPE_ALIGN (TREE_TYPE (field));

>

>           if (maximum_field_alignment != 0)

>             type_align = MIN (type_align, maximum_field_alignment);

> --- gcc/testsuite/gcc.dg/bf-ms-layout.c.jj      2017-06-20 21:38:01.634906200 +0200

> +++ gcc/testsuite/gcc.dg/bf-ms-layout.c 2018-02-27 18:04:06.923851839 +0100

> @@ -153,27 +153,27 @@ int main(){

>    struct ten test_ten;

>

>  #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER)

> -  size_t exp_sizeof_one = 8;

> -  size_t exp_sizeof_two = 12;

> +  size_t exp_sizeof_one = 12;

> +  size_t exp_sizeof_two = 16;

>    size_t exp_sizeof_three =6;

>    size_t exp_sizeof_four = 8;

>    size_t exp_sizeof_five = 3;

>    size_t exp_sizeof_six = 8;

>    size_t exp_sizeof_seven = 3;

> -  size_t exp_sizeof_eight = 2;

> +  size_t exp_sizeof_eight = 4;

>    size_t exp_sizeof_nine = 8;

> -  size_t exp_sizeof_ten = 8;

> +  size_t exp_sizeof_ten = 16;

>

> -  unsigned char exp_one_c = 7;

> -  unsigned char exp_two_c  = 9;

> +  unsigned char exp_one_c = 8;

> +  unsigned char exp_two_c  = 12;

>    unsigned char exp_three_c = 4;

>    unsigned char exp_four_c = 4;

>    char exp_five_c = 2;

>    char exp_six_c = 5;

>    char exp_seven_c = 2;

> -  char exp_eight_c = 1;

> +  char exp_eight_c = 2;

>    char exp_nine_c = 0;

> -  char exp_ten_c = 1;

> +  char exp_ten_c = 8;

>

>  #else /* testing -mno-ms-bitfields */

>

> --- gcc/testsuite/gcc.dg/bf-ms-layout-2.c.jj    2017-06-20 21:38:02.112900704 +0200

> +++ gcc/testsuite/gcc.dg/bf-ms-layout-2.c       2018-02-27 18:04:06.923851839 +0100

> @@ -158,27 +158,27 @@ int main(){

>    struct ten test_ten;

>

>  #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER)

> -  size_t exp_sizeof_one = 8;

> -  size_t exp_sizeof_two = 12;

> +  size_t exp_sizeof_one = 12;

> +  size_t exp_sizeof_two = 16;

>    size_t exp_sizeof_three =6;

>    size_t exp_sizeof_four = 8;

>    size_t exp_sizeof_five = 3;

>    size_t exp_sizeof_six = 8;

>    size_t exp_sizeof_seven = 3;

> -  size_t exp_sizeof_eight = 2;

> +  size_t exp_sizeof_eight = 4;

>    size_t exp_sizeof_nine = 8;

> -  size_t exp_sizeof_ten = 8;

> +  size_t exp_sizeof_ten = 16;

>

> -  unsigned char exp_one_c = 7;

> -  unsigned char exp_two_c  = 9;

> +  unsigned char exp_one_c = 8;

> +  unsigned char exp_two_c  = 12;

>    unsigned char exp_three_c = 4;

>    unsigned char exp_four_c = 4;

>    char exp_five_c = 2;

>    char exp_six_c = 5;

>    char exp_seven_c = 2;

> -  char exp_eight_c = 1;

> +  char exp_eight_c = 2;

>    char exp_nine_c = 0;

> -  char exp_ten_c = 1;

> +  char exp_ten_c = 8;

>

>  #else /* testing -mno-ms-bitfields */

>

> --- gcc/testsuite/gcc.dg/bf-ms-layout-4.c.jj    2018-02-27 18:09:09.544421580 +0100

> +++ gcc/testsuite/gcc.dg/bf-ms-layout-4.c       2018-02-27 18:18:00.845039925 +0100

> @@ -0,0 +1,43 @@

> +/* PR target/52991 */

> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */

> +

> +#define CHECK(expr) extern char c[(expr) ? 1 : -1]

> +#define offsetof(x, y) __builtin_offsetof (x, y)

> +

> +struct test_sp1 {

> +    int a;

> +    short b;

> +    int c;

> +    char d;

> +} __attribute__((packed,ms_struct));

> +

> +CHECK (sizeof (struct test_sp1) == 11);

> +CHECK (offsetof (struct test_sp1, a) == 0);

> +CHECK (offsetof (struct test_sp1, b) == 4);

> +CHECK (offsetof (struct test_sp1, c) == 6);

> +CHECK (offsetof (struct test_sp1, d) == 10);

> +

> +struct test_sp3 {

> +    int a;

> +    short b __attribute__((aligned(8)));

> +    int c;

> +    char d;

> +} __attribute__((packed,ms_struct));

> +

> +CHECK (sizeof (struct test_sp3) == 16);

> +CHECK (offsetof (struct test_sp3, a) == 0);

> +CHECK (offsetof (struct test_sp3, b) == 8);

> +CHECK (offsetof (struct test_sp3, c) == 10);

> +CHECK (offsetof (struct test_sp3, d) == 14);

> +

> +struct test_s4 {

> +    int a;

> +    short b;

> +    int c:15;

> +    char d;

> +} __attribute__((ms_struct));

> +

> +CHECK (sizeof (struct test_s4) == 16);

> +CHECK (offsetof (struct test_s4, a) == 0);

> +CHECK (offsetof (struct test_s4, b) == 4);

> +CHECK (offsetof (struct test_s4, d) == 12);

> --- gcc/testsuite/gcc.dg/bf-ms-layout-5.c.jj    2018-02-27 18:31:24.043753173 +0100

> +++ gcc/testsuite/gcc.dg/bf-ms-layout-5.c       2018-02-27 18:35:29.825676223 +0100

> @@ -0,0 +1,45 @@

> +/* PR target/52991 */

> +/* { dg-do run { target i?86-*-* x86_64-*-* } } */

> +

> +struct S {

> +  int a : 2;

> +  __attribute__((aligned (8))) int b : 2;

> +  int c : 28;

> +  __attribute__((aligned (16))) int d : 2;

> +  int e : 30;

> +} __attribute__((ms_struct));

> +

> +struct S s;

> +

> +int

> +main ()

> +{

> +  int i;

> +  if (sizeof (s) != 32)

> +    __builtin_abort ();

> +  s.a = -1;

> +  for (i = 0; i < 32; ++i)

> +    if (((char *) &s)[i] != (i ? 0 : 3))

> +      __builtin_abort ();

> +  s.a = 0;

> +  s.b = -1;

> +  for (i = 0; i < 32; ++i)

> +    if (((char *) &s)[i] != (i ? 0 : 12))

> +      __builtin_abort ();

> +  s.b = 0;

> +  s.c = -1;

> +  for (i = 0; i < 32; ++i)

> +    if (((signed char *) &s)[i] != (i > 3 ? 0 : (i ? -1 : -16)))

> +      __builtin_abort ();

> +  s.c = 0;

> +  s.d = -1;

> +  for (i = 0; i < 32; ++i)

> +    if (((signed char *) &s)[i] != (i == 16 ? 3 : 0))

> +      __builtin_abort ();

> +  s.d = 0;

> +  s.e = -1;

> +  for (i = 0; i < 32; ++i)

> +    if (((signed char *) &s)[i] != ((i < 16 || i > 19) ? 0 : (i == 16 ? -4 : -1)))

> +      __builtin_abort ();

> +  return 0;

> +}

>

>         Jakub
JonY Feb. 28, 2018, 10:13 a.m. | #2
On 02/28/2018 12:26 AM, Jakub Jelinek wrote:
> Hi!

> 

> The following patch fixes the reported ms_struct/-mms-bitfields structure

> layout issues from PR52991.

> 

> There are multiple issues, two of them introduced by the

> https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html -mms-bitfields

> revamp from Eric and follow-up fix r114552, the rest has been introduced

> later when the known_align < desired_align case has been enabled for the ms

> bitfield layout.

> 

> The first 2 hunks fix alignment of packed non-bitfield fields, we can't

> ignore all the alignment updates for them, just should use only

> desired_align which takes DECL_PACKED into account, rather than

> MAX (type_align, desired_align).  Similarly, the last hunk in stor-layout.c

> makes sure that for DECL_PACKED fields we use BITS_PER_UNIT alignment rather

> than the type alignment.

> 

> The rest attempts to unbreak r184409 which enabled known_align < desired_align

> case; doing that if rli->prev_field and ms layout is wrong, we first need to

> deal with the bitfield packing and if we are within a bitfield word, we

> shouldn't do any realignment, only in between them.

> 

> The patch reverts changes to bf-ms-layout{,-2}.c tests done in 2012, which

> were done just to match the r184409 changes, and adds 2 new tests.  All of

> these 4 I've tested (slightly tweaked, so that it compiles with VC) with

> the online VC compiler http://rextester.com/l/c_online_compiler_visual .

> 

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

> 


No objections.

Patch

--- gcc/stor-layout.c.jj	2018-02-22 14:35:33.135216198 +0100
+++ gcc/stor-layout.c	2018-02-27 18:56:26.906494801 +0100
@@ -1038,7 +1038,7 @@  update_alignment_for_field (record_layou
 	 the type, except that for zero-size bitfields this only
 	 applies if there was an immediately prior, nonzero-size
 	 bitfield.  (That's the way it is, experimentally.) */
-      if ((!is_bitfield && !DECL_PACKED (field))
+      if (!is_bitfield
 	  || ((DECL_SIZE (field) == NULL_TREE
 	       || !integer_zerop (DECL_SIZE (field)))
 	      ? !DECL_PACKED (field)
@@ -1047,7 +1047,10 @@  update_alignment_for_field (record_layou
 		 && ! integer_zerop (DECL_SIZE (rli->prev_field)))))
 	{
 	  unsigned int type_align = TYPE_ALIGN (type);
-	  type_align = MAX (type_align, desired_align);
+	  if (!is_bitfield && DECL_PACKED (field))
+	    type_align = desired_align;
+	  else
+	    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);
@@ -1303,7 +1306,9 @@  place_field (record_layout_info rli, tre
 
   /* Does this field automatically have alignment it needs by virtue
      of the fields that precede it and the record's own alignment?  */
-  if (known_align < desired_align)
+  if (known_align < desired_align
+      && (! targetm.ms_bitfield_layout_p (rli->t)
+	  || rli->prev_field == NULL))
     {
       /* No, we need to skip space before this field.
 	 Bump the cumulative size to multiple of field alignment.  */
@@ -1331,8 +1336,6 @@  place_field (record_layout_info rli, tre
 
       if (! TREE_CONSTANT (rli->offset))
 	rli->offset_align = desired_align;
-      if (targetm.ms_bitfield_layout_p (rli->t))
-	rli->prev_field = NULL;
     }
 
   /* Handle compatibility with PCC.  Note that if the record has any
@@ -1448,6 +1451,8 @@  place_field (record_layout_info rli, tre
       /* This is a bitfield if it exists.  */
       if (rli->prev_field)
 	{
+	  bool realign_p = known_align < desired_align;
+
 	  /* 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
@@ -1481,7 +1486,10 @@  place_field (record_layout_info rli, tre
 		    rli->remaining_in_alignment = typesize - bitsize;
 		}
 	      else
-		rli->remaining_in_alignment -= bitsize;
+		{
+		  rli->remaining_in_alignment -= bitsize;
+		  realign_p = false;
+		}
 	    }
 	  else
 	    {
@@ -1512,6 +1520,31 @@  place_field (record_layout_info rli, tre
 		rli->prev_field = NULL;
 	    }
 
+	  /* Does this field automatically have alignment it needs by virtue
+	     of the fields that precede it and the record's own alignment?  */
+	  if (realign_p)
+	    {
+	      /* If the alignment is still within offset_align, just align
+		 the bit position.  */
+	      if (desired_align < rli->offset_align)
+		rli->bitpos = round_up (rli->bitpos, desired_align);
+	      else
+		{
+		  /* First adjust OFFSET by the partial bits, then align.  */
+		  tree d = size_binop (CEIL_DIV_EXPR, rli->bitpos,
+				       bitsize_unit_node);
+		  rli->offset = size_binop (PLUS_EXPR, rli->offset,
+					    fold_convert (sizetype, d));
+		  rli->bitpos = bitsize_zero_node;
+
+		  rli->offset = round_up (rli->offset,
+					  desired_align / BITS_PER_UNIT);
+		}
+
+	      if (! TREE_CONSTANT (rli->offset))
+		rli->offset_align = desired_align;
+	    }
+
 	  normalize_rli (rli);
         }
 
@@ -1530,7 +1563,7 @@  place_field (record_layout_info rli, tre
       if (!DECL_BIT_FIELD_TYPE (field)
 	  || (prev_saved != NULL
 	      ? !simple_cst_equal (TYPE_SIZE (type), TYPE_SIZE (prev_type))
-	      : !integer_zerop (DECL_SIZE (field)) ))
+	      : !integer_zerop (DECL_SIZE (field))))
 	{
 	  /* Never smaller than a byte for compatibility.  */
 	  unsigned int type_align = BITS_PER_UNIT;
@@ -1555,7 +1588,8 @@  place_field (record_layout_info rli, tre
 	    }
 
 	  /* Now align (conventionally) for the new type.  */
-	  type_align = TYPE_ALIGN (TREE_TYPE (field));
+	  if (! DECL_PACKED (field))
+	    type_align = TYPE_ALIGN (TREE_TYPE (field));
 
 	  if (maximum_field_alignment != 0)
 	    type_align = MIN (type_align, maximum_field_alignment);
--- gcc/testsuite/gcc.dg/bf-ms-layout.c.jj	2017-06-20 21:38:01.634906200 +0200
+++ gcc/testsuite/gcc.dg/bf-ms-layout.c	2018-02-27 18:04:06.923851839 +0100
@@ -153,27 +153,27 @@  int main(){
   struct ten test_ten;
 
 #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER)
-  size_t exp_sizeof_one = 8;
-  size_t exp_sizeof_two = 12;
+  size_t exp_sizeof_one = 12;
+  size_t exp_sizeof_two = 16;
   size_t exp_sizeof_three =6;
   size_t exp_sizeof_four = 8;
   size_t exp_sizeof_five = 3;
   size_t exp_sizeof_six = 8;
   size_t exp_sizeof_seven = 3;
-  size_t exp_sizeof_eight = 2;
+  size_t exp_sizeof_eight = 4;
   size_t exp_sizeof_nine = 8;
-  size_t exp_sizeof_ten = 8;
+  size_t exp_sizeof_ten = 16;
 
-  unsigned char exp_one_c = 7;
-  unsigned char exp_two_c  = 9;
+  unsigned char exp_one_c = 8;
+  unsigned char exp_two_c  = 12;
   unsigned char exp_three_c = 4;
   unsigned char exp_four_c = 4;
   char exp_five_c = 2;
   char exp_six_c = 5;
   char exp_seven_c = 2;
-  char exp_eight_c = 1;
+  char exp_eight_c = 2;
   char exp_nine_c = 0;
-  char exp_ten_c = 1;
+  char exp_ten_c = 8;
 
 #else /* testing -mno-ms-bitfields */
 
--- gcc/testsuite/gcc.dg/bf-ms-layout-2.c.jj	2017-06-20 21:38:02.112900704 +0200
+++ gcc/testsuite/gcc.dg/bf-ms-layout-2.c	2018-02-27 18:04:06.923851839 +0100
@@ -158,27 +158,27 @@  int main(){
   struct ten test_ten;
 
 #if defined (_TEST_MS_LAYOUT) || defined (_MSC_VER)
-  size_t exp_sizeof_one = 8;
-  size_t exp_sizeof_two = 12;
+  size_t exp_sizeof_one = 12;
+  size_t exp_sizeof_two = 16;
   size_t exp_sizeof_three =6;
   size_t exp_sizeof_four = 8;
   size_t exp_sizeof_five = 3;
   size_t exp_sizeof_six = 8;
   size_t exp_sizeof_seven = 3;
-  size_t exp_sizeof_eight = 2;
+  size_t exp_sizeof_eight = 4;
   size_t exp_sizeof_nine = 8;
-  size_t exp_sizeof_ten = 8;
+  size_t exp_sizeof_ten = 16;
 
-  unsigned char exp_one_c = 7;
-  unsigned char exp_two_c  = 9;
+  unsigned char exp_one_c = 8;
+  unsigned char exp_two_c  = 12;
   unsigned char exp_three_c = 4;
   unsigned char exp_four_c = 4;
   char exp_five_c = 2;
   char exp_six_c = 5;
   char exp_seven_c = 2;
-  char exp_eight_c = 1;
+  char exp_eight_c = 2;
   char exp_nine_c = 0;
-  char exp_ten_c = 1;
+  char exp_ten_c = 8;
 
 #else /* testing -mno-ms-bitfields */
 
--- gcc/testsuite/gcc.dg/bf-ms-layout-4.c.jj	2018-02-27 18:09:09.544421580 +0100
+++ gcc/testsuite/gcc.dg/bf-ms-layout-4.c	2018-02-27 18:18:00.845039925 +0100
@@ -0,0 +1,43 @@ 
+/* PR target/52991 */
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+
+#define CHECK(expr) extern char c[(expr) ? 1 : -1]
+#define offsetof(x, y) __builtin_offsetof (x, y)
+
+struct test_sp1 {
+    int a;
+    short b;
+    int c;
+    char d;
+} __attribute__((packed,ms_struct));
+
+CHECK (sizeof (struct test_sp1) == 11);
+CHECK (offsetof (struct test_sp1, a) == 0);
+CHECK (offsetof (struct test_sp1, b) == 4);
+CHECK (offsetof (struct test_sp1, c) == 6);
+CHECK (offsetof (struct test_sp1, d) == 10);
+
+struct test_sp3 {
+    int a;
+    short b __attribute__((aligned(8)));
+    int c;
+    char d;
+} __attribute__((packed,ms_struct));
+
+CHECK (sizeof (struct test_sp3) == 16);
+CHECK (offsetof (struct test_sp3, a) == 0);
+CHECK (offsetof (struct test_sp3, b) == 8);
+CHECK (offsetof (struct test_sp3, c) == 10);
+CHECK (offsetof (struct test_sp3, d) == 14);
+
+struct test_s4 {
+    int a;
+    short b;
+    int c:15;
+    char d;
+} __attribute__((ms_struct));
+
+CHECK (sizeof (struct test_s4) == 16);
+CHECK (offsetof (struct test_s4, a) == 0);
+CHECK (offsetof (struct test_s4, b) == 4);
+CHECK (offsetof (struct test_s4, d) == 12);
--- gcc/testsuite/gcc.dg/bf-ms-layout-5.c.jj	2018-02-27 18:31:24.043753173 +0100
+++ gcc/testsuite/gcc.dg/bf-ms-layout-5.c	2018-02-27 18:35:29.825676223 +0100
@@ -0,0 +1,45 @@ 
+/* PR target/52991 */
+/* { dg-do run { target i?86-*-* x86_64-*-* } } */
+
+struct S {
+  int a : 2;
+  __attribute__((aligned (8))) int b : 2;
+  int c : 28;
+  __attribute__((aligned (16))) int d : 2;
+  int e : 30;
+} __attribute__((ms_struct));
+
+struct S s;
+
+int
+main ()
+{
+  int i;
+  if (sizeof (s) != 32)
+    __builtin_abort ();
+  s.a = -1;
+  for (i = 0; i < 32; ++i)
+    if (((char *) &s)[i] != (i ? 0 : 3))
+      __builtin_abort ();
+  s.a = 0;
+  s.b = -1;
+  for (i = 0; i < 32; ++i)
+    if (((char *) &s)[i] != (i ? 0 : 12))
+      __builtin_abort ();
+  s.b = 0;
+  s.c = -1;
+  for (i = 0; i < 32; ++i)
+    if (((signed char *) &s)[i] != (i > 3 ? 0 : (i ? -1 : -16)))
+      __builtin_abort ();
+  s.c = 0;
+  s.d = -1;
+  for (i = 0; i < 32; ++i)
+    if (((signed char *) &s)[i] != (i == 16 ? 3 : 0))
+      __builtin_abort ();
+  s.d = 0;
+  s.e = -1;
+  for (i = 0; i < 32; ++i)
+    if (((signed char *) &s)[i] != ((i < 16 || i > 19) ? 0 : (i == 16 ? -4 : -1)))
+      __builtin_abort ();
+  return 0;
+}