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

[Jan Beulich <jbeulich@xxxxxxxx>]

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