Re: [PATCH] xen/x86: Work around Clang code generation bug with asm parameters

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/11/2020 15:11, Jan Beulich wrote:
> On 11.11.2020 13:45, Andrew Cooper wrote:
>> Clang 9 and later don't handle the clobber of %r10 correctly in
>> _hypercall64_4().  See https://bugs.llvm.org/show_bug.cgi?id=48122
> Are you sure this is a bug?

Yes.

>  With ...
>
>>  #define _hypercall64_4(type, hcall, a1, a2, a3, a4)                     \
>>      ({                                                                  \
>> -        long res, tmp__;                                                \
>> -        register long _a4 asm ("r10") = ((long)(a4));                   \
>> +        long res, _a1 = (long)(a1), _a2 = (long)(a2),                   \
>> +            _a3 = (long)(a3);                                           \
>> +        register long _a4 asm ("r10") = (long)(a4);                     \
>>          asm volatile (                                                  \
>>              "call hypercall_page + %c[offset]"                          \
>> -            : "=a" (res), "=D" (tmp__), "=S" (tmp__), "=d" (tmp__),     \
>> -              "=&r" (tmp__) ASM_CALL_CONSTRAINT                         \
> ... this we've requested "any register", while with ...
>
>> -            : [offset] "i" (hcall * 32),                                \
>> -              "1" ((long)(a1)), "2" ((long)(a2)), "3" ((long)(a3)),     \
>> -              "4" (_a4)                                                 \
> ... this we've asked for that specific register to be initialized
> from r10 (and without telling the compiler that r10 is going to
> change).

Consider applying that same reasoning to "1" instead of "4".  In that
case, a1 would no longer be bound to %rdi.

The use of "4" explicitly binds the input and the output, which includes
requiring them to be the same register.

Furthermore, LLVM tends to consider "not behaving in the same was as
GCC" a bug.

> Also, by what I would have hoped is convention meanwhile, the new
> macro local variables' names shouldn't start with an underscore,
> but instead perhaps end in one. But to be honest, despite knowing
> of the latent (albeit highly hypothetical) issue, each time I
> find myself making such a comment I'm one tiny step closer to
> giving up.

Convention is not created by you getting irritated at others getting
irritated at you for requesting bizarre and unusual changes in submitted
code, or by continuing to point things out, knowing that others
specifically disagree with your reasoning in this case.

Convention is created when there is general agreement over the technical
merits of writing code in one particular way vs the alternatives, *and*
its written down somewhere, so this argument does not continue any further.

There is no restrictions or guidance in the coding style to prefer one
form over another, therefore anything is acceptable.

~Andrew