Skip to content

8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 #6193

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

Closed
wants to merge 2 commits into from
Closed

8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 #6193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f57d7f0
8273967.cr0.patch
dcubed-ojdk Oct 30, 2021
ecb230d
8273967.cr1.patch
dcubed-ojdk Nov 4, 2021
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
6 changes: 6 additions & 0 deletions src/hotspot/os/bsd/decoder_machO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ bool MachODecoder::demangle(const char* symbol, char *buf, int buflen) {

bool MachODecoder::decode(address addr, char *buf,
int buflen, int *offset, const void *mach_base) {
if (addr == (address)(intptr_t)-1) {
// dladdr() in macOS12/Monterey returns success for -1, but that addr value
// won't work in this function. Should have been handled by the caller.
ShouldNotReachHere();
return false;
}
struct symtab_command * symt = (struct symtab_command *)
mach_find_command((struct mach_header_64 *)mach_base, LC_SYMTAB);
if (symt == NULL) {
Expand Down
34 changes: 28 additions & 6 deletions src/hotspot/os/bsd/os_bsd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,17 @@ int os::current_process_id() {

const char* os::dll_file_extension() { return JNI_LIB_SUFFIX; }

static int local_dladdr(const void* addr, Dl_info* info) {
#ifdef __APPLE__
if (addr == (void*)-1) {
// dladdr() in macOS12/Monterey returns success for -1, but that addr
// value should not be allowed to work to avoid confusion.
return 0;
}
#endif
return dladdr(addr, info);
}

// This must be hard coded because it's the system's temporary
// directory not the java application's temp directory, ala java.io.tmpdir.
#ifdef __APPLE__
Expand Down Expand Up @@ -877,19 +888,15 @@ bool os::address_is_in_vm(address addr) {
return false;
}


#define MACH_MAXSYMLEN 256

bool os::dll_address_to_function_name(address addr, char *buf,
int buflen, int *offset,
bool demangle) {
// buf is not optional, but offset is optional
assert(buf != NULL, "sanity check");

Dl_info dlinfo;
char localbuf[MACH_MAXSYMLEN];

if (dladdr((void*)addr, &dlinfo) != 0) {
if (local_dladdr((void*)addr, &dlinfo) != 0) {
// see if we have a matching symbol
if (dlinfo.dli_saddr != NULL && dlinfo.dli_sname != NULL) {
if (!(demangle && Decoder::demangle(dlinfo.dli_sname, buf, buflen))) {
Expand All @@ -898,6 +905,14 @@ bool os::dll_address_to_function_name(address addr, char *buf,
if (offset != NULL) *offset = addr - (address)dlinfo.dli_saddr;
return true;
}

#ifndef __APPLE__
// The 6-parameter Decoder::decode() function is not implemented on macOS.
// The Mach-O binary format does not contain a "list of files" with address
// ranges like ELF. That makes sense since Mach-O can contain binaries for
// than one instruction set so there can be more than one address range for
// each "file".
Copy link
Member

Choose a reason for hiding this comment

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

Small nit, it seems confusing to have a Mac-specific comment in the BSD section.

Maybe this would be better in MachDecoder? E.g. implement the 6-arg version of decode() but stubbed out returning false, and with your comment there.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually fairly common to have Mac-specific stuff in the BSD files. The macOS
port was built on top of the BSD port and the BSD port was built by copying a LOT
of code from Linux into BSD specific files with modifications as needed.

If I pushed this change down into MachDecoder, then I would have to lose the
ShouldNotReachHere() call in order to not assert in non-release bits. I don't
think I want to do that since this may not be the only place that calls the
6-arg version of decode().

Copy link
Member

Choose a reason for hiding this comment

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

It's actually fairly common to have Mac-specific stuff in the BSD files. The macOS port was built on top of the BSD port and the BSD port was built by copying a LOT of code from Linux into BSD specific files with modifications as needed.

I always wondered whether anyone actually builds the BSDs in head. I assume Oracle does not, right? I know there are downstream porters somewhere but only for old releases, or?

If I pushed this change down into MachDecoder, then I would have to lose the ShouldNotReachHere() call in order to not assert in non-release bits. I don't think I want to do that since this may not be the only place that calls the 6-arg version of decode().

Fair enough, thanks for the clarification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oracle does not build BSD in head. At one point, Dmitry Samersoff used to build BSD
in his lab, but I don't know if he still does that.


// no matching symbol so try for just file info
if (dlinfo.dli_fname != NULL && dlinfo.dli_fbase != NULL) {
if (Decoder::decode((address)(addr - (address)dlinfo.dli_fbase),
Expand All @@ -906,6 +921,10 @@ bool os::dll_address_to_function_name(address addr, char *buf,
}
}

#else // __APPLE__
#define MACH_MAXSYMLEN 256

char localbuf[MACH_MAXSYMLEN];
Copy link

Choose a reason for hiding this comment

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

This __APPLE__ section is the only one, that I can see, using MACH_MAXSYMLEN, why not move:

#if defined(__APPLE__)
 #define MACH_MAXSYMLEN 256
 #endif

here (i.e. just the #define MACH_MAXSYMLEN 256 and minimize the need for __APPLE__ sections?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm.... I'll take a look at cleaning this up a bit.

// Handle non-dynamic manually:
if (dlinfo.dli_fbase != NULL &&
Decoder::decode(addr, localbuf, MACH_MAXSYMLEN, offset,
Expand All @@ -915,6 +934,9 @@ bool os::dll_address_to_function_name(address addr, char *buf,
}
return true;
}

#undef MACH_MAXSYMLEN
#endif // __APPLE__
}
buf[0] = '\0';
if (offset != NULL) *offset = -1;
Expand All @@ -929,7 +951,7 @@ bool os::dll_address_to_library_name(address addr, char* buf,

Dl_info dlinfo;

if (dladdr((void*)addr, &dlinfo) != 0) {
if (local_dladdr((void*)addr, &dlinfo) != 0) {
if (dlinfo.dli_fname != NULL) {
jio_snprintf(buf, buflen, "%s", dlinfo.dli_fname);
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/runtime/os.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,7 +907,7 @@ bool os::print_function_and_library_name(outputStream* st,
addr = addr2;
}
}
#endif // HANDLE_FUNCTION_DESCRIPTORS
#endif // HAVE_FUNCTION_DESCRIPTORS

if (have_function_name) {
// Print function name, optionally demangled
Expand Down
8 changes: 3 additions & 5 deletions test/hotspot/gtest/runtime/test_os.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,11 +709,7 @@ TEST_VM(os, pagesizes_test_print) {
ASSERT_EQ(strcmp(expected, buffer), 0);
}

#if defined(__APPLE__) // See JDK-8273967.
TEST_VM(os, DISABLED_dll_address_to_function_and_library_name) {
#else
TEST_VM(os, dll_address_to_function_and_library_name) {
#endif
TEST_VM(os, dll_address_to_function_and_library_name) {
char tmp[1024];
char output[1024];
stringStream st(output, sizeof(output));
Expand All @@ -726,8 +722,10 @@ TEST_VM(os, pagesizes_test_print) {
#define LOG(...)

// Invalid addresses
LOG("os::print_function_and_library_name(st, -1) expects FALSE.");
address addr = (address)(intptr_t)-1;
EXPECT_FALSE(os::print_function_and_library_name(&st, addr));
LOG("os::print_function_and_library_name(st, NULL) expects FALSE.");
addr = NULL;
EXPECT_FALSE(os::print_function_and_library_name(&st, addr));

Expand Down