| Message ID | 20230728142748.305341-6-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use Clang for compiling in the 64-bit MSYS2 job | expand |
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. thanks -- PMM
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? Thomas
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
On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote: > 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? Is there any way we can separate out the "take 25 minutes to install a pile of packages" part from the "actually compile and test QEMU" part ? thanks -- PMM
On 31/07/2023 11.32, Daniel P. Berrangé wrote: > On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote: ... >> 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. I tried that two years ago, but the results were rather disappointing: https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02189.html ... but that was only with the Linux runners. Maybe it helps with the MSYS runners? Thomas
On 31/07/2023 12.05, Peter Maydell wrote: > On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote: >> 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? > > Is there any way we can separate out the "take 25 minutes to > install a pile of packages" part from the "actually compile and > test QEMU" part ? At least I don't have a clue ... we'd need someone who knows how to deal with that Windows stuff... Thomas
On Mon, Jul 31, 2023 at 11:05:42AM +0100, Peter Maydell wrote: > On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote: > > 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? > > Is there any way we can separate out the "take 25 minutes to > install a pile of packages" part from the "actually compile and > test QEMU" part ? Possibly, depends if msys installs all the packages to a specific sub-directory location, or splatters all over the hierarchy. If the former we might be able to put the whole installed sub-tree into the gitlab cache, which is likely faster to deploy. Testing this out in gitlab itself is kinda tedious, so needs a local windows VM to debug. If msys relies on the registry for anything during package install, then all bets are off, as I don't know how we'd cache & restore that. With regards, Daniel
On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote: > 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? The registerfields.h usage doesn't seem to be documented at all from the quick look I had. IOW, in addition to a mention in coding style, it would be nice if someone can document the general usage pattern for it. With regards, Daniel
On 31/7/23 14:57, Daniel P. Berrangé wrote: > On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote: >> 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? > > The registerfields.h usage doesn't seem to be documented at all from the > quick look I had. IOW, in addition to a mention in coding style, it would > be nice if someone can document the general usage pattern for it. Cc'ing ex-Xilinx folks. > With regards, > Daniel
On 31/7/23 11:32, Daniel P. Berrangé wrote: > 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. I tried that few years ago and this had very negative impact on custom runners (maybe I wasn't doing it correctly). Hopefully that changed. See some previous comments: https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02220.html > 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
On Mon, Jul 31, 2023 at 04:10:36PM +0200, Philippe Mathieu-Daudé wrote: > On 31/7/23 11:32, Daniel P. Berrangé wrote: > > > 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. > > I tried that few years ago and this had very negative impact on custom > runners (maybe I wasn't doing it correctly). Hopefully that changed. Our runner usage model has changed since then quite alot. What was previously mostly on shared runners, is now on Azure private runners. I can imagine it will vary tremendously on what you're using as a private runner. In the specific case of the windows jobs though, we're using the shared runners. Either way, if our jobs are all wired up for ccache correctly, it is then trivial to selectively turn off usage of ccache by just tweaking a few env vars. > See some previous comments: > https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02220.html > > > 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 > With regards, Daniel
On 31/7/23 19:25, Daniel P. Berrangé wrote: > On Mon, Jul 31, 2023 at 04:10:36PM +0200, Philippe Mathieu-Daudé wrote: >> On 31/7/23 11:32, Daniel P. Berrangé wrote: >> >>> 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. >> >> I tried that few years ago and this had very negative impact on custom >> runners (maybe I wasn't doing it correctly). Hopefully that changed. > > Our runner usage model has changed since then quite alot. What was previously > mostly on shared runners, is now on Azure private runners. I can imagine it > will vary tremendously on what you're using as a private runner. > > In the specific case of the windows jobs though, we're using the shared > runners. > > Either way, if our jobs are all wired up for ccache correctly, it is then > trivial to selectively turn off usage of ccache by just tweaking a few > env vars. IIRC we weren't using runner tag to filter, so jobs expected to run on shared runner were ending on private runner, and using env vars was not working. But you are right, the comments I pointed are obsolete, I clearly haven't followed all CI changes. Thanks for the tips, Phil.
On Mon, Jul 31, 2023 at 11:05:42AM +0100, Peter Maydell wrote: > On Mon, 31 Jul 2023 at 10:11, Thomas Huth <thuth@redhat.com> wrote: > > 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? > > Is there any way we can separate out the "take 25 minutes to > install a pile of packages" part from the "actually compile and > test QEMU" part ? I was thinking a little about what we actually aim to achieve with the Windows MSys2 jobs. We have long had the mingw cross compilation jobs for testing the Windows platform compile phase. Please correct me if I'm wrong, but IIUC, the msys2 jobs haven't identified any compilation problems that weren't already found from the linux mingw cross compile jobs. The definitely unique thing that the msys2 jobs *can* do though, is to run the test suite. We can't run the test suite from the mingw cross jobs unless we do something slightly crazy like adding Wine to the containers. In libvirt we considered the latter until we realized that the smallest possible wine install would still be enourmous. In the Linux world we have the jobs split into three stages * Create the container images (eg amd64-fedora-container) * Do the compilation work (eg build-system-fedora) * Run the test suite (eg check-system-fedora/avocado-system-fedora) If the value of the msys2 jobs is that they let us run the test suite, can we limit the usage of msys2 to just that part, and chain it upto the fedora mingw cross compile jobs ie. win64-fedora-cross-container | +-> cross-win64-system | +-> check-win64-msys In the "cross-win64-system" job we would have to publish the entire QEMU 'build' directory as an artifact, to pass it over to the msys job. If we also published /usr/x86_64-w64-mingw32/ as an artifact, then we would not need to install any mingw packages under msys. The basic msys installer can be run (which takes a couple of minutes), and then then we just dump the Fedora artifacts of /usr/x86_64-w64-mingw32/ into the right place and run the test suite. With regards, Daniel
On 01/08/2023 15.49, Daniel P. Berrangé wrote: ... > If the value of the msys2 jobs is that they let us run the test suite, > can we limit the usage of msys2 to just that part, and chain it upto > the fedora mingw cross compile jobs ie. > > win64-fedora-cross-container > | > +-> cross-win64-system > | > +-> check-win64-msys > > In the "cross-win64-system" job we would have to publish the entire QEMU > 'build' directory as an artifact, to pass it over to the msys job. If > we also published /usr/x86_64-w64-mingw32/ as an artifact, then we would > not need to install any mingw packages under msys. The basic msys installer > can be run (which takes a couple of minutes), and then then we just dump > the Fedora artifacts of /usr/x86_64-w64-mingw32/ into the right place > and run the test suite. It's a nice idea at a first glance - but at a second glance I doubt that this is easy to realize. You need the configured meson environment for running the tests, and you cannot easily pass that from the Fedora container to MSYS2. So you'd either need to re-run meson setup in the MSYS2 job and then trick it into believing that the binaries have already been built, or you have to run the test binaries "manually" without meson... both might be possible, but it sounds rather fragile to me. Thomas
On Mon, Jul 31, 2023 at 02:04:42PM +0200, Thomas Huth wrote: > On 31/07/2023 11.32, Daniel P. Berrangé wrote: > > On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote: > ... > > > 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. > > I tried that two years ago, but the results were rather disappointing: > > https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02189.html > > ... but that was only with the Linux runners. Maybe it helps with the MSYS > runners? I've done some tests over the last few days and my results are rather different. The speed up is *spectacular* if the hit rate is high. eg, consider you are retriggering broadly the same pipeline over & over with iterations of your patches. eg 99% of the QEMU code probably doesn't change as you're just re-trying to fix some edge case in 1 file, and thus you'll get near 100% hit rate. With such a case I see: The msys64 job can complete in 24 minutes, instead of 50-60 minutes The build-system-fedora job can complete in 6 minutes instead of 20 minutes The build-user-hexagon job can cmplete in 3 minutes instead of 6 Basically a 50% cut in time on all compile jobs I've looked at so far. NB, these are both shared runners, since the pipeline is in my fork. I've no idea what the impact will be on the Azure private runners for upstream. Given the CI limitations these days, such a speedup is massively beneficial for our contributors though. The downside is storage for the cache. In theory gitlab has defined a quota for storage per account. In practice they are still yet to enforce that. If they do ever enforce it, we're likely doomed as our container images alone will exceed it several times over, let alone leave room for build logs, job caches, artifacts, etc. While storage quota isn't enforced though, we might as well use ccache. With regards, Daniel
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))
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(-)