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. 11, 2012, 10:04 p.m.
Message ID <CAEwic4ZnX2ZiDhMsgXoXoxZEx=u+Fm7CpgjeGrdL2qPrrjw9jg@mail.gmail.com>
Download mbox | patch
Permalink /patch/205314/
State New
Headers show

Comments

Kai Tietz - Dec. 11, 2012, 10:04 p.m.
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?

Regards,
Kai
Richard Guenther - Dec. 12, 2012, 10:57 a.m.
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);
Richard Henderson - Dec. 12, 2012, 3:11 p.m.
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~
Richard Guenther - Dec. 12, 2012, 3:49 p.m.
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~
Kai Tietz - Dec. 12, 2012, 4:23 p.m.
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
Richard Guenther - Dec. 13, 2012, 10:03 a.m.
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
Jakub Jelinek - Dec. 13, 2012, 10:07 a.m.
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
Richard Guenther - Dec. 13, 2012, 10:29 a.m.
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
Jakub Jelinek - Dec. 13, 2012, 10:56 a.m.
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

Patch

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