Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Tue, 19 Dec 2023 12:52:23 GMT, Thomas Stuefe wrote: >> Hi @JoKern65 Is this good to integrate now ? > > @suchismith1993 > >> Once this code goes in I can push in my changes. We are targeting the fix >> for January. > > If you talk about https://github.com/openjdk/jdk/pull/16604, no, you cannot > push that even if Joachim finishes his work. > > Your patch has not even a single review, is quite controversial, and none of > the issues the reviewers have mentioned are addressed. This needs a lot more > discussion time. > > @tstuefe Sorry to tag you. Can you review the code. Once this code goes in > > I can push in my changes. > > We are targeting the fix for January. > > > Hi @JoKern65 Is this good to integrate now ? > > @suchismith1993 Please don't put pressure on patch authors and developers. > There is zero reason why this patch should be rushed. > > > Hi @suchismith1993, I'm waiting for a second review. Complex hotspot > > changes should be reviewed twice. > > Not only that, hotspot changes _need_ to be reviewed by at least two > reviewers. That is not optional. See OpenJDK bylaws. Sorry about that. The fix was critical for the adoptium builds and hence was looking to fix this soon. - PR Comment: https://git.openjdk.org/jdk/pull/16920#issuecomment-1862776678
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Tue, 19 Dec 2023 12:37:33 GMT, Suchismith Roy wrote: >> The libpath parsing code is from me, so no license problems. > > Hi @JoKern65 Is this good to integrate now ? > @suchismith1993 > > > Once this code goes in I can push in my changes. We are targeting the fix > > for January. > > If you talk about #16604, no, you cannot push that even if Joachim finishes > his work. > > Your patch has not even a single review, is quite controversial, and none of > the issues the reviewers have mentioned are addressed. This needs a lot more > discussion time. I have the patch ready based on the changes in this patch, as I take the diff and apply. But I cannot push since it will end up adding the entire file. - PR Comment: https://git.openjdk.org/jdk/pull/16920#issuecomment-1862768974
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Tue, 19 Dec 2023 12:37:33 GMT, Suchismith Roy wrote: >> The libpath parsing code is from me, so no license problems. > > Hi @JoKern65 Is this good to integrate now ? @suchismith1993 > Once this code goes in I can push in my changes. We are targeting the fix for > January. If you talk about https://github.com/openjdk/jdk/pull/16604, no, you cannot push that even if Joachim finishes his work. Your patch has not even a single review, is quite controversial, and none of the issues the reviewers have mentioned are addressed. This needs a lot more discussion time. - PR Comment: https://git.openjdk.org/jdk/pull/16920#issuecomment-1862704694
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Tue, 19 Dec 2023 12:37:33 GMT, Suchismith Roy wrote: >> The libpath parsing code is from me, so no license problems. > > Hi @JoKern65 Is this good to integrate now ? Hi @suchismith1993, I'm waiting for a second review. Complex hotspot changes should be reviewed twice. - PR Comment: https://git.openjdk.org/jdk/pull/16920#issuecomment-1862690708
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 11:51:43 GMT, Joachim Kern wrote: >> Joachim Kern has updated the pull request incrementally with one additional >> commit since the last revision: >> >> followed the proposals > > The libpath parsing code is from me, so no license problems. Hi @JoKern65 Is this good to integrate now ? - PR Comment: https://git.openjdk.org/jdk/pull/16920#issuecomment-1862684040
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Tue, 12 Dec 2023 14:05:48 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: > > followed the proposals The libpath parsing code is from me, so no license problems. - PR Comment: https://git.openjdk.org/jdk/pull/16920#issuecomment-1857762912
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 07:27:14 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> followed the proposals
>
> 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...
> }
> }
Sorry, I did not clearly understand how this should work. The mystery must be
in _... do the token thing ..._
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427877337
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 07:20:47 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> followed the proposals
>
> src/hotspot/os/aix/os_aix.cpp line 1187:
>
>> 1185: fread(buffer, 1, LDHDRSZ_64, f);
>> 1186: memcpy((char*), 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.
Do you mean the space `fseek (` ?
Done.
> src/hotspot/os/aix/os_aix.cpp line 1191:
>
>> 1189: }
>> 1190: else
>> 1191: buffer[0] = 0;
>
> {}
Done, due to complete rewriting. s.o.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427869786
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427870433
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 07:01:06 GMT, Thomas Stuefe wrote: >> Joachim Kern has updated the pull request incrementally with one additional >> commit since the last revision: >> >> followed the proposals > > src/hotspot/os/aix/os_aix.cpp line 1174: > >> 1172: struct _S_(ldhdr) ldr64; >> 1173: memcpy((char*), 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.. Yes, the section numbers are 1 based. e.g. Beginning of section 4 has an offset of 3 section sizes. - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427866203
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 06:15:56 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> followed the proposals
>
> src/hotspot/os/aix/os_aix.cpp line 1135:
>
>> 1133:
>> 1134: if (libpath)
>> 1135: return libpath;
>
> { }
done
> 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.
adopted. use buffer instead of pgmpath
> 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.
Completely rewritten; Only xcoff64 handled
> 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_`?
Not needed any more, because only xcoff64 handled
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427862523
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427862370
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427863562
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427864005
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 10:18:53 GMT, Joachim Kern wrote:
>> 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.
>
> I moved the static variable declarations and the functions `Aix_dlopen(),
> search_file_in_LIBPATH(), rtv_linkedin_libpath()` and `os::pd_dll_unload()`
> to porting_aix.cpp. This links, but in my opinion `os::pd_dll_unload()`
> should reside in os_aix.cpp, because it is member of the os class. But there
> it will not compile anymore if the static variables are moved away.
No, what I meant was to provide a "libc-like" equivalent for dlopen, similar to
what we do with dladdr (see
https://github.com/openjdk/jdk/blob/b7676822886eac21f61ff361a32928a966d8fe31/src/hotspot/os/aix/porting_aix.cpp#L306).
But never mind; I am also fine with moving os::pd_dlopen into a different cpp
file, e.g. "dlopen_aix.cpp". Just move it out of os_aix.cpp, since that is
already massive and you add >300 lines of more code and more dependencies.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427812795
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 09:57:19 GMT, Joachim Kern wrote: > If we omit the xcoff32 we have to ensure that no xcoff32 executable file > comes into play. xcoff32 is for 32-bit binaries. The AIX port only exists for 64-bit, and there will never be a 32-bit AIX port, so there is no reason for handling 32-bit xcoff headers. - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427803763
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 06:15:15 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> followed the proposals
>
> 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.
I moved the static variable declarations and the functions `Aix_dlopen(),
search_file_in_LIBPATH(), rtv_linkedin_libpath()` and `os::pd_dll_unload()` to
porting_aix.cpp. This links, but in my opinion `os::pd_dll_unload()` should
reside in os_aix.cpp, because it is member of the os class. But there it will
not compile anymore if the static variables are moved away.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427803856
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 06:44:03 GMT, Thomas Stuefe wrote: >> 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. > > Also, why are we parsing xcoff32 headers in there? AIX OpenJDK will always be > 64-bit. So, you can replace the whole xcoff32 section with assert( f_magic == > U802TOCMAGIC, ..). The function becomes a lot simpler then. I found a leak in my previous implementation. It is more or less academical, but this solution is the complete one. I would prefer this complete solution even it is complex, because if dlopen follows a slightly different algorithm in resolving the library we surely get into trouble. If we omit the xcoff32 we have to ensure that no xcoff32 executable file comes into play. - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427782107
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 06:22:39 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> followed the proposals
>
> 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.
Also, why are we parsing xcoff32 headers in there? AIX OpenJDK will always be
64-bit. So, you can replace the whole xcoff32 section with assert( f_magic ==
U802TOCMAGIC, ..). The function becomes a lot simpler then.
> 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.
But looking at the using code, returning NULL in case there is no contained
libpath would be actually easier, see below.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427609926
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427639138
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Tue, 12 Dec 2023 14:05:48 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:
>
> 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
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, _xcoff64)) {
assert?
}
...
if (!my_checked_fread(f, _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:
>
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/2d32c43b..b7676822 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16920=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16920=02-03 Stats: 485 lines in 6 files changed: 329 ins; 149 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16920.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16920/head:pull/16920 PR: https://git.openjdk.org/jdk/pull/16920
