RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
Daniel D.Daugherty
dcubed at openjdk.java.net
Wed Nov 3 19:23:13 UTC 2021
On Wed, 3 Nov 2021 17:49:40 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
>> macOS12 has changed the dladdr() function to accept "-1" as a valid address and
>> we have functions that use dladdr() to convert DLL addresses into function or
>> library names. We also have a gtest that verifies that "-1" is not a valid value to use
>> as a symbol address.
>>
>> As you might imagine, existing code that uses `os::dll_address_to_function_name()`
>> or `os::dll_address_to_library_name()` can get quite confused (and sometimes crash)
>> if an `addr` parameter of `-1` was allowed to be used.
>>
>> I've also made two cleanup changes as part of this fix:
>>
>> 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that should
>> be properly `#ifdef`'ed. There is also some code that makes sense for ELF format
>> files, but not for Mach-O format files so that code needs to be excluded on macOS.
>>
>> 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment on an
>> `#endif` that I fixed. That typo does not appear anywhere else in the HotSpot code
>> base so I'd like to fix it with this bug ID since I'm in related areas.
>>
>> This fix has been tested with Mach5 Tier[1-6].
>
> src/hotspot/os/bsd/os_bsd.cpp line 902:
>
>> 900: return false;
>> 901: }
>> 902: #endif
>
> We use dladdr() in several places in this code. I wonder whether it would make sense to fix all of those with a wrapper instead:
>
> static int my_dladdr(const void* addr, Dl_info* info) {
> if (addr != (void*)-1) {
> return dladdr(addr, info);
> } else {
> // add comment here
> return 0;
> }
> }
> #define dladdr my_dladdr
I'll take a look at the other calls to dladdr(). I'm trying to limit what I change
here to things that actually failed in our test on macOS12 on X64 and aarch64.
> src/hotspot/os/bsd/os_bsd.cpp line 918:
>
>> 916: // ranges like ELF. That makes sense since Mach-O can contain binaries for
>> 917: // than one instruction set so there can be more than one address range for
>> 918: // each "file".
>
> 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.
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().
-------------
PR: https://git.openjdk.java.net/jdk/pull/6193
More information about the serviceability-dev
mailing list