| Message ID | CAEwic4ZnX2ZiDhMsgXoXoxZEx=u+Fm7CpgjeGrdL2qPrrjw9jg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 11, 2012 at 11:04 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > Hello, > > This fixes an old regression about ms-structure-layout in combination > with packed-attribute. > > ChangeLog > > 2012-12-11 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. > > > Tested for i686-w64-mingw32, x86_64-w64-mingw32, and for > x86_64-unknown-linux-gnu. Ok for apply? Please add a testcase or two. > Regards, > Kai > > Index: stor-layout.c > =================================================================== > --- stor-layout.c (Revision 194386) > +++ stor-layout.c (Arbeitskopie) > @@ -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 (TYPE_PACKED (t)) > + rli->record_align = BITS_PER_UNIT; > + else > + rli->record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (t)); That looks wrong. Having both TYPE_PACKED and TYPE_ALIGN != BITS_PER_UNIT is inconsistent, so this part of the patch should not be necessary. > rli->unpacked_align = rli->record_align; > rli->offset_align = MAX (rli->record_align, BIGGEST_ALIGNMENT); > > @@ -952,15 +955,20 @@ update_alignment_for_field (record_layout_info rli > 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; See above - this looks redundant. > /* 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,13 @@ place_field (record_layout_info rli, tree field) > } > > /* 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);
On 12/12/2012 02:57 AM, Richard Biener wrote: > That looks wrong. Having both TYPE_PACKED and TYPE_ALIGN != BITS_PER_UNIT > is inconsistent, so this part of the patch should not be necessary. No, that is the only way to give a 4 byte int 2 byte alignment: use both packed and aligned attributes. struct S { char x; int y; }; struct T { char x; int y __attribute__((aligned(2))); }; struct U { char x; int y __attribute__((packed, aligned(2))); }; int s_y = __builtin_offsetof(struct S, y); int t_y = __builtin_offsetof(struct T, y); int u_y = __builtin_offsetof(struct U, y); r~
On Wed, Dec 12, 2012 at 4:11 PM, Richard Henderson <rth@redhat.com> wrote: > On 12/12/2012 02:57 AM, Richard Biener wrote: >> That looks wrong. Having both TYPE_PACKED and TYPE_ALIGN != BITS_PER_UNIT >> is inconsistent, so this part of the patch should not be necessary. > > No, that is the only way to give a 4 byte int 2 byte alignment: > use both packed and aligned attributes. > > struct S { > char x; > int y; > }; > > struct T { > char x; > int y __attribute__((aligned(2))); > }; > > struct U { > char x; > int y __attribute__((packed, aligned(2))); > }; > > int s_y = __builtin_offsetof(struct S, y); > int t_y = __builtin_offsetof(struct T, y); > int u_y = __builtin_offsetof(struct U, y); But the patch changes it for the RECORD_TYPE case and struct T __attribute__((packed,aligned(2))) { char x; short s; char y; short a; }; struct T x; doesn't work as I would have expected (giving x.x and x.a 2-byte alignment). In fact, the type attribute docs for 'packed' only say that fields are packed, not that alignment of the type itself is affected (and 'aligned' is not specifed in the docs for types at all it seems). Richard. > > r~
2012/12/12 Richard Biener <richard.guenther@gmail.com>: > On Wed, Dec 12, 2012 at 4:11 PM, Richard Henderson <rth@redhat.com> wrote: >> On 12/12/2012 02:57 AM, Richard Biener wrote: >>> That looks wrong. Having both TYPE_PACKED and TYPE_ALIGN != BITS_PER_UNIT >>> is inconsistent, so this part of the patch should not be necessary. >> >> No, that is the only way to give a 4 byte int 2 byte alignment: >> use both packed and aligned attributes. >> >> struct S { >> char x; >> int y; >> }; >> >> struct T { >> char x; >> int y __attribute__((aligned(2))); >> }; >> >> struct U { >> char x; >> int y __attribute__((packed, aligned(2))); >> }; >> >> int s_y = __builtin_offsetof(struct S, y); >> int t_y = __builtin_offsetof(struct T, y); >> int u_y = __builtin_offsetof(struct U, y); > > But the patch changes it for the RECORD_TYPE case and > > struct T __attribute__((packed,aligned(2))) { > char x; > short s; > char y; > short a; > }; > struct T x; > > doesn't work as I would have expected (giving x.x and x.a 2-byte alignment). > > In fact, the type attribute docs for 'packed' only say that fields are packed, > not that alignment of the type itself is affected (and 'aligned' is not specifed > in the docs for types at all it seems). > > Richard. Hmm, I see the attribute aligned explicit mention for types. See 5.33 Specifying Attributes of Types. Well, the case to combine aligned and packed attribute seems indeed not to be explicit mentioned. Nevertheless documention tells for packed-attribute for types "This attribute, attached to `struct' or `union' type definition, specifies that each member of the structure or union is placed to minimize the memory required.", which implies - I might be wrong here - that an alignment of 1 is active by default. So to put those two attributes wiithin one attribute doesn't make sense, as either the aligned or the packed have to be interpreted. To specify within a packed-struct-type for a sepcific variable a special alignment - as shown in Rth's testcase - makes sense and seems to be covered by the docs. Regards, Kai
On Wed, Dec 12, 2012 at 5:23 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2012/12/12 Richard Biener <richard.guenther@gmail.com>: >> On Wed, Dec 12, 2012 at 4:11 PM, Richard Henderson <rth@redhat.com> wrote: >>> On 12/12/2012 02:57 AM, Richard Biener wrote: >>>> That looks wrong. Having both TYPE_PACKED and TYPE_ALIGN != BITS_PER_UNIT >>>> is inconsistent, so this part of the patch should not be necessary. >>> >>> No, that is the only way to give a 4 byte int 2 byte alignment: >>> use both packed and aligned attributes. >>> >>> struct S { >>> char x; >>> int y; >>> }; >>> >>> struct T { >>> char x; >>> int y __attribute__((aligned(2))); >>> }; >>> >>> struct U { >>> char x; >>> int y __attribute__((packed, aligned(2))); >>> }; >>> >>> int s_y = __builtin_offsetof(struct S, y); >>> int t_y = __builtin_offsetof(struct T, y); >>> int u_y = __builtin_offsetof(struct U, y); >> >> But the patch changes it for the RECORD_TYPE case and >> >> struct T __attribute__((packed,aligned(2))) { >> char x; >> short s; >> char y; >> short a; >> }; >> struct T x; >> >> doesn't work as I would have expected (giving x.x and x.a 2-byte alignment). >> >> In fact, the type attribute docs for 'packed' only say that fields are packed, >> not that alignment of the type itself is affected (and 'aligned' is not specifed >> in the docs for types at all it seems). >> >> Richard. > > Hmm, I see the attribute aligned explicit mention for types. See 5.33 > Specifying Attributes of Types. > Well, the case to combine aligned and packed attribute seems indeed > not to be explicit mentioned. Nevertheless documention tells for > packed-attribute for types "This attribute, attached to `struct' or > `union' type definition, specifies that each member of the structure > or union is placed to minimize the memory required.", which implies - > I might be wrong here - that an alignment of 1 is active by default. > So to put those two attributes wiithin one attribute doesn't make > sense, as either the aligned or the packed have to be interpreted. To > specify within a packed-struct-type for a sepcific variable a special > alignment - as shown in Rth's testcase - makes sense and seems to be > covered by the docs. Rth's case is covered by the decl attribute section which explicitely specifies the behavior of mixing aligned and packed. But you are checking packed on a type. I am talking about struct X { char c; short s; char c2; short s2; } __attribute__((packed,aligned(2))); struct X *x; int main() { __builtin_printf ("%d, %d, %d\n", __alignof (*x), __alignof__ (x->c), __alignof__ (x->s2)); return 0; } which is where you'd get TYPE_PACKED set on struct X but also the aligned attribute: Breakpoint 5, start_record_layout (t=0x7ffff68fc3f0) at /space/rguenther/src/svn/trunk/gcc/stor-layout.c:752 752 record_layout_info rli = XNEW (struct record_layout_info_s); (gdb) call debug_tree (t) <record_type 0x7ffff68fc3f0 X packed type_0 VOID user align 16 symtab 0 alias set -1 canonical type 0x7ffff68fc3f0 your patch does change rli->record_align from 16 to 8 independent of ms-bitfield-layout being active or not. Of course the testcase doesn't behave as optimistic as I would expect: > ./t 2, 1, 1 but your patch I believe would make it print > ./t 1, 1, 1 I would have expected > ./t 2, 2, 2 (same actual layout but less conservative DECL_ALIGN on the fields which simply have align of 1 and DECL_PACKED set). As you said, the docs on the packed _type_ attribute merely says fields get packed densely, it does _not_ say that all fields get alignment 1! At least, my testcase changes behavior with your patch, independent on ms-bitfield-layout (derived from reading the patch, not from applying and checking). Joseph, what would you say is documented behavior for my testcase? Is it also desired behavior? Thanks, Richard. > Regards, > Kai
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. Jakub
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? Richard. > Jakub
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. By default it tests just current gcc vs. itself, one needs ALT_CC_UNDER_TEST=gcc ALT_CXX_UNDER_TEST=g++ or so to test against system compiler. Jakub
Index: stor-layout.c =================================================================== --- stor-layout.c (Revision 194386) +++ stor-layout.c (Arbeitskopie) @@ -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 (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_layout_info rli 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,13 @@ place_field (record_layout_info rli, tree field) } /* 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);