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

[Blue Swirl]

On Tue, Aug 30, 2011 at 6:29 PM, Alexander Graf <address@hidden> wrote:
>
> 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 :)

I'm leaning slightly towards mechanical approach for consistency.