[PATCH 0/2] Fix atomic test in "configure" + bump FreeBSD CI to 12.2

[PATCH 0/2] Fix atomic test in "configure" + bump FreeBSD CI to 12.2

[Thomas Huth]

FreeBSD 12.1 is out of service now, so the Cirrus-CI task is failing due
to using some packages from 12.2 on the 12.1 image. We have to update
it to 12.2. However, there are two obstacles:

First, the check for the 128-bit atomic functions in the configure
script does not work right with the version of Clang in FreeBSD 12.2
anymore. Looks like the __atomic_*_16() functions do not have valid
prototypes anymore. First patch fixes this issue.

Second, there is a problem with libtasn1 and the latest version of
Clang that also triggers on FreeBSD 12.2. We have to disable gnutls
in this task until libtasn1 got fixed in FreeBSD.

Thomas Huth (2):
  configure: Don't use the __atomic_*_16 functions for testing 128-bit
    support
  cirrus.yml: Update the FreeBSD task to version 12.2

 .cirrus.yml | 5 +++--
 configure   | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.27.0

[PATCH 1/2] configure: Don't use the __atomic_*_16 functions for testing 128-bit support

[Thomas Huth]

The test for 128-bit atomics is causing trouble with FreeBSD 12.2 and
--enable-werror:

 cc -Werror -fPIE -DPIE -std=gnu99 -Wall -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong -o config-temp/qemu-conf.exe config-temp/qemu-conf.c -pie -Wl,-z,relro -Wl,-z,now -m64 -fstack-protector-strong
 config-temp/qemu-conf.c:4:7: error: implicit declaration of function '__atomic_load_16' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
   y = __atomic_load_16(&x, 0);
       ^
 config-temp/qemu-conf.c:5:3: error: implicit declaration of function '__atomic_store_16' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
   __atomic_store_16(&x, y, 0);
   ^
 config-temp/qemu-conf.c:5:3: note: did you mean '__atomic_load_16'?
 config-temp/qemu-conf.c:4:7: note: '__atomic_load_16' declared here
   y = __atomic_load_16(&x, 0);
       ^
 config-temp/qemu-conf.c:6:3: error: implicit declaration of function '__atomic_compare_exchange_16' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
   __atomic_compare_exchange_16(&x, &y, x, 0, 0, 0);
   ^
 3 errors generated.

Looking for they way we are using atomic functions in QEMU, we are not
using these functions with the _16 suffix anyway. Switch to the same
functions that we use in the include/qemu/atomic.h header.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index f7d022a5db..4526af87b2 100755
--- a/configure
+++ b/configure
@@ -4761,9 +4761,9 @@ if test "$int128" = "yes"; then
 int main(void)
 {
   unsigned __int128 x = 0, y = 0;
-  y = __atomic_load_16(&x, 0);
-  __atomic_store_16(&x, y, 0);
-  __atomic_compare_exchange_16(&x, &y, x, 0, 0, 0);
+  y = __atomic_load(&x, 0);
+  __atomic_store(&x, y, 0);
+  __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
   return 0;
 }
 EOF
-- 
2.27.0

[PATCH 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Thomas Huth]

FreeBSD version 12.1 is out of service now, and the task in the
Cirrus-CI is failing. Update to 12.2 to get it working again.
Unfortunately, there is a bug in libtasn1 that triggers with the
new version of Clang that is used there (see this thread for details:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00739.html ),
so we have to disable gnutls for now to make it work again. We can
enable it later again once libtasn1 has been fixed in FreeBSD.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .cirrus.yml | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index bc40a0550d..af118a5a74 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -3,7 +3,7 @@ env:
 
 freebsd_12_task:
   freebsd_instance:
-    image_family: freebsd-12-1
+    image_family: freebsd-12-2
     cpu: 8
     memory: 8G
   install_script:
@@ -13,7 +13,8 @@ freebsd_12_task:
   script:
     - mkdir build
     - cd build
-    - ../configure --enable-werror || { cat config.log meson-logs/meson-log.txt; exit 1; }
+    - ../configure --enable-werror --disable-gnutls
+      || { cat config.log meson-logs/meson-log.txt; exit 1; }
     - gmake -j$(sysctl -n hw.ncpu)
     - gmake -j$(sysctl -n hw.ncpu) check V=1
 
-- 
2.27.0

Re: [PATCH 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Peter Maydell]

