On Wed, 3 Nov 2021 17:49:40 GMT, Thomas Stuefe <stu...@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