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

Remove dependency on libunwind #17094

Merged
merged 4 commits into from
Mar 22, 2018
Merged

Conversation

janvorli
Copy link
Member

This change removes dependency on the libunwind for all Unixes except OSX by
including its sources in coreclr PAL.
In OSX the libunwind is part of the OS and also has support for compact unwind
info that is OSX specific, so we keep using that one.

The sources were taken from https://github.com/libunwind/libunwind/tree/v1.3-stable
We already have the license for it described in https://github.com/dotnet/coreclr/blob/master/THIRD-PARTY-NOTICES.TXT#L175-L198

There are two commits - one is just a pure copy of the original sources with added
changes in .gitignore that contained entries that were filtering out some files in the
libunwind (*_i.h pattern and x86/ folders).
The second commit enables building libunwind for all the architectures we support.
I have tested the build for x64 on Ubuntu, Alpine and FreeBSD. I've also tested
build for arm, arm64 and x86 on Ubuntu using cross build.

I've found a bug in the x86 code of the libunwind, but I'll leave the fix for later and
create an issue for it. While the fix looks trivial, it still needs to be tested.

This change enables building libunwind and removes
dependency on the libunwind package for all Unixes
except OSX. In OSX the libunwind is part of the OS
and also has support for compact unwind info that
is OSX specific, so we keep using that one.
@janvorli janvorli added this to the 2.1.0 milestone Mar 21, 2018
@janvorli janvorli self-assigned this Mar 21, 2018
@janvorli janvorli requested a review from jkotas March 21, 2018 16:18
@janvorli
Copy link
Member Author

FYI: @eerhardt, @Petermarcu, @sergiy-k

@danmoseley
Copy link
Member

How often do we expect to have to take updates from the libunwind repo? I guess it would not be difficult since it is a verbatim copy of the tree.

@janvorli
Copy link
Member Author

@danmosemsft I expect it to be infrequent. The library is very mature. It would be good to monitor the original repo for bug fixes and issues we care about (x86, x64, arm, arm64), but if things work, we can stay on the same version.

@Petermarcu
Copy link
Member

Assuming all the testing has gone well, can we get this in today?

@janvorli
Copy link
Member Author

Assuming all the testing has gone well, can we get this in today?

Unless there are some issues with the code review, I don't see a problem with doing that.

@echesakov
Copy link

echesakov commented Mar 21, 2018

@janvorli I was trying to build Linux/arm coreclr with your changes using ./build.sh arm checked cross crosscomponent and hit this issue during the cross-arch components build:

Building CXX object src/vm/eventpipe/CMakeFiles/eventpipe.dir/eventpipe/dotnetruntime.cpp.o
/home/dotnet-bot/coreclr/src/pal/src/libunwind/src/arm/is_fpreg.c:33:22: error: use of undeclared identifier 'UNW_ARM_S0'
  return ((regnum >= UNW_ARM_S0 && regnum <= UNW_ARM_S31)
                     ^
/home/dotnet-bot/coreclr/src/pal/src/libunwind/src/arm/is_fpreg.c:33:46: error: use of undeclared identifier 'UNW_ARM_S31'; did you mean 'UNW_X86_ST1'?
  return ((regnum >= UNW_ARM_S0 && regnum <= UNW_ARM_S31)
                                             ^~~~~~~~~~~
                                             UNW_X86_ST1
/home/dotnet-bot/coreclr/src/pal/src/libunwind/include/libunwind-x86.h:89:5: note: 'UNW_X86_ST1' declared here
    UNW_X86_ST1,        /* scratch */
    ^
/home/dotnet-bot/coreclr/src/pal/src/libunwind/src/arm/is_fpreg.c:34:25: error: use of undeclared identifier 'UNW_ARM_F0'; did you mean 'UNW_X86_FS'?
          || (regnum >= UNW_ARM_F0 && regnum <= UNW_ARM_F7)
                        ^~~~~~~~~~
                        UNW_X86_FS
/home/dotnet-bot/coreclr/src/pal/src/libunwind/include/libunwind-x86.h:128:5: note: 'UNW_X86_FS' declared here
    UNW_X86_FS,         /* special */
    ^
/home/dotnet-bot/coreclr/src/pal/src/libunwind/src/arm/is_fpreg.c:34:49: error: use of undeclared identifier 'UNW_ARM_F7'; did you mean 'UNW_X86_FS'?
          || (regnum >= UNW_ARM_F0 && regnum <= UNW_ARM_F7)
                                                ^~~~~~~~~~
                                                UNW_X86_FS
/home/dotnet-bot/coreclr/src/pal/src/libunwind/include/libunwind-x86.h:128:5: note: 'UNW_X86_FS' declared here
    UNW_X86_FS,         /* special */
    ^

@janvorli
Copy link
Member Author

@echesakovMSFT hmm I have not tried to build the cross components. I need to look into it.

@janvorli
Copy link
Member Author

@echesakovMSFT I've fixed it, now the crosscomponents are building correctly.

@janvorli
Copy link
Member Author

@eerhardt has found the release build on arm has a build error - using printf without declaration in one of the libunwind sources. I was testing only debug builds locally before.
The cross components build for ARM also exposes the issue in x86 code I've mentioned in the PR description above.
All of these are warnings reported as errors and these are in code that we never exercise. So I will disable these warnings for now and create an issue to fix them in our repo and then in the libunwind repo.

@janvorli janvorli merged commit d104270 into dotnet:master Mar 22, 2018
@janvorli janvorli deleted the embed-libunwin-source branch March 22, 2018 00:12
@ghost
Copy link

ghost commented Mar 22, 2018

Great! I have added one more to the wish list https://github.com/dotnet/corefx/issues/28353 😄

@tmds
Copy link
Member

tmds commented Mar 22, 2018

fyi @omajid

@omajid
Copy link
Member

omajid commented Mar 23, 2018

FWIW, I can't seem to build coreclr without libunwind-devel installed after this change. It seems like some cmake configure tests and some header files are still looking for a system-wide libunwind.h header. I will open a separate PR to address that when I figure out the details.

@omajid
Copy link
Member

omajid commented Mar 23, 2018

I opened #17164 to make it possible to link against a system libunwind

@xiangzhai
Copy link

:mips-interest

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

Successfully merging this pull request may close these issues.

8 participants