Re: [PATCH v2] x86/altcall: use an union as register type for function parameters

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 23, 2024 at 11:43:14AM +0100, Jan Beulich wrote:
> On 23.02.2024 10:19, Roger Pau Monné wrote:
> > On Thu, Feb 22, 2024 at 05:55:00PM +0100, Jan Beulich wrote:
> >> On 22.02.2024 17:44, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/include/asm/alternative.h
> >>> +++ b/xen/arch/x86/include/asm/alternative.h
> >>> @@ -167,9 +167,25 @@ extern void alternative_branches(void);
> >>>  #define ALT_CALL_arg5 "r8"
> >>>  #define ALT_CALL_arg6 "r9"
> >>>  
> >>> +#ifdef CONFIG_CC_IS_CLANG
> >>> +/*
> >>> + * Use an 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.
> >>> + * https://github.com/llvm/llvm-project/issues/82598
> >>> + */
> >>
> >> I think it needs saying that this is relying on compiler behavior not
> >> mandated by the standard, thus explaining why it's restricted to
> >> Clang (down the road we may even want to restrict to old versions,
> >> assuming they fix the issue at some point). Plus also giving future
> >> readers a clear understanding that if something breaks with this, it's
> >> not really a surprise.
> > 
> > What about:
> > 
> > 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.
> 
> >  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.
> > 
> > This has been reported upstream at:
> > https://github.com/llvm/llvm-project/issues/82598
> > 
> >> Aiui this bug is only a special case of the other, much older one, so
> >> referencing that one here too would seem advisable.
> > 
> > My report has been resolved as a duplicate of:
> > 
> > https://github.com/llvm/llvm-project/issues/43573
> > 
> > FWIW, I think for the context the link is used in (altcall) my bug
> > report is more representative, and readers can always follow the trail
> > into the other inter-related bugs.
> 
> While true, the comment extension suggested above goes beyond that
> territory, and there the other bug is quite relevant directly. After all
> what your change does is papering over a knock-on effect of them not
> following the ABI. And that simply because it is pretty hard to see how
> we could work around the ABI non-conformance itself (which btw could
> bite us if we had any affected C function called from assembly).
> 
> 43537 looks to be a newer instance of 12579; funny they didn't close
> that as a duplicate then, too.

So would you be OK with the following:

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.

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.

This has been reported upstream at:
https://github.com/llvm/llvm-project/issues/12579

Thanks, Roger.