The current code for alternative calls uses the caller parameter types as the
types for the register variables that serve as function parameters:

uint8_t foo;
[...]
alternative_call(myfunc, foo);

Would expand roughly into:

register unint8_t a1_ asm("rdi") = foo;
register unsigned long a2_ asm("rsi");
[...]
asm volatile ("call *%c[addr](%%rip)"...);

However under certain circumstances clang >= 16.0.0 with -O2 can generate
incorrect code, given the following example:

unsigned int func(uint8_t t)
{
    return t;
}

static void bar(uint8_t b)
{
    int ret_;
    register uint8_t di asm("rdi") = b;
    register unsigned long si asm("rsi");
    register unsigned long dx asm("rdx");
    register unsigned long cx asm("rcx");
    register unsigned long r8 asm("r8");
    register unsigned long r9 asm("r9");
    register unsigned long r10 asm("r10");
    register unsigned long r11 asm("r11");

    asm volatile ( "call %c[addr]"
                   : "+r" (di), "=r" (si), "=r" (dx),
                     "=r" (cx), "=r" (r8), "=r" (r9),
                     "=r" (r10), "=r" (r11), "=a" (ret_)
                   : [addr] "i" (&(func)), "g" (func)
                   : "memory" );
}

void foo(unsigned int a)
{
    bar(a);
}

Clang generates the following code:

func:                                   # @func
        movl    %edi, %eax
        retq
foo:                                    # @foo
        callq   func
        retq

Note the truncation of the unsigned int parameter 'a' of foo() to uint8_t when
passed into bar() is lost.

The above can be worked around by using an union when defining the register
variables, so that `di` becomes:

register union {
    uint8_t e;
    unsigned long r;
} di asm("rdi") = { .e = b };

Which results in following code generated for `foo()`:

foo:                                    # @foo
        movzbl  %dil, %edi
        callq   func
        retq

So the truncation is not longer lost.  Apply such workaround only when built
with clang.

Reported-by: Matthew Grooms <mgrooms@shrew.net>
Link: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277200
Link: https://github.com/llvm/llvm-project/issues/82598
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Only apply the union workaround with clang.

Seems like all gitlab build tests are OK with this approach.
---
 xen/arch/x86/include/asm/alternative.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index a1cd6a9fe5b8..3fe27ea791bf 100644
--- 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
+ */
+#define ALT_CALL_ARG(arg, n)                                            \
+    register union {                                                    \
+        typeof(arg) e;                                                  \
+        unsigned long r;                                                \
+    } a ## n ## _ asm ( ALT_CALL_arg ## n ) = {                         \
+        .e = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })   \
+    }
+#else
 #define ALT_CALL_ARG(arg, n) \
     register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
         ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })
+#endif
 #define ALT_CALL_NO_ARG(n) \
     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
 
-- 
2.43.0

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.

Aiui this bug is only a special case of the other, much older one, so
referencing that one here too would seem advisable.

As a nit: I think it is "a union" (spelling according to pronunciation),
and I guess the title could now do with saying "optionally" or
mentioning Clang or some such.

Jan

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.  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.

> As a nit: I think it is "a union" (spelling according to pronunciation),
> and I guess the title could now do with saying "optionally" or
> mentioning Clang or some such.

OK.

Thanks, Roger

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

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.

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

On 22/02/2024 4:55 pm, 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.
>
> Aiui this bug is only a special case of the other, much older one, so
> referencing that one here too would seem advisable.
>
> As a nit: I think it is "a union" (spelling according to pronunciation),
> and I guess the title could now do with saying "optionally" or
> mentioning Clang or some such.

Yes.  "a union" is the right form here.

~Andrew

P.S. Spoken, "an union" would come across as "an onion", which is
something very different.

P.P.S.  As I noted to Matthew concerning
https://mjg59.dreamwidth.org/66907.html

"There's an interesting sidechannel in your blog post.  You seem to call
it an S-O-C and not a soc(k), based on the "an""