Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Optionally support using the system-installed libunwind #17164

Merged
merged 1 commit into from
Mar 26, 2018

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 23, 2018

This allows coreclr to link against a system-installed libunwind instead of using the copy included with the coreclr sources.

The default is still to use the bundled version, except on macOS, where we always use the system-installed version.

Many Linux distributions prefer to avoid bundled libraries as much as possible. For their reasons, please see:

One thing to note is that this removes the include_directories(libunwind/include) call on macOS. It doesn't make sense to me to try and use the bundled include headers but link against the system library. I can revert this change if that doesn't make sense.

This change also doesn't mark the new flag as an OPTION. I need to look into that. In the meantime, I have been using ./build.sh -cmakeargs -DCLR_USE_SYSTEM_LIBUNWIND=1

Other things I found while implementing this:

  • The configure tests are using #include <libunwind.h> (unconditionally). If libunwind-devel (or similar) is not installed, they simply don't find libunwind headers and the configure tests fail. If they do find the headers, they can't find a libunwind to link against and stiil fail. IMO, they should be fixed to find the bundled copy and test against that (after that bundled libunwind is build).
  • If I remove libunwind-devel from my system, the build fails at compiling seh-unwind.cpp
In file included from /home/omajid/devel/dotnet/coreclr/src/pal/src/exception/seh.cpp:442:      
/home/omajid/devel/dotnet/coreclr/src/pal/src/exception/seh-unwind.cpp:31:10: fatal error: 'libunwind.h' file not
      found
#include <libunwind.h>
         ^~~~~~~~~~~~~                                                                                  
1 error generated.

Neither of these is affected by this change.

cc @janvorli @jkotas

@janvorli
Copy link
Member

Ah, I can see what's causing it to not to find the headers when there is no libunwind-dev package installed. I have forgotten that the libunwind.h is a generated file and as such, it lives in the binary directories. So that folder needs to be added to include paths too.
And I should have removed the two checks for libunwind features from the configure.cmake (before your change). With your change, they should be in only if the CLR_USE_SYSTEM_LIBUNWIND is set and the places where these two HAVE_xxx are used need to be updated accordingly.

Many Linux distributions prefer to avoid bundled libraries as much as possible.

I understand that it makes sense in the case our stuff is built as a native package for a distro.
cc: @Petermarcu, @eerhardt

@Petermarcu
Copy link
Member

Yes, when a distro builds .NET Core from source, they should be able to depend on their build of libunwind and not have it duplicated in our runtime binaries.

@omajid
Copy link
Member Author

omajid commented Mar 24, 2018

And I should have removed the two checks for libunwind features from the configure.cmake (before your change). With your change, they should be in only if the CLR_USE_SYSTEM_LIBUNWIND is set and the places where these two HAVE_xxx are used need to be updated accordingly.

When I was looking through those feature checks, there was a mix of items. Some checks looked safe to eliminate for the bundled copy of libunwind (HAVE_UNW_GET_SAVE_LOC depends on the version of libunwind). Some others actually seem to vary by architectures (UNWIND_CONTEXT_IS_UCONTEXT_T). If we don't want to duplicate the logic in libunwind we might want to build libunwind and then run these feature checks against the built version.

I see that the tizen build failed. Is a known failure or something specific to my changes?

@janvorli
Copy link
Member

@omajid actually, the tests should be included in all cases, as you've indicated. I was not thinking right on Friday night.
However, we don't need the libunwind to be compiled before the test. We just need to change the test so that it just tries to compile and not link when detecting it. CMake allows setting compiler flags per test, so I can add "-c" option to ask it to compile only. Then the headers are sufficient.
The problem with the detection not using the right libunwind.h was due to the fact that I have added the include folders for the test only for the UNWIND_CONTEXT_IS_UCONTEXT_T and not for the other ones.

I am preparing a fix.

@janvorli
Copy link
Member

As for the Tizen, it seems to be broken now.


add_compile_options(-fPIC)

if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
if(NOT CLR_USE_SYSTEM_LIBUNWIND)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit - could you please rename the symbol to CLR_CMAKE_USE_SYSTEM_LIBUNWIND? We have other symbols that we pass to cmake from our build and they are all named with CLR_CMAKE prefix, so it would be good to keep the convention.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't realize the convention. Fixed now.

This allows coreclr to link against a system-installed libunwind instead
of using the copy included with the coreclr sources.

The default is still to use the bundled version, except on macOS, where
we always use the system-installed version.

Many Linux distributions prefer to avoid bundled libraries as much as
possible. For their reasons, please see:

- https://fedoraproject.org/wiki/Bundled_Libraries
- https://wiki.debian.org/UpstreamGuide#No_inclusion_of_third_party_code
- https://wiki.gentoo.org/wiki/Why_not_bundle_dependencies
@omajid omajid force-pushed the system-libunwind branch from 25c8bdd to 53b208a Compare March 26, 2018 15:07
@janvorli
Copy link
Member

Thank you!

@omajid
Copy link
Member Author

omajid commented Mar 26, 2018

No problem. Happy to scratch my own itch :) And thanks for the prompt reviews and feedback!

@janvorli
Copy link
Member

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test please

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants