Page MenuHomePhabricator

[CMake][compiler-rt][libunwind] Compile assembly files as ASM not C, unify workarounds
ClosedPublic

Authored by tambre on Aug 20 2020, 10:19 AM.

Details

Summary

It isn't very wise to pass an assembly file to the compiler and tell it to compile as a C file and hope that the compiler recognizes it as assembly instead.
Simply don't mark the file as C and CMake will recognize the rest.

This was attempted earlier in https://reviews.llvm.org/D85706, but reverted due to architecture issues on Apple.
Subsequent digging revealed a similar change was done earlier for libunwind in https://reviews.llvm.org/rGb780df052dd2b246a760d00e00f7de9ebdab9d09.
Afterwards workarounds were added for MinGW and Apple:

The workarounds in libunwind and compiler-rt are unified and comments added pointing to each other.
The workaround is updated to only be used for MinGW for CMake versions before 3.17, which fixed the issue (https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4287).

Additionally fixed Clang not being passed as the assembly compiler for compiler-rt runtime build.

Example error:
[525/634] Building C object lib/tsan/CMakeFiles/clang_rt.tsan-aarch64.dir/rtl/tsan_rtl_aarch64.S.o
FAILED: lib/tsan/CMakeFiles/clang_rt.tsan-aarch64.dir/rtl/tsan_rtl_aarch64.S.o
/opt/tooling/drive/host/bin/clang --target=aarch64-linux-gnu -I/opt/tooling/drive/llvm/compiler-rt/lib/tsan/.. -isystem /opt/tooling/drive/toolchain/opt/drive/toolchain/include -x c -Wall -Wno-unused-parameter -fno-lto -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -fPIE -fno-rtti -Wframe-larger-than=530 -Wglobal-constructors --sysroot=. -MD -MT lib/tsan/CMakeFiles/clang_rt.tsan-aarch64.dir/rtl/tsan_rtl_aarch64.S.o -MF lib/tsan/CMakeFiles/clang_rt.tsan-aarch64.dir/rtl/tsan_rtl_aarch64.S.o.d -o lib/tsan/CMakeFiles/clang_rt.tsan-aarch64.dir/rtl/tsan_rtl_aarch64.S.o -c /opt/tooling/drive/llvm/compiler-rt/lib/tsan/rtl/tsan_rtl_aarch64.S
/opt/tooling/drive/llvm/compiler-rt/lib/tsan/rtl/tsan_rtl_aarch64.S:29:1: error: expected identifier or '('
.section .text
^
1 error generated.

Diff Detail

Event Timeline

tambre created this revision.Aug 20 2020, 10:19 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 20 2020, 10:19 AM
Herald added subscribers: Restricted Project, cfe-commits, dexonsmith and 3 others. · View Herald Transcript
tambre requested review of this revision.Aug 20 2020, 10:19 AM

I'm pretty sure add_asm_sources() has nothing to do. The ASM language is enabled by compiler-rt anyway and CMake can recognize the files as assembly anyway.

@teemperor Could you give this a try? If this doesn't work, could you upload the Ninja file for me to investigate? I've never used macOS so the simulators and many architectures at once is quite new to me.

I'm pretty sure add_asm_sources() has nothing to do. The ASM language is enabled by compiler-rt anyway and CMake can recognize the files as assembly anyway.

Can we remove that function altogether then?

@teemperor Could you give this a try? If this doesn't work, could you upload the Ninja file for me to investigate? I've never used macOS so the simulators and many architectures at once is quite new to me.

Just FWIW, a similar change was made in libunwind earlier (c48974ffd7d1676f79d39d3b1e70f07d3a5e2e44), which then required workarounds for cmake issues on both mingw and macos (see b780df052dd2b246a760d00e00f7de9ebdab9d09 and d4ded05ba851304b26a437896bc3962ef56f62cb) to reintroduce the code for building the asm code as C.

