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

Richard Biener richard.guenther@gmail.com
Thu Dec 13 10:04:00 GMT 2012


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



More information about the Gcc-patches mailing list