RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12

[Gerard Ziemski]

On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty <dcubed 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].

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