Re: Supporting clang on windows

[Pierrick Bouvier]

On 11/24/24 04:10, Paolo Bonzini wrote:
> On 11/24/24 01:21, Pierrick Bouvier wrote:
> > After thinking about it, a simple, exhaustive and reliable way to find
> > this type information is the debug (dwarf) info.
> > By compiling qemu binaries with --enable-debug, and extracting info
> > using llvm-dwarfdump plus a custom filter [4], we can obtain a text
> > representation of all structures QEMU uses.
>
> Yes, this is a good idea.
>
> > As there is a lot of repetition between all qemu binaries, the reduced
> > list of structs concerned is [6]:
> > +name:ArduinoMachineClass size:0x0198
> > +name:ARMCacheAttrs size:0x04
> > +name:ARMVAParameters size:0x04
> > +name:AspeedMachineClass size:0x01d0
> > +name:_GIOChannel size:0x70
>
> This one unfortunately shows why the global change is wrong.  The size
> of _GIOChannel must match between glib and QEMU, otherwise you have an
> ABI mismatch.

In the codebase, we always use this type as an opaque type (through 
pointer, using library functions to interact with it and never use it's 
size). So the fact we see a different layout is *not* an issue for QEMU. 
I don't see it as a counter example that this does not work.

> In other words, the global default _must_ be -mms-bitfields, because all
> other libraries (and also Windows itself, though you didn't find any
> occurrences) are built with MSVC ABI compatibility.  Bitfields are
> relatively rare, and therefore you only found one occurrence; however,
> this is a constraint that we cannot get rid of.

-mms-bitfields is already the (silent) gcc default on windows, to mimic 
MSVC behaviour. Yes, it would be preferable to use this default and 
gcc_struct attribute, but right now, it does not work with clang. So the 
whole point is to accept a compromise for this.
We can even apply this selectively when clang is detected in case QEMU 
community is scared changing this default would break existing code.

I don't understand the strong pushback against clang support on windows. 
Because of a "theoretical" problem, that was proved here we don't have 
currently, we are stuck with gcc_struct attribute. This is currently 
blocking clang, and I don't see the point about that.
If clang supports gcc_struct attribute in the future, we can always 
decide to revert this change, and use it again. Meanwhile (1 month? 1 
year? 5 years?), we are just blocking progress support for the sake of 
blocking it.
The risk of this approach is that MSYS2, and Google folks using clang-cl 
will simply move forward and remove this restriction, even if the 
upstream community does not agree. Closing eyes and say "not supported" 
is not the best way to move forward.

> However, your script lets you do the opposite experiment: remove
> gcc_struct QEMU_PACKED and check if anything changes, i.e. whether there
> are any QEMU_PACKED structs that do rely on the gcc_struct attribute.
> If there are any, then it should be possible to change the definition
> and fix them.

It does not only remove gcc_struct attribute, it replaces it with an 
option that does the same thing globally for all packed structs.

I did the experiment with gcc only, simply to have an easier diff 
because code layout stays the same, while clang would order symbols 
differently.

> Thanks,
>
> Paolo

Thanks,
Pierrick