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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.02.2024 13:18, Roger Pau Monné wrote:
> 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:

Yes, ...

> 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

... yet perhaps still list your new bug report here as well.

Jan