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

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

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

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

Thanks, Roger.