On 25/11/2024 10.39, Paolo Bonzini wrote:
On Mon, Nov 25, 2024 at 10:34 AM Daniel P. Berrangé <berra...@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
