Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote:
> On 28/07/2023 17.13, Peter Maydell wrote:
> > On Fri, 28 Jul 2023 at 15:28, Thomas Huth <thuth@redhat.com> wrote:
> > > 
> > > Clang on Windows does not seem to know the "gcc_struct" attribute
> > > and emits a warning when we try to use it. Add an additional check
> > > here with __has_attribute() to avoid this problem.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   include/qemu/compiler.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> > > index a309f90c76..5065b4447c 100644
> > > --- a/include/qemu/compiler.h
> > > +++ b/include/qemu/compiler.h
> > > @@ -22,7 +22,7 @@
> > >   #define QEMU_EXTERN_C extern
> > >   #endif
> > > 
> > > -#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> > > +#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__)) && !defined(__clang__)
> > >   # define QEMU_PACKED __attribute__((gcc_struct, packed))
> > >   #else
> > >   # define QEMU_PACKED __attribute__((packed))
> > 
> > I'm not sure about this. The idea of QEMU_PACKED is that
> > it's supposed to give you the same struct layout
> > regardless of compiler. With this change it no longer
> > does that, and there's no compile-time guard against
> > using something in a packed struct that has a different
> > layout on Windows clang vs everything else.
> > 
> > If it was OK to use plain attribute(packed) we wouldn't
> > need the ifdef at all because we could use it on GCC too.
> 
> I still haven't quite grasped whether this attribute just affects structures
> with bitfields in it, or whether it could also affect other structures
> without bitfields.
> 
> Looking at https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html and
> https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html#index-mms-bitfields , it
> sounds like it only affects structs with bitfields, unless you specify an
> "aligned(x)" attribute, too?
> 
> Anyway, using bitfields in structs for exchanging data with the guest is
> just way too error-prone, as you can see in the discussion about that
> VTD_IR_TableEntry in my other patch. We should maybe advise against
> bitfields in our coding style and point people to registerfields.h instead
> for new code? ... so that we use QEMU_PACKED mainly for legacy code. Would
> it then be OK for you, Peter, to go on with this approach?
> 
> Or do you see another possibility how we could fix that timeout problem in
> the 64-bit MSYS2 job? Still switching to clang, but compiling with
> --extra-cflags="-Wno-unknown-attributes" maybe?

I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
help the from-clean compile time, but thereafter it ought to help. I'm doing
some tests with that to see the impact.

Another option might be to try precompiled headers, which meson supports
quite nicely / transparently. Might especially help on Windows where the
entire world is declared in windows.h


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