On Wed, 3 Nov 2021 21:14:17 GMT, Gerard Ziemski <gziem...@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 929: > >> 927: #endif >> 928: >> 929: #if defined(__APPLE__) > > Why not just do: > > `#else` > > here instead and collapse these 3 lines into 1? Hmmm... I'll take a look at doing that. > src/hotspot/os/bsd/os_bsd.cpp line 930: > >> 928: >> 929: #if defined(__APPLE__) >> 930: char localbuf[MACH_MAXSYMLEN]; > > 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? Hmmm.... I'll take a look at cleaning this up a bit. > src/hotspot/os/bsd/os_bsd.cpp line 964: > >> 962: if (offset) *offset = -1; >> 963: return false; >> 964: } > > Do we need this here? Wouldn't the earlier call to `Decoder::decode(addr, > localbuf, MACH_MAXSYMLEN, offset, dlinfo.dli_fbase))` catch this with > `ShouldNotReachHere`? That's the 5-parameter version of decode() and it doesn't have `ShouldNotReachHere`. So if that code site is called and returns `false`, then we get into `dll_address_to_library_name()` and reach this `dladdr()` call which will accept the "-1"... ------------- PR: https://git.openjdk.java.net/jdk/pull/6193