On Wed, 17 Mar 2021 at 11:09, Thomas Huth <thuth@redhat.com> wrote:
>
> FreeBSD version 12.1 is out of service now, and the task in the
> Cirrus-CI is failing. Update to 12.2 to get it working again.
> Unfortunately, there is a bug in libtasn1 that triggers with the
> new version of Clang that is used there (see this thread for details:
> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00739.html ),
> so we have to disable gnutls for now to make it work again. We can
> enable it later again once libtasn1 has been fixed in FreeBSD.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Is it feasible to make configure check for "is libtasn1 broken"?
I guess since it only shows up as "when you try to use it
it fails" that would be a runtime check rather than compile
time, which isn't really possible :-(

thanks
-- PMM

Re: [PATCH 1/2] configure: Don't use the __atomic_*_16 functions for testing 128-bit support

[Paolo Bonzini]

On 17/03/21 12:05, Thomas Huth wrote:
> Looking for they way we are using atomic functions in QEMU, we are not
> using these functions with the _16 suffix anyway. Switch to the same
> functions that we use in the include/qemu/atomic.h header.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Thomas Huth]

On 17/03/2021 12.16, Peter Maydell wrote:
> On Wed, 17 Mar 2021 at 11:09, Thomas Huth <thuth@redhat.com> wrote:
>>
>> FreeBSD version 12.1 is out of service now, and the task in the
>> Cirrus-CI is failing. Update to 12.2 to get it working again.
>> Unfortunately, there is a bug in libtasn1 that triggers with the
>> new version of Clang that is used there (see this thread for details:
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00739.html ),
>> so we have to disable gnutls for now to make it work again. We can
>> enable it later again once libtasn1 has been fixed in FreeBSD.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Is it feasible to make configure check for "is libtasn1 broken"?
> I guess since it only shows up as "when you try to use it
> it fails" that would be a runtime check rather than compile
> time, which isn't really possible :-(

I don't really have a clue about this crypto stuff... Daniel, Stefan, Roman, 
Christian, Eric ... you debugged the original problem on macOS, do you think 
it's possible to add a check for this libtasn1 problem to our "configure" 
(or meson.build file)?

  Thomas

Re: [PATCH 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Daniel P. Berrangé]

On Wed, Mar 17, 2021 at 01:44:05PM +0100, Thomas Huth wrote:
> On 17/03/2021 12.16, Peter Maydell wrote:
> > On Wed, 17 Mar 2021 at 11:09, Thomas Huth <thuth@redhat.com> wrote:
> > > 
> > > FreeBSD version 12.1 is out of service now, and the task in the
> > > Cirrus-CI is failing. Update to 12.2 to get it working again.
> > > Unfortunately, there is a bug in libtasn1 that triggers with the
> > > new version of Clang that is used there (see this thread for details:
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00739.html ),
> > > so we have to disable gnutls for now to make it work again. We can
> > > enable it later again once libtasn1 has been fixed in FreeBSD.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > Is it feasible to make configure check for "is libtasn1 broken"?
> > I guess since it only shows up as "when you try to use it
> > it fails" that would be a runtime check rather than compile
> > time, which isn't really possible :-(
> 
> I don't really have a clue about this crypto stuff... Daniel, Stefan, Roman,
> Christian, Eric ... you debugged the original problem on macOS, do you think
> it's possible to add a check for this libtasn1 problem to our "configure"
> (or meson.build file)?

It is tricky because the flaw is not specific to the version of
tasn1, rather to the combination of tasn1 and the compiler used,
so there's no simple way to detect it statically from configure.

If don't want the big hammer of disabling gnutls, perhaps adding
a flag to disable tasn1 would be simpler, as that would only
disable test suite, and no actual QEMU core functionality (which
is all working fine)

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: [PATCH 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Roman Bolshakov]

On Wed, Mar 17, 2021 at 01:44:05PM +0100, Thomas Huth wrote:
> On 17/03/2021 12.16, Peter Maydell wrote:
> > On Wed, 17 Mar 2021 at 11:09, Thomas Huth <thuth@redhat.com> wrote:
> > > 
> > > FreeBSD version 12.1 is out of service now, and the task in the
> > > Cirrus-CI is failing. Update to 12.2 to get it working again.
> > > Unfortunately, there is a bug in libtasn1 that triggers with the
> > > new version of Clang that is used there (see this thread for details:
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00739.html ),
> > > so we have to disable gnutls for now to make it work again. We can
> > > enable it later again once libtasn1 has been fixed in FreeBSD.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > Is it feasible to make configure check for "is libtasn1 broken"?
> > I guess since it only shows up as "when you try to use it
> > it fails" that would be a runtime check rather than compile
> > time, which isn't really possible :-(
> 
> I don't really have a clue about this crypto stuff... Daniel, Stefan, Roman,
> Christian, Eric ... you debugged the original problem on macOS, do you think
> it's possible to add a check for this libtasn1 problem to our "configure"
> (or meson.build file)?
> 

