Patchwork [c/c++] : Fix for PR c/52991 issue about ignored packed-attribute for ms-structure-layout

login
register
mail settings
Submitter Kai Tietz
Date Dec. 13, 2012, 12:02 p.m.
Message ID <CAEwic4Z267bg-HFtHRBxJG26m3BScn+0rztNr1jhTzF+K0WmQg@mail.gmail.com>
Download mbox | patch
Permalink /patch/205817/
State New
Headers show

Comments

Kai Tietz - Dec. 13, 2012, 12:02 p.m.
2012/12/13 Jakub Jelinek <jakub@redhat.com>:
> On Thu, Dec 13, 2012 at 11:29:47AM +0100, Richard Biener wrote:
>> On Thu, Dec 13, 2012 at 11:07 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Thu, Dec 13, 2012 at 11:03:58AM +0100, Richard Biener wrote:
>> >> struct X
>> >> {
>> >>   char c;
>> >>   short s;
>> >>   char c2;
>> >>   short s2;
>> >> } __attribute__((packed,aligned(2)));
>> >
>> > As struct-layout-1.exp tests show, this is something that was ABI-wise
>> > changed already several times.  That said, for non-ms-bitfield-layout I'd
>> > strongly prefer if we could avoid yet another ABI change for it.
>>
>> Probably not exactly this case - 2.95, 3.2, 3.3, 4.1 ... all show the same
>> behavior.  And Kai should have seen regressions, no?
>
> Maybe my memory is too weak, dunno if it was packed,aligned(N) or aligned(N),packed,
> but I remember seeing FAILs in such tests when testing against older
> compilers.

Well, I see the point about having packed and aligned in combination
only for final-element in struct/union.  As packed enforces unaligned
accesses due it tries to save memory-use.  If an user alignment is
present the packing can just happen on last member for struct, as
otherwise the alignment has to be applied and padding happens.

But I agree that I didn't intended to change sysv_abi here by this
patch.  So I added to the change in start_record_layout a check to
ms-bitfields use.

2012-12-13  Kai Tietz

        PR c/52991
        * stor-layout.c (start_record_layout): Handle
        packed-attribute for ms-structure-layout.
        (update_alignment_for_field): Likewise.
        (place_field): Likewise.

2012-12-12  Kai Tietz

        PR c/52991
	* gcc.dg/attr-ms_struct-packed2.c: New file.
	* gcc.dg/attr-ms_struct-packed3.c: New file.

Ok for apply?

Regards,
Kai

+}
Richard Guenther - Dec. 13, 2012, 12:09 p.m.
On Thu, Dec 13, 2012 at 1:02 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2012/12/13 Jakub Jelinek <jakub@redhat.com>:
>> On Thu, Dec 13, 2012 at 11:29:47AM +0100, Richard Biener wrote:
>>> On Thu, Dec 13, 2012 at 11:07 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> > On Thu, Dec 13, 2012 at 11:03:58AM +0100, Richard Biener wrote:
>>> >> struct X
>>> >> {
>>> >>   char c;
>>> >>   short s;
>>> >>   char c2;
>>> >>   short s2;
>>> >> } __attribute__((packed,aligned(2)));
>>> >
>>> > As struct-layout-1.exp tests show, this is something that was ABI-wise
>>> > changed already several times.  That said, for non-ms-bitfield-layout I'd
>>> > strongly prefer if we could avoid yet another ABI change for it.
>>>
>>> Probably not exactly this case - 2.95, 3.2, 3.3, 4.1 ... all show the same
>>> behavior.  And Kai should have seen regressions, no?
>>
>> Maybe my memory is too weak, dunno if it was packed,aligned(N) or aligned(N),packed,
>> but I remember seeing FAILs in such tests when testing against older
>> compilers.
>
> Well, I see the point about having packed and aligned in combination
> only for final-element in struct/union.  As packed enforces unaligned
> accesses due it tries to save memory-use.  If an user alignment is
> present the packing can just happen on last member for struct, as
> otherwise the alignment has to be applied and padding happens.
>
> But I agree that I didn't intended to change sysv_abi here by this
> patch.  So I added to the change in start_record_layout a check to
> ms-bitfields use.
>
> 2012-12-13  Kai Tietz
>
>         PR c/52991
>         * stor-layout.c (start_record_layout): Handle
>         packed-attribute for ms-structure-layout.
>         (update_alignment_for_field): Likewise.
>         (place_field): Likewise.
>
> 2012-12-12  Kai Tietz
>
>         PR c/52991
>         * gcc.dg/attr-ms_struct-packed2.c: New file.
>         * gcc.dg/attr-ms_struct-packed3.c: New file.
>
> Ok for apply?
>
> Regards,
> Kai
>
> Index: gcc/gcc/stor-layout.c
> ===================================================================
> --- gcc.orig/gcc/stor-layout.c
> +++ gcc/gcc/stor-layout.c
> @@ -756,7 +756,10 @@ start_record_layout (tree t)
>    /* If the type has a minimum specified alignment (via an attribute
>       declaration, for example) use it -- otherwise, start with a
>       one-byte alignment.  */
> -  rli->record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (t));
> +  if (targetm.ms_bitfield_layout_p (t) && TYPE_PACKED (t))
> +    rli->record_align = BITS_PER_UNIT;
> +  else
> +    rli->record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (t));

I think this hunk still just shows that the ms-bitfield-code fails to
handle TYPE_PACKED properly.  Simply adjusting rli->record_align
does not look like the correct solution to me.

Btw, did you test struct-layout against 4.7 with ms-bitfields as Jakub
noted how to do that?

Btw, any ABI changes should be documented in changes.html at least.

Richard.

>    rli->unpacked_align = rli->record_align;
>    rli->offset_align = MAX (rli->record_align, BIGGEST_ALIGNMENT);
>
> @@ -952,15 +955,20 @@ update_alignment_for_field (record_layou
>       meaningless.  */
>    if (targetm.ms_bitfield_layout_p (rli->t))
>      {
> +      if (rli->t && TYPE_PACKED (rli->t)
> +          && (is_bitfield || !DECL_PACKED (field)
> +              || DECL_SIZE (field) == NULL_TREE
> +              || !integer_zerop (DECL_SIZE (field))))
> +        desired_align = BITS_PER_UNIT;
>        /* 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, nonzero-size
>          bitfield.  (That's the way it is, experimentally.) */
> -      if ((!is_bitfield && !DECL_PACKED (field))
> -         || ((DECL_SIZE (field) == NULL_TREE
> -              || !integer_zerop (DECL_SIZE (field)))
> +      else if ((!is_bitfield && !DECL_PACKED (field))
> +              || ((DECL_SIZE (field) == NULL_TREE
> +                  || !integer_zerop (DECL_SIZE (field)))
>               ? !DECL_PACKED (field)
>               : (rli->prev_field
>                  && DECL_BIT_FIELD_TYPE (rli->prev_field)
> @@ -1414,7 +1422,12 @@ place_field (record_layout_info rli, tre
>             }
>
>           /* Now align (conventionally) for the new type.  */
> -         type_align = TYPE_ALIGN (TREE_TYPE (field));
> +         if (!TYPE_PACKED (rli->t))
> +           {
> +             type_align = TYPE_ALIGN (TREE_TYPE (field));
> +             if (DECL_PACKED (field))
> +               type_align = MIN (type_align, BITS_PER_UNIT);
> +           }
>
>           if (maximum_field_alignment != 0)
>             type_align = MIN (type_align, maximum_field_alignment);
> Index: gcc/gcc/testsuite/gcc.dg/attr-ms_struct-packed2.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.dg/attr-ms_struct-packed2.c
> @@ -0,0 +1,31 @@
> +/* Test for MS structure with packed attribute.  */
> +/* { dg-do run { target *-*-interix* *-*-mingw* *-*-cygwin*
> i?86-*-darwin* i?86-*-linux* x86_64-*-linux* } }
> +/* { dg-options "-std=gnu99" } */
> +
> +extern void abort ();
> +
> +struct A {
> +    short s;
> +    struct { int i; } __attribute__((__ms_struct__));
> +} __attribute__((__ms_struct__, __packed__));
> +
> +struct B {
> +    short s;
> +    struct { int i; } __attribute__((__ms_struct__, __packed__));
> +} __attribute__((__ms_struct__, __packed__));
> +
> +struct C {
> +    struct { int i; } __attribute__((__ms_struct__));
> +    short s;
> +} __attribute__((__ms_struct__, __packed__));
> +
> +int
> +main (void)
> +{
> +  if (sizeof (struct C) != sizeof (struct B)
> +      || sizeof (struct A) != sizeof (struct B)
> +      || sizeof (struct B) != 6)
> +    abort ();
> +
> +  return 0;
> +}
Eric Botcazou - Dec. 15, 2012, 11:17 a.m.
> I think this hunk still just shows that the ms-bitfield-code fails to
> handle TYPE_PACKED properly.  Simply adjusting rli->record_align
> does not look like the correct solution to me.
> 
> Btw, did you test struct-layout against 4.7 with ms-bitfields as Jakub
> noted how to do that?

Ideally this should be done against the vendor compiler as well, since all 
this is done in the name of binary compatibility with it.

> Btw, any ABI changes should be documented in changes.html at least.

Definitely, and with examples if possible.

Patch

Index: gcc/gcc/stor-layout.c
===================================================================
--- gcc.orig/gcc/stor-layout.c
+++ gcc/gcc/stor-layout.c
@@ -756,7 +756,10 @@  start_record_layout (tree t)
   /* If the type has a minimum specified alignment (via an attribute
      declaration, for example) use it -- otherwise, start with a
      one-byte alignment.  */
-  rli->record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (t));
+  if (targetm.ms_bitfield_layout_p (t) && TYPE_PACKED (t))
+    rli->record_align = BITS_PER_UNIT;
+  else
+    rli->record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (t));
   rli->unpacked_align = rli->record_align;
   rli->offset_align = MAX (rli->record_align, BIGGEST_ALIGNMENT);

@@ -952,15 +955,20 @@  update_alignment_for_field (record_layou
      meaningless.  */
   if (targetm.ms_bitfield_layout_p (rli->t))
     {
+      if (rli->t && TYPE_PACKED (rli->t)
+          && (is_bitfield || !DECL_PACKED (field)
+              || DECL_SIZE (field) == NULL_TREE
+              || !integer_zerop (DECL_SIZE (field))))
+        desired_align = BITS_PER_UNIT;
       /* 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, nonzero-size
 	 bitfield.  (That's the way it is, experimentally.) */
-      if ((!is_bitfield && !DECL_PACKED (field))
-	  || ((DECL_SIZE (field) == NULL_TREE
-	       || !integer_zerop (DECL_SIZE (field)))
+      else if ((!is_bitfield && !DECL_PACKED (field))
+	       || ((DECL_SIZE (field) == NULL_TREE
+		   || !integer_zerop (DECL_SIZE (field)))
 	      ? !DECL_PACKED (field)
 	      : (rli->prev_field
 		 && DECL_BIT_FIELD_TYPE (rli->prev_field)
@@ -1414,7 +1422,12 @@  place_field (record_layout_info rli, tre
 	    }

 	  /* Now align (conventionally) for the new type.  */
-	  type_align = TYPE_ALIGN (TREE_TYPE (field));
+	  if (!TYPE_PACKED (rli->t))
+	    {
+	      type_align = TYPE_ALIGN (TREE_TYPE (field));
+	      if (DECL_PACKED (field))
+	        type_align = MIN (type_align, BITS_PER_UNIT);
+	    }

 	  if (maximum_field_alignment != 0)
 	    type_align = MIN (type_align, maximum_field_alignment);
Index: gcc/gcc/testsuite/gcc.dg/attr-ms_struct-packed2.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.dg/attr-ms_struct-packed2.c
@@ -0,0 +1,31 @@ 
+/* Test for MS structure with packed attribute.  */
+/* { dg-do run { target *-*-interix* *-*-mingw* *-*-cygwin*
i?86-*-darwin* i?86-*-linux* x86_64-*-linux* } }
+/* { dg-options "-std=gnu99" } */
+
+extern void abort ();
+
+struct A {
+    short s;
+    struct { int i; } __attribute__((__ms_struct__));
+} __attribute__((__ms_struct__, __packed__));
+
+struct B {
+    short s;
+    struct { int i; } __attribute__((__ms_struct__, __packed__));
+} __attribute__((__ms_struct__, __packed__));
+
+struct C {
+    struct { int i; } __attribute__((__ms_struct__));
+    short s;
+} __attribute__((__ms_struct__, __packed__));
+
+int
+main (void)
+{
+  if (sizeof (struct C) != sizeof (struct B)
+      || sizeof (struct A) != sizeof (struct B)
+      || sizeof (struct B) != 6)
+    abort ();
+
+  return 0;