[PULL 0/1] Quick fix patches

[PULL 0/1] Quick fix patches

[Warner Losh]

The following changes since commit 813bac3d8d70d85cb7835f7945eb9eed84c2d8d0:

  Merge tag '2023q3-bsd-user-pull-request' of https://gitlab.com/bsdimp/qemu into staging (2023-08-29 08:58:00 -0400)

are available in the Git repository at:

  https://gitlab.com/bsdimp/qemu.git tags/quick-fix-pull-request

for you to fetch changes up to de287fb4e8987b32e133f7f37b990e09f3aa6325:

  linux-user: Move PRAGMA_DISABLE_PACKED_WARNING to compiler.h (2023-08-29 20:12:25 -0600)

----------------------------------------------------------------
Pull request: Quick fix for clang user-mode job

Move the linux-user version of PRAGMA_DISABLE_PACKED_WARNING
to qemu/compiler.h and remove it from linux-user/qemu.h.
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - https://gpgtools.org

iQIzBAABCgAdFiEEIDX4lLAKo898zeG3bBzRKH2wEQAFAmTup4sACgkQbBzRKH2w
EQChXw/9H/hKENL0vLz9LE1iq05+bJ6/uY/Kl4avXX3/ZBq39/ZvNHrgT6h26PMb
wU2vFYFL8UTZQsfC8t35B1khmoK3ZUtbTYYUjzpmVQQA1+MGNpflgciSQhsITGkG
zOraHo9kSkM/ByHE246zSxqJlgHTziE/mQ1Hg8AFNvI5KChgedMblFz4gu99ADMA
sVQwBUTAeOJv3uvY9DhXCxtvg5Lj+ZcJd7Uu4pYl86jHp0RSE7Jk6jrJXo+Xp3GF
MnDxK9IrShEmIK1ci+tG8YBiY91GW/GEPVJJxL03JsvWxuRhj8GQsIopD1Mo4xbp
mniDs6AbDTpxnE3DpqrN8UFh+3Ko0qZw+/OjCxckYbQadYrWVeL6n+uHxcs15Z+R
SmIURzBrcLqzvFmvpUD4KHBQxSdIGdZrCQA+PC54Ghx0tqBBPapd/4LPL5kJsVqX
6DOYwegbbnDNcGIXv/5RXoL+sIF00mWpWslV+xCrTP5Dz9KQmjSC/fgPnNucr2H5
MBbe0BAxZvn1KHbgUhxCVNd1WFyaq1Gu5XZRNsXy0BBs5/NzkrJm614JOLbS+jtO
DgvEHvbo57LGB145IBZQOUfAUQUnizyUhb27cK+L8hzg3MHsMG+coQyi5I4zaBGn
5JddIPvbpUFk9mS+i9oeZRr/gPmDgPbylaOQhNGQvYpu8xB8/lU=
=QC7n
-----END PGP SIGNATURE-----

----------------------------------------------------------------

Warner Losh (1):
  linux-user: Move PRAGMA_DISABLE_PACKED_WARNING to compiler.h

 include/qemu/compiler.h |  6 +-----
 linux-user/qemu.h       | 26 --------------------------
 2 files changed, 1 insertion(+), 31 deletions(-)

-- 
2.41.0

[PULL 1/1] linux-user: Move PRAGMA_DISABLE_PACKED_WARNING to compiler.h

[Warner Losh]

Replace the slightly older version of this in include/qemu/compiler.h
that was commit as part of bsd-user changes with the newer one from
linux-user. bsd-user has no regreassions with this.

Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/compiler.h |  6 +-----
 linux-user/qemu.h       | 26 --------------------------
 2 files changed, 1 insertion(+), 31 deletions(-)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index b0374425180..0df9febbe2f 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -31,17 +31,13 @@
  *   functions than host-endian unaligned load/store plus tswapN.
  * - The pragmas are necessary only to silence a clang false-positive
  *   warning: see https://bugs.llvm.org/show_bug.cgi?id=39113 .
- * - We have to disable -Wpragmas warnings to avoid a complaint about
- *   an unknown warning type from older compilers that don't know about
- *   -Waddress-of-packed-member.
  * - gcc has bugs in its _Pragma() support in some versions, eg
  *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83256 -- so we only
  *   include the warning-suppression pragmas for clang
  */
