1. Patchew
  2. QEMU
  3. View series

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

Peter Maydell posted 1 patch 1 year, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230622130823.1631719-1-peter.maydell@linaro.org
include/qemu/compiler.h   | 13 +++++++++++++
include/qemu/host-utils.h |  2 +-
2 files changed, 14 insertions(+), 1 deletion(-)
[PATCH] host-utils: Avoid using __builtin_subcll on buggy versions of Apple Clang
Posted by Peter Maydell 1 year, 4 months ago 
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.
+ */
+#if defined(__apple_build_version__) && __clang_major__ >= 14
+#define BUILTIN_SUBCLL_BROKEN
+#endif
+
 #endif /* COMPILER_H */
diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
index d3b4dce6a93..011618373e5 100644
--- a/include/qemu/host-utils.h
+++ b/include/qemu/host-utils.h
@@ -649,7 +649,7 @@ static inline uint64_t uadd64_carry(uint64_t x, uint64_t y, bool *pcarry)
  */
 static inline uint64_t usub64_borrow(uint64_t x, uint64_t y, bool *pborrow)
 {
-#if __has_builtin(__builtin_subcll)
+#if __has_builtin(__builtin_subcll) && !defined(BUILTIN_SUBCLL_BROKEN)
     unsigned long long b = *pborrow;
     x = __builtin_subcll(x, y, b, &b);
     *pborrow = b & 1;
-- 
2.34.1
Re: [PATCH] host-utils: Avoid using __builtin_subcll on buggy versions of Apple Clang
Posted by Daniel P. Berrangé 1 year, 4 months ago 
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 :|
Re: [PATCH] host-utils: Avoid using __builtin_subcll on buggy versions of Apple Clang
Posted by Peter Maydell 1 year, 4 months ago 
On Thu, 22 Jun 2023 at 15:35, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> 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

That was what I intended the "When..." sentence to be. We
can add "TODO:" on the front to make it stand out a bit more I guess.

-- PMM
Re: [PATCH] host-utils: Avoid using __builtin_subcll on buggy versions of Apple Clang
Posted by Richard Henderson 1 year, 4 months ago 
On 6/22/23 15:08, 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(-)

Ouch.

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

r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.