Messages in this thread | | | From | Nick Desaulniers <> | Date | Mon, 13 Apr 2020 11:07:37 -0700 | Subject | Re: [PATCH] arm64: Delete the space separator in __emit_inst |
| |
On Sun, Apr 12, 2020 at 8:38 PM 'Fangrui Song' via Clang Built Linux <clang-built-linux@googlegroups.com> 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. > > 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
Thanks for the patch and for leading the discussion with the binutils developers on this curious parsing case.
> > Link: https://github.com/ClangBuiltLinux/linux/issues/939 > Cc: clang-built-linux@googlegroups.com > Signed-off-by: Fangrui Song <maskray@google.com>
Shouldn't this have: Suggested-by: Ilie Halip <ilie.halip@gmail.com> Since Ilie sugguested this in https://github.com/ClangBuiltLinux/linux/issues/939#issuecomment-601776123?
> --- > 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"
What happens if someone starts using `__emit_inst` from inline assembly, and passes that subexpression to a macro? There are no users today in arch/arm64/, but I think it's better to change both.
With this applied on -next, and testing via: $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make LLVM=1 LLVM_IAS=1 -j71 defconfig $ qemu-system-aarch64 -kernel arch/arm64/boot/Image.gz -machine virt -cpu cortex-a57 -nographic --append "console=ttyAMA0" -m 2048 -initrd <path to my buildroot.cpio>
I was able to build and boot, modulo: https://github.com/ClangBuiltLinux/linux/issues/988 (new) https://github.com/ClangBuiltLinux/linux/issues/716 https://github.com/ClangBuiltLinux/linux/issues/510 (more specific to LLD than AS) so we're pretty close to being able to assemble an arm64 defconfig with Clang. -- Thanks, ~Nick Desaulniers
| |