-#ifdef __clang__
+#if defined(__clang__) && __has_warning("-Waddress-of-packed-member")
 #define PRAGMA_DISABLE_PACKED_WARNING                                   \
     _Pragma("GCC diagnostic push");                                     \
-    _Pragma("GCC diagnostic ignored \"-Wpragmas\"");                    \
     _Pragma("GCC diagnostic ignored \"-Waddress-of-packed-member\"")
 
 #define PRAGMA_REENABLE_PACKED_WARNING          \
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 4f8b55e2fb0..12821e54d0a 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -193,32 +193,6 @@ static inline bool access_ok(CPUState *cpu, int type,
    These are usually used to access struct data members once the struct has
    been locked - usually with lock_user_struct.  */
 
-/*
- * Tricky points:
- * - Use __builtin_choose_expr to avoid type promotion from ?:,
- * - Invalid sizes result in a compile time error stemming from
- *   the fact that abort has no parameters.
- * - It's easier to use the endian-specific unaligned load/store
- *   functions than host-endian unaligned load/store plus tswapN.
- * - The pragmas are necessary only to silence a clang false-positive
- *   warning: see https://bugs.llvm.org/show_bug.cgi?id=39113 .
- * - gcc has bugs in its _Pragma() support in some versions, eg
- *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83256 -- so we only
- *   include the warning-suppression pragmas for clang
- */
-#if defined(__clang__) && __has_warning("-Waddress-of-packed-member")
-#define PRAGMA_DISABLE_PACKED_WARNING                                   \
-    _Pragma("GCC diagnostic push");                                     \
-    _Pragma("GCC diagnostic ignored \"-Waddress-of-packed-member\"")
-
-#define PRAGMA_REENABLE_PACKED_WARNING          \
-    _Pragma("GCC diagnostic pop")
-
-#else
-#define PRAGMA_DISABLE_PACKED_WARNING
-#define PRAGMA_REENABLE_PACKED_WARNING
-#endif
-
 #define __put_user_e(x, hptr, e)                                            \
     do {                                                                    \
         PRAGMA_DISABLE_PACKED_WARNING;                                      \
-- 
2.41.0

Re: [PULL 0/1] Quick fix patches

[Stefan Hajnoczi]

Hi,
The patch introduces the following build failure:

cc -m64 -mcx16 -Isubprojects/libvhost-user/libvhost-user.a.p
-Isubprojects/libvhost-user -I../subprojects/libvhost-user
-fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g
-Wsign-compare -Wdeclaration-after-statement -Wstrict-aliasing
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
-fno-common -fwrapv -fPIE -pthread -D_GNU_SOURCE -MD -MQ
subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -MF
subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o.d -o
subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -c
../subprojects/libvhost-user/libvhost-user.c
In file included from ../subprojects/libvhost-user/include/atomic.h:18,
from ../subprojects/libvhost-user/libvhost-user.c:53:
../subprojects/libvhost-user/include/compiler.h:38:40: error: missing
binary operator before token "("
38 | #if defined(__clang__) && __has_warning("-Waddress-of-packed-member")
| ^

https://gitlab.com/qemu-project/qemu/-/jobs/4981576093

Stefan

Re: [PULL 0/1] Quick fix patches

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

On Wed, Aug 30, 2023, 7:16 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> Hi,
> The patch introduces the following build failure:
>
> cc -m64 -mcx16 -Isubprojects/libvhost-user/libvhost-user.a.p
> -Isubprojects/libvhost-user -I../subprojects/libvhost-user
> -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g
> -Wsign-compare -Wdeclaration-after-statement -Wstrict-aliasing
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
> -fno-common -fwrapv -fPIE -pthread -D_GNU_SOURCE -MD -MQ
> subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -MF
> subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o.d -o
> subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -c
> ../subprojects/libvhost-user/libvhost-user.c
> In file included from ../subprojects/libvhost-user/include/atomic.h:18,
> from ../subprojects/libvhost-user/libvhost-user.c:53:
> ../subprojects/libvhost-user/include/compiler.h:38:40: error: missing
> binary operator before token "("
> 38 | #if defined(__clang__) && __has_warning("-Waddress-of-packed-member")
> | ^
>
> https://gitlab.com/qemu-project/qemu/-/jobs/4981576093


Looks like the macros should be removed there too...  but I don't know
about the subproject tree. Can I submit to it the same way?

And for a quick fix... maybe I just move it back to bsd-user/qemu.h until I
have more minutes and can test things better or fund the time to setup a
Linux build box for docker...

Warner


> Stefan
>

[-- Attachment #2: Type: text/html, Size: 2304 bytes --]

Re: [PULL 0/1] Quick fix patches

[Thomas Huth]

On 30/08/2023 15.16, Stefan Hajnoczi wrote:
> Hi,
> The patch introduces the following build failure:
> 
> cc -m64 -mcx16 -Isubprojects/libvhost-user/libvhost-user.a.p
> -Isubprojects/libvhost-user -I../subprojects/libvhost-user
> -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g
> -Wsign-compare -Wdeclaration-after-statement -Wstrict-aliasing
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
> -fno-common -fwrapv -fPIE -pthread -D_GNU_SOURCE -MD -MQ
> subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -MF
> subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o.d -o
> subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -c
> ../subprojects/libvhost-user/libvhost-user.c
> In file included from ../subprojects/libvhost-user/include/atomic.h:18,
> from ../subprojects/libvhost-user/libvhost-user.c:53:
> ../subprojects/libvhost-user/include/compiler.h:38:40: error: missing
> binary operator before token "("
> 38 | #if defined(__clang__) && __has_warning("-Waddress-of-packed-member")
> | ^
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/4981576093

IIRC older versions of GCC do not have __has_warning() yet, so if you want 
to use this in compiler.h, you have to do it below the line in compiler.h 
that adds this:

#ifndef __has_warning
#define __has_warning(x) 0 /* compatibility with non-clang compilers */
#endif

  HTH,
   Thomas

Re: [PULL 0/1] Quick fix patches

[Daniel P. Berrangé]

On Wed, Aug 30, 2023 at 03:26:25PM +0200, Thomas Huth wrote:
> On 30/08/2023 15.16, Stefan Hajnoczi wrote:
> > Hi,
> > The patch introduces the following build failure:
> > 
> > cc -m64 -mcx16 -Isubprojects/libvhost-user/libvhost-user.a.p
> > -Isubprojects/libvhost-user -I../subprojects/libvhost-user
> > -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g
> > -Wsign-compare -Wdeclaration-after-statement -Wstrict-aliasing
> > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
> > -fno-common -fwrapv -fPIE -pthread -D_GNU_SOURCE -MD -MQ
> > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -MF
> > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o.d -o
> > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -c
> > ../subprojects/libvhost-user/libvhost-user.c
> > In file included from ../subprojects/libvhost-user/include/atomic.h:18,
> > from ../subprojects/libvhost-user/libvhost-user.c:53:
> > ../subprojects/libvhost-user/include/compiler.h:38:40: error: missing
> > binary operator before token "("
> > 38 | #if defined(__clang__) && __has_warning("-Waddress-of-packed-member")
> > | ^
> > 
> > https://gitlab.com/qemu-project/qemu/-/jobs/4981576093
> 
> IIRC older versions of GCC do not have __has_warning() yet, so if you want
> to use this in compiler.h, you have to do it below the line in compiler.h
> that adds this:
> 
> #ifndef __has_warning
> #define __has_warning(x) 0 /* compatibility with non-clang compilers */
> #endif

We should probably move those basic __has_XXX compat defs right to
the top of compiler.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 :|

Re: [PULL 0/1] Quick fix patches

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Wed, Aug 30, 2023, 7:26 AM Thomas Huth <thuth@redhat.com> wrote:

> On 30/08/2023 15.16, Stefan Hajnoczi wrote:
> > Hi,
> > The patch introduces the following build failure:
> >
> > cc -m64 -mcx16 -Isubprojects/libvhost-user/libvhost-user.a.p
> > -Isubprojects/libvhost-user -I../subprojects/libvhost-user
> > -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g
> > -Wsign-compare -Wdeclaration-after-statement -Wstrict-aliasing
> > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
> > -fno-common -fwrapv -fPIE -pthread -D_GNU_SOURCE -MD -MQ
> > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -MF
> > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o.d -o
> > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -c
> > ../subprojects/libvhost-user/libvhost-user.c
> > In file included from ../subprojects/libvhost-user/include/atomic.h:18,
> > from ../subprojects/libvhost-user/libvhost-user.c:53:
> > ../subprojects/libvhost-user/include/compiler.h:38:40: error: missing
> > binary operator before token "("
> > 38 | #if defined(__clang__) &&
> __has_warning("-Waddress-of-packed-member")
> > | ^
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/4981576093
>
> IIRC older versions of GCC do not have __has_warning() yet, so if you want
> to use this in compiler.h, you have to do it below the line in compiler.h
> that adds this:
>
> #ifndef __has_warning
> #define __has_warning(x) 0 /* compatibility with non-clang compilers */
> #endif
>

This already works for linux-user. If there are gcc versions that break,
our current CI jobs don't show it. Why add complexity for unsupported gcc
versions? And how do I know I got it right?

I'm really starting to think the feedback 'move it to compilers.h' should
have just been ignored... it's turning into a lot of my time to correct
that I don't have when I'm also out of CI minutes to test with.

Warner





>   HTH,
>    Thomas
>
>

[-- Attachment #2: Type: text/html, Size: 2982 bytes --]

Re: [PULL 0/1] Quick fix patches

[Daniel P. Berrangé]

On Wed, Aug 30, 2023 at 08:31:15AM -0600, Warner Losh wrote:
> On Wed, Aug 30, 2023, 7:26 AM Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 30/08/2023 15.16, Stefan Hajnoczi wrote:
> > > Hi,
> > > The patch introduces the following build failure:
> > >
> > > cc -m64 -mcx16 -Isubprojects/libvhost-user/libvhost-user.a.p
> > > -Isubprojects/libvhost-user -I../subprojects/libvhost-user
> > > -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu99 -O2 -g
> > > -Wsign-compare -Wdeclaration-after-statement -Wstrict-aliasing
> > > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
> > > -fno-common -fwrapv -fPIE -pthread -D_GNU_SOURCE -MD -MQ
> > > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -MF
> > > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o.d -o
> > > subprojects/libvhost-user/libvhost-user.a.p/libvhost-user.c.o -c
> > > ../subprojects/libvhost-user/libvhost-user.c
> > > In file included from ../subprojects/libvhost-user/include/atomic.h:18,
> > > from ../subprojects/libvhost-user/libvhost-user.c:53:
> > > ../subprojects/libvhost-user/include/compiler.h:38:40: error: missing
> > > binary operator before token "("
> > > 38 | #if defined(__clang__) &&
> > __has_warning("-Waddress-of-packed-member")
> > > | ^
> > >
> > > https://gitlab.com/qemu-project/qemu/-/jobs/4981576093
> >
> > IIRC older versions of GCC do not have __has_warning() yet, so if you want
> > to use this in compiler.h, you have to do it below the line in compiler.h
> > that adds this:
> >
> > #ifndef __has_warning
> > #define __has_warning(x) 0 /* compatibility with non-clang compilers */
> > #endif
> >
> 
> This already works for linux-user. If there are gcc versions that break,
> our current CI jobs don't show it. Why add complexity for unsupported gcc
> versions? And how do I know I got it right?

IIUC, /no/ GCC version has __has_warning. The no-op stub we have works
because we merely need the preprocessor to be able to parse the
expression

  #if defined(__clang__) && __has_warning("....")

it'll never actually evaluate the __has_warning clause under GCC
because the defined(__clang__) will be false.

> I'm really starting to think the feedback 'move it to compilers.h' should
> have just been ignored... it's turning into a lot of my time to correct
> that I don't have when I'm also out of CI minutes to test with.

FWIW, if you have a Linux VM with docker/podman present it also possible
to run the CI environments locally with 'make docker-help' has more info

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