On Tue, 5 Dec 2023 12:11:46 GMT, Joachim Kern 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:
>
> encapsulate everything in os::Aix::dlopen
Excellent, this is how I have pictured a good solution. Very nice.
A number of remarks, but nothing fundamental.
src/hotspot/os/aix/os_aix.cpp line 1137:
> 1135: if (ebuf != nullptr && ebuflen > 0) {
> 1136: ::strncpy(ebuf, "dll_load: empty filename specified", ebuflen -
> 1);
> 1137: }
Are there any cases where we don't hand in the error buffer? If so, I would
just assert ebuf and ebuflen. No need for this kind of flexibility.
src/hotspot/os/aix/os_aix.cpp line 3051:
> 3049:
> 3050: // Simulate the library search algorithm of dlopen() (in os::dll_load)
> 3051: int os::Aix::stat64x_via_LIBPATH(const char* path, struct stat64x*
> stat) {
- no need to export this, make it filescope static
- please return bool, with false = error
- please rename it to something like "search_file_in_LIBPATH"
src/hotspot/os/aix/os_aix.cpp line 3055:
> 3053: return -1;
> 3054:
> 3055: char *path2 = strdup (path);
Please use os::strdup and os::free. If you really intent to use the plain libc
versions, use `::strdup` and `::free` to make sure - and indicate to code
readers - you use the global libc variants.
src/hotspot/os/aix/os_aix.cpp line 3059:
> 3057: int idx = strlen(path2) - 1;
> 3058: if (path2[idx] == ')') {
> 3059: while (path2[idx] != '(' && idx > 0) idx--;
? Why not `strrchr()`?
src/hotspot/os/aix/os_aix.cpp line 3067:
> 3065: if (path2[0] == '/' ||
> 3066: (path2[0] == '.' && (path2[1] == '/' ||
> 3067: (path2[1] == '.' && path2[2] == '/' {
This complexity is not needed, nor is it sufficient, since it does not handle
relative paths ("mydirectory/hallo.so")
https://www.ibm.com/docs/en/aix/7.1?topic=d-dlopen-subroutine
"If FilePath contains a slash character, FilePath is used directly, and no
directories are searched. "
So, just scan for a '/' - if you find one, its a path to be opened directly:
const bool use_as_filepath = strchr(path2, '/');
src/hotspot/os/aix/os_aix.cpp line 3085:
> 3083: strcpy(libpath, env);
> 3084: for (token = strtok_r(libpath, ":", ); token != nullptr;
> token = strtok_r(nullptr, ":", )) {
> 3085: sprintf(combined, "%s/%s", token, path2);
You can save a lot of pain and manual labor by using `stringStream` here.
stringStream combined;
combined.print("%s/%s", token, path2);
const char* combined_path_string = combined.base();
no need for manual allocation and byte counting.
src/hotspot/os/aix/os_aix.cpp line 3099:
> 3097: // filled by os::dll_load(). This way we mimic dl handle equality for a
> library
> 3098: // opened a second time, as it is implemented on other platforms.
> 3099: void* os::Aix::dlopen(const char* filename, int Flags) {
Does not need to be exported, nor does os::AIX::dlclose. Make file scope
static. See my remarks in os_posix.cpp.
src/hotspot/os/aix/os_aix.cpp line 3103:
> 3101: struct stat64x libstat;
> 3102:
> 3103: if (os::Aix::stat64x_via_LIBPATH(filename, )) {
Please return bool, not unix int -1, this hurts my brain :-)
src/hotspot/os/aix/os_aix.cpp line 3108:
> 3106: if (result != nullptr) {
> 3107: assert(false, "dll_load: Could not stat() file %s, but dlopen()
> worked; Have to improve stat()", filename);
> 3108:
