Re: [PATCH] x86/altcall: further refine clang workaround

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Fri Jul 26, 2024 at 3:17 PM BST, Alejandro Vallejo wrote:
> On Fri Jul 26, 2024 at 9:05 AM BST, Jan Beulich wrote:
> > On 26.07.2024 09:52, Roger Pau Monné wrote:
> > > On Fri, Jul 26, 2024 at 09:36:15AM +0200, Jan Beulich wrote:
> > >> On 26.07.2024 09:31, Roger Pau Monné wrote:
> > >>> On Thu, Jul 25, 2024 at 05:00:22PM +0200, Jan Beulich wrote:
> > >>>> On 25.07.2024 16:54, Roger Pau Monné wrote:
> > >>>>> On Thu, Jul 25, 2024 at 03:18:29PM +0200, Jan Beulich wrote:
> > >>>>>> On 25.07.2024 12:56, Roger Pau Monne wrote:
> > >>>>>>> --- a/xen/arch/x86/include/asm/alternative.h
> > >>>>>>> +++ b/xen/arch/x86/include/asm/alternative.h
> > >>>>>>> @@ -184,11 +184,11 @@ extern void alternative_branches(void);
> > >>>>>>>   * https://github.com/llvm/llvm-project/issues/82598
> > >>>>>>>   */
> > >>>>>>>  #define ALT_CALL_ARG(arg, n)                                       
> > >>>>>>>      \
> > >>>>>>> -    register union {                                               
> > >>>>>>>      \
> > >>>>>>> -        typeof(arg) e[sizeof(long) / sizeof(arg)];                 
> > >>>>>>>      \
> > >>>>>>> -        unsigned long r;                                           
> > >>>>>>>      \
> > >>>>>>> +    register struct {                                              
> > >>>>>>>      \
> > >>>>>>> +        typeof(arg) e;                                             
> > >>>>>>>      \
> > >>>>>>> +        char pad[sizeof(void *) - sizeof(arg)];                    
> > >>>>>>>      \
> > >>>>>>
> > >>>>>> One thing that occurred to me only after our discussion, and I then 
> > >>>>>> forgot
> > >>>>>> to mention this before you would send a patch: What if sizeof(void 
> > >>>>>> *) ==
> > >>>>>> sizeof(arg)? Zero-sized arrays are explicitly something we're trying 
> > >>>>>> to
> > >>>>>> get rid of.
> > >>>>>
> > >>>>> I wondered about this, but I though it was only [] that we were trying
> > >>>>> to get rid of, not [0].
> > >>>>
> > >>>> Sadly (here) it's actually the other way around, aiui.
> > >>>
> > >>> The only other option I have in mind is using an oversized array on
> > >>> the union, like:
> > >>>
> > >>> #define ALT_CALL_ARG(arg, n)                                            
> > >>> \
> > >>>     union {                                                             
> > >>> \
> > >>>         typeof(arg) e[(sizeof(long) + sizeof(arg) - 1) / sizeof(arg)];  
> > >>> \
> > >>>         unsigned long r;                                                
> > >>> \
> > >>>     } a ## n ## __  = {                                                 
> > >>> \
> > >>>         .e[0] = ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); 
> > >>> })\
> > >>>     };                                                                  
> > >>> \
> > >>>     register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) =      
> > >>> \
> > >>>         a ## n ## __.r
> > >>
> > >> Yet that's likely awful code-gen wise?
> > > 
> > > Seems OK: https://godbolt.org/z/nsdo5Gs8W
> >
> > In which case why not go this route. If the compiler is doing fine with
> > that, maybe the array dimension expression could be further simplified,
> > accepting yet more over-sizing? Like "sizeof(void *) / sizeof (arg) + 1"
> > or even simply "sizeof(void *)"? Suitably commented of course ...
> >
> > >> For the time being, can we perhaps
> > >> just tighten the BUILD_BUG_ON(), as iirc Alejandro had suggested?
> > > 
> > > My main concern with tightening the BUILD_BUG_ON() is that then I
> > > would also like to do so for the GCC one, so that build fails
> > > uniformly.
> >
> > If we were to take that route, then yes, probably should constrain both
> > (with a suitable comment on the gcc one).
> >
> > Jan
>
> Yet another way would be to have an intermediate `long` to cast onto. 
> Compilers
> will optimise away the copy. It ignores the different-type aliasing rules in
> the C spec, so there's an assumption that we have -fno-strict-aliasing. But I
> belive we do? Otherwise it should pretty much work on anything.
>
> ```
>   #define ALT_CALL_ARG(arg, n)                                              \
>       unsigned long __tmp = 0;                                              \
>       *(typeof(arg) *)&__tmp =                                              \
>           ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); })          \
>       register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) = __tmp; \
> ```
>
> fwiw, clang18 emits identical code compared with the previous godbolt link.
>
> Link: https://godbolt.org/z/facd1M9xa
>
> Cheers,
> Alejandro

Bah. s/b/__tmp/ in line15. Same output though, so the point still stands.

Cheers,
Alejandro