[libcxx-commits] [PATCH] D112112: [libunwind] Link with -unwindlib=none

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

In D112112#3075751 <https://reviews.llvm.org/D112112#3075751>, @hvdijk wrote:

> Keep in mind that this applies to situations where both 1) clang passes -lunwind by default and 2) libunwind is not available.

Yes, this is exactly the setup that I’m focusing on

> How this gets handled may differ between different CMake versions, but in the one I'm using (3.21.3) that's a hard error by default because the toolchain doesn't work, it requires adding `-DCMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY` to the CMake options anyway (even in LLVM 12) which disables CMake's link tests. And when that is added, that means there are no longer any link tests performed from CMake that LLVM needs to disable.

Oh, I see, so you’re manually passing that option? Ok, that would explain the difference between our outcomes. I don’t add that option to libunwind. As long as the cmake file manages to get the right options (`-unwindlib=none` and `-nostdlib++`) into `CMAKE_REQUIRED_FLAGS`, the rest of the tests from that point succeed.

I’ve experimented with setting `-DCMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY` when building some of the runtimes, but in my experience, it broke other things (making tests falsely succeed as it doesn’t test linking, enabling things that won’t work in the end).

If you try my version of the patch, does it work if you omit setting `-DCMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY`? It does for me.

In D112112#3075702 <https://reviews.llvm.org/D112112#3075702>, @phosek wrote:

> @mstorsjo CMake introduced `CMAKE_REQUIRED_LINK_OPTIONS` to address specifically this issue (see https://cmake.org/cmake/help/latest/module/CheckCSourceCompiles.html), but unfortunately it was only introduced in 3.14 and LLVM is still on 3.13.
>
> In my testing, in CMake versions <3.14 `CMAKE_REQUIRED_FLAGS` apply to link command as well but in >=3.14 you need to use `CMAKE_REQUIRED_LINK_OPTIONS` so I think we'll need to have conditional logic based on the CMake version.

That’s not what I was referring to. If newer versions of cmake change whether `CMAKE_REQUIRED_FLAGS` apply to linking, that’d be a breaking change that requires opt in with a policy, right?

The thing I’m referring to is that when `-unwindlib=none` is needed for linking to succeed, one can’t test for it with `check_c_compiler_flag` (which compiles one object file with the flag, where it has no effect, and then tries linking the object file without the flag), but one has to set it in  `CMAKE_REQUIRED_FLAGS` (so it gets included both when compiling and linking, in `check_c_compiler_flag`) and run a test with it set, see D112126 <https://reviews.llvm.org/D112126>.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112112