Messages in this thread Patch in this message | | | Date | Tue, 14 Apr 2020 08:43:07 -0700 | From | Fangrui Song <> | Subject | Re: [PATCH] arm64: Delete the space separator in __emit_inst |
| |
On 2020-04-14, Mark Rutland wrote: >Hi Fangrui, > >On Sun, Apr 12, 2020 at 08:38:11PM -0700, Fangrui Song wrote: >> Many instances of __emit_inst(x) expand to a directive. In a few places >> it is used as a macro argument, e.g. >> >> arch/arm64/include/asm/sysreg.h >> #define __emit_inst(x) .inst (x) >> >> arch/arm64/include/asm/sysreg.h >> #define SET_PSTATE_PAN(x) __emit_inst(0xd500401f | PSTATE_PAN | ((!!x) << PSTATE_Imm_shift)) >> >> arch/arm64/kvm/hyp/entry.S >> ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN) >> >> Clang integrated assembler parses `.inst (x)` as two arguments passing >> to a macro. We delete the space separator so that `.inst(x)` will be >> parsed as one argument. > >I'm a little confused by the above; sorry if the below sounds stupid or >pedantic, but I just want to make sure I've understood the problem >correctly. > >For the above, ALTERNATIVE() and SET_PSTATE_PAN() are both preprocessor >macros, so I would expect those to be expanded before either the >integrated assembler or an external assembler consumes any of the >assembly (and both would see the same expanded text). Given that, I'm a >bit confused as to why the integrated assembly would have an impact on >preprocessing. > >Does compiling the pre-processed source using the integrated assembler >result in the same behaviour? Can we see the expanded text to make that >clear? > >... at what stage exactly does this go wrong? > >Thanks, >Mark.
Hi Mark,
The C preprocessor expands arch/arm64/kvm/hyp/entry.S ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
to: alternative_insn nop, .inst (0xd500401f | ((0) << 16 | (4) << 5) | ((!!1) << 8)), 4, 1
`alternative_insn` is an assembler macro, not handled by the C preprocessor.
Both comma and space are separators, with an exception that content inside a pair of parentheses/quotes is not split, so clang -cc1as or GNU as x86 splits arguments this way:
nop, .inst, (0xd500401f | ((0) << 16 | (4) << 5) | ((!!1) << 8)), 4, 1
I actually feel that GNU as arm64's behavior is a little bit buggy. It works just because GNU as has another preprocessing step `do_scrub_chars` and its arm64 backend deletes the space before '('
alternative_insn nop,.inst(0xd500401f|((0)<<16|(4)<<5)|((!!1)<<8)),4,1
The x86 backend keeps the space before the outmost '('
alternative_insn nop,.inst (0xd500401f|((0)<<16|(4)<<5)|((!!1)<<8)),4,1
By reading its state machine, I think keeping the spaces will be the most reasonable behavior.
If .inst were only used as arguments,
alternative_insn nop, ".inst (0xd500401f | ((0) << 16 | (4) << 5) | ((!!1) << 8))", 4, 1
would be the best to avoid parsing issues.
>> >> Note, GNU as parsing `.inst (x)` as one argument is unintentional (for >> example the x86 backend will parse the construct as two arguments). >> See https://sourceware.org/bugzilla/show_bug.cgi?id=25750#c10 >> >> Link: https://github.com/ClangBuiltLinux/linux/issues/939 >> Cc: clang-built-linux@googlegroups.com >> Signed-off-by: Fangrui Song <maskray@google.com> >> --- >> arch/arm64/include/asm/sysreg.h | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h >> index ebc622432831..af21e2ec5e3e 100644 >> --- a/arch/arm64/include/asm/sysreg.h >> +++ b/arch/arm64/include/asm/sysreg.h >> @@ -49,7 +49,9 @@ >> #ifndef CONFIG_BROKEN_GAS_INST >> >> #ifdef __ASSEMBLY__ >> -#define __emit_inst(x) .inst (x) >> +// The space separator is omitted so that __emit_inst(x) can be parsed as >> +// either a directive or a macro argument. >> +#define __emit_inst(x) .inst(x) >> #else >> #define __emit_inst(x) ".inst " __stringify((x)) "\n\t" >> #endif >> -- >> 2.26.0.110.g2183baf09c-goog >>
| |