RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]

[Martin Doerr]

On Wed, 20 Dec 2023 14:53:06 GMT, Joachim Kern <jkern at openjdk.org> wrote:

>> On AIX, repeated calls to dlopen referring to the same shared library may result in different, unique dl handles to be returned from libc. In that it differs from typical libc implementations that cache dl handles.
>> 
>> This causes problems in the JVM with code that assumes equality of handles. One such problem is in the JVMTI agent handler. That problem was fixed with a local fix to said handler ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this fix causes follow-up problems since it assumes that the file name passed to `os::dll_load()` is the file that has been opened. It prevents efficient, os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality. See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it is a hack that causes other, more uglier hacks to follow (see discussion of https://github.com/openjdk/jdk/pull/16604).
>> 
>> We propose a different, cleaner way of handling this:
>> 
>> - Handle this entirely inside the AIX versions of os::dll_load and os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>> 
>> This way we mimic dl handle equality as it is implemented on other platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional commit since the last revision:
> 
>   improve error handling

A pretty complex solution, but I couldn't spot any real bug. Please consider my suggestions.

src/hotspot/os/aix/porting_aix.cpp line 25:

> 23:  */
> 24: // needs to be defined first, so that the implicit loaded xcoff.h header defines
> 25: // the right structures to analyze the loader header of 32 and 64 Bit executable files

I don't think we support 32 bit executables.

src/hotspot/os/aix/porting_aix.cpp line 916:

> 914: constexpr int max_handletable = 1024;
> 915: static int g_handletable_used = 0;
> 916: static struct handletableentry g_handletable[max_handletable] = {{0, 0, 0, 0}};

Wouldn't `ConcurrentHashTable` be a better data structure? It is already used in hotspot, can grow dynamically and doesn't need linear search.

src/hotspot/os/aix/porting_aix.cpp line 921:

> 919: // If the libpath cannot be retrieved return an empty path
> 920: static const char* rtv_linkedin_libpath() {
> 921:   static char buffer[4096];

Maybe define a constant for the buffer size?

src/hotspot/os/aix/porting_aix.cpp line 927:

> 925:   // let libpath point to buffer, which then contains a valid libpath
> 926:   // or an empty string
> 927:   if (libpath) {

`!= nullptr` is common in hotspot.

src/hotspot/os/aix/porting_aix.cpp line 934:

> 932:   // to open it
> 933:   snprintf(buffer, 100, "/proc/%ld/object/a.out", (long)getpid());
> 934:   FILE* f = 0;

Should be nullptr.

src/hotspot/os/aix/porting_aix.cpp line 990:

> 988:     }
> 989:     ret = (0 == stat64x(combined.base(), stat));
> 990:     os::free (path2);

Please remove the extra whitespace.

src/hotspot/os/aix/porting_aix.cpp line 1026:

> 1024: 
> 1025:   os::free (libpath);
> 1026:   os::free (path2);

Same here.

-------------

PR Review: https://git.openjdk.org/jdk/pull/16920#pullrequestreview-1791807521
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433267331
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433283111
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433273616
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433270399
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433289382
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433290839
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433291127