Re: [PATCH 2/5] xen/common: Cleanup use of __attribute__((packed))

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/03/14 10:40, Jan Beulich wrote:
>>>> On 13.03.14 at 11:22, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 13/03/14 08:15, Jan Beulich wrote:
>>>>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/common/trace.c
>>>> +++ b/xen/common/trace.c
>>>> @@ -641,11 +641,11 @@ static inline void insert_wrap_record(struct t_buf 
>>>> *buf,
>>>>  
>>>>  static inline void insert_lost_records(struct t_buf *buf)
>>>>  {
>>>> -    struct {
>>>> +    struct __packed {
>>>>          u32 lost_records;
>>>>          u32 did:16, vid:16;
>>>>          u64 first_tsc;
>>>> -    } __attribute__((packed)) ed;
>>>> +    } ed;
>>> So why did you not strip this one in the previous patch?
>> My reading of a recent C spec draft would indicate that the compiler is
>> perfectly at liberty to expand these :16 bitfields up 32 bits each, if
>> it feels like doing so.
> Which would then be better addressed by changing them both
> to u16, dropping the bit fields altogether.
>
> But I don't think the liberty given to a compiler is that wide: "An
> implementation may allocate any addressable storage unit large
> enough to hold a bitfield. If enough space remains, a bit-field
> that immediately follows another bit-field in a structure shall be
> packed into adjacent bits of the same unit."
>
> I.e. the compiler has basically two choices: Use a 2-byte storage
> unit for each of them, or use a 4-byte storage unit and put them
> both in there. The end result is the same. What you're concerned
> about can only happen when crossing storage unit boundaries.
>
> Jan
>

Ok - I will switch them to u16 and drop the __packed.

~Andrew