Supporting clang on windows

Supporting clang on windows

[Pierrick Bouvier]

Hi all,

very recently, I've been working on enabling clang builds on Windows 
[1]. As part of it, the major blocking point is that currently, clang 
does not support gcc_attribute on Windows.
The change proposed [2] was to use the compiler option [3] 
-mno-ms-bitfields, which enables the same behaviour. This option *only* 
has an effect on structs containing bitfields or structs that are 
packed, not on *all* structs.

The question that was left is: Is it was we want?

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.

As expected, turning the option globally has an effect. The exact list 
of structures impacted (those having bitfields, and not being packed) 
can be seen for each binary here: [5].
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
+name:MachineClass size:0x0188
+name:MicrovmMachineClass size:0x01a0
+name:MPS2MachineClass size:0x01a8
+name:MPS2TZMachineClass size:0x01e8
+name:MPS3RMachineClass size:0x01a0
+name:MuscaMachineClass size:0x01a8
+name:NPCM7xxMachineClass size:0x0190
+name:PCMachineClass size:0x01c0
+name:PnvMachineClass size:0x01b0
+name:PPCE500MachineClass size:0x01e0
+name:RaspiBaseMachineClass size:0x0190
+name:RxGdbSimMachineClass size:0x0198
+name:S390CcwMachineClass size:0x0190
+name:SpaprMachineClass size:0x01d0
+name:Sun4mMachineClass size:0x0190
+name:TriBoardMachineClass size:0x01a0
+name:VexpressMachineClass size:0x0190
+name:VirtMachineClass size:0x01a0
+name:X86MachineClass size:0x0190

The interesting point I discovered is that, *after* this change, the new 
size start to match size when compiling for Linux.
After all, it makes totally sense: gcc by default mimics MSVC behaviour 
for bitfields, which is different from its behaviour on Linux.

As a conclusion, I think that applying gcc_struct selectively is a wrong 
default, and using globally the -mno-ms-bitfields allows to mimic what 
gcc is doing on Linux. We don't need any ABI compatibility with MSVC, so 
it's safe to enable this.

Before pushing the change again in a new series, I would like to get 
feedback from the concerned developers, and hope it can convince you 
this time it's a better approach than what we have.
As a side effect of all this, it will unlock supporting clang on 
windows, and with that, building it officially for windows-arm64, which 
was one of the goal of the original series.

Regards,
Pierrick

[1] 
https://lore.kernel.org/all/20241031040426.772604-1-pierrick.bouvier@linaro.org/
[2] 
https://lore.kernel.org/all/20241031040426.772604-8-pierrick.bouvier@linaro.org/
[3] https://gcc.gnu.org/onlinedocs/gcc/x86-Variable-Attributes.html
[4] 
https://github.com/pbo-linaro/qemu_packed_structs/blob/master/filter_dwarf_dump.sh
[5] 
https://github.com/pbo-linaro/qemu_packed_structs/commit/a47747b5220a933a94dbdc6e7bcd96589ae9abd0
[6] 
https://github.com/pbo-linaro/qemu_packed_structs/commit/9d41b7bd1fbd1d1553ae532e36c9305438549ae4

Re: Supporting clang on windows

[Paolo Bonzini]

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 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.

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.

Thanks,

Paolo

Re: Supporting clang on windows

[Philippe Mathieu-Daudé]

On 24/11/24 01:21, Pierrick Bouvier wrote:
> Hi all,


> 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.
> 
> As expected, turning the option globally has an effect. The exact list 
> of structures impacted (those having bitfields, and not being packed) 
> can be seen for each binary here: [5].
> 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
> +name:MachineClass size:0x0188
> +name:MicrovmMachineClass size:0x01a0
> +name:MPS2MachineClass size:0x01a8
> +name:MPS2TZMachineClass size:0x01e8
> +name:MPS3RMachineClass size:0x01a0
> +name:MuscaMachineClass size:0x01a8
> +name:NPCM7xxMachineClass size:0x0190
> +name:PCMachineClass size:0x01c0
> +name:PnvMachineClass size:0x01b0
> +name:PPCE500MachineClass size:0x01e0
> +name:RaspiBaseMachineClass size:0x0190
> +name:RxGdbSimMachineClass size:0x0198
> +name:S390CcwMachineClass size:0x0190
> +name:SpaprMachineClass size:0x01d0
> +name:Sun4mMachineClass size:0x0190
> +name:TriBoardMachineClass size:0x01a0
> +name:VexpressMachineClass size:0x0190
> +name:VirtMachineClass size:0x01a0
> +name:X86MachineClass size:0x0190

