Fix CLR_CMAKE_USE_SYSTEM_LIBUNWIND on macOS

[Bo98]

On macOS, libunwind does not exist, which causes find_unwind_libs to fail. macOS does not require any linker flags as libunwind exists in libSystem, which is implicitly linked.

If accepted, I'd like to request this change is backported to 6.0 since it affects that too.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[am11]

@Bo98, thank you for your contribution. Could you describe why are you passing -DCLR_CMAKE_USE_SYSTEM_LIBUNWIND=ON to ./build.sh on macOS? The usage of this argument from command-line is intended for linux.

[Bo98]

Can definitely gate my passing to a Linux conditional if that's preferred. I was torn a bit between that and this. The reason I opted for this way because it makes more semantic sense, given the likes of:

runtime/src/coreclr/pal/src/CMakeLists.txt

Lines 1 to 4 in 9c9ebb9

	if(CLR_CMAKE_TARGET_OSX OR CLR_CMAKE_TARGET_FREEBSD)
	# On OSX and *BSD, we use the libunwind that's part of the OS
	set(CLR_CMAKE_USE_SYSTEM_LIBUNWIND 1)
	endif(CLR_CMAKE_TARGET_OSX OR CLR_CMAKE_TARGET_FREEBSD)

and

runtime/src/coreclr/pal/src/CMakeLists.txt

Lines 19 to 20 in 9c9ebb9

	elseif(NOT CLR_CMAKE_TARGET_OSX)
	find_unwind_libs(UNWIND_LIBS)

The default probably should not be defined twice in two different places. Feels inconsistent to me but if that's intentional then that's fine.

[Bo98]

(It was also something that didn't used to be conditioned per OS: b9f273a - I missed that setting it manually isn't supported much anymore and auto-detection is preferred, apologies.)

[am11]

Your change looks alright; fixing this inconsistency is good. 🙂 I was mainly curious about the need of backporting to 6.0, since it is not a widespread issue and it is actually working as designed (the original need for this bailing option emerged from Linux distro package).

---

Note that the copy of libunwind sources, that we have in this repo, has handful of carefully optimized tweaks to improve, enhance and even bugfix (in case of ARM architecutre) the runtime's interaction with stack unwinder on top of latest libunwind v1.6.2. We have upstreamed some of those tweaks back to https://github.com/libunwind/libunwind, but there are still a few patches not upstreamed yet. So compiling libunwind from the in-tree sources (i.e. build without -DCLR_CMAKE_USE_SYSTEM_LIBUNWIND=ON) is definitely better than using the one from some distro packages, e.g. Debian, which are providing very old version (1.3.2: https://tracker.debian.org/pkg/libunwind). Last time I checked, their (low priority) criteria of revving this package version was based on the number of its direct dependents (which was just three packages when I checked it).

In runtime repo, there are some (regular) dependencies like libkrb5, liblttng-ust and libicu which are needed from the system packages (or they are optional like libnuma). Then there are some dependencies which have sources in-tree (all these: https://github.com/dotnet/runtime/tree/e448104/src/native/external), which IMO are best to be compiled with the given runtime version / commit. (hint: there is a reason why the copy of those sources deemed necessary in first place). If there is a security issue which pops up, consider that as part of .NET release (not as a separate one), and wait for the patch issued from this repo.

[Bo98]

definitely better than using the one from some distro packages, e.g. Debian, which are providing very old version

Yeah, to be clear: I'm in a package manager environment where libunwind is latest 1.6.2. I'll have a look further into the additional patches you are mentioning however (but as you say, upstreaming is definitely ideal where possible).

I was mainly curious about the need of backporting to 6.0, since it is not a widespread issue and it is actually working as designed (the original need for this bailing option emerged from Linux distro package).

Fair enough. It's one of those things that worked in 5.0, but it definitely does not need backporting if it's not seen as important.

[am11]

I'll have a look further into the additional patches you are mentioning however

We maintain the logs in the corresponding version files, e.g. https://github.com/dotnet/runtime/blob/e448104/src/native/external/libunwind-version.txt.

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

On macOS, libunwind does not exist, which causes find_unwind_libs to fail. macOS does not require any linker flags as libunwind exists in libSystem, which is implicitly linked.

If accepted, I'd like to request this change is backported to 6.0 since it affects that too.

Author:	Bo98
Assignees:	-
Labels:	area-Host, community-contribution
Milestone:	-

[janvorli]

LGTM, thank you!