| Message ID | CAEwic4Z267bg-HFtHRBxJG26m3BScn+0rztNr1jhTzF+K0WmQg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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; > +}
> 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.
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;