The *MachineClass ones come from [*]:

   struct MachineClass {
       ...
       unsigned int no_serial:1,
           no_parallel:1,
           no_floppy:1,
           no_cdrom:1,
           no_sdcard:1,
           pci_allow_0_address:1,
           legacy_fw_cfg_order:1;

IMHO Using bitfield to manually micro-optimize memory size seems
a bit a waste of developer time/focus, I'd rather see compilers
doing that for us.

Regards,

Phil.

[*] Interestingly Thomas is also looking at those fields:
 
https://lore.kernel.org/qemu-devel/20241122084923.1542743-1-thuth@redhat.com/

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

Re: Supporting clang on windows

[Pierrick Bouvier]

On 11/24/24 04:28, Philippe Mathieu-Daudé wrote:
> On 24/11/24 01:21, Pierrick Bouvier wrote:
>> Hi all,
> 
> 
>> 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.
>>
>> As expected, turning the option globally has an effect. The exact list
>> of structures impacted (those having bitfields, and not being packed)
>> can be seen for each binary here: [5].
>> 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
>> +name:MachineClass size:0x0188
>> +name:MicrovmMachineClass size:0x01a0
>> +name:MPS2MachineClass size:0x01a8
>> +name:MPS2TZMachineClass size:0x01e8
>> +name:MPS3RMachineClass size:0x01a0
>> +name:MuscaMachineClass size:0x01a8
>> +name:NPCM7xxMachineClass size:0x0190
>> +name:PCMachineClass size:0x01c0
>> +name:PnvMachineClass size:0x01b0
>> +name:PPCE500MachineClass size:0x01e0
>> +name:RaspiBaseMachineClass size:0x0190
>> +name:RxGdbSimMachineClass size:0x0198
>> +name:S390CcwMachineClass size:0x0190
>> +name:SpaprMachineClass size:0x01d0
>> +name:Sun4mMachineClass size:0x0190
>> +name:TriBoardMachineClass size:0x01a0
>> +name:VexpressMachineClass size:0x0190
>> +name:VirtMachineClass size:0x01a0
>> +name:X86MachineClass size:0x0190
> 
> The *MachineClass ones come from [*]:
> 
>     struct MachineClass {
>         ...
>         unsigned int no_serial:1,
>             no_parallel:1,
>             no_floppy:1,
>             no_cdrom:1,
>             no_sdcard:1,
>             pci_allow_0_address:1,
>             legacy_fw_cfg_order:1;
> 
> IMHO Using bitfield to manually micro-optimize memory size seems
> a bit a waste of developer time/focus, I'd rather see compilers
> doing that for us.
> 

Yes, it's (another) lesson that C bitfields are a good synonym of non 
portable code in general.

> Regards,
> 
> Phil.
> 
> [*] Interestingly Thomas is also looking at those fields:
>   
> https://lore.kernel.org/qemu-devel/20241122084923.1542743-1-thuth@redhat.com/

Re: Supporting clang on windows

[Thomas Huth]

On 24/11/2024 13.28, Philippe Mathieu-Daudé wrote:
> On 24/11/24 01:21, Pierrick Bouvier wrote:
>> Hi all,
> 
> 
>> 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.
>>
>> As expected, turning the option globally has an effect. The exact list of 
>> structures impacted (those having bitfields, and not being packed) can be 
>> seen for each binary here: [5].
>> 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
>> +name:MachineClass size:0x0188
>> +name:MicrovmMachineClass size:0x01a0
>> +name:MPS2MachineClass size:0x01a8
>> +name:MPS2TZMachineClass size:0x01e8
>> +name:MPS3RMachineClass size:0x01a0
>> +name:MuscaMachineClass size:0x01a8
>> +name:NPCM7xxMachineClass size:0x0190
>> +name:PCMachineClass size:0x01c0
>> +name:PnvMachineClass size:0x01b0
>> +name:PPCE500MachineClass size:0x01e0
>> +name:RaspiBaseMachineClass size:0x0190
>> +name:RxGdbSimMachineClass size:0x0198
>> +name:S390CcwMachineClass size:0x0190
>> +name:SpaprMachineClass size:0x01d0
>> +name:Sun4mMachineClass size:0x0190
>> +name:TriBoardMachineClass size:0x01a0
>> +name:VexpressMachineClass size:0x0190
>> +name:VirtMachineClass size:0x01a0
>> +name:X86MachineClass size:0x0190
> 
> The *MachineClass ones come from [*]:
> 
>    struct MachineClass {
>        ...
>        unsigned int no_serial:1,
>            no_parallel:1,
>            no_floppy:1,
>            no_cdrom:1,
>            no_sdcard:1,
>            pci_allow_0_address:1,
>            legacy_fw_cfg_order:1;
> 
> IMHO Using bitfield to manually micro-optimize memory size seems
> a bit a waste of developer time/focus, I'd rather see compilers
> doing that for us.
> 
> Regards,
> 
> Phil.
> 
> [*] Interestingly Thomas is also looking at those fields:
> 
> https://lore.kernel.org/qemu-devel/20241122084923.1542743-1-thuth@redhat.com/

Yes, I got curious by the msbitfield discussion last week and remembered 
that we had a look at the differences of the structs with "pahole" in the 
past already, so I wondered what might be the difference nowadays, and the 
machine class is the most prominent difference, indeed, that's why I came up 
with that patch.

  Thomas

Re: Supporting clang on windows

[Paolo Bonzini]

On Sun, Nov 24, 2024 at 9:23 PM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
> > 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.

_GIOChannel is just an example, and in principle macros could be
relying on the layout of GIOChannel. My point is that compiling a
program with a different ABI than the rest of the system is a ticking
time bomb, and therefore compiling QEMU with -mno-ms-bitfields is not
a solution.

This is also explained at
https://github.com/GNOME/glib/blob/main/docs/win32-build.md: "You
should link to GLib using the -mms-bitfields GCC flag. This flag means
that the struct layout rules are identical to those used by MSVC. This
is essential if the same DLLs are to be usable both from gcc- and
MSVC-compiled code".

> -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.
> 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.

I think you are unnecessarily pessimistic:

* there is no need for a compromise, eliminating gcc_struct is the
best solution but it needs proof that it introduces no guest-visible
changes

* there is no pushback against clang support, there is pushback
against asking for a change without understanding the problem

On the other hand, the problem is very real even though we might not
have it *now*; it's not hard to find out when and why the option was
introduced. If you do "git log -Sgcc_struct", and then Google for the
commit subject "Add new macro QEMU_PACKED for packed C structures",
you'll quickly find one of these two links:

https://patchwork.ozlabs.org/project/qemu-devel/patch/1314564200-6872-2-git-send-email-weil@mail.berlios.de/
https://lists.gnu.org/archive/html/qemu-devel/2011-08/msg01877.html

As an aside, at https://github.com/msys2/MINGW-packages/pull/21540 you
said "I think too it's more a FUD argument than a real problem", which
is a bit too dismissive. If anything it's a case of "once bitten,
twice shy".

Anyway, the problem is *not* that QEMU uses gcc_struct. Rather, the
rare but real problem is that there is, in some cases involving
bitfields, incompatible struct layout between Linux and Windows
compilers.

gcc_struct is *one* solution to that problem, and the one that QEMU is
currently using. It has the advantage that it cannot go wrong, and the
disadvantage that clang doesn't support it. Note that it's perfectly
possible that there are no such cases in QEMU, i.e. that the attribute
has no effect now. But it's been there for 13 years and it was
introduced because of a bug whose cause was MSVC bitfield layout.
People *should* be wary of removing it.

clang's lack of support for gcc_struct is stupid, but we have to work
around it and we can, without introducing potential ABI breaks. Just
verify that gcc_struct still has any effect. Initially I mentioned
checking sizeof() but actually it's possible to do the same using
debug info, similar to your scripts.

For example, I tried doing another kind of analysis based on
llvm-dwarfdump. This will give the list of source files with bitfields
in them:

find . -name "*.c.o" | xargs llvm-dwarfdump|\
  grep -we DW_AT_data_bit_offset -e DW_AT_bit_size -e DW_AT_decl_file|\
  grep -B1 _bit_ | grep decl_file | sort -u

Restricting the list of files further to those that have QEMU_PACKED
in them, the list becomes just:

hw/nvme/ctrl.c
hw/pci/pcie_doe.c
net/util.h
include/hw/cxl/cxl_device.h
include/hw/hyperv/dynmem-proto.h
include/hw/i386/intel_iommu.h
include/hw/i386/x86-iommu.h
include/net/eth.h

Looking at these eight files, in some of them the bitfields are
defined via registers.h. In others the bitfields:
- have the same type within a word
- don't span multiple words
- use the smallest integer type that fits them (e.g. uint8_t in net/util.h)
- always have a nonzero width

So it *should* be fine to remove gcc_struct from QEMU even without
-mms-bitfields. But we need to be *sure* that it is, hence the next
part of the email...

> > 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 understand that, and I'm asking you to do another experiment. Do not
change the compile-time options. Instead, change QEMU_PACKED to just

#define QEMU_PACKED __attribute__((packed))

and see if any struct definitions (which will all follow the ms_struct
rules) change. If there are changes, let's examine what they are and
why my analysis above was incorrect. Fix those cases, add
QEMU_BUILD_BUG_ON checks only to the affected structs, and once you've
addressed any differences (if they exist), you can proceed with
dropping gcc_struct since there will be concrete evidence proving it's
safe.

Paolo

Re: Supporting clang on windows

[Thomas Huth]

On 24/11/2024 21.23, Pierrick Bouvier wrote:
> 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.

If you have a look at the difference between the binaries with "pahole", you 
can see also differences in bitfields in glib structs (GdkEventKey), even if 
the overall size of the struct does not change.

So even if it would be working by chance with the current libraries, this is 
a ticking time bomb - for each future library, this could break at any point 
in time, so IMHO we really must not enable -mno-ms-bitfields globally on 
Windows.

>> 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. 

It's certainly not a dislike of Clang. It's simply that we've seen problems 
in the past with bitfields in structures, and so far nobody went ahead and 
checked each and every target whether we are clean nowadays. And as long as 
nobody does this tedious job, we cannot say that Clang is really a supported 
compiler 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.

It's not a "theoretical" problem, it's a real one. Have a look at commit 
642ba89672279fbdd14016a90da239c85e845d18 for example. If you revert the 
change to VTD_IR_TableEntry in include/hw/i386/intel_iommu.h and have a look 
a the difference of the output of "pahole" of the binaries, one compiled 
with -mms-bitfields and one with -mnoms-bitfields, you can see:

  union VTD_IR_TableEntry {
         struct {
                 uint32_t           present:1;          /*     0: 0  4 */
                 uint32_t           fault_disable:1;    /*     0: 1  4 */
                 uint32_t           dest_mode:1;        /*     0: 2  4 */
                 uint32_t           redir_hint:1;       /*     0: 3  4 */
                 uint32_t           trigger_mode:1;     /*     0: 4  4 */
                 uint32_t           delivery_mode:3;    /*     0: 5  4 */
                 uint32_t           __avail:4;          /*     0: 8  4 */
                 uint32_t           __reserved_0:3;     /*     0:12  4 */
                 uint32_t           irte_mode:1;        /*     0:15  4 */
                 uint32_t           vector:8;           /*     0:16  4 */
                 uint32_t           __reserved_1:8;     /*     0:24  4 */
                 uint32_t           dest_id;            /*     4     4 */
                 uint16_t           source_id;          /*     8     2 */

-               /* XXX 6 bytes hole, try to pack */
+               /* Bitfield combined with previous fields */

-               uint64_t           sid_q:2;            /*    16: 0  8 */
-               uint64_t           sid_vtype:2;        /*    16: 2  8 */
-               uint64_t           __reserved_2:44;    /*    16: 4  8 */
-       } irte;                                        /*     0    24 */
+               uint64_t           sid_q:2;            /*     8:16  8 */
+               uint64_t           sid_vtype:2;        /*     8:18  8 */
+               uint64_t           __reserved_2:44;    /*     8:20  8 */
+       } irte;                                        /*     0    16 */
         uint64_t                   data[2];            /*     0    16 */
  };

This problem is fixed nowadays since it also caused issues on big endian 
hosts (btw, using bitfields in structs that are used for data interchange is 
evil, we should maybe forbid this in the coding styles) ... but do we know 
whether there are other problems left? So far, nobody really looked at it 
yet, I think.

I checked the x86_64, aarch64 and ppc64 targets with pahole, and we seem to 
be clear there nowadays. But before we could drop the attribute on Clang on 
Windows, I think we need someone who *carefully* looks at *all* targets and 
confirms that we don't have any issues left.

  Thomas

Re: Supporting clang on windows

[Daniel P. Berrangé]

On Mon, Nov 25, 2024 at 08:00:00AM +0100, Paolo Bonzini wrote:
> On Sun, Nov 24, 2024 at 9:23 PM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> > -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.
> > 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.
> 
> I think you are unnecessarily pessimistic:
> 
> * there is no need for a compromise, eliminating gcc_struct is the
> best solution but it needs proof that it introduces no guest-visible
> changes
> 
> * there is no pushback against clang support, there is pushback
> against asking for a change without understanding the problem

To expand on this...

Bear in mind that when stuff breaks, users file bugs, and the QEMU
maintainers are on the receiving end of user demands to investigate
and fix the problem.

These kind of issues have the potential to be very hard to debug
and diagnose. Maintainer time is precious, which is why we're
conservative at making decisions that expose us to potential long
term negative consequences / increased workload. Compiling with
flags that were known to be incompatible with our code historically
is a reasonable thing to reject. It should not be unexpected that
when a request to change this has a higher burden of proof that
other proposals.

> clang's lack of support for gcc_struct is stupid, but we have to work
> around it and we can, without introducing potential ABI breaks. Just
> verify that gcc_struct still has any effect. Initially I mentioned
> checking sizeof() but actually it's possible to do the same using
> debug info, similar to your scripts.

IMHO we need to have confidence not only in the current state of the code,
but also that we're not going to accidentally regresss it in the future.
This is what the gcc_struct attribute gives us confidence in.

If we can do an assert with a 'sizeof' check, that would give us similar,
but I presume any sizeof checks need to be manually written to specify
the expected size ? A manual check for pahole output by comparison only
tells us about the current point in time of QEMU code.

As an alternative is it practical for us to eliminate all bitfields
from our structs ? IIUC, we already have the preference that we use
the 'BIT(n)' and 'BIT_ULL(n)' macros for accessing bits from full
sized int32/int64 fields which avoids the semantic layout problems.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Supporting clang on windows

[Paolo Bonzini]

On Mon, Nov 25, 2024 at 10:34 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> IMHO we need to have confidence not only in the current state of the code,
> but also that we're not going to accidentally regresss it in the future.
> This is what the gcc_struct attribute gives us confidence in.

Yes, as you say below the "solution" to that could be simply to avoid
bitfields. They are problematic anyway for big-endian machines, see
the commit that Thomas pointed out. Unfortunately it's a
human-enforced solution, but then it is already human-enforced for
endianness issues.

> As an alternative is it practical for us to eliminate all bitfields
> from our structs ?

It is (at least for guest-visible structs), but it is a lot of work
and I don't think it's reasonable for it to block clang enablement on
Windows.

Of course it is possible for future contributions. Pierrick, would you
like to contribute a patch to docs/devel/style.rst as well?

Paolo

Re: Supporting clang on windows

[Peter Maydell]

On Mon, 25 Nov 2024 at 09:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Mon, Nov 25, 2024 at 10:34 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > IMHO we need to have confidence not only in the current state of the code,
> > but also that we're not going to accidentally regresss it in the future.
> > This is what the gcc_struct attribute gives us confidence in.
>
> Yes, as you say below the "solution" to that could be simply to avoid
> bitfields. They are problematic anyway for big-endian machines, see
> the commit that Thomas pointed out. Unfortunately it's a
> human-enforced solution, but then it is already human-enforced for
> endianness issues.

Mmm. The closest we get to auto-enforcement is CI on the big-endian
host, which requires that there's a test case that exercises the struct.

> > As an alternative is it practical for us to eliminate all bitfields
> > from our structs ?
>
> It is (at least for guest-visible structs), but it is a lot of work
> and I don't think it's reasonable for it to block clang enablement on
> Windows.

It is such a pity that C does not make it easy to have a portable
way to say "this is a struct that must match a particular layout
for ABI compatibility and it should be like this exactly regardless
of host system details, even if that isn't what you would 'naturally'
use". Does Rust provide any better facilities for this?

-- PMM

Re: Supporting clang on windows

[Thomas Huth]

On 25/11/2024 10.39, Paolo Bonzini wrote:
> On Mon, Nov 25, 2024 at 10:34 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>> IMHO we need to have confidence not only in the current state of the code,
>> but also that we're not going to accidentally regresss it in the future.
>> This is what the gcc_struct attribute gives us confidence in.
> 
> Yes, as you say below the "solution" to that could be simply to avoid
> bitfields. They are problematic anyway for big-endian machines, see
> the commit that Thomas pointed out. Unfortunately it's a
> human-enforced solution, but then it is already human-enforced for
> endianness issues.
> 
>> As an alternative is it practical for us to eliminate all bitfields
>> from our structs ?
> 
> It is (at least for guest-visible structs), but it is a lot of work
> and I don't think it's reasonable for it to block clang enablement on
> Windows.
> 
> Of course it is possible for future contributions. Pierrick, would you
> like to contribute a patch to docs/devel/style.rst as well?

FWIW, I now did some compiling in the background, one time with GCC + 
"-mms-bitfields" + "#define QEMU_PACKED __attribute__((gcc_struct, packed))" 
and a second time with the same setting, but with "gcc_struct" removed from 
the #define. Then I ran "pahole" on all qemu-system-* binaries from both 
builds and compared the corresponding outputs. Seems like we are currently 
clean and that 642ba89672279fbdd14016a90da239c85e845d18 likely was the last 
problematic structure that we had.

So fine for me if we allow compiling with Clang on Windows now, but we 
should make sure to avoid that those problems are sneaking in again. So 
Pierrick, please provide a patch to docs/devel/style.rst to say that we 
disallow bitfields in packed structs in future contributions. Then I think 
it's ok to remove the gcc_struct from QEMU_PACKED. (Just my 2 cents, of course)

  Thomas

Re: Supporting clang on windows

[Richard Henderson]

On 11/23/24 18: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.

It appears you are re-creating libabigail's abidiff tool.

https://sourceware.org/libabigail/manual/abidiff.html


r~

Re: Supporting clang on windows

[Pierrick Bouvier]

On 11/25/24 06:04, Richard Henderson wrote:
> On 11/23/24 18: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.
> 
> It appears you are re-creating libabigail's abidiff tool.
> 
> https://sourceware.org/libabigail/manual/abidiff.html
> 
> 
> r~

Thanks for mentioning it.

To make sure we don't miss anything, I made sure to run that using 
windows binaries.
So, except if I missed something on how to use those tools, this 
excludes support for abidiff and pahole which are ELF centric.
Thus the idea to simply to dump full dwarf info included in COFF and 
compare the result.

Re: Supporting clang on windows

[Pierrick Bouvier]

Thanks everyone for your valuable feedback on this, and taking time to 
give more rationale about the previous position you had.

On 11/25/24 03:19, Thomas Huth wrote:
> 
> So fine for me if we allow compiling with Clang on Windows now, but we
> should make sure to avoid that those problems are sneaking in again. So
> Pierrick, please provide a patch to docs/devel/style.rst to say that we
> disallow bitfields in packed structs in future contributions. Then I think
> it's ok to remove the gcc_struct from QEMU_PACKED. (Just my 2 cents, of course)
> 
>    Thomas
> 

I just compared windows binaries with/without gcc_struct, and as Thomas 
found for qemu linux binaries using pahole, there is no difference with 
or without gcc_struct, so the attribute is unused now.

In case you want to check that by yourself, I uploaded binaries and 
dwarf info on this repo:
https://github.com/pbo-linaro/qemu_packed_structs
(gcc_attribute has gcc_struct binaries, gcc_no_attribute_no_option has 
binaries without it).

Before sending the a series removing gcc_struct and editing the 
documentation, do we all agree here it's the right move forward?
If yes, should we apply this to 9.2 release?

Thanks,
Pierrick

Re: Supporting clang on windows

[Pierrick Bouvier]

On 11/24/24 23:00, Paolo Bonzini wrote:
> On Sun, Nov 24, 2024 at 9:23 PM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> * there is no pushback against clang support, there is pushback
> against asking for a change without understanding the problem
>

Thanks for taking time to share more insights about it.

> As an aside, at https://github.com/msys2/MINGW-packages/pull/21540 you
> said "I think too it's more a FUD argument than a real problem", which
> is a bit too dismissive. If anything it's a case of "once bitten,
> twice shy".
>

There was no intention to have a personal jugdment, not blame anyone, 
and I hope you didn't take it this way. If that's the case, sorry about 
that.

FUD applies when Fear and Uncertainty applies, and it's definitely where 
we are on this - we fear something from a past experience, and we are 
uncertain about the current status. I totally understand that's a very 
hard issue to diagnose when you meet those kind of memory layout bugs.

> I understand that, and I'm asking you to do another experiment. Do not
> change the compile-time options. Instead, change QEMU_PACKED to just
> 
> #define QEMU_PACKED __attribute__((packed))
> 
> and see if any struct definitions (which will all follow the ms_struct
> rules) change. If there are changes, let's examine what they are and
> why my analysis above was incorrect. Fix those cases, add
> QEMU_BUILD_BUG_ON checks only to the affected structs, and once you've
> addressed any differences (if they exist), you can proceed with
> dropping gcc_struct since there will be concrete evidence proving it's
> safe.
>

I didn't expect the issue of our conversation would be to get rid of 
gcc_struct entirely. Thanks for pushing in the right direction.

I answered later on this thread for this, and it's a very positive 
conclusion: no difference was found with/without gcc_struct attribute.

> Paolo
> 

Thanks,
Pierrick

Re: Supporting clang on windows

[Peter Maydell]

On Mon, 25 Nov 2024 at 16:48, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
> Before sending the a series removing gcc_struct and editing the
> documentation, do we all agree here it's the right move forward?
> If yes, should we apply this to 9.2 release?

I would prefer not to put it into the 9.2 release -- it's one
of those "small change with global effects" which it's hard to be
sure don't have unexpected consequences[*], this isn't a
regression, and we're already some way into the freeze-and-rc
process.

[*] though in this case the stuff you've done with comparing
binaries is great and very reassuring

thanks
-- PMM

Re: Supporting clang on windows

[Pierrick Bouvier]

On 11/25/24 09:05, Peter Maydell wrote:
> On Mon, 25 Nov 2024 at 16:48, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>> Before sending the a series removing gcc_struct and editing the
>> documentation, do we all agree here it's the right move forward?
>> If yes, should we apply this to 9.2 release?
> 
> I would prefer not to put it into the 9.2 release -- it's one
> of those "small change with global effects" which it's hard to be
> sure don't have unexpected consequences[*], this isn't a
> regression, and we're already some way into the freeze-and-rc
> process.
> 

Ok! no rush then.

I'll send a series with that removal and the doc edit (+ fixing plugins 
build for clang on windows).
I'm just waiting for plugin build with meson to be merged in, which 
should be very soon as Alex already sent the PR for this.

> [*] though in this case the stuff you've done with comparing
> binaries is great and very reassuring
> 
> thanks
> -- PMM

Thanks,
Pierrick

Re: Supporting clang on windows

[Pierrick Bouvier]

On 11/25/24 13:47, Pierrick Bouvier wrote:
> On 11/25/24 09:05, Peter Maydell wrote:
>> On Mon, 25 Nov 2024 at 16:48, Pierrick Bouvier
>> <pierrick.bouvier@linaro.org> wrote:
>>> Before sending the a series removing gcc_struct and editing the
>>> documentation, do we all agree here it's the right move forward?
>>> If yes, should we apply this to 9.2 release?
>>
>> I would prefer not to put it into the 9.2 release -- it's one
>> of those "small change with global effects" which it's hard to be
>> sure don't have unexpected consequences[*], this isn't a
>> regression, and we're already some way into the freeze-and-rc
>> process.
>>
> 
> Ok! no rush then.
> 
> I'll send a series with that removal and the doc edit (+ fixing plugins
> build for clang on windows).
> I'm just waiting for plugin build with meson to be merged in, which
> should be very soon as Alex already sent the PR for this.
>

Sent the series, as plugin build with meson was merged yesterday:
https://lore.kernel.org/qemu-devel/20241126211736.122285-1-pierrick.bouvier@linaro.org/T/#t

>> [*] though in this case the stuff you've done with comparing
>> binaries is great and very reassuring
>>
>> thanks
>> -- PMM
> 
> Thanks,
> Pierrick
>