Re: Supporting clang on windows

[Thomas Huth <thuth@redhat.com>]

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