On Wed, 20 Dec 2023 14:53:06 GMT, Joachim Kern <jk...@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