Hi,

We need to define an ASN.1 object

https://gitlab.com/gnutls/libtasn1/-/blob/master/tests/Test_tree.c#L230

  {ACT_CREATE, "TEST_TREE.OidAndTimeTest", 0, 0, ASN1_SUCCESS, __LINE__},

The object is:
OidAndTimeTest ::= SEQUENCE{
   set     SET OF INTEGER,
   oid     OBJECT IDENTIFIER,
   time2   GeneralizedTime,
   bol     BOOLEAN,
   oct     OCTET STRING,
   bit     BIT STRING OPTIONAL,
   bol2    BOOLEAN DEFAULT TRUE,
   enum    ENUMERATED {v1(1),v2(2)} DEFAULT v1,
   any     [1] ANY OPTIONAL,
   gen     GeneralString OPTIONAL,
   time1   UTCTime
}

Create it with:

asn1_create_element (definitions, "TEST.OidAndTimeTest", &asn1_element);

and try to get it's DER length to mimic the part of the failing test:

https://gitlab.com/gnutls/libtasn1/-/blob/master/tests/Test_tree.c#L254

  result = asn1_der_coding (asn1_element, "", NULL, &der_len, errorDescription);

The result should be ASN1_MEM_ERROR.

Something like this should work as configure-time test.

Thanks,
Roman

Re: [PATCH 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Peter Maydell]

On Wed, 17 Mar 2021 at 13:16, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
>   result = asn1_der_coding (asn1_element, "", NULL, &der_len, errorDescription);
>
> The result should be ASN1_MEM_ERROR.
>
> Something like this should work as configure-time test.

Unfortunately at configure time you can't run code, only
compile and link it, because you might be cross-compiling.
So a test that needs us to actually look at return values
from function calls won't work for configure.

thanks
-- PMM

Re: [PATCH 1/2] configure: Don't use the __atomic_*_16 functions for testing 128-bit support

[Richard Henderson]

On 3/17/21 5:05 AM, Thomas Huth wrote:
> The test for 128-bit atomics is causing trouble with FreeBSD 12.2 and
> --enable-werror:
> 
>   cc -Werror -fPIE -DPIE -std=gnu99 -Wall -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong -o config-temp/qemu-conf.exe config-temp/qemu-conf.c -pie -Wl,-z,relro -Wl,-z,now -m64 -fstack-protector-strong
>   config-temp/qemu-conf.c:4:7: error: implicit declaration of function '__atomic_load_16' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>     y = __atomic_load_16(&x, 0);
>         ^
>   config-temp/qemu-conf.c:5:3: error: implicit declaration of function '__atomic_store_16' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>     __atomic_store_16(&x, y, 0);
>     ^
>   config-temp/qemu-conf.c:5:3: note: did you mean '__atomic_load_16'?
>   config-temp/qemu-conf.c:4:7: note: '__atomic_load_16' declared here
>     y = __atomic_load_16(&x, 0);
>         ^
>   config-temp/qemu-conf.c:6:3: error: implicit declaration of function '__atomic_compare_exchange_16' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>     __atomic_compare_exchange_16(&x, &y, x, 0, 0, 0);
>     ^
>   3 errors generated.
> 
> Looking for they way we are using atomic functions in QEMU, we are not
> using these functions with the _16 suffix anyway. Switch to the same
> functions that we use in the include/qemu/atomic.h header.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> ---
>   configure | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index f7d022a5db..4526af87b2 100755
> --- a/configure
> +++ b/configure
> @@ -4761,9 +4761,9 @@ if test "$int128" = "yes"; then
>   int main(void)
>   {
>     unsigned __int128 x = 0, y = 0;
> -  y = __atomic_load_16(&x, 0);
> -  __atomic_store_16(&x, y, 0);
> -  __atomic_compare_exchange_16(&x, &y, x, 0, 0, 0);
> +  y = __atomic_load(&x, 0);
> +  __atomic_store(&x, y, 0);
> +  __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
>     return 0;
>   }
>   EOF
> 

