Skip to content
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.

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

Remove usage of HP libunwind on Apple platforms #110023

Closed
wants to merge 8 commits into from

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Nov 20, 2024

Remove fallback to HP libunwind in remote-unwind.cpp. There's already an implementation copied from LLVM's libunwind that handles both compact unwinding and DWARF.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 20, 2024
@filipnavara filipnavara changed the title Use <sys/ucontext.h> instead of <ucontext.h> Remove usage of HP libunwind on Apple platforms Nov 22, 2024
@filipnavara
Copy link
Member Author

Keeping as draft until we come up with a test for this. /cc @dotnet/dotnet-diag

@filipnavara filipnavara marked this pull request as ready for review November 30, 2024 16:46
@jkotas jkotas requested a review from mikem8361 November 30, 2024 16:55
@@ -917,35 +916,6 @@ ExtractProcInfoFromFde(
return false;
}

// Now fill out the proc info if requested
Copy link
Member

Choose a reason for hiding this comment

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

Removing this code breaks a fallback path if HAVE_GET_PROC_INFO_IN_RANGE is not set and not HOST_UNIX. I'm not sure exactly what platforms still will use that path (see line 2398).

Copy link
Member

Choose a reason for hiding this comment

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

HAVE_GET_PROC_INFO_IN_RANGE is not defined on Unix when libunwind lacks support for unw_get_proc_info_in_range. This function is only available in HP libunwind version 1.6 and later. To ensure compatibility, we need to handle this for LLVM libunwind and HP libunwind versions earlier than 1.6.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fundamental misunderstanding of what this PR does (or I am missing something).

This code is Apple specific. It was called explicitly on Apple platforms already without any libunwind involvement (aside from operating on the same structures). I completely removed the code path that goes to (HP) libunwind on Apple platforms because there are only two unwinding encodings (compact unwinding and DWARF; with the DWARF always using compact unwinding as index because the linker enforces that) and both are already handled by the special Apple code path.

Copy link
Member

Choose a reason for hiding this comment

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

ExtractProcInfoFromFde is not Apple specific. Yes, most of the calls are in Apple specific except here:

if (!ExtractProcInfoFromFde(info, &fdeAddr, pip, need_unwind_info)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. I think that code path never takes place with the HP libunwind fork that we ship. It may be relevant for platforms that use system libunwind (FreeBSD or Haiku). I can add that code path back...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, add it back to make sure ever platform is covered.

@janvorli
Copy link
Member

janvorli commented Dec 2, 2024

There's already an implementation copied from LLVM's libunwind that handles both compact unwinding and DWARF.

Are you referring to the copy of the whole LLVM's libunwind in src/native/external/llvm-libunwind? If that's the case, that implementation is not used on CoreCLR, it is only for NativeAOT.

@filipnavara
Copy link
Member Author

filipnavara commented Dec 2, 2024

Are you referring to the copy of the whole LLVM's libunwind in src/native/external/llvm-libunwind?

No. I am referring specifically to the code in remote-unwind.cpp that's guarded by __APPLE__ definition:

//===-- CompactUnwindInfo.cpp ---------------------------------------------===//

HP libunwind doesn't know how to handle Apple compact unwinding. So the LLVM code was copied there. Whoever copied it also included the DWARF code paths too, which renders the HP libunwind fallback moot on Apple platforms since both cases are already covered by the Apple specific code.

@filipnavara
Copy link
Member Author

Closing for now. I realized this is flawed in concept for non-compact unwinding where HP libunwind was still used to interpret the DWARF opcodes.

I may do another take with trying to use llvm-libunwind C++ API (as used by NativeAOT).

@filipnavara filipnavara closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants