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

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

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.