Re: [PATCH v1] xen: Work around Clang-IAS macro expansion bug

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/02/2023 10:31 am, Jan Beulich wrote:
> On 17.02.2023 13:22, Andrew Cooper wrote:
>> https://github.com/llvm/llvm-project/issues/60792
>>
>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
>> unit, and the XSA-426 change tickles this bug:
>>
>>   <instantiation>:4:1: error: invalid symbol redefinition
>>   .L1_fill_rsb_loop:
>>   ^
>>   make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>>
>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
>> too, which Clang does seem to expand properly.
>>
>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address 
>> Predictions")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>>
>> v1 (vs RFC):
>>  * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.

Sadly, this is not quite correct.  There needs to be a non-numeric
character separating \@ and \x or the concatenation of the two can end
up non-unique.  So I think we need the \().

>> I originally tried a parameter named uniq but this found a different 
>> Clang-IAS
>> bug:
>>
>>   In file included from arch/x86/asm-macros.c:3:
>>   ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no 
>> following hex digits; treating as '\' followed by identifier 
>> [-Werror,-Wunicode]
>>   .L\@\uniq\()fill_rsb_loop:
>>       ^
>>
>> It appears that Clang is looking for unicode escapes before completing
>> parameter substitution.  But the macro didn't originate from a context where 
>> a
>> unicode escape was even applicable to begin with.
>>
>> The consequence is that you can't use macro prameters beginning with a u.
> How nice. If really needed, I guess we could use -Wno-unicode on the
> assumption that we don't use anything that could legitimately trigger that
> warning.

It's proving very stubborn to narrow down.

I really can't reproduce it outside of this specific occurrence in the
Xen build system, but my gut feeling is that it's something to do with
the asm level .include.

>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
>>      wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>>  
>>      /* (ab)use alternative_input() to specify clobbers. */
>> -    alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
>> +    alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
>>                        : "rax", "rcx");
>>  }
>>  
>> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct 
>> cpu_info *info)
>>       *
>>       * (ab)use alternative_input() to specify clobbers.
>>       */
>> -    alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
>> +    alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
>>                        : "rax", "rcx");
>>  }
> Since inside the macro you retain the uses of \@, I'd find it desirable
> to keep gcc-generated code tidy by omitting the extra argument there.

As I said in the RFC, I tried to have x=\@ but gas really didn't like that.

But given the concatenation observation, we also cannot simply replace
\@ with %= (given the option, which I couldn't figure out), because they
can overlap.

> This could be done by introducing another C macro along the lines of:
>
> #ifdef __clang__
> #define CLANG_UNIQUE(name) " " #name "=%="
> #else
> #define CLANG_UNIQUE(name)
> #endif
>
> Besides the less confusing label names this would also have the benefit
> of providing a link at the use sites to what the issue is that is being
> worked around. Plus, once (if ever) fixed in Clang, we could then adjust
> the condition to just the affected versions.

It's only Clang IAS, but there's no suitable define to figure this out.

And while I appreciate the effort to be more descriptive, this name is
literally longer than the thing it wraps and ...

>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> @@ -117,11 +117,15 @@
>>  .L\@_done:
>>  .endm
>>  
>> -.macro DO_OVERWRITE_RSB tmp=rax
>> +.macro DO_OVERWRITE_RSB tmp=rax x=
> The "=" in "x=" isn't needed, is it? It looks a little confusing to me,
> raising the question "Why is this there?"

Because I started out with u=\@

>>  /*
>>   * Requires nothing
>>   * Clobbers \tmp (%rax by default), %rcx
>>   *
>> + * x= is an optional parameter to work around
>> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't
>> + * expand \@ uniquely, and is intended for muxing %= in too.
> With the above suggestion I'd see this comment move to next to that
> helper macro, with a link to there left here.

... there's no getting rid of this comment, at least in some form.  The
parameter is odd, and needs explaining.

Passing %= unconditionally doesn't matter for GCC/Binuitls, and in this
case, attempts to "declutter" the niche usecase of inspecting the
assembled file comes with a substantial complexity at the C level.  
It's really not worth the extra complexity.

~Andrew