On Thu, Dec 13, 2012 at 1:02 PM, Kai Tietz <ktiet...@googlemail.com> wrote:
> 2012/12/13 Jakub Jelinek <ja...@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 <ja...@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;
> +}
