On Tue, 12 Dec 2023 14:05:48 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: > > followed the proposals Is this libpath parsing code copied from the R3 kernel? If yes, pls make sure there are no licensing issues. src/hotspot/os/aix/os_aix.cpp line 206: > 204: constexpr int max_handletable = 1024; > 205: static int g_handletable_used = 0; > 206: static struct handletableentry g_handletable[max_handletable] = {{0, 0, > 0, 0}}; I would move all that new and clearly delineated dlopen stuff into an own file, e.g. dlopen_aix.cpp or porting_aix.cpp (in porting_aix.cpp, we already have wrappers for other functions). os_aix.cpp is already massive. src/hotspot/os/aix/os_aix.cpp line 1129: > 1127: > 1128: // get the library search path burned in to the executable file during > linking > 1129: // If the libpath cannot be retrieved return an empty path This is new. Is this complexity needed, if yes, why? Don't see a comment, may have missed it. src/hotspot/os/aix/os_aix.cpp line 1131: > 1129: // If the libpath cannot be retrieved return an empty path > 1130: static const char* rtv_linkedin_libpath() { > 1131: static char buffer[4096]; This coding has some issues: - a generic char buffer is not a good idea. Forces you to do casts all over the place, and introduces alignment issues with unaligned char buffer. Which I assume is the reason for all the separate memcpy-into-structures below. I would just read into the structures directly. - you need to check the return codes for fread to make sure you read the number of bytes expected, lest you work with uninitialized memory and maybe to handle sporadic EINTR. - I don't get all the separate "SZ" macros. They must be equal to sizeof(structure), right, otherwise you get buffer overruns or work with uninitialized memory? Proposal: add a local wrapper function like this: template <class T> static bool my_checked_fread(FILE* f, T* out) { // read sizeof(T) from f. // Check return code. // Return bool if sizeof(T) bytes were read. e.g. in a very trivial form: int bytesread = fread(out, sizeof(T), 1, f); return bytesread == sizeof(T); } and use it in your code like this: struct xcoff64 the_xcoff64; struct scn64 the_scn64; struct ldr64 the_ldr64; if (!my_checked_fread(f, &the_xcoff64)) { assert? } ... if (!my_checked_fread(f, &the_ldr64) { .. handle error } src/hotspot/os/aix/os_aix.cpp line 1132: > 1130: static const char* rtv_linkedin_libpath() { > 1131: static char buffer[4096]; > 1132: static const char* libpath = 0; If your intent is to return an empty buffer if there is no contained libpath, I would just: static const char* libpath = ""; then you can always just return libpath. src/hotspot/os/aix/os_aix.cpp line 1135: > 1133: > 1134: if (libpath) > 1135: return libpath; { } src/hotspot/os/aix/os_aix.cpp line 1137: > 1135: return libpath; > 1136: > 1137: char pgmpath[32+1]; Will overflow if pid_t is 64bit. Give it a larger size; after all, you are giving buffer 4K above, so you are not overly concerned with saving stack space. src/hotspot/os/aix/os_aix.cpp line 1146: > 1144: fread(buffer, 1, FILHSZ_64 + _AOUTHSZ_EXEC_64, f); > 1145: > 1146: if (((struct filehdr*)buffer)->f_magic == U802TOCMAGIC ) { as stated above, I don't think this section is needed. src/hotspot/os/aix/os_aix.cpp line 1170: > 1168: else if (((struct filehdr*)buffer)->f_magic == U64_TOCMAGIC ) { > 1169: // __XCOFF64__ > 1170: struct _S_(xcoffhdr) xcoff64; whats with the `_S_`? src/hotspot/os/aix/os_aix.cpp line 1174: > 1172: struct _S_(ldhdr) ldr64; > 1173: memcpy((char*)&xcoff64, buffer, FILHSZ_64 + _AOUTHSZ_EXEC_64); > 1174: int ldroffset = FILHSZ_64 + xcoff64.filehdr.f_opthdr + > (xcoff64.aouthdr.o_snloader -1)*SCNHSZ_64; why the -1? I assume thats the section number? is it 1 based? how odd.. src/hotspot/os/aix/os_aix.cpp line 1187: > 1185: fread(buffer, 1, LDHDRSZ_64, f); > 1186: memcpy((char*)&ldr64, buffer, LDHDRSZ_64); > 1187: fseek (f, scn64.s_scnptr + ldr64.l_impoff, SEEK_SET); nit: please use consistent spacing according to hotspot rules. here, remove space. src/hotspot/os/aix/os_aix.cpp line 1191: > 1189: } > 1190: else > 1191: buffer[0] = 0; {} src/hotspot/os/aix/os_aix.cpp line 1234: > 1232: > 1233: stringStream Libpath; > 1234: if (env == nullptr) { Proposal for shorter version not needing string assembly: const char* paths [2] = { env, rtv_linkedin_libpath() }: for (int i = 0; i < 2; i ++) { const char* this_libpath = paths[i]; if (this_libpath == nullptr) { continue; } ... do the token thing... } } ------------- Changes requested by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16920#pullrequestreview-1783187856 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427593949 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427597791 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427606275 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427632243 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427594255 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427604761 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427610156 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427610650 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427622550 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427635888 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427633296 PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427640624
