Skip to content

Re-enable system libunwind support #1761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 17, 2020
Merged

Re-enable system libunwind support #1761

merged 1 commit into from
Nov 17, 2020

Conversation

omajid
Copy link
Member

@omajid omajid commented Sep 22, 2020

Recent discussion on dotnet/coreclr#26082 suggests this should be working.

@omajid omajid marked this pull request as ready for review September 22, 2020 22:33
dagood
dagood reviewed Sep 23, 2020
View reviewed changes
repos/runtime.common.props Outdated
<!-- coreclr -->
<!-- coreclr build fails when passing this flag after this PR https://github.com/dotnet/coreclr/pull/26082 -->
<!-- <CoreClrBuildArguments Condition="'$(UseSystemLibraries)' == 'true' AND '$(OS)' != 'Windows_NT'">$(CoreClrBuildArguments) cmakeargs -DCLR_CMAKE_USE_SYSTEM_LIBUNWIND=TRUE</CoreClrBuildArguments> -->
<CoreClrBuildArguments Condition="'$(UseSystemLibraries)' == 'true' AND '$(OS)' != 'Windows_NT'">$(CoreClrBuildArguments) cmakeargs -DCLR_CMAKE_USE_SYSTEM_LIBUNWIND=TRUE</CoreClrBuildArguments>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I've followed these flags closely enough, but should we have a UseSystemLibunwind we use here rather than UseSystemLibraries? In 3.1 we have this:

source-build/repos/coreclr.common.props

Line 11 in 2146d2f

<BuildArguments Condition="'$(UseSystemLibunwind)' == 'true' AND '$(OS)' != 'Windows_NT'">$(BuildArguments) cmakeargs -DCLR_CMAKE_USE_SYSTEM_LIBUNWIND=TRUE</BuildArguments>

Actually, searching latest master, this is the only place UseSystemLibraries shows up. 😕 But I also don't see anywhere UseSystemLibunwind is being set to true.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like master is missing this from 3.1:

source-build/Directory.Build.props

Lines 25 to 26 in 32a3fcd

<UseSystemLibraries Condition="'$(UseSystemLibraries)' == '' AND '$(PortableBuild)' != 'true'">true</UseSystemLibraries>
<UseSystemLibunwind Condition="'$(UseSystemLibunwind)' == ''">$(UseSystemLibraries)</UseSystemLibunwind>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this!

Copy link
Member Author

Choose a reason for hiding this comment

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

After fixing this, it turns out that system-libunwind is still broken in dotnet/runtime. The singlefilehost doesn't link to the system-libunwind library, resulting in some symbols being undefined when doing the final linking. This will need a fix in dotnet/runtime too.

@omajid omajid force-pushed the re-enable-system-libunwind branch from 62a288c to 19011ba Compare September 24, 2020 20:19
@omajid omajid changed the title Re-enable system libunwind support WIP: Re-enable system libunwind support Sep 24, 2020
@omajid
Copy link
Member Author

omajid commented Sep 24, 2020 •
edited
Loading

This includes a backport of work-in-progress dotnet/runtime#42661 dotnet/runtime#42689

@omajid omajid force-pushed the re-enable-system-libunwind branch from 19011ba to 17dc345 Compare September 28, 2020 14:32
This was referenced Sep 28, 2020
Closed
Closed
@omajid omajid changed the title WIP: Re-enable system libunwind support Re-enable system libunwind support Sep 29, 2020
@omajid omajid closed this Sep 30, 2020
@omajid omajid reopened this Sep 30, 2020
@omajid omajid force-pushed the re-enable-system-libunwind branch from 17dc345 to b83635a Compare October 28, 2020 15:34
@omajid omajid closed this Nov 12, 2020
@omajid omajid reopened this Nov 12, 2020
@omajid omajid force-pushed the re-enable-system-libunwind branch from b83635a to b766e47 Compare November 16, 2020 20:02
@omajid
Copy link
Member Author

omajid commented Nov 16, 2020

dotnet/runtime#42853 was merged into 5.0, so all this PR needs to do now is set the cmake properties.

@omajid
Copy link
Member Author

omajid commented Nov 17, 2020

@crummel @dagood @dseefeld can someone please review/merge this? Thanks!

@dagood dagood merged commit 06f0c40 into dotnet:master Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dagood dagood dagood approved these changes

@dseefeld dseefeld Awaiting requested review from dseefeld

+1 more reviewer

@crummel crummel crummel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@omajid @crummel @dagood