Re: [Qemu-devel] [PATCH 0/7] Fix packing for MinGW with -mms-bitfields

[Alexander Graf]

On 30.08.2011, at 19:25, Stefan Weil wrote:

> Am 30.08.2011 09:44, schrieb Kevin Wolf:
>> Am 29.08.2011 21:55, schrieb Stefan Weil:
>>> Am 29.08.2011 10:34, schrieb TeLeMan:
>>>> On Mon, Aug 29, 2011 at 13:01, Stefan Weil <address@hidden> wrote:
>>>>> Am 28.08.2011 23:43, schrieb Blue Swirl:
>>>>>> 
>>>>>> On Sun, Aug 28, 2011 at 8:43 PM, Stefan Weil <address@hidden>
>>>>>> wrote:
>>>>>>> 
>>>>>>> These patches fix the packing of structures which were affected by
>>>>>>> the new compiler attribute -mms-bitfields (which is needed for
>>>>>>> glib-2.0).
>>>>>>> 
>>>>>>> I compiled qemu.exe with and without -mms-bitfields and compared
>>>>>>> the resulting struct alignment using pahole and codiff.
>>>>>> 
>>>>>> If a structure is only used internally by QEMU (not used in network,
>>>>>> disk or guest interfaces), changes in padding don't matter. In fact,
>>>>>> in those cases it may be better to remove the packing, because then
>>>>>> the fields may be naturally aligned and that gives better performance
>>>>>> on most architectures. Could you please check if this is the case for
>>>>>> any of the structs?
>>>>> 
>>>>> I did this already, but also forward your question to the maintainers.
>>>>> Here is my result:
>>>>> 
>>>>> [PATCH 2/7] block/vvfat: Fix packing for w32: needs packing (disk)
>>>>> [PATCH 3/7] acpi: Fix packing for w32: needs packing (bios interface)
>>>>> [PATCH 4/7] hpet: Fix packing for w32: needs packing (bios interface)
>>>>> [PATCH 5/7] usb: Fix packing for w32: needs packing (usb interface)
>>>>> [PATCH 6/7] virtio: Fix packing for w32: needs packing? (guest
>>>>> interface?)
>>>>> [PATCH 7/7] slirp: Fix packing for w32: needs packing (network interface)
>>>>> 
>>>>> All those struct statements need the pack attribute (otherwise the code
>>>>> would have to be rewritten which is of course always possible).
>>>> gesn_cdb in atapi.c, VMDK4Header in vmdk.c and many structures in
>>>> bt.h need be fixed too.
>>> 
>>> Oops, you are right. Obviously I missed all anonymous structs:
>>> codiff simply ignores them, and pahole must be called with
>>> flags -a -A to show them. Who invented packing of structs?
>>> 
>>> Comparing the output of pahole -a -A is less elegant than using
>>> codiff, but shows the structs which you mentioned.
>>> 
>>> I suggest to apply my patch series first because it fixes
>>> the most important bugs in networking. The remaining
>>> bugs are in code which is used less often. They will be
>>> fixed by a second patch series which replaces all remaining
>>> packed attributes.
>> 
>> Shouldn't we have a look at every packed structure instead of just
>> fixing what we notice as broken in the x86 emulator binary with one
>> given configuration? I think if there is a QEMU_PACKED, we should use it
>> consistently, or is there a reason not to do so?
>> 
>> Kevin
> 
> Hi Kevin,
> 
> yes, we should use QEMU_PACKED instead of any __attribute__((packed)).
> 
> The first 7 patches simply introduce QEMU_PACKED
> and fix the most important bugs for those users who run
> QEMU on Windows. There was only a bug report for broken
> networking (fixed by Jan's committed patch and the above
> slirp patch). These fixes work for all targets, so
> chances are good that Windows users will have
> working binaries for the commonly used scenarios with
> any target - although I only examined qemu.exe.
> 
> For this reason, these patches should be applied to git
> master as soon as possible.
> 
> I did not intend to have a look at every packed structure
> as was suggested by Alex, Blue and others.
> I simply wanted to run a global replace (perl -pi -e ...)
> which replaced the remaining __attributes__.
> 
> Reviewing every __attribute__ takes much more time of course:
> there are more than 250 of them.
> I don't think that a review is really necessary, because usually
> "packed" is not added just for fun, and most QEMU code
> was already reviewed. A small rate of unnecessary QEMU_PACKED
> would do no harm, because only performance suffers a little.
> 
> If more people agreed that QEMU_PACKED can be introduced
> mechanically by a script without a new review, I could send
> a patch very soon.

I think that's the better approach to the partial commit. Just introduce 
QEMU_PACKED, provide the script/sed cmdline you ran over the tree and replace 
it in every file. That makes more sense to commit than the partial conversion.

But please wait for a second opinion here :)


Alex