tambre updated this revision to Diff 286957.Aug 21 2020, 12:22 AM

Add workaround for Apple, and for MinGW prior to CMake 3.17.

Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2020, 12:22 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
tambre retitled this revision from Reland [compiler-rt] Compile assembly files as ASM not C to [CMake][compiler-rt][libunwind] Compile assembly files as ASM not C, unify workarounds.Aug 21 2020, 12:30 AM
tambre edited the summary of this revision. (Show Details)
tambre edited the summary of this revision. (Show Details)Aug 21 2020, 12:33 AM

Just FWIW, a similar change was made in libunwind earlier (c48974ffd7d1676f79d39d3b1e70f07d3a5e2e44), which then required workarounds for cmake issues on both mingw and macos (see b780df052dd2b246a760d00e00f7de9ebdab9d09 and d4ded05ba851304b26a437896bc3962ef56f62cb) to reintroduce the code for building the asm code as C.

Thanks for the links! That helps a ton.
I've unified the workaround code to be the same and expanded the comments in compiler-rt to refer to the relevant upstream issues and to each other.

I'm pretty sure add_asm_sources() has nothing to do. The ASM language is enabled by compiler-rt anyway and CMake can recognize the files as assembly anyway.

Can we remove that function altogether then?

Seems we'll need it for the workarounds.

teemperor accepted this revision.Aug 21 2020, 12:48 AM

Sorry, just got around to check this out. With the new workaround this seems to work on macOS (the initial patch did produce the same error).

Sorry, just got around to check this out. With the new workaround this seems to work on macOS (the initial patch did produce the same error).

Many thanks!
I've submitted an upstream CMake MR to hopefully fix this. I'd appreciate if you could test that too. There's an example testcase in CMake issue 20771.

Sorry, just got around to check this out. With the new workaround this seems to work on macOS (the initial patch did produce the same error).

Many thanks!
I've submitted an upstream CMake MR to hopefully fix this. I'd appreciate if you could test that too. There's an example testcase in CMake issue 20771.

With that CMake patch your original patch compiles just fine on macOS. Feel free to add a FIXME that this workaround can be removed when the CMake version you merged this is because the new minimum required. Thanks!

tambre updated this revision to Diff 287421.Aug 24 2020, 9:32 AM

Gate Apple workaround behind CMake version 3.19

tambre added a comment.EditedAug 24 2020, 9:34 AM

Sorry, just got around to check this out. With the new workaround this seems to work on macOS (the initial patch did produce the same error).

Many thanks!
I've submitted an upstream CMake MR to hopefully fix this. I'd appreciate if you could test that too. There's an example testcase in CMake issue 20771.

With that CMake patch your original patch compiles just fine on macOS. Feel free to add a FIXME that this workaround can be removed when the CMake version you merged this is because the new minimum required. Thanks!

Thanks for testing!

I've guarded the Apple workaround behind CMake 3.19 version check. I'll ensure that the fix on the CMake side makes it into that version.

I'll commit this tomorrow if there are no objections.

tambre updated this revision to Diff 288292.Aug 27 2020, 5:27 AM

Rebase, just in case

This revision was not accepted when it landed; it landed in state Needs Review.Aug 27 2020, 5:40 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

@tstellar, can we have this in 11.0.1? I've already ported it back in openSUSE because we were having problems with the CMake 3.19 update.

@tstellar, can we have this in 11.0.1? I've already ported it back in openSUSE because we were having problems with the CMake 3.19 update.

This has already been backported to 11.0.1: https://github.com/llvm/llvm-project/commit/03565ffd5da8370f5b89b69cd9868f32e2d75403

Thanks! I didn't see any mention of that here so I assumed it wasn't backported without checking.

Thanks! I didn't see any mention of that here so I assumed it wasn't backported without checking.

Also, the issue was an accidental breaking change in CMake 3.19.0 and has been reverted in 3.19.1.