Revert "[Compiler-rt][AArch64] Workaround for .cfi_startproc assembler parser bug."
This reverts commit 3000c19df64f89ff319590f3a6e4d6b93d20983d.
Differential Revision: https://reviews.llvm.org/D93379
tambre on Dec 16 2020, 12:14 AM.Authored by
I'm troubleshooting LLVM build on Windows on Arm, which is currently failing with
Looking at https://reviews.llvm.org/rG039cb03dd0dcff4daa17a062f7414ac22bf5f2eb I don't understand how that fixes the problem of masm parser. Could you explain how it is supposed to work, please?
The issue turned out to not be parser bug, but rather the incorrect statement separator being used by compiler-rt on certain platforms.
Could you provide the full compile command that throws an error during your build?
I'm seeing this only on aarch64-windows-msvc target, and not on any aarch64-linux-gnu targets. The command line is below, but I'm not sure you'll reproduce the problem outside of Windows environment.
C:\Users\tcwg\source\llvm-package\bin\clang-cl.exe /nologo -DHAS_ASM_LSE -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -DL_cas -DMODEL=3 -DSIZE=2 -IC:\Users\tcwg\source\maxim\llvm_package_92a5106e\llvm-project\compiler-rt\lib\builtins -Iprojects\compiler-rt\lib\builtins -IC:\Users\tcwg\source\maxim\llvm_package_92a5106e\llvm-project\compiler-rt\lib\builtins -Iinclude -IC:\Users\tcwg\source\maxim\llvm_package_92a5106e\llvm-project\llvm\include -march=armv8-a -DCOMPILER_RT_HAS_FLOAT16 /showIncludes /Foprojects\compiler-rt\lib\builtins\CMakeFiles\clang_rt.builtins-aarch64.dir\outline_atomic_helpers.dir\outline_atomic_cas2_3.S.obj /Fdprojects\compiler-rt\lib\builtins\CMakeFiles\clang_rt.builtins-aarch64.dir\clang_rt.builtins-aarch64.pdb -c -- projects\compiler-rt\lib\builtins\outline_atomic_helpers.dir\outline_atomic_cas2_3.S
Thanks. This can be reproduced with:
Turns out Microsoft COFF style assembler's comment syntax like Apple is ;, but it's the only one lacking a statement separator syntax.
There were probably important statements ending up as comments before this, but I guess no one had stumbled upon them yet.
Avoiding the issue would mean forgoing the macros completely or adding another preprocessing step for Windows only for all assembly files.
aarch64-windows-gnu might be close enough to not cause any problems?
Alterantively, maybe a compiler extension to support statement separators?
A few clarifications here. When clang-cl (for x86) parses assembly, it parses GAS-style .S assembly, including support for GAS macros and all that. MASM (ml.exe/ml64.exe) style assembly is something entirely different, and that's only handled by llvm-ml (which is a work in progress). So there's no real reference for what the MSVC-dialect (triggered by x86*-windows-msvc triples) of GAS assembly would be, as no such thing really exists, so in practice I guess it's just mostly equivalent to the corresponding x86*-windows-gnu ones.
Same thing for aarch64; there's no MSVC dialect of GAS assembly as reference for what llvm/clang should do with aarch64-windows-msvc targets. And for arm/aarch64 in general, there's no MASM format at all; MSVC tools ship with armasm.exe/armasm64.exe, which use the ARM (company) armasm format (which, just like MASM for x86, uses entirely different directives than GAS).
So with that in mind, my preference would be to just make the aarch64-windows-msvc targets behave just like the -gnu ones in this aspect. That was my initial intent when I fixed the comment char for aarch64 windows targets in D36366 (to make semicolon work as separator), but in review it was requested to only make that change for -gnu triples, see https://reviews.llvm.org/D36366#833185. Maybe we should try to revisit that discussion?
In the meantime, readding this workaround would seem sensible to me, in order not to rely on separators in assembly.
Thanks for the background.
The workaround wasn't sufficient and only hid the compile error, but would still not work in certain configurations as reported in https://reviews.llvm.org/D93278#2457928.
Yes. We should backport this to 12.0 before release if possible.
I'm building LLVM 12 release binaries for Windows on Arm, and this seems to be the only problem left.
There is no such option, as clang outline atomics (enabled by default for Linux so far with intend to extend it for other targets) are rely on compiler-rt support as well.
It seems I'm still missing some key piece of the puzzle here. If I understand your comment correctly, you say that ";" should be a statement separator for *-windows-msvc targets. But when I build .S files of LLVM 12 using LLVM 10 compiler on Windows on Arm, I do see assembler dropping everything after ";" -- i.e., at least LLVM 10's integrated assembler appears to be configured to treat ";" as beginning of a comment, with no apparent statement separator syntax. This is, apparently, configured in AArch64MCAsmInfoMicrosoftCOFF::AArch64MCAsmInfoMicrosoftCOFF .
Am I missing something?
The comment was about *-windows-msvc targets in general, in particular about x86 targets.
Yes, for the aarch64-windows-msvc target, currently semicolon is set as both comment and separator, making no actual char available as separator symbol. I'd suggest changing the comment char to '//' for aarch64-windows-msvc, like it is for aarch64-windows-gnu and ELF targets. I initially tried to do this, but was suggested to only change the -gnu target in the review I linked before. At the time I shrugged and just went with the suggestion, but at the current state I'd suggest revisiting this decision, as there's a clear benefit for making it match the other platforms, and there is no reference for what GAS assembly for aarch64 msvc targets should look like anyway.
Agree. Would you please post a patch for this?