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 <w...@mail.berlios.de> wrote: >>>>> Am 28.08.2011 23:43, schrieb Blue Swirl: >>>>>> >>>>>> On Sun, Aug 28, 2011 at 8:43 PM, Stefan Weil <w...@mail.berlios.de> >>>>>> 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
