* [PATCH] gitlab: remove duplication between msys jobs
@ 2023-07-26 16:19 Daniel P. Berrangé
2023-07-26 18:21 ` Thomas Huth
0 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrangé @ 2023-07-26 16:19 UTC (permalink / raw)
To: qemu-devel
Cc: Beraldo Leal, Wainer dos Santos Moschetta, Yonggang Luo,
Philippe Mathieu-Daudé,
Thomas Huth, Alex Bennée, Daniel P. Berrangé
Although they share a common parent, the two msys jobs still have
massive duplication in their script definitions that can easily be
collapsed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
.gitlab-ci.d/windows.yml | 132 +++++++++++++++------------------------
1 file changed, 49 insertions(+), 83 deletions(-)
diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index f889a468b5..f086540e40 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -35,97 +35,63 @@
- .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu' # Core update
- .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu' # Normal update
- taskkill /F /FI "MODULES eq msys-2.0.dll"
-
-msys2-64bit:
- extends: .shared_msys2_builder
script:
- .\msys64\usr\bin\bash -lc "pacman -Sy --noconfirm --needed
bison diffutils flex
git grep make sed
- mingw-w64-x86_64-capstone
- mingw-w64-x86_64-curl
- mingw-w64-x86_64-cyrus-sasl
- mingw-w64-x86_64-dtc
- mingw-w64-x86_64-gcc
- mingw-w64-x86_64-glib2
- mingw-w64-x86_64-gnutls
- mingw-w64-x86_64-gtk3
- mingw-w64-x86_64-libgcrypt
- mingw-w64-x86_64-libjpeg-turbo
- mingw-w64-x86_64-libnfs
- mingw-w64-x86_64-libpng
- mingw-w64-x86_64-libssh
- mingw-w64-x86_64-libtasn1
- mingw-w64-x86_64-libusb
- mingw-w64-x86_64-lzo2
- mingw-w64-x86_64-nettle
- mingw-w64-x86_64-ninja
- mingw-w64-x86_64-pixman
- mingw-w64-x86_64-pkgconf
- mingw-w64-x86_64-python
- mingw-w64-x86_64-SDL2
- mingw-w64-x86_64-SDL2_image
- mingw-w64-x86_64-snappy
- mingw-w64-x86_64-spice
- mingw-w64-x86_64-usbredir
- mingw-w64-x86_64-zstd "
+ $MINGW_TARGET-capstone
+ $MINGW_TARGET-curl
+ $MINGW_TARGET-cyrus-sasl
+ $MINGW_TARGET-dtc
+ $MINGW_TARGET-gcc
+ $MINGW_TARGET-glib2
+ $MINGW_TARGET-gnutls
+ $MINGW_TARGET-gtk3
+ $MINGW_TARGET-libgcrypt
+ $MINGW_TARGET-libjpeg-turbo
+ $MINGW_TARGET-libnfs
+ $MINGW_TARGET-libpng
+ $MINGW_TARGET-libssh
+ $MINGW_TARGET-libtasn1
+ $MINGW_TARGET-libusb
+ $MINGW_TARGET-lzo2
+ $MINGW_TARGET-nettle
+ $MINGW_TARGET-ninja
+ $MINGW_TARGET-pixman
+ $MINGW_TARGET-pkgconf
+ $MINGW_TARGET-python
+ $MINGW_TARGET-SDL2
+ $MINGW_TARGET-SDL2_image
+ $MINGW_TARGET-snappy
+ $MINGW_TARGET-spice
+ $MINGW_TARGET-usbredir
+ $MINGW_TARGET-zstd "
- $env:CHERE_INVOKING = 'yes' # Preserve the current working directory
- - $env:MSYSTEM = 'MINGW64' # Start a 64-bit MinGW environment
- $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
- mkdir build
- cd build
- # Note: do not remove "--without-default-devices"!
- # commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using --without-default-devices"
- # changed to compile QEMU with the --without-default-devices switch
- # for the msys2 64-bit job, due to the build could not complete within
- # the project timeout.
- - ..\msys64\usr\bin\bash -lc '../configure --target-list=x86_64-softmmu
- --without-default-devices --enable-fdt=system'
- - ..\msys64\usr\bin\bash -lc 'make'
- # qTests don't run successfully with "--without-default-devices",
- # so let's exclude the qtests from CI for now.
- - ..\msys64\usr\bin\bash -lc 'make check MTESTARGS=\"--no-suite qtest\" || { cat meson-logs/testlog.txt; exit 1; } ;'
+ - ..\msys64\usr\bin\bash -lc "../configure --enable-fdt=system $CONFIGURE_ARGS"
+ - ..\msys64\usr\bin\bash -lc "make"
+ - ..\msys64\usr\bin\bash -lc "make check MTESTARGS='$TEST_ARGS' || { cat meson-logs/testlog.txt; exit 1; } ;"
+
+msys2-64bit:
+ extends: .shared_msys2_builder
+ variables:
+ MINGW_TARGET: mingw-w64-x86_64
+ MSYSTEM: MINGW64
+ # do not remove "--without-default-devices"!
+ # commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using --without-default-devices"
+ # changed to compile QEMU with the --without-default-devices switch
+ # for the msys2 64-bit job, due to the build could not complete within
+ CONFIGURE_ARGS: --target-list=x86_64-softmmu --without-default-devices
+ # qTests don't run successfully with "--without-default-devices",
+ # so let's exclude the qtests from CI for now.
+ TEST_ARGS: --no-suite qtest
msys2-32bit:
extends: .shared_msys2_builder
- script:
- - .\msys64\usr\bin\bash -lc "pacman -Sy --noconfirm --needed
- bison diffutils flex
- git grep make sed
- mingw-w64-i686-capstone
- mingw-w64-i686-curl
- mingw-w64-i686-cyrus-sasl
- mingw-w64-i686-dtc
- mingw-w64-i686-gcc
- mingw-w64-i686-glib2
- mingw-w64-i686-gnutls
- mingw-w64-i686-gtk3
- mingw-w64-i686-libgcrypt
- mingw-w64-i686-libjpeg-turbo
- mingw-w64-i686-libnfs
- mingw-w64-i686-libpng
- mingw-w64-i686-libssh
- mingw-w64-i686-libtasn1
- mingw-w64-i686-libusb
- mingw-w64-i686-lzo2
- mingw-w64-i686-nettle
- mingw-w64-i686-ninja
- mingw-w64-i686-pixman
- mingw-w64-i686-pkgconf
- mingw-w64-i686-python
- mingw-w64-i686-SDL2
- mingw-w64-i686-SDL2_image
- mingw-w64-i686-snappy
- mingw-w64-i686-spice
- mingw-w64-i686-usbredir
- mingw-w64-i686-zstd "
- - $env:CHERE_INVOKING = 'yes' # Preserve the current working directory
- - $env:MSYSTEM = 'MINGW32' # Start a 32-bit MinGW environment
- - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
- - mkdir build
- - cd build
- - ..\msys64\usr\bin\bash -lc '../configure --target-list=ppc64-softmmu
- --enable-fdt=system'
- - ..\msys64\usr\bin\bash -lc 'make'
- - ..\msys64\usr\bin\bash -lc 'make check MTESTARGS=\"--no-suite qtest\" ||
- { cat meson-logs/testlog.txt; exit 1; }'
+ variables:
+ MINGW_TARGET: mingw-w64-i686
+ MSYSTEM: MINGW32
+ CONFIGURE_ARGS: --target-list=ppc64-softmmu
+ TEST_ARGS: --no-suite qtest
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-26 16:19 [PATCH] gitlab: remove duplication between msys jobs Daniel P. Berrangé
@ 2023-07-26 18:21 ` Thomas Huth
2023-07-26 18:42 ` Marc-André Lureau
2023-07-27 10:39 ` Daniel P. Berrangé
0 siblings, 2 replies; 10+ messages in thread
From: Thomas Huth @ 2023-07-26 18:21 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Beraldo Leal, Wainer dos Santos Moschetta, Yonggang Luo,
Philippe Mathieu-Daudé,
Alex Bennée, Marc-André Lureau, Bin Meng
On 26/07/2023 18.19, Daniel P. Berrangé wrote:
> Although they share a common parent, the two msys jobs still have
> massive duplication in their script definitions that can easily be
> collapsed.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> .gitlab-ci.d/windows.yml | 132 +++++++++++++++------------------------
> 1 file changed, 49 insertions(+), 83 deletions(-)
We originally had different sets of packages in the 32-bit and 64-bit jobs,
to distribute the load between the two jobs ... but it got unified in commit
14547e0877f3522. Now considering that we are facing timeouts again, we
should maybe rather revert that commit instead of unifying the lists forever?
Anyway, before we unify the compiler package name suffix between the two
jobs, I really would like to see whether the mingw Clang builds QEMU faster
in the 64-bit job ... but so far I failed to convince meson to accept the
Clang from the mingw package ... does anybody know how to use Clang with
MSYS2 properly?
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-26 18:21 ` Thomas Huth
@ 2023-07-26 18:42 ` Marc-André Lureau
2023-07-27 10:39 ` Daniel P. Berrangé
1 sibling, 0 replies; 10+ messages in thread
From: Marc-André Lureau @ 2023-07-26 18:42 UTC (permalink / raw)
To: Thomas Huth
Cc: Daniel P. Berrangé,
qemu-devel, Beraldo Leal, Wainer dos Santos Moschetta,
Yonggang Luo, Philippe Mathieu-Daudé,
Alex Bennée, Bin Meng
Hi
On Wed, Jul 26, 2023 at 10:21 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 26/07/2023 18.19, Daniel P. Berrangé wrote:
> > Although they share a common parent, the two msys jobs still have
> > massive duplication in their script definitions that can easily be
> > collapsed.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > .gitlab-ci.d/windows.yml | 132 +++++++++++++++------------------------
> > 1 file changed, 49 insertions(+), 83 deletions(-)
>
> We originally had different sets of packages in the 32-bit and 64-bit jobs,
> to distribute the load between the two jobs ... but it got unified in commit
> 14547e0877f3522. Now considering that we are facing timeouts again, we
> should maybe rather revert that commit instead of unifying the lists forever?
>
> Anyway, before we unify the compiler package name suffix between the two
> jobs, I really would like to see whether the mingw Clang builds QEMU faster
> in the 64-bit job ... but so far I failed to convince meson to accept the
> Clang from the mingw package ... does anybody know how to use Clang with
> MSYS2 properly?
I checked it this week (because of bug #1782), and it compiled
successfully. Although I think we may have some issues with clang on
windows, as it doesn't pack struct the expected way. See also:
https://discourse.llvm.org/t/how-to-undo-the-effect-of-mms-bitfields/72271.
It may be a good idea to add some extra static checks about our packed
struct padding expectations..
Eh, it didn't feel much faster to compile with clang :)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-26 18:21 ` Thomas Huth
2023-07-26 18:42 ` Marc-André Lureau
@ 2023-07-27 10:39 ` Daniel P. Berrangé
2023-07-28 8:35 ` Thomas Huth
1 sibling, 1 reply; 10+ messages in thread
From: Daniel P. Berrangé @ 2023-07-27 10:39 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Beraldo Leal, Wainer dos Santos Moschetta,
Yonggang Luo, Philippe Mathieu-Daudé,
Alex Bennée, Marc-André Lureau, Bin Meng
On Wed, Jul 26, 2023 at 08:21:33PM +0200, Thomas Huth wrote:
> On 26/07/2023 18.19, Daniel P. Berrangé wrote:
> > Although they share a common parent, the two msys jobs still have
> > massive duplication in their script definitions that can easily be
> > collapsed.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > .gitlab-ci.d/windows.yml | 132 +++++++++++++++------------------------
> > 1 file changed, 49 insertions(+), 83 deletions(-)
>
> We originally had different sets of packages in the 32-bit and 64-bit jobs,
> to distribute the load between the two jobs ... but it got unified in commit
> 14547e0877f3522. Now considering that we are facing timeouts again, we
> should maybe rather revert that commit instead of unifying the lists
> forever?
IMHO we shouldn't be maintaining package lists at all, because they get
stale over time. IOW, I would like to make this job use the package list
from lcitool.
We could get the same end result, however, but having a package exclude
list defined as a variable per job
variables:
MINGW_TARGET: mingw-w64-i686
MSYSTEM: MINGW32
CONFIGURE_ARGS: --target-list=ppc64-softmmu
TEST_ARGS: --no-suite qtest
PACKAGE_EXCLUDE: mingw-w64-gtk3 mingw-w64-nettle mingw-w64-zstd
> Anyway, before we unify the compiler package name suffix between the two
> jobs, I really would like to see whether the mingw Clang builds QEMU faster
> in the 64-bit job ... but so far I failed to convince meson to accept the
> Clang from the mingw package ... does anybody know how to use Clang with
> MSYS2 properly?
AFAIK it shouldn't be anything worse than
CC=clang ./configure ....
if that doesn't work then its a bug IMHO
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-27 10:39 ` Daniel P. Berrangé
@ 2023-07-28 8:35 ` Thomas Huth
2023-07-28 8:58 ` Daniel P. Berrangé
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2023-07-28 8:35 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Beraldo Leal, Wainer dos Santos Moschetta,
Yonggang Luo, Philippe Mathieu-Daudé,
Alex Bennée, Marc-André Lureau, Bin Meng
On 27/07/2023 12.39, Daniel P. Berrangé wrote:
> On Wed, Jul 26, 2023 at 08:21:33PM +0200, Thomas Huth wrote:
>> On 26/07/2023 18.19, Daniel P. Berrangé wrote:
...
>> Anyway, before we unify the compiler package name suffix between the two
>> jobs, I really would like to see whether the mingw Clang builds QEMU faster
>> in the 64-bit job ... but so far I failed to convince meson to accept the
>> Clang from the mingw package ... does anybody know how to use Clang with
>> MSYS2 properly?
>
> AFAIK it shouldn't be anything worse than
>
> CC=clang ./configure ....
>
> if that doesn't work then its a bug IMHO
No, it's not that easy ... As Marc-André explained to me, MSYS2 maintains a
completely separate environment for Clang, i.e. you have to select this
different environment with $env:MSYSTEM = 'CLANG64' and then install the
packages that have the "mingw-w64-clang-x86_64-" prefix.
After lots of trial and error, I was able to get a test build here:
https://gitlab.com/thuth/qemu/-/jobs/4758605925
I had to disable Spice and use --disable-werror in that build to make it
succeed, but at least it shows that Clang seems to be a little bit faster -
the job finished in 58 minutes. So if we can get the warnings fixed, this
might be a solution for the timeouts here...
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-28 8:35 ` Thomas Huth
@ 2023-07-28 8:58 ` Daniel P. Berrangé
2023-07-28 9:32 ` Marc-André Lureau
0 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrangé @ 2023-07-28 8:58 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Beraldo Leal, Wainer dos Santos Moschetta,
Yonggang Luo, Philippe Mathieu-Daudé,
Alex Bennée, Marc-André Lureau, Bin Meng
On Fri, Jul 28, 2023 at 10:35:35AM +0200, Thomas Huth wrote:
> On 27/07/2023 12.39, Daniel P. Berrangé wrote:
> > On Wed, Jul 26, 2023 at 08:21:33PM +0200, Thomas Huth wrote:
> > > On 26/07/2023 18.19, Daniel P. Berrangé wrote:
> ...
> > > Anyway, before we unify the compiler package name suffix between the two
> > > jobs, I really would like to see whether the mingw Clang builds QEMU faster
> > > in the 64-bit job ... but so far I failed to convince meson to accept the
> > > Clang from the mingw package ... does anybody know how to use Clang with
> > > MSYS2 properly?
> >
> > AFAIK it shouldn't be anything worse than
> >
> > CC=clang ./configure ....
> >
> > if that doesn't work then its a bug IMHO
>
> No, it's not that easy ... As Marc-André explained to me, MSYS2 maintains a
> completely separate environment for Clang, i.e. you have to select this
> different environment with $env:MSYSTEM = 'CLANG64' and then install the
> packages that have the "mingw-w64-clang-x86_64-" prefix.
>
> After lots of trial and error, I was able to get a test build here:
>
> https://gitlab.com/thuth/qemu/-/jobs/4758605925
>
> I had to disable Spice and use --disable-werror in that build to make it
> succeed, but at least it shows that Clang seems to be a little bit faster -
> the job finished in 58 minutes. So if we can get the warnings fixed, this
> might be a solution for the timeouts here...
Those packing warnings look pretty serious
C:/GitLab-Runner/builds/thuth/qemu/include/block/nvme.h:1781:16: warning: unknown attribute 'gcc_struct' ignored [-Wunknown-attributes]
This means CLang is using the MSVC struct packing ABI for bitfields,
which is different from the GCC struct packing ABI. If any of those
structs use bitfields and are exposed as guest hardware ABI, or in
migration vmstate, then this is potentially broken compilation.
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-28 8:58 ` Daniel P. Berrangé
@ 2023-07-28 9:32 ` Marc-André Lureau
2023-07-28 9:50 ` Thomas Huth
0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2023-07-28 9:32 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Thomas Huth, qemu-devel, Beraldo Leal,
Wainer dos Santos Moschetta, Yonggang Luo,
Philippe Mathieu-Daudé,
Alex Bennée, Bin Meng
Hi
On Fri, Jul 28, 2023 at 12:59 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Jul 28, 2023 at 10:35:35AM +0200, Thomas Huth wrote:
> > On 27/07/2023 12.39, Daniel P. Berrangé wrote:
> > > On Wed, Jul 26, 2023 at 08:21:33PM +0200, Thomas Huth wrote:
> > > > On 26/07/2023 18.19, Daniel P. Berrangé wrote:
> > ...
> > > > Anyway, before we unify the compiler package name suffix between the two
> > > > jobs, I really would like to see whether the mingw Clang builds QEMU faster
> > > > in the 64-bit job ... but so far I failed to convince meson to accept the
> > > > Clang from the mingw package ... does anybody know how to use Clang with
> > > > MSYS2 properly?
> > >
> > > AFAIK it shouldn't be anything worse than
> > >
> > > CC=clang ./configure ....
> > >
> > > if that doesn't work then its a bug IMHO
> >
> > No, it's not that easy ... As Marc-André explained to me, MSYS2 maintains a
> > completely separate environment for Clang, i.e. you have to select this
> > different environment with $env:MSYSTEM = 'CLANG64' and then install the
> > packages that have the "mingw-w64-clang-x86_64-" prefix.
> >
> > After lots of trial and error, I was able to get a test build here:
> >
> > https://gitlab.com/thuth/qemu/-/jobs/4758605925
> >
> > I had to disable Spice and use --disable-werror in that build to make it
> > succeed, but at least it shows that Clang seems to be a little bit faster -
> > the job finished in 58 minutes. So if we can get the warnings fixed, this
> > might be a solution for the timeouts here...
>
> Those packing warnings look pretty serious
>
> C:/GitLab-Runner/builds/thuth/qemu/include/block/nvme.h:1781:16: warning: unknown attribute 'gcc_struct' ignored [-Wunknown-attributes]
>
> This means CLang is using the MSVC struct packing ABI for bitfields,
> which is different from the GCC struct packing ABI. If any of those
> structs use bitfields and are exposed as guest hardware ABI, or in
> migration vmstate, then this is potentially broken compilation.
>
Yes .. gcc >=4.7 and clang >=12 have mms-bitfiles enabled by default,
but we can't undo that MS struct packing on clang apparently:
https://discourse.llvm.org/t/how-to-undo-the-effect-of-mms-bitfields/72271
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-28 9:32 ` Marc-André Lureau
@ 2023-07-28 9:50 ` Thomas Huth
2023-07-28 10:01 ` Thomas Huth
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2023-07-28 9:50 UTC (permalink / raw)
To: Marc-André Lureau, Daniel P. Berrangé
Cc: qemu-devel, Beraldo Leal, Wainer dos Santos Moschetta,
Yonggang Luo, Philippe Mathieu-Daudé,
Alex Bennée, Bin Meng
On 28/07/2023 11.32, Marc-André Lureau wrote:
> Hi
>
> On Fri, Jul 28, 2023 at 12:59 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Fri, Jul 28, 2023 at 10:35:35AM +0200, Thomas Huth wrote:
>>> On 27/07/2023 12.39, Daniel P. Berrangé wrote:
>>>> On Wed, Jul 26, 2023 at 08:21:33PM +0200, Thomas Huth wrote:
>>>>> On 26/07/2023 18.19, Daniel P. Berrangé wrote:
>>> ...
>>>>> Anyway, before we unify the compiler package name suffix between the two
>>>>> jobs, I really would like to see whether the mingw Clang builds QEMU faster
>>>>> in the 64-bit job ... but so far I failed to convince meson to accept the
>>>>> Clang from the mingw package ... does anybody know how to use Clang with
>>>>> MSYS2 properly?
>>>>
>>>> AFAIK it shouldn't be anything worse than
>>>>
>>>> CC=clang ./configure ....
>>>>
>>>> if that doesn't work then its a bug IMHO
>>>
>>> No, it's not that easy ... As Marc-André explained to me, MSYS2 maintains a
>>> completely separate environment for Clang, i.e. you have to select this
>>> different environment with $env:MSYSTEM = 'CLANG64' and then install the
>>> packages that have the "mingw-w64-clang-x86_64-" prefix.
>>>
>>> After lots of trial and error, I was able to get a test build here:
>>>
>>> https://gitlab.com/thuth/qemu/-/jobs/4758605925
>>>
>>> I had to disable Spice and use --disable-werror in that build to make it
>>> succeed, but at least it shows that Clang seems to be a little bit faster -
>>> the job finished in 58 minutes. So if we can get the warnings fixed, this
>>> might be a solution for the timeouts here...
>>
>> Those packing warnings look pretty serious
>>
>> C:/GitLab-Runner/builds/thuth/qemu/include/block/nvme.h:1781:16: warning: unknown attribute 'gcc_struct' ignored [-Wunknown-attributes]
>>
>> This means CLang is using the MSVC struct packing ABI for bitfields,
>> which is different from the GCC struct packing ABI. If any of those
>> structs use bitfields and are exposed as guest hardware ABI, or in
>> migration vmstate, then this is potentially broken compilation.
>>
>
> Yes .. gcc >=4.7 and clang >=12 have mms-bitfiles enabled by default,
> but we can't undo that MS struct packing on clang apparently:
> https://discourse.llvm.org/t/how-to-undo-the-effect-of-mms-bitfields/72271
I wonder whether we really still need the gcc_struct in QEMU...
As far as I understand, this was mainly required for bitfields in packed
structs in the past, but using bitfields in structs for exchanging data with
other systems is anyway not very portable (e.g. due to endianess issues) and
thus frowned upon nowdays.
Looking at the history:
https://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03441.html
... this indicates that this was mainly required for slirp - which has been
removed from the QEMU repository and is a separate project nowadays.
And https://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02356.html
mentiones that there might have been issues with the bluetooth code, too,
but that is also long gone...
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-28 9:50 ` Thomas Huth
@ 2023-07-28 10:01 ` Thomas Huth
2023-07-28 10:39 ` Daniel P. Berrangé
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2023-07-28 10:01 UTC (permalink / raw)
To: Marc-André Lureau, Daniel P. Berrangé
Cc: qemu-devel, Beraldo Leal, Wainer dos Santos Moschetta,
Yonggang Luo, Philippe Mathieu-Daudé,
Alex Bennée, Bin Meng
On 28/07/2023 11.50, Thomas Huth wrote:
> On 28/07/2023 11.32, Marc-André Lureau wrote:
>> Hi
>>
>> On Fri, Jul 28, 2023 at 12:59 PM Daniel P. Berrangé <berrange@redhat.com>
>> wrote:
>>>
>>> On Fri, Jul 28, 2023 at 10:35:35AM +0200, Thomas Huth wrote:
>>>> On 27/07/2023 12.39, Daniel P. Berrangé wrote:
>>>>> On Wed, Jul 26, 2023 at 08:21:33PM +0200, Thomas Huth wrote:
>>>>>> On 26/07/2023 18.19, Daniel P. Berrangé wrote:
>>>> ...
>>>>>> Anyway, before we unify the compiler package name suffix between the two
>>>>>> jobs, I really would like to see whether the mingw Clang builds QEMU
>>>>>> faster
>>>>>> in the 64-bit job ... but so far I failed to convince meson to accept the
>>>>>> Clang from the mingw package ... does anybody know how to use Clang with
>>>>>> MSYS2 properly?
>>>>>
>>>>> AFAIK it shouldn't be anything worse than
>>>>>
>>>>> CC=clang ./configure ....
>>>>>
>>>>> if that doesn't work then its a bug IMHO
>>>>
>>>> No, it's not that easy ... As Marc-André explained to me, MSYS2 maintains a
>>>> completely separate environment for Clang, i.e. you have to select this
>>>> different environment with $env:MSYSTEM = 'CLANG64' and then install the
>>>> packages that have the "mingw-w64-clang-x86_64-" prefix.
>>>>
>>>> After lots of trial and error, I was able to get a test build here:
>>>>
>>>> https://gitlab.com/thuth/qemu/-/jobs/4758605925
>>>>
>>>> I had to disable Spice and use --disable-werror in that build to make it
>>>> succeed, but at least it shows that Clang seems to be a little bit faster -
>>>> the job finished in 58 minutes. So if we can get the warnings fixed, this
>>>> might be a solution for the timeouts here...
>>>
>>> Those packing warnings look pretty serious
>>>
>>> C:/GitLab-Runner/builds/thuth/qemu/include/block/nvme.h:1781:16: warning:
>>> unknown attribute 'gcc_struct' ignored [-Wunknown-attributes]
>>>
>>> This means CLang is using the MSVC struct packing ABI for bitfields,
>>> which is different from the GCC struct packing ABI. If any of those
>>> structs use bitfields and are exposed as guest hardware ABI, or in
>>> migration vmstate, then this is potentially broken compilation.
>>>
>>
>> Yes .. gcc >=4.7 and clang >=12 have mms-bitfiles enabled by default,
>> but we can't undo that MS struct packing on clang apparently:
>> https://discourse.llvm.org/t/how-to-undo-the-effect-of-mms-bitfields/72271
>
> I wonder whether we really still need the gcc_struct in QEMU...
> As far as I understand, this was mainly required for bitfields in packed
> structs in the past
Ok, never mind, according to this post:
https://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00964.html
this affects all structs, not only the ones with bitfieds.
And it seems like we also still have packed structs with bitfields in code
base, see e.g. "struct ip" in net/util.h, so using Clang on Windows likely
currently can't work?
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitlab: remove duplication between msys jobs
2023-07-28 10:01 ` Thomas Huth
@ 2023-07-28 10:39 ` Daniel P. Berrangé
0 siblings, 0 replies; 10+ messages in thread
From: Daniel P. Berrangé @ 2023-07-28 10:39 UTC (permalink / raw)
To: Thomas Huth
Cc: Marc-André Lureau, qemu-devel, Beraldo Leal,
Wainer dos Santos Moschetta, Yonggang Luo,
Philippe Mathieu-Daudé,
Alex Bennée, Bin Meng
On Fri, Jul 28, 2023 at 12:01:29PM +0200, Thomas Huth wrote:
> On 28/07/2023 11.50, Thomas Huth wrote:
> > On 28/07/2023 11.32, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Fri, Jul 28, 2023 at 12:59 PM Daniel P. Berrangé
> > > <berrange@redhat.com> wrote:
> > > >
> > > > On Fri, Jul 28, 2023 at 10:35:35AM +0200, Thomas Huth wrote:
> > > > > On 27/07/2023 12.39, Daniel P. Berrangé wrote:
> > > > > > On Wed, Jul 26, 2023 at 08:21:33PM +0200, Thomas Huth wrote:
> > > > > > > On 26/07/2023 18.19, Daniel P. Berrangé wrote:
> > > > > ...
> > > > > > > Anyway, before we unify the compiler package name suffix between the two
> > > > > > > jobs, I really would like to see whether the mingw
> > > > > > > Clang builds QEMU faster
> > > > > > > in the 64-bit job ... but so far I failed to convince meson to accept the
> > > > > > > Clang from the mingw package ... does anybody know how to use Clang with
> > > > > > > MSYS2 properly?
> > > > > >
> > > > > > AFAIK it shouldn't be anything worse than
> > > > > >
> > > > > > CC=clang ./configure ....
> > > > > >
> > > > > > if that doesn't work then its a bug IMHO
> > > > >
> > > > > No, it's not that easy ... As Marc-André explained to me, MSYS2 maintains a
> > > > > completely separate environment for Clang, i.e. you have to select this
> > > > > different environment with $env:MSYSTEM = 'CLANG64' and then install the
> > > > > packages that have the "mingw-w64-clang-x86_64-" prefix.
> > > > >
> > > > > After lots of trial and error, I was able to get a test build here:
> > > > >
> > > > > https://gitlab.com/thuth/qemu/-/jobs/4758605925
> > > > >
> > > > > I had to disable Spice and use --disable-werror in that build to make it
> > > > > succeed, but at least it shows that Clang seems to be a little bit faster -
> > > > > the job finished in 58 minutes. So if we can get the warnings fixed, this
> > > > > might be a solution for the timeouts here...
> > > >
> > > > Those packing warnings look pretty serious
> > > >
> > > > C:/GitLab-Runner/builds/thuth/qemu/include/block/nvme.h:1781:16:
> > > > warning: unknown attribute 'gcc_struct' ignored
> > > > [-Wunknown-attributes]
> > > >
> > > > This means CLang is using the MSVC struct packing ABI for bitfields,
> > > > which is different from the GCC struct packing ABI. If any of those
> > > > structs use bitfields and are exposed as guest hardware ABI, or in
> > > > migration vmstate, then this is potentially broken compilation.
> > > >
> > >
> > > Yes .. gcc >=4.7 and clang >=12 have mms-bitfiles enabled by default,
> > > but we can't undo that MS struct packing on clang apparently:
> > > https://discourse.llvm.org/t/how-to-undo-the-effect-of-mms-bitfields/72271
> >
> > I wonder whether we really still need the gcc_struct in QEMU...
> > As far as I understand, this was mainly required for bitfields in packed
> > structs in the past
>
> Ok, never mind, according to this post:
>
> https://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00964.html
>
> this affects all structs, not only the ones with bitfieds.
>
> And it seems like we also still have packed structs with bitfields in code
> base, see e.g. "struct ip" in net/util.h, so using Clang on Windows likely
> currently can't work?
Just because it has bitfields doesn't mean it will definitely be
different.
I'm not sure if it is an entirely accurate comparison, but I modified the
native linux build to use 'gcc_struct' and again to use 'ms_struct'. Then
fed all the .o files to 'pahole', and compared the output. There was only
a single difference:
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 */
/* Bitfield combined with previous fields */
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 18 */
+ } irte; /* 0 16 */
uint64_t data[2]; /* 0 16 */
};
from the intel_iommu.c file.
IOW, ms_struct added a 2 byte padding after the uint16_t source_id
field despite 'packed' attribute, but gcc_struct collapsed the
uint16_t into the uint64_t bitfield since only 48 bits were consumed.
IIUC, this could be made portable by changing
uint16_t source_id; /* 8 2 */
to
uint64_t source_id:16; /* 8 2 */
NB, this was a --target-list=x86_64-softmmu build only, so hasn't
covered the hole codebase. Still shows the gcc_struct annotation
might not be as critical as we imagined.
NB a limitation of the pahole analysis is that it only reports structs that
are actually declared as variabls somewhere - either stack allocated or
heap allocate is fine, as long as there's a declarion of usage somewhre.
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-07-28 11:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-26 16:19 [PATCH] gitlab: remove duplication between msys jobs Daniel P. Berrangé
2023-07-26 18:21 ` Thomas Huth
2023-07-26 18:42 ` Marc-André Lureau
2023-07-27 10:39 ` Daniel P. Berrangé
2023-07-28 8:35 ` Thomas Huth
2023-07-28 8:58 ` Daniel P. Berrangé
2023-07-28 9:32 ` Marc-André Lureau
2023-07-28 9:50 ` Thomas Huth
2023-07-28 10:01 ` Thomas Huth
2023-07-28 10:39 ` Daniel P. Berrangé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.