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.

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

Remove usage of HP libunwind on Apple platforms #110023

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
+22 −80
Draft

Remove usage of HP libunwind on Apple platforms #110023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a3e1a72
WIP: Test sys/ucontext.h
filipnavara Nov 20, 2024
cc5afa3
Drop HP libunwind fallback on Apple platforms
filipnavara Nov 22, 2024
8d38616
Revert accidental changes
filipnavara Nov 22, 2024
9c3fb30
Revert one more unnecessary change
filipnavara Nov 22, 2024
9aa1e0a
:(
filipnavara Nov 22, 2024
67d8271
Fix osx-x64 build
filipnavara Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/coreclr/nativeaot/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ if(CLR_CMAKE_HOST_UNIX)
add_compile_options(-nostdlib)

if(CLR_CMAKE_TARGET_APPLE)
add_definitions(-D_XOPEN_SOURCE)
add_definitions(-DFEATURE_OBJCMARSHAL)
endif(CLR_CMAKE_TARGET_APPLE)

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/nativeaot/Runtime/unix/UnixContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#ifndef __UNIX_CONTEXT_H__
#define __UNIX_CONTEXT_H__

#include <ucontext.h>
#include <sys/ucontext.h>

// Convert Unix native context to PAL_LIMITED_CONTEXT
void NativeContextToPalContext(const void* context, PAL_LIMITED_CONTEXT* palContext);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/pal/src/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ endif()

check_c_source_compiles("
#include <libunwind.h>
#include <ucontext.h>
#include <sys/ucontext.h>
int main(int argc, char **argv)
{
unw_context_t libUnwindContext;
Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/pal/src/include/pal/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ extern "C"
/* A type to wrap the native context type, which is ucontext_t on some
* platforms and another type elsewhere. */
#if HAVE_UCONTEXT_T
#if HAVE_UCONTEXT_H
#include <ucontext.h>
#endif // HAVE_UCONTEXT_H
#include <sys/ucontext.h>
Copy link
Member

Choose a reason for hiding this comment

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

This is basically reverting 796cbee. ucontext.h is POSIX while sys/ucontext.h is platform detail. Please put it under TARGET_OSX if it is only needed on mac.

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 suppose you are technically correct. We actually have some unguarded usages of sys/ucontext.h and few ones that are guarded with HAVE_SYS_UCONTEXT_H. That convinced me that all supported platforms actually have sys/ucontext.h.

This originally started off as an attempt to avoid defining _XOPEN_SOURCE. Weirdly enough the commit you referenced should have made it defined everywhere, yet I had build failures related to it, and it's still redefined in NativeAOT and Mono CMakeFiles.

Context: #109928 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, the current version of the code explains the bit:

elseif(CLR_CMAKE_HOST_OSX AND NOT CLR_CMAKE_HOST_MACCATALYST AND NOT CLR_CMAKE_HOST_IOS AND NOT CLR_CMAKE_HOST_TVOS)
  add_definitions(-D_XOPEN_SOURCE)

I wonder why the mobile platforms were excluded.

Copy link
Member Author

Choose a reason for hiding this comment

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

#110092. Let's check that first. If it gets accepted then I can revert back to <ucontext.h>.


typedef ucontext_t native_context_t;
#else // HAVE_UCONTEXT_T
Expand Down
6 changes: 5 additions & 1 deletion src/native/external/libunwind/include/libunwind-aarch64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@ extern "C" {

#include <inttypes.h>
#include <stddef.h>
#include <ucontext.h>
#include <stdalign.h>
#include <stdint.h>
#ifdef __APPLE__
#include <sys/ucontext.h>
#else
#include <ucontext.h>
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Please upstream this patch (https://github.com/libunwind/libunwind) and add a line next to

runtime/src/native/external/libunwind-version.txt

Line 13 in a3e1a72

Apply https://github.com/libunwind/libunwind/pull/741

The next update (v1.9) will automatically pick it up from upstream, eliminating the need for manual verification on our end.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just CI test at this point. This should not even be used on Apple platforms but I think DAC references it for one reason or another. I'd rather resolve that and drop the change in libunwind.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, windows and macOS only use HP libunwind for remote unwinding feature. It's a bit unfortunately that llvm-libunwind doesn't provide that and HP libunwind doesn't fully support macOS. Converging to one lib was attempted a few times but there isn't a substantial progress on that front. :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the code and there's actually three unwinding libraries...

  • HP libunwind
  • LLVM libunwind
  • The copied LLVM libunwind logic in remote-uniwind.cpp under __APPLE__ guard

LLVM libunwind actually does provide remote unwinding but only in the C++ API. That's essentially the API used by UnwindHelpers.cpp in NativeAOT. There's a way to pass custom AddressSpace implementation and initial register values. However, using that would be substantial rewrite and I am not sure if we have test coverage for that.

It would probably be possible remove the use of HP libunwind on Apple platforms with relatively minor changes. It's only used for few structs that are not exposed externally and as a fallback that should never be hit (since both compact unwinding and DWARF unwinding is implemented in remote-unwind.cpp). Am I missing something here? Maybe some CrossDAC scenarios?

Copy link
Member

Choose a reason for hiding this comment

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

It would probably be possible to remove the use of HP libunwind on Apple platforms with relatively minor changes.

Sounds good. I think we should go ahead with the switch. Both mac and windows have minimal remote unwind helpers in HP-libunwind which were upstreamed from dotnet/runtime: https://github.com/libunwind/libunwind/tree/master/src/remote

We have the option to build coreclr with llvm-libunwind on linux as well, which has been supported since 2016: 5a2478f (not sure if it's been tested recently though). Also, unw_get_save_loc API (which was the main reason of using HP-libunwind #4193 (comment)) was implemented in our in-tree copy of llvm-libunwind since dotnet/corert@c6571c6, though we haven’t upstreamed those patches yet. There’s an open issue for this: #72655, which is a minor problem; it’s simply something we continue to carry forward during llvm-libunwind version bumps.

The majority of HP-libunwind usage in coreclr is due to HELPER_METHOD_FRAME (HMF) and FCThrow: dotnet/corert#3784. Work is actively ongoing to remove these, and folks have made good progress: #95695. We are down to ~80 instances of HMF from 400+, and FCThrow is down to ~75. Once the HMF removal is completed, we can revisit the remaining uses of HP-libunwind, particularly for remote unwinding.

CrossDAC

The result of:
git grep -E 'libunwind|unw_' -- 'src/coreclr' ':!src/coreclr/pal' ':!src/coreclr/nativeaot'
command suggests that we don’t have direct usages of libunwind outside of the CoreCLR-PAL (I maybe missing something), which means we might be able to switch everything over to llvm-libunwind if we switch PAL to llvm-libunwind. Once HMF is removed, PAL implementation can be reduced to bare minimum without having to work out two libunwind flavors.

AFAICT, the llvm-libunwind project started back in 2013 when Apple contributed their unwinder to LLVM. While the system libunwind on macOS may have diverged somewhat, their C API seems to be the same as llvm-libunwind (which itself uses consistent naming as the original HP-libunwind), with the exception of for three functions we care for: unw_get_save_loc (which our llvm-libunwind copy includes), unw_get_accessors (which we don’t directly use but cmake introspection is in pal/src/configure.cmake for some reason..), and unw_get_proc_info_in_range (for which we have a fallback implementation that matches the one originally ported by us to HP-libunwind upstream, and it works with llvm-libunwind as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

The diff for removing the HP libunwind fallback on Apple platforms seems to be quite small. Let's see if I broke anything on CI. Honestly, not very sure on how to test it. I am not aware of any createdump tests.


#ifndef UNW_EMPTY_STRUCT
# define UNW_EMPTY_STRUCT uint8_t unused;
Expand Down
4 changes: 4 additions & 0 deletions src/native/external/libunwind/include/libunwind-x86_64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ extern "C" {
#include <sys/types.h>
#include <inttypes.h>
#include <stdint.h>
#ifdef __APPLE__
#include <sys/ucontext.h>
#else
#include <ucontext.h>
#endif

#ifndef UNW_EMPTY_STRUCT
# define UNW_EMPTY_STRUCT uint8_t unused;
Expand Down
Loading