[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 8a45962be33d4f7449567f8b9e07c724733ca303:

  bsd-user: Move PRAGMA_DISABLE_PACKED_WARNING etc to qemu.h (2023-08-30 08:57:08 -0600)

----------------------------------------------------------------
Pull request: fix ci by fixing clang-user
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - https://gpgtools.org

iQIzBAABCgAdFiEEIDX4lLAKo898zeG3bBzRKH2wEQAFAmTvYpcACgkQbBzRKH2w
EQCKvhAA3nfvm+UMoRbOjFGuOOZKy+HR+nk40rxHa8CzOabbJnx4EtdBZTnNJ+f5
WxLvEIzQw6iLYoqr6Rkcn0MkfYryesDScig7tQW0A87DtivXbDyrRbcmAQuCrcSe
EpEqjBOueiC9xe1U9hzdpNyBkuXQWFUNGKX6swR5vpOEb1hHKzFMY+60vEgcjcjQ
/y7cQcwRJeMNyBEwAM4HSgcVIKxB8ZmIQpIbsWJPAJhEZZE6SvTiKhM0PlZvrwnv
dlEV8F68f3cBka0QmX5JJNJQdXn+2gb2Ix06dm8z7BWtiZoH/rcJ0FfVjMk7bkcC
2e0J0jI9JTf1MKNGHbO8V2YoZw+jCB9nr6N8HEymgla7bK6QPD6LIdTs0i6PUEB8
PUf902NrOS+kbWTGsb9GINegklk3pJ0jnFZagqfCUxARQ2qrqmA6q5vg9d0FjPVl
vJ13weKkSHW126/4Wr23EhUFIoYtiuMjgz/Bjd5TvzhvnVGJiPJaY9sGpgZZ3PJJ
EPCRms5MRut/NE0znKQ9ozAz1FMdVd2XnXeClWJd2NUmGc7ZbBGMcSvUYdC9KLIK
oSQRBTUDgGIdHnB+g367vKs98ir+03gOcpQk9z0fy25wCymmp/Uco8jsm6hCgSGn
x7b9R6/+92xd/IZFZm5N3/llu6JHYPEMhdg8evNp9VTFPoi9PYc=
=jG8p
-----END PGP SIGNATURE-----

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

Warner Losh (1):
  bsd-user: Move PRAGMA_DISABLE_PACKED_WARNING etc to qemu.h

 bsd-user/qemu.h         | 27 +++++++++++++++++++++++++++
 include/qemu/compiler.h | 30 ------------------------------
 2 files changed, 27 insertions(+), 30 deletions(-)

-- 
2.41.0

[PULL 1/1] bsd-user: Move PRAGMA_DISABLE_PACKED_WARNING etc to qemu.h

[Warner Losh]

For the moment, move PRAGMA_DISABLE_PACKED_WARNING and
PRAGMA_ENABLE_PACKED_WARNING back to bsd-user/qemu.h.

Of course, these should be in compiler.h, but that interferes with too
many things at the moment, so take one step back to unbreak clang
linux-user builds first. Use the exact same version that's in
linux-user/qemu.h since that's what should be in compiler.h.

Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 bsd-user/qemu.h         | 27 +++++++++++++++++++++++++++
 include/qemu/compiler.h | 30 ------------------------------
 2 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 4cfd5c63371..d3158bc2edd 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -273,6 +273,33 @@ static inline bool access_ok(int type, abi_ulong addr, abi_ulong size)
  * 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;                                      \
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index b0374425180..a309f90c768 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -22,36 +22,6 @@
 #define QEMU_EXTERN_C extern
 #endif
 
-/*
- * 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 .
- * - 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__
-#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          \
-    _Pragma("GCC diagnostic pop")
-
-#else
-#define PRAGMA_DISABLE_PACKED_WARNING
-#define PRAGMA_REENABLE_PACKED_WARNING
-#endif
-
 #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
 # define QEMU_PACKED __attribute__((gcc_struct, packed))
 #else
-- 
2.41.0

Re: [PULL 0/1] Quick fix patches

[Stefan Hajnoczi]

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

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[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

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