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
