The current code in ALT_CALL_ARG() won't successfully workaround the clang
code-generation issue if the arg parameter has a size that's not a power of 2.
While there are no such sized parameters at the moment, improve the workaround
to also be effective when such sizes are used.
Instead of using a union with a long use an unsigned long that's first
initialized to 0 and afterwards set to the argument value.
Reported-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Suggested-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
- Fix indentation and style issues.
- Adjust comment to match the new workaround.
---
xen/arch/x86/include/asm/alternative.h | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index e63b45927643..c5fa242e76b3 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -169,27 +169,25 @@ extern void alternative_branches(void);
#ifdef CONFIG_CC_IS_CLANG
/*
- * Use a union with an unsigned long in order to prevent clang from
- * skipping a possible truncation of the value. By using the union any
- * truncation is carried before the call instruction, in turn covering
- * for ABI-non-compliance in that the necessary clipping / extension of
- * the value is supposed to be carried out in the callee.
+ * Clang doesn't follow the psABI and doesn't truncate parameter values at the
+ * callee. This can lead to bad code being generated when using alternative
+ * calls.
*
- * Note this behavior is not mandated by the standard, and hence could
- * stop being a viable workaround, or worse, could cause a different set
- * of code-generation issues in future clang versions.
+ * Workaround it by using a temporary intermediate variable that's zeroed
+ * before being assigned the parameter value, as that forces clang to zero the
+ * register at the caller.
*
* This has been reported upstream:
* https://github.com/llvm/llvm-project/issues/12579
* https://github.com/llvm/llvm-project/issues/82598
*/
#define ALT_CALL_ARG(arg, n) \
- register union { \
- typeof(arg) e[sizeof(long) / sizeof(arg)]; \
- unsigned long r; \
- } a ## n ## _ asm ( ALT_CALL_arg ## n ) = { \
- .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
- }
+ register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \
+ unsigned long tmp = 0; \
+ *(typeof(arg) *)&tmp = (arg); \
+ BUILD_BUG_ON(sizeof(arg) > sizeof(unsigned long)); \
+ tmp; \
+ })
#else
#define ALT_CALL_ARG(arg, n) \
register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
--
2.45.2
On 30.07.2024 17:53, Roger Pau Monne wrote:
> The current code in ALT_CALL_ARG() won't successfully workaround the clang
> code-generation issue if the arg parameter has a size that's not a power of 2.
> While there are no such sized parameters at the moment, improve the workaround
> to also be effective when such sizes are used.
>
> Instead of using a union with a long use an unsigned long that's first
> initialized to 0 and afterwards set to the argument value.
>
> Reported-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> Suggested-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Albeit if you don't mind ...
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -169,27 +169,25 @@ extern void alternative_branches(void);
>
> #ifdef CONFIG_CC_IS_CLANG
> /*
> - * Use a union with an unsigned long in order to prevent clang from
> - * skipping a possible truncation of the value. By using the union any
> - * truncation is carried before the call instruction, in turn covering
> - * for ABI-non-compliance in that the necessary clipping / extension of
> - * the value is supposed to be carried out in the callee.
> + * Clang doesn't follow the psABI and doesn't truncate parameter values at the
> + * callee. This can lead to bad code being generated when using alternative
> + * calls.
> *
> - * Note this behavior is not mandated by the standard, and hence could
> - * stop being a viable workaround, or worse, could cause a different set
> - * of code-generation issues in future clang versions.
> + * Workaround it by using a temporary intermediate variable that's zeroed
> + * before being assigned the parameter value, as that forces clang to zero the
> + * register at the caller.
> *
> * This has been reported upstream:
> * https://github.com/llvm/llvm-project/issues/12579
> * https://github.com/llvm/llvm-project/issues/82598
> */
> #define ALT_CALL_ARG(arg, n) \
> - register union { \
> - typeof(arg) e[sizeof(long) / sizeof(arg)]; \
> - unsigned long r; \
> - } a ## n ## _ asm ( ALT_CALL_arg ## n ) = { \
> - .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
> - }
> + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \
> + unsigned long tmp = 0; \
> + *(typeof(arg) *)&tmp = (arg); \
> + BUILD_BUG_ON(sizeof(arg) > sizeof(unsigned long)); \
... I'd like to switch around these two lines while committing.
Jan
> + tmp; \
> + })
> #else
> #define ALT_CALL_ARG(arg, n) \
> register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
On Wed, Jul 31, 2024 at 08:26:02AM +0200, Jan Beulich wrote:
> On 30.07.2024 17:53, Roger Pau Monne wrote:
> > The current code in ALT_CALL_ARG() won't successfully workaround the clang
> > code-generation issue if the arg parameter has a size that's not a power of 2.
> > While there are no such sized parameters at the moment, improve the workaround
> > to also be effective when such sizes are used.
> >
> > Instead of using a union with a long use an unsigned long that's first
> > initialized to 0 and afterwards set to the argument value.
> >
> > Reported-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > Suggested-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Albeit if you don't mind ...
>
> > --- a/xen/arch/x86/include/asm/alternative.h
> > +++ b/xen/arch/x86/include/asm/alternative.h
> > @@ -169,27 +169,25 @@ extern void alternative_branches(void);
> >
> > #ifdef CONFIG_CC_IS_CLANG
> > /*
> > - * Use a union with an unsigned long in order to prevent clang from
> > - * skipping a possible truncation of the value. By using the union any
> > - * truncation is carried before the call instruction, in turn covering
> > - * for ABI-non-compliance in that the necessary clipping / extension of
> > - * the value is supposed to be carried out in the callee.
> > + * Clang doesn't follow the psABI and doesn't truncate parameter values at the
> > + * callee. This can lead to bad code being generated when using alternative
> > + * calls.
> > *
> > - * Note this behavior is not mandated by the standard, and hence could
> > - * stop being a viable workaround, or worse, could cause a different set
> > - * of code-generation issues in future clang versions.
> > + * Workaround it by using a temporary intermediate variable that's zeroed
> > + * before being assigned the parameter value, as that forces clang to zero the
> > + * register at the caller.
> > *
> > * This has been reported upstream:
> > * https://github.com/llvm/llvm-project/issues/12579
> > * https://github.com/llvm/llvm-project/issues/82598
> > */
> > #define ALT_CALL_ARG(arg, n) \
> > - register union { \
> > - typeof(arg) e[sizeof(long) / sizeof(arg)]; \
> > - unsigned long r; \
> > - } a ## n ## _ asm ( ALT_CALL_arg ## n ) = { \
> > - .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })\
> > - }
> > + register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = ({ \
> > + unsigned long tmp = 0; \
> > + *(typeof(arg) *)&tmp = (arg); \
> > + BUILD_BUG_ON(sizeof(arg) > sizeof(unsigned long)); \
>
> ... I'd like to switch around these two lines while committing.
Sure, feel free to swap them.
Thanks, Roger.
© 2016 - 2025 Red Hat, Inc.