Re: [PATCH] host-utils: Avoid using __builtin_subcll on buggy versions of Apple Clang

[Daniel P . Berrangé]

On Thu, Jun 22, 2023 at 02:08:23PM +0100, Peter Maydell wrote:
> We use __builtin_subcll() to do a 64-bit subtract with borrow-in and
> borrow-out when the host compiler supports it.  Unfortunately some
> versions of Apple Clang have a bug in their implementation of this
> intrinsic which means it returns the wrong value.  The effect is that
> a QEMU built with the affected compiler will hang when emulating x86
> float80 division.
> 
> The upstream LLVM issue is:
> https://github.com/llvm/llvm-project/issues/55253
> 
> The commit that introduced the bug apparently never made it into an
> upstream LLVM release without the subsequent fix
> https://github.com/llvm/llvm-project/commit/fffb6e6afdbaba563189c1f715058ed401fbc88d
> but unfortunately it did make it into Apple Clang 14.0, as shipped
> in Xcode 14.3 (14.2 is reported to be OK). The Apple bug number is
> FB12210478.
> 
> Add ifdefs to avoid use of __builtin_subcll() on Apple Clang version
> 14 or greater.  There is not currently a version of Apple Clang which
> has the bug fix -- when one appears we should be able to add an upper
> bound to the ifdef condition so we can start using the builtin again.
> We make the lower bound a conservative "any Apple clang with major
> version 14 or greater" because the consequences of incorrectly
> disabling the builtin when it would work are pretty small and the
> consequences of not disabling it when we should are pretty bad.
> 
> Many thanks to those users who both reported this bug and also
> did a lot of work in identifying the root cause; in particular
> to Daniel Bertalan and osy.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1631
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1659
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I don't have a setup to test this, so this needs testing by the
> people who've encountered this compiler bug to confirm it does
> the right thing...
> ---
>  include/qemu/compiler.h   | 13 +++++++++++++
>  include/qemu/host-utils.h |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index c2f49df1f91..a309f90c768 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -184,4 +184,17 @@
>  #define QEMU_DISABLE_CFI
>  #endif
>  
> +/*
> + * Apple clang version 14 has a bug in its __builtin_subcll(); define
> + * BUILTIN_SUBCLL_BROKEN for the offending versions so we can avoid it.
> + * When a version of Apple clang which has this bug fixed is released
> + * we can add an upper bound to this check.
> + * See https://gitlab.com/qemu-project/qemu/-/issues/1631
> + * and https://gitlab.com/qemu-project/qemu/-/issues/1659 for details.
> + * The bug never made it into any upstream LLVM releases, only Apple ones.

Perhaps add a reminder:

 * TODO: put a max cap on __clang_major__/__clang_minor once
 * Apple have released a version with the fix

> + */
> +#if defined(__apple_build_version__) && __clang_major__ >= 14
> +#define BUILTIN_SUBCLL_BROKEN
> +#endif

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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