On Wed, Dec 12, 2012 at 5:23 PM, Kai Tietz <ktiet...@googlemail.com> wrote:
> 2012/12/12 Richard Biener <richard.guent...@gmail.com>:
>> On Wed, Dec 12, 2012 at 4:11 PM, Richard Henderson <r...@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
