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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan