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

[tambre]

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:

• https://reviews.llvm.org/rGb780df052dd2b246a760d00e00f7de9ebdab9d09
• https://reviews.llvm.org/rGd4ded05ba851304b26a437896bc3962ef56f62cb

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.

[tambre]

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.

[phosek]

In D86308#2228917, @tambre wrote:

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.

[mstorsjo]

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]

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

[tambre]

In D86308#2229199, @mstorsjo wrote:

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.

In D86308#2229176, @phosek wrote:

In D86308#2228917, @tambre wrote:

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]

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

[tambre]

In D86308#2229901, @teemperor wrote:

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.

[teemperor]

In D86308#2229936, @tambre wrote:

In D86308#2229901, @teemperor wrote:

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]

Gate Apple workaround behind CMake version 3.19

[tambre]

In D86308#2233934, @teemperor wrote:

In D86308#2229936, @tambre wrote:

In D86308#2229901, @teemperor wrote:

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]

Rebase, just in case

[aaronpuchert]

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

[tambre]

In D86308#2421228, @aaronpuchert wrote:

@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

[aaronpuchert]

In D86308#2421237, @tambre wrote:

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.

[tambre]

In D86308#2421249, @aaronpuchert wrote:

In D86308#2421237, @tambre wrote:

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.

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

clang/runtime/CMakeLists.txt

ExternalProject_Add(compiler-rt
     DEPENDS llvm-config clang ${compiler_rt_configure_deps}
     PREFIX ${COMPILER_RT_PREFIX}
     SOURCE_DIR ${COMPILER_RT_SRC_ROOT}
     STAMP_DIR ${STAMP_DIR}
     BINARY_DIR ${BINARY_DIR}
     CMAKE_ARGS ${CLANG_COMPILER_RT_CMAKE_ARGS}
                -DCMAKE_C_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
                -DCMAKE_CXX_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++
+               -DCMAKE_ASM_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
                -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
                -DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
                -DLLVM_CONFIG_PATH=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-config
                -DLLVM_LIT_ARGS=${LLVM_LIT_ARGS}
                -DCOMPILER_RT_OUTPUT_DIR=${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}
                -DCOMPILER_RT_EXEC_OUTPUT_DIR=${LLVM_RUNTIME_OUTPUT_INTDIR}
                -DCOMPILER_RT_INSTALL_PATH:STRING=lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}
                -DCOMPILER_RT_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}

compiler-rt/cmake/Modules/AddCompilerRT.cmake

function(add_compiler_rt_component name)
   if(COMMAND runtime_register_component)
     runtime_register_component(${name})
   endif()
   add_dependencies(compiler-rt ${name})
 endfunction()
 
 function(add_asm_sources output)
   set(${output} ${ARGN} PARENT_SCOPE)
-  # Xcode will try to compile asm files as C ('clang -x c'), and that will fail.
-  if (${CMAKE_GENERATOR} STREQUAL "Xcode")
-    enable_language(ASM)
-  else()
-    # Pass ASM file directly to the C++ compiler.
+  # CMake doesn't pass the correct architecture for Apple prior to CMake 3.19. https://gitlab.kitware.com/cmake/cmake/-/issues/20771
+  # MinGW didn't work correctly with assembly prior to CMake 3.17. https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4287 and https://reviews.llvm.org/rGb780df052dd2b246a760d00e00f7de9ebdab9d09
+  # Workaround these two issues by compiling as C.
+  # Same workaround used in libunwind. Also update there if changed here.
+  if((APPLE AND CMAKE_VERSION VERSION_LESS 3.19) OR (MINGW AND CMAKE_VERSION VERSION_LESS 3.17))
     set_source_files_properties(${ARGN} PROPERTIES LANGUAGE C)
   endif()
 endfunction()
 
 macro(set_output_name output name arch)
   if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR)
     set(${output} ${name})
   else()

libunwind/src/CMakeLists.txt

 set_source_files_properties(${LIBUNWIND_C_SOURCES}
                             PROPERTIES
                               COMPILE_FLAGS "-std=c99")
 
 set(LIBUNWIND_ASM_SOURCES
     UnwindRegistersRestore.S
     UnwindRegistersSave.S
     )
-if (MINGW OR APPLE)
-  # CMake doesn't build assembly sources for windows/gnu targets properly
-  # (up to current CMake, 3.16), so treat them as C files.
-  # Additionally, CMake ignores OSX_ARCHITECTURE for ASM files when targeting
-  # Apple platforms.
-  set_source_files_properties(${LIBUNWIND_ASM_SOURCES}
-                              PROPERTIES
-                                LANGUAGE C)
+
+# See add_asm_sources() in compiler-rt for explanation of this workaround.
+if((APPLE AND CMAKE_VERSION VERSION_LESS 3.19) OR (MINGW AND CMAKE_VERSION VERSION_LESS 3.17))
+  set_source_files_properties(${LIBUNWIND_ASM_SOURCES} PROPERTIES LANGUAGE C)
 endif()
 
 set(LIBUNWIND_HEADERS
     AddressSpace.hpp
     assembly.h
     CompactUnwinder.hpp
     config.h
     dwarf2.h