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 all commits
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
22 changes: 2 additions & 20 deletions src/coreclr/pal/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,29 +222,11 @@ add_library(coreclrpal
)

# Build separate pal library for DAC (addition to regular pal library)
if(CLR_CMAKE_TARGET_OSX)
set(LIBUNWIND_DAC_OBJECTS $<TARGET_OBJECTS:libunwind_dac>)

if(NOT FEATURE_CROSSBITNESS)
add_library(coreclrpal_dac STATIC
exception/remote-unwind.cpp
${LIBUNWIND_DAC_OBJECTS}
)

target_include_directories(coreclrpal_dac PUBLIC
${CLR_SRC_NATIVE_DIR}/external/libunwind/include
${CLR_SRC_NATIVE_DIR}/external/libunwind/include/tdep
${CLR_ARTIFACTS_OBJ_DIR}/external/libunwind/include
${CLR_ARTIFACTS_OBJ_DIR}/external/libunwind/include/tdep
)

target_compile_definitions(coreclrpal_dac PUBLIC -DUNW_REMOTE_ONLY)
else()
if(NOT FEATURE_CROSSBITNESS)
add_library(coreclrpal_dac STATIC
exception/remote-unwind.cpp
)
endif(NOT FEATURE_CROSSBITNESS)
endif(CLR_CMAKE_TARGET_OSX)
endif(NOT FEATURE_CROSSBITNESS)

# There is only one function exported in 'tracepointprovider.cpp' namely 'PAL_InitializeTracing',
# which is guarded with '#if defined(__linux__)'. On macOS, Xcode issues the following warning:
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
71 changes: 17 additions & 54 deletions src/coreclr/pal/src/exception/remote-unwind.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,8 +887,7 @@ static bool
ExtractProcInfoFromFde(
const libunwindInfo* info,
unw_word_t* addrp,
unw_proc_info_t *pip,
int need_unwind_info)
unw_proc_info_t *pip)
{
unw_word_t addr = *addrp;

Expand Down Expand Up @@ -917,35 +916,6 @@ ExtractProcInfoFromFde(
return false;
}

// Now fill out the proc info if requested
if (need_unwind_info)
{
if (dci.have_abi_marker)
{
if (!ReadValue16(info, &addr, &dci.abi)) {
return false;
}
if (!ReadValue16(info, &addr, &dci.tag)) {
return false;
}
}
if (dci.sized_augmentation) {
dci.fde_instr_start = augmentationEndAddr;
}
else {
dci.fde_instr_start = addr;
}
dci.fde_instr_end = fdeEndAddr;

pip->format = UNW_INFO_FORMAT_TABLE;
pip->unwind_info_size = sizeof(dci);
pip->unwind_info = malloc(sizeof(dci));
if (pip->unwind_info == nullptr) {
return -UNW_ENOMEM;
}
memcpy(pip->unwind_info, &dci, sizeof(dci));
}

return true;
}

