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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2023 13:26, Andrew Cooper wrote:
> 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.

How that if \@ is always 1?

>  So I think we need the \().

Or put one at the start of the label and the other at the end?

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

"this" being what? The case of Clang but with / without integrated as?
Since we have to turn that off, we could as well add a -D option along
with the -no-integrated-as one.

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

Of course, hence my "with a link to there left here".

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

I wouldn't call that one simple macro "complexity". I'm in particular
not advocating for (conditionally) removing the extra macro parameter.

Jan