On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty <dcu...@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]. Looks good, just a few optional nitpicks that I personally would have done, if it were me doing the change. 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? 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? 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`? ------------- Marked as reviewed by gziemski (Committer). PR: https://git.openjdk.java.net/jdk/pull/6193
