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

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

[Roger Pau Monne]

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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Seems like all gitlab build tests are OK with this approach.
---
 xen/arch/x86/include/asm/alternative.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index a1cd6a9fe5b8..837dfd953d2f 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -167,9 +167,18 @@ extern void alternative_branches(void);
 #define ALT_CALL_arg5 "r8"
 #define ALT_CALL_arg6 "r9"
 
-#define ALT_CALL_ARG(arg, n) \
-    register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \
-        ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })
+/*
+ * Use an union with an unsigned long in order to prevent clang >= 16.0.0 from
+ * skipping a possible truncation of the value.  By using the union any
+ * truncation is carried before the call instruction.
+ */
+#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); })   \
+    }
 #define ALT_CALL_NO_ARG(n) \
     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
 
-- 
2.43.0

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

[Roger Pau Monné]

On Wed, Feb 21, 2024 at 06:03:31PM +0100, Roger Pau Monne wrote:
> 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.
> 

This is missing:

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

Last one is the bug report against llvm.

Thanks, Roger.

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

[Jan Beulich]

On 21.02.2024 18:03, Roger Pau Monne wrote:
> 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,

Considering that the related (wider) ABI issue looks to also be present on
Clang5, is the more specific issue here really limited to >= 16?

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

But how do you explain this behavior? I see absolutely no reason why filling
the one union field should lead to zero-extension. If I'm not mistaken the
language allows the rest of the union to retain undefined contents. So to me
this looks like you're converting something that failed to build due to a
(presumed) bug in Clang to something that any compiler would be permitted to
translate to other than what we want.

In this context note in particular that the spec distinguishes aggregates
from unions, and the clause regarding filling unmentioned fields is limited
to aggregates:

"If there are fewer initializers in a brace-enclosed list than there are
 elements or members of an aggregate, or fewer characters in a string
 literal used to initialize an array of known size than there are elements
 in the array, the remainder of the aggregate shall be initialized
 implicitly the same as objects that have static storage duration."

Which makes sense, as a union initializer shall mention a single of the
members only anyway.

All of this of course doesn't invalidate your approach as a possible
workaround, but it then needs limiting to Clang versions where we are sure
the more strict behavior than demanded by the standard actually applies.

Jan

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

[Roger Pau Monné]

On Thu, Feb 22, 2024 at 11:32:14AM +0100, Jan Beulich wrote:
> On 21.02.2024 18:03, Roger Pau Monne wrote:
> > 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,
> 
> Considering that the related (wider) ABI issue looks to also be present on
> Clang5, is the more specific issue here really limited to >= 16?

No, this is wrong.  I did check clang 15.0.0 and I messed up the
output, all clang versions (on godbolt) seem to be affected.

> > 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.
> 
> But how do you explain this behavior? I see absolutely no reason why filling
> the one union field should lead to zero-extension. If I'm not mistaken the
> language allows the rest of the union to retain undefined contents. So to me
> this looks like you're converting something that failed to build due to a
> (presumed) bug in Clang to something that any compiler would be permitted to
> translate to other than what we want.

Oh, right, I was expecting the compiler to zero extend it, confabulating
how unmentioned fields are initialized in structs.

However, if as mentioned in the psABI thread, the callee is required
to do any zero extension as necessary, using the union shouldn't cause
issues for compilers that implement the ABI properly.

IOW: I don't think the proposed workaround would cause issues for gcc.

Thanks, Roger.

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

[Jan Beulich]

On 22.02.2024 11:50, Roger Pau Monné wrote:
> On Thu, Feb 22, 2024 at 11:32:14AM +0100, Jan Beulich wrote:
>> On 21.02.2024 18:03, Roger Pau Monne wrote:
>>> 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.
>>
>> But how do you explain this behavior? I see absolutely no reason why filling
>> the one union field should lead to zero-extension. If I'm not mistaken the
>> language allows the rest of the union to retain undefined contents. So to me
>> this looks like you're converting something that failed to build due to a
>> (presumed) bug in Clang to something that any compiler would be permitted to
>> translate to other than what we want.
> 
> Oh, right, I was expecting the compiler to zero extend it, confabulating
> how unmentioned fields are initialized in structs.
> 
> However, if as mentioned in the psABI thread, the callee is required
> to do any zero extension as necessary, using the union shouldn't cause
> issues for compilers that implement the ABI properly.
> 
> IOW: I don't think the proposed workaround would cause issues for gcc.

Well, that's making a lot of assumptions then. There may indeed be little
reason for the compiler to emit different code, but it is absolutely free
to do so. I continue to think that we want to limit this workaround to as
narrow a set of compilers as possible. Clearly for Clang the code is
better than without the workaround, so there's no reason to take this as
an improvement even if in some obscure case it would still cause an issue.
But for a compiler that produces correct code without this workaround, we
shouldn't chance the workaround breaking some case somewhere.

Jan