[libcxx-commits] [PATCH] D112126: [libunwind] Try to add -unwindlib=none while configuring and building libunwind

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libunwind/CMakeLists.txt:28
+  # linking.
+  include(EnableLanguageNolink)
+  project(libunwind LANGUAGES NONE)
----------------
phosek wrote:
> When reading through CMake modules, I noticed that some CMake platform modules set `_CMAKE_FEATURE_DETECTION_TARGET_TYPE ` (see https://github.com/Kitware/CMake/blob/master/Modules/Platform/iOS-Initialize.cmake#L9), this is then used by C and C++ compiler test, see https://github.com/Kitware/CMake/blob/4e84a4763d702590fb06d62540e35a614dcd5133/Modules/CMakeTestCCompiler.cmake#L17 and https://github.com/Kitware/CMake/blob/4e84a4763d702590fb06d62540e35a614dcd5133/Modules/CMakeTestCXXCompiler.cmake#L17.
> 
> I wonder if we can use `_CMAKE_FEATURE_DETECTION_TARGET_TYPE` here as well instead of wrapping `enable_language`.
I guess we might be able to, but I guess `_CMAKE_FEATURE_DETECTION_TARGET_TYPE` is undocumented and shouldn't be used outside of cmake's own modules?


================
Comment at: libunwind/cmake/Modules/CheckCLinkerFlag.cmake:4
+
+function(check_c_linker_flag flag dest)
+  # If testing a flag with check_c_compiler_flag, it gets added to the compile
----------------
phosek wrote:
> The same function already exists in compiler-rt, see https://github.com/llvm/llvm-project/blob/2d3953499c8ca73c12e9417f5c4516c8a930a689/compiler-rt/cmake/config-ix.cmake#L9
> 
> I'd prefer choosing a name that's unlikely to conflict, for example `libunwind_check_linker_flag` or `llvm_check_linker_flag`. The latter might be preferable if we decide to move this file to a shared location later.
Oh, nice.

Hmm, I see that the new shared files in `llvm-project/cmake` seems to have stuck around without being reverted for a while now, so I guess I could add the new ones directly there, with `llvm_` prefixes, as I'd need to use them in `llvm-project/runtimes` directly in the next patch anyway.


================
Comment at: libunwind/cmake/Modules/EnableLanguageNolink.cmake:6
+  # linking may fail.
+  set(CMAKE_TRY_COMPILE_TARGET_TYPE_ORIG ${CMAKE_TRY_COMPILE_TARGET_TYPE})
+  set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
----------------
phosek wrote:
> This is just a nit, but I'd prefer using a name that starts with `__` to make it clear that this is an internal variable. CMake uses `__CMAKE_SAVED_TRY_COMPILE_TARGET_TYPE` in its own macros which might be a good choice.
Sounds good, especially as this is a macro. I'm wondering if it'd be better to use a different name to avoid potential clashes with the internal variable though...


================
Comment at: libunwind/cmake/Modules/EnableLanguageNolink.cmake:9
+  enable_language(${ARGV})
+  set(CMAKE_TRY_COMPILE_TARGET_TYPE ${CMAKE_TRY_COMPILE_TARGET_TYPE_ORIG})
+endmacro()
----------------
phosek wrote:
> Can you also do `unset(CMAKE_TRY_COMPILE_TARGET_TYPE_ORIG)` to avoid potential errors if someone else tries to use this variable elsewhere?
Yes, that's a good idea.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112126/new/

https://reviews.llvm.org/D112126