Re: [PATCH 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Mar 17, 2021 at 01:44:05PM +0100, Thomas Huth wrote:
>> On 17/03/2021 12.16, Peter Maydell wrote:
>> > On Wed, 17 Mar 2021 at 11:09, Thomas Huth <thuth@redhat.com> wrote:
>> > > 
>> > > FreeBSD version 12.1 is out of service now, and the task in the
>> > > Cirrus-CI is failing. Update to 12.2 to get it working again.
>> > > Unfortunately, there is a bug in libtasn1 that triggers with the
>> > > new version of Clang that is used there (see this thread for details:
>> > > https://lists.gnu.org/archive/html/qemu-dove/2021-02/msg00739.html ),
>> > > so we have to disable gnutls for now to make it work again. We can
>> > > enable it later again once libtasn1 has been fixed in FreeBSD.
>> > > 
>> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
>> > 
>> > Is it feasible to make configure check for "is libtasn1 broken"?
>> > I guess since it only shows up as "when you try to use it
>> > it fails" that would be a runtime check rather than compile
>> > time, which isn't really possible :-(
>> 
>> I don't really have a clue about this crypto stuff... Daniel, Stefan, Roman,
>> Christian, Eric ... you debugged the original problem on macOS, do you think
>> it's possible to add a check for this libtasn1 problem to our "configure"
>> (or meson.build file)?
>
> It is tricky because the flaw is not specific to the version of
> tasn1, rather to the combination of tasn1 and the compiler used,
> so there's no simple way to detect it statically from configure.
>
> If don't want the big hammer of disabling gnutls, perhaps adding
> a flag to disable tasn1 would be simpler, as that would only
> disable test suite, and no actual QEMU core functionality (which
> is all working fine)

I suspect we'll forget to remove this flag as well when the time comes.
Without adding new things I think this patch would be fine with an
additional comments:

  # --disable-gnutls until libtasn1 is fixed in FreeBSD (#ref?)

>
> Regards,
> Daniel


-- 
Alex Bennée

[PATCH v2 2/2] cirrus.yml: Update the FreeBSD task to version 12.2

[Thomas Huth]

FreeBSD version 12.1 is out of service now, and the task in the
Cirrus-CI is failing. Update to 12.2 to get it working again.
Unfortunately, there is a bug in libtasn1 that triggers with the
new version of Clang that is used there (see this thread for details:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00739.html ),
so we have to disable gnutls for now to make it work again. We can
enable it later again once libtasn1 has been fixed in FreeBSD.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2: Added a TODO comment so that we remember to enable gnutls later again

 .cirrus.yml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index bc40a0550d..f53c519447 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -3,7 +3,7 @@ env:
 
 freebsd_12_task:
   freebsd_instance:
-    image_family: freebsd-12-1
+    image_family: freebsd-12-2
     cpu: 8
     memory: 8G
   install_script:
@@ -13,7 +13,10 @@ freebsd_12_task:
   script:
     - mkdir build
     - cd build
-    - ../configure --enable-werror || { cat config.log meson-logs/meson-log.txt; exit 1; }
+    # TODO: Enable gnutls again once FreeBSD's libtasn1 got fixed
+    # See: https://gitlab.com/gnutls/libtasn1/-/merge_requests/71
+    - ../configure --enable-werror --disable-gnutls
+      || { cat config.log meson-logs/meson-log.txt; exit 1; }
     - gmake -j$(sysctl -n hw.ncpu)
     - gmake -j$(sysctl -n hw.ncpu) check V=1
 
-- 
2.27.0

Re: [PATCH 0/2] Fix atomic test in "configure" + bump FreeBSD CI to 12.2

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> FreeBSD 12.1 is out of service now, so the Cirrus-CI task is failing due
> to using some packages from 12.2 on the 12.1 image. We have to update
> it to 12.2. However, there are two obstacles:
>
> First, the check for the 128-bit atomic functions in the configure
> script does not work right with the version of Clang in FreeBSD 12.2
> anymore. Looks like the __atomic_*_16() functions do not have valid
> prototypes anymore. First patch fixes this issue.
>
> Second, there is a problem with libtasn1 and the latest version of
> Clang that also triggers on FreeBSD 12.2. We have to disable gnutls
> in this task until libtasn1 got fixed in FreeBSD.
>
> Thomas Huth (2):
>   configure: Don't use the __atomic_*_16 functions for testing 128-bit
>     support
>   cirrus.yml: Update the FreeBSD task to version 12.2

Queued to for-6.0/fixes-for-rc1, thanks. b4 did manage to pick up v2 of
2/2 which confused me a little.


-- 
Alex Bennée