Expand Down Expand Up @@ -1157,8 +1127,7 @@ SearchDwarfSection(
unw_word_t dwarfSectionAddr,
unw_word_t dwarfSectionSize,
uint32_t fdeSectionHint,
unw_proc_info_t *pip,
int need_unwind_info)
unw_proc_info_t *pip)
{
unw_word_t addr = dwarfSectionAddr + fdeSectionHint;
unw_word_t fdeAddr;
Expand All @@ -1175,7 +1144,7 @@ SearchDwarfSection(
}

if (ip >= ipStart && ip < ipEnd) {
if (!ExtractProcInfoFromFde(info, &fdeAddr, pip, need_unwind_info)) {
if (!ExtractProcInfoFromFde(info, &fdeAddr, pip)) {
ERROR("ExtractProcInfoFromFde FAILED for ip %p\n", (void*)ip);
break;
}
Expand All @@ -1188,7 +1157,7 @@ SearchDwarfSection(


static bool
GetProcInfo(unw_word_t ip, unw_proc_info_t *pip, libunwindInfo* info, bool* step, int need_unwind_info)
GetProcInfo(unw_word_t ip, unw_proc_info_t *pip, libunwindInfo* info, bool* step)
{
memset(pip, 0, sizeof(*pip));
*step = false;
Expand Down Expand Up @@ -1278,7 +1247,7 @@ GetProcInfo(unw_word_t ip, unw_proc_info_t *pip, libunwindInfo* info, bool* step
#else
#error unsupported architecture
#endif
if (SearchDwarfSection(info, ip, ehframeSectionAddr, ehframeSectionSize, dwarfOffsetHint, pip, need_unwind_info)) {
if (SearchDwarfSection(info, ip, ehframeSectionAddr, ehframeSectionSize, dwarfOffsetHint, pip)) {
TRACE("SUCCESS: found in eh frame from compact hint for %p\n", (void*)ip);
return true;
}
Expand All @@ -1294,7 +1263,7 @@ GetProcInfo(unw_word_t ip, unw_proc_info_t *pip, libunwindInfo* info, bool* step
// Look in dwarf unwind info next
if (ehframeSectionAddr != 0)
{
if (SearchDwarfSection(info, ip, ehframeSectionAddr, ehframeSectionSize, 0, pip, need_unwind_info)) {
if (SearchDwarfSection(info, ip, ehframeSectionAddr, ehframeSectionSize, 0, pip)) {
TRACE("SUCCESS: found in eh frame for %p\n", (void*)ip);
return true;
}
Expand Down Expand Up @@ -1495,7 +1464,7 @@ StepWithCompactEncodingFrameless(const libunwindInfo* info, compact_unwind_encod
}
}

uint64_t savedRegisters = context->Rsp + stack_size - 8 - (8 * register_count);
unw_word_t savedRegisters = context->Rsp + stack_size - 8 - (8 * register_count);
for (int i = 0; i < register_count; i++)
{
uint64_t reg;
Expand Down Expand Up @@ -2033,6 +2002,7 @@ static void UnwindContextToContext(unw_cursor_t *cursor, CONTEXT *winContext)
#endif
}

#ifndef __APPLE__
static int
get_dyn_info_list_addr(unw_addr_space_t as, unw_word_t *dilap, void *arg)
{
Expand Down Expand Up @@ -2217,14 +2187,6 @@ static int
find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pip, int need_unwind_info, void *arg)
{
auto *info = (libunwindInfo*)arg;
#ifdef __APPLE__
bool step;
if (!GetProcInfo(ip, pip, info, &step, need_unwind_info)) {
return -UNW_EINVAL;
}
_ASSERTE(!step);
return UNW_ESUCCESS;
#else
memset(pip, 0, sizeof(*pip));

Ehdr ehdr;
Expand Down Expand Up @@ -2395,7 +2357,7 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pip, int nee
}

// Now get the unwind info
if (!ExtractProcInfoFromFde(info, &fdeAddr, pip, need_unwind_info)) {
if (!ExtractProcInfoFromFde(info, &fdeAddr, pip)) {
ERROR("ExtractProcInfoFromFde\n");
return -UNW_EINVAL;
}
Expand All @@ -2407,8 +2369,6 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pip, int nee
info->FunctionStart = pip->start_ip;
return UNW_ESUCCESS;
#endif // HAVE_GET_PROC_INFO_IN_RANGE || !defined(HOST_UNIX)

#endif // __APPLE__
}

static void
Expand Down Expand Up @@ -2439,6 +2399,7 @@ static unw_accessors_t init_unwind_accessors()
};

static unw_accessors_t unwind_accessors = init_unwind_accessors();
#endif // __APPLE__

/*++
Function:
Expand Down Expand Up @@ -2476,10 +2437,10 @@ PAL_VirtualUnwindOutOfProc(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *cont
bool step;
#if defined(TARGET_AMD64)
TRACE("Unwind: rip %p rsp %p rbp %p\n", (void*)context->Rip, (void*)context->Rsp, (void*)context->Rbp);
result = GetProcInfo(context->Rip, &procInfo, &info, &step, false);
result = GetProcInfo(context->Rip, &procInfo, &info, &step);
#elif defined(TARGET_ARM64)
TRACE("Unwind: pc %p sp %p fp %p\n", (void*)context->Pc, (void*)context->Sp, (void*)context->Fp);
result = GetProcInfo(context->Pc, &procInfo, &info, &step, false);
result = GetProcInfo(context->Pc, &procInfo, &info, &step);
if (result && step)
{
// If the PC is at the start of the function, the previous instruction is BL and the unwind encoding is frameless
Expand All @@ -2498,7 +2459,7 @@ PAL_VirtualUnwindOutOfProc(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *cont
(opcode & ARM64_BLRA_OPCODE_MASK) == ARM64_BLRA_OPCODE)
{
TRACE("Unwind: getting unwind info for PC - 1 opcode %08x\n", opcode);
result = GetProcInfo(context->Pc - 1, &procInfo, &info, &step, false);
result = GetProcInfo(context->Pc - 1, &procInfo, &info, &step);
}
else
{
Expand All @@ -2519,8 +2480,7 @@ PAL_VirtualUnwindOutOfProc(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *cont
result = StepWithCompactEncoding(&info, procInfo.format, procInfo.start_ip);
goto exit;
}
#endif

#else
addrSpace = unw_create_addr_space(&unwind_accessors, 0);

st = unw_init_remote(&cursor, addrSpace, &info);
Expand All @@ -2544,16 +2504,19 @@ PAL_VirtualUnwindOutOfProc(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *cont
GetContextPointers(&cursor, NULL, contextPointers);
}
result = TRUE;
#endif // __APPLE__

exit:
if (functionStart)
{
*functionStart = info.FunctionStart;
}
#ifndef __APPLE__
if (addrSpace != 0)
{
unw_destroy_addr_space(addrSpace);
}
#endif // !__APPLE__
return result;
}

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