Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v26]
On Mon, 26 Feb 2024 11:24:13 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
>Address review comments
src/hotspot/os/aix/os_aix.cpp line 1175:
> 1173: // Shared object in .so format dont have braces, hence they get
> removed for archives with members.
> 1174: if (result == nullptr && pointer_to_dot != nullptr &&
> strcmp(pointer_to_dot, old_extension) == 0) {
> 1175: if (strcmp(pointer_to_dot, old_extension) == 0) {
Can you please remove this redundancy?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1503728978
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v23]
On Mon, 19 Feb 2024 13:00:01 GMT, Suchismith Roy wrote: >> Suchi, errno is a global static variable. If some runtime API sets it, it >> will continue to have this value until the next runtime call updates it. If >> you call dll_load_library, there are many execution paths not passing >> dlopen(). So you receive an errno from some unknown runtime API called >> before. The correct errno handling is: >> >> errno = 0; >> runtime_API_which_might_set_errno_in_error_case(); >> error_code = errno; >> >> But what you really need is the result of the `search_file_in_LIBPATH(...)` >> call in `Aix_dlopen()`. If it is false, then the error_report starts with >> the string "Could not load module . ." >> This is called in any case. A `dlopen()` is not called in any case. > > Thanks for the detailed explanation @JoKern65 . Do then in this errno check > may not be necessary ? or can we still set errno and access it some way ? In this special case here I would not use errno, but the string returned in ebuf, in case the result is nullptr. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1494546476
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v23]
On Mon, 19 Feb 2024 11:11:23 GMT, Suchismith Roy wrote: >> src/hotspot/os/aix/os_aix.cpp line 1181: >> >>> 1179: // First try to load the existing file. >>> 1180: result = dll_load_library(filename, ebuf, ebuflen); >>> 1181: int error_code = errno; >> >> this might not necessarily be the `errno` of the underlying `dlopen()`, >> because there is to much code in-between and branches without a `dlopen()` >> call. > >> this might not necessarily be the `errno` of the underlying `dlopen()`, >> because there is to much code in-between and branches without a `dlopen()` >> call. > > As i see the code in Aix_dlopen , there is no additional functional call > after the dlopen which might change the errno . Could you tell me the how the > errno would get overriden ? Suchi, errno is a global static variable. If some runtime API sets it, it will continue to have this value until the next runtime call updates it. If you call dll_load_library, there are many execution paths not passing dlopen(). So you receive an errno from some unknown runtime API called before. The correct errno handling is: errno = 0; runtime_API_which_might_set_errno_in_error_case(); error_code = errno; But what you really need is the result of the `search_file_in_LIBPATH(...)` call in `Aix_dlopen()`. If it is false, then the error_report starts with the string "Could not load module . ." This is called in any case. A `dlopen()` is not called in any case. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1494459722
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v23]
On Mon, 19 Feb 2024 10:05:17 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments src/hotspot/os/aix/os_aix.cpp line 1181: > 1179: // First try to load the existing file. > 1180: result = dll_load_library(filename, ebuf, ebuflen); > 1181: int error_code = errno; this might not necessarily be the `errno` of the underlying `dlopen()`, because there is to much code in-between and branches without a `dlopen()` call. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1494354118
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v22]
On Mon, 12 Feb 2024 18:04:21 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Remove not matched trailing whitespaces Everything is OK for me now. - Marked as reviewed by jkern (Author). PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1877784574
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 14:47:26 GMT, Joachim Kern wrote: >> And also `#define statvfs statvfs64` is not necessary with the same >> explanation as for the `opendir` defines above -- sorry again. >> The very only difference between statvfs and statvfs64 is that the >> filesystem ID field in statvfs has a width of 8 Bytes, while in statvfs64 it >> has a width of 16 Bytes. > >> @JoKern65 So what about `#define fstatvfs fstatvfs64`? Is that still needed? >> If so, I could not be bothered to make another change. Otherwise, we can get >> rid of _all_ AIX 64-bit redefines, and then I'll (probably) do it. > > Same as `statvfs`. Only the file system ID field is smaller. > @JoKern65 @MBaesken I did not receive any reply about what to do with > `fstatvfs`, so I decided to keep the last verified change for AIX. If you > want to clean this up, then please do so, but at that time it will be an > AIX-only patch. @magicus I have to reach out to IBM asking if the different size of the 'filesystem ID' field in statvfs makes an important difference. If not, I will remove the defines in an AIX-only patch. Thanks a lot for your effort. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1938300228
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 09:03:10 GMT, Joachim Kern wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Once more, remove AIX dirent64 et al defines > > And also `#define statvfs statvfs64` is not necessary with the same > explanation as for the `opendir` defines above -- sorry again. > The very only difference between statvfs and statvfs64 is that the filesystem > ID field in statvfs has a width of 8 Bytes, while in statvfs64 it has a width > of 16 Bytes. > @JoKern65 So what about `#define fstatvfs fstatvfs64`? Is that still needed? > If so, I could not be bothered to make another change. Otherwise, we can get > rid of _all_ AIX 64-bit redefines, and then I'll (probably) do it. Same as `statvfs`. Only the file system ID field is smaller. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1934275624
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v15]
On Thu, 8 Feb 2024 11:01:25 GMT, Suchismith Roy wrote: > > > > > > May be this is academical: Your code works for plain libraries > > > > > > replacing the .so extension by .a. Suppose the case the goal is to > > > > > > load a member of an archive libname.a(member.o), but as in the > > > > > > plain case you get as input libname.so(member.o). In this case you > > > > > > will cut of the member producing the resulting string libname.a > > > > > > instead of libname.a(member.o). Should this situation also be > > > > > > handled or is this forbidden? > > > > > > > > > > > > > > > Hi Joachim I think the case for member archives exists in dl_open. It > > > > > checks for braces and sets the RTLD_MEMBER flag. (Lines 1132-1134) > > > > > > > > > > > > Hi Suchi, but **before** you call dll_load_library, you remove the > > > > member part in my mentioned case > > > > > > > > > Do we have a case where a .so files has braces mentioning the archive > > > members ? I think not. > > > > > > That's why I am asking. If not this is an academical concern. But it should > > be mentioned in a comment, that this is not expected to work. > > Yes, but i was also asking if at all there is a case ,which i am not aware > of. Sure i will mention it in comments. But there is one case that keep me > thinking, is that ..will a particular .so file have an .a file with same > name, but also referred to a member ? No, there is no case I'm aware of. Just theoretical thinking. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1934227854
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v15]
On Thu, 8 Feb 2024 10:38:37 GMT, Suchismith Roy wrote: > > > > May be this is academical: Your code works for plain libraries > > > > replacing the .so extension by .a. Suppose the case the goal is to load > > > > a member of an archive libname.a(member.o), but as in the plain case > > > > you get as input libname.so(member.o). In this case you will cut of the > > > > member producing the resulting string libname.a instead of > > > > libname.a(member.o). Should this situation also be handled or is this > > > > forbidden? > > > > > > > > > Hi Joachim I think the case for member archives exists in dl_open. It > > > checks for braces and sets the RTLD_MEMBER flag. (Lines 1132-1134) > > > > > > Hi Suchi, but **before** you call dll_load_library, you remove the member > > part in my mentioned case > > Do we have a case where a .so files has braces mentioning the archive members > ? I think not. That's why I am asking. If not this is an academical concern. But it should be mentioned in a comment, that this is not expected to work. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1933808741
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v15]
On Thu, 8 Feb 2024 10:22:53 GMT, Suchismith Roy wrote: > > May be this is academical: Your code works for plain libraries replacing > > the .so extension by .a. Suppose the case the goal is to load a member of > > an archive libname.a(member.o), but as in the plain case you get as input > > libname.so(member.o). In this case you will cut of the member producing the > > resulting string libname.a instead of libname.a(member.o). Should this > > situation also be handled or is this forbidden? > > Hi Joachim I think the case for member archives exists in dl_open. It checks > for braces and sets the RTLD_MEMBER flag. (Lines 1132-1134) Hi Suchi, but **before** you call dll_load_library, you remove the member part in my mentioned case - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1933782205
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v16]
On Thu, 8 Feb 2024 09:59:11 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Free the buffer
src/hotspot/os/aix/os_aix.cpp line 1114:
> 1112:
> 1113: log_info(os)("attempting shared library load of %s", filename);
> 1114: printf("Loading the filename %s\n",filename);
Is this just accidentally remaining debug code or do you want to protocol each
dlopen on stdout?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1482709076
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 07:44:18 GMT, Magnus Ihse Bursie wrote: >> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we >> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK >> native libraries. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Once more, remove AIX dirent64 et al defines And also `#define statvfs statvfs64` is not necessary with the same explanation as for the `opendir` defines above -- sorry again. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1933630674
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v9]
On Tue, 6 Feb 2024 08:18:14 GMT, Magnus Ihse Bursie wrote: >> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we >> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK >> native libraries. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also fix fstatvfs on AIX My apologies the additional defines `#define DIR DIR64` `#define dirent dirent64` `#define opendir opendir64` `#define readdir readdir64` `#define closedir closedir64` are not necessary. Indeed they do not react on _LARGE_FILES, but the DIR struct and the functions are automatically 64 when compiling in 64bit mode (-m64) as we do. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1932343048
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v15]
On Tue, 6 Feb 2024 08:45:12 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > change control flow May be this is academical: Your code works for plain libraries replacing the .so extension by .a. Suppose the case the goal is to load a member of an archive libname.a(member.o), but as in the plain case you get as input libname.so(member.o). In this case you will cut of the member producing the resulting string libname.a instead of libname.a(member.o). Should this situation also be handled or is this forbidden? - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1929210294
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Mon, 5 Feb 2024 12:07:45 GMT, Matthias Baesken wrote: > Current commit compiles nicely on AIX. One issue we might still have > statvfs/statvfs64 is not mentioned here in the table of functions/structs > redefined on AIX > https://www.ibm.com/docs/en/aix/7.1?topic=volumes-writing-programs-that-access-large-files > so would we fall back to statvfs from the *64 - variant ? The define > _LARGE_FILES might not help in this case on AIX . Yes, if statvfs64() is replaced by statvfs() in the code, we will fallback on AIX to 32-Bit. _LARGE_FILES really does not help in this case! - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1926865295
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v11]
On Wed, 31 Jan 2024 07:42:49 GMT, Suchismith Roy wrote: >> src/hotspot/os/aix/os_aix.cpp line 1166: >> >>> 1164: Search order: >>> 1165: libfilename-> load "libfilename.so" first,then load libfilename.a,on >>> failure. >>> 1166: In,OpenJ9,the libary with .so extension is loaded first and then .a >>> extension,on failure. >> >> Hi Suchi, I'm puzzled. Your comment implies for me, that load library gets a >> 'base' filename without 'lib' prefix and without extension (e.g. 'name'). >> Then the j9 code creates the filename 'libname.so' first and on failure >> 'libname.a' second. What about given libname.so explicitly (e.g. >> libname.so)? Does j9 really use 'libname.a' as a failure fallback in this >> case? > > The load library gets the entire library name, after construction from > dll_build_name. This is always a .so file name. When .so file name fails to > load, we fallback to .a filename. > Do i need to mention the filename as libfilename.so then ? Yes, I think this would make it clear. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1472683336
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v11]
On Sat, 27 Jan 2024 17:38:59 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > update comment src/hotspot/os/aix/os_aix.cpp line 1166: > 1164: Search order: > 1165: libfilename-> load "libfilename.so" first,then load libfilename.a,on > failure. > 1166: In,OpenJ9,the libary with .so extension is loaded first and then .a > extension,on failure. Hi Suchi, I'm puzzled. Your comment implies for me, that load library gets a 'base' filename without 'lib' prefix and without extension (e.g. 'name'). Then the j9 code creates the filename 'libname.so' first and on failure 'libname.a' second. What about given libname.so explicitly (e.g. libname.so)? Does j9 really use 'libname.a' as a failure fallback in this case? - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1469331769
Integrated: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case
On Thu, 11 Jan 2024 15:46:59 GMT, Joachim Kern wrote: > In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with > a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than 32, > because getaddrinfo seems to detect IPv4 family, if IPv6 address has set only > some of the last 32 Bits. So we take the wrong maxValue. This pull request has now been integrated. Changeset: 22642ff0 Author:Joachim Kern Committer: Matthias Baesken URL: https://git.openjdk.org/jdk/commit/22642ff0aac71eceb71f6a9eebb2988a9bd5f091 Stats: 37 lines in 1 file changed: 3 ins; 24 del; 10 mod 8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case Reviewed-by: mbaesken, amenkov - PR: https://git.openjdk.org/jdk/pull/17374
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v7]
On Tue, 16 Jan 2024 08:43:34 GMT, Suchismith Roy wrote: >> src/hotspot/os/aix/os_aix.cpp line 1168: >> >>> 1166: int extension_length = 3; >>> 1167: char* file_path = NEW_C_HEAP_ARRAY(char, buffer_length + >>> extension_length + 1, mtInternal); >>> 1168: strncpy(file_path,filename, buffer_length + 1); >> >> Why not using >> `char* file_path = os::strdup (filename);` >> which would replace lines 1167+1168 >> and use the corresponding >> `os::free (file_path);` >> at the end > > Ok. Any performance advantage to using that ? No, I do not believe that it has performance advantage, but I think it is simpler to understand. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1453249951
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v3]
On Fri, 12 Jan 2024 21:22:35 GMT, Alex Menkov wrote: >> Joachim Kern has updated the pull request incrementally with one additional >> commit since the last revision: >> >> following proposals of alexmenkov > > src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c line 400: > >> 398: /* >> 399: * Input is in_addr just because all clients have it. >> 400: */ > > The comment does not make sense anymore: in_addr represents IPv4 address, > in6_addr represents IPv6 address. > Could you remove it please. Done - PR Review Comment: https://git.openjdk.org/jdk/pull/17374#discussion_r1452098775
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v4]
> In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with > a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than 32, > because getaddrinfo seems to detect IPv4 family, if IPv6 address has set only > some of the last 32 Bits. So we take the wrong maxValue. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: cosmetic changes - Changes: - all: https://git.openjdk.org/jdk/pull/17374/files - new: https://git.openjdk.org/jdk/pull/17374/files/0266dc12..f31e1d98 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17374&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17374&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17374.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17374/head:pull/17374 PR: https://git.openjdk.org/jdk/pull/17374
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v2]
On Thu, 11 Jan 2024 16:14:39 GMT, Joachim Kern wrote: >> In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with >> a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than >> 32, because getaddrinfo seems to detect IPv4 family, if IPv6 address has set >> only some of the last 32 Bits. So we take the wrong maxValue. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > cosmetic changes I tried to implement all of Alex proposals. - PR Comment: https://git.openjdk.org/jdk/pull/17374#issuecomment-1888922000
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v3]
> In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with > a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than 32, > because getaddrinfo seems to detect IPv4 family, if IPv6 address has set only > some of the last 32 Bits. So we take the wrong maxValue. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: following proposals of alexmenkov - Changes: - all: https://git.openjdk.org/jdk/pull/17374/files - new: https://git.openjdk.org/jdk/pull/17374/files/a5bfdd1a..0266dc12 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17374&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17374&range=01-02 Stats: 11 lines in 1 file changed: 1 ins; 3 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/17374.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17374/head:pull/17374 PR: https://git.openjdk.org/jdk/pull/17374
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v2]
> In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with > a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than 32, > because getaddrinfo seems to detect IPv4 family, if IPv6 address has set only > some of the last 32 Bits. So we take the wrong maxValue. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: cosmetic changes - Changes: - all: https://git.openjdk.org/jdk/pull/17374/files - new: https://git.openjdk.org/jdk/pull/17374/files/cbc56dd4..a5bfdd1a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17374&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17374&range=00-01 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17374.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17374/head:pull/17374 PR: https://git.openjdk.org/jdk/pull/17374
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v2]
On Thu, 11 Jan 2024 16:00:45 GMT, Matthias Baesken wrote: >> Joachim Kern has updated the pull request incrementally with one additional >> commit since the last revision: >> >> cosmetic changes > > src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c line 428: > >> 426: convertIPv4ToIPv6(&sa, &addr6); >> 427: *isIPv4 = 1; >> 428: } else > > Better use braces here too in the `else` part. Done - PR Review Comment: https://git.openjdk.org/jdk/pull/17374#discussion_r1449087986
RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case
In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than 32, because getaddrinfo seems to detect IPv4 family, if IPv6 address has set only some of the last 32 Bits. So we take the wrong maxValue. - Commit messages: - JDK-8319382 Changes: https://git.openjdk.org/jdk/pull/17374/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17374&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8319382 Stats: 30 lines in 1 file changed: 2 ins; 19 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17374.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17374/head:pull/17374 PR: https://git.openjdk.org/jdk/pull/17374
Integrated: JDK-8320890: [AIX] Find a better way to mimic dl handle equality
On Fri, 1 Dec 2023 11:33: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. This pull request has now been integrated. Changeset: b8ae4a8c Author:Joachim Kern Committer: Martin Doerr URL: https://git.openjdk.org/jdk/commit/b8ae4a8c0985d1763ac48ba78943d8b992d7be77 Stats: 446 lines in 12 files changed: 332 ins; 108 del; 6 mod 8320890: [AIX] Find a better way to mimic dl handle equality Reviewed-by: stuefe, mdoerr - PR: https://git.openjdk.org/jdk/pull/16920
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v12]
> 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 two additional commits since the last revision: - cosmetic changes - cosmetic changes - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/acf306d4..d908a969 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=10-11 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v11]
> 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: No need for malloc - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/dc2ea51b..acf306d4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=09-10 Stats: 28 lines in 1 file changed: 3 ins; 14 del; 11 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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v10]
> 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: additional fix of sideeffect reported in JDK-8322691 - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/359080d3..dc2ea51b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=08-09 Stats: 44 lines in 1 file changed: 22 ins; 0 del; 22 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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Thu, 21 Dec 2023 10:17:18 GMT, Martin Doerr wrote: >> Let's keep it simple. A linear array of only a few items is easily scanned, >> probably faster than pointer hopping hash table entries. Not that it matters >> in any way for the few calls to dlopen. >> >> Also, avoiding hotspot structures preserves layer integrity (porting_aix >> does not pull anything from hotspot so far) and prevents initialisation time >> dependencies. Not sure whether ConcurrentHashTable works before VM init, but >> with Joachimes current solution, we can call dlopen at any time in VM life. > > I don't like introducing unnecessary limitations. Are we sure nobody will > ever need more than 1024 handles? > Can't we at least use a GrowableArray or something like that? In principle you are right, but in my opinion 1024 is an academical limit. I never saw processes with more than a few dozen loaded libraries. - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433942205
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v9]
> 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: cosmetic changes - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/7486ddb9..359080d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=07-08 Stats: 10 lines in 1 file changed: 2 ins; 0 del; 8 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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Wed, 20 Dec 2023 23:45:16 GMT, Martin Doerr wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> improve error handling
>
> 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.
There will be only few libraries in the list. With this assumption Thomas
suggested to use just a simple array.
> 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.
Done
> src/hotspot/os/aix/porting_aix.cpp line 1026:
>
>> 1024:
>> 1025: os::free (libpath);
>> 1026: os::free (path2);
>
> Same here.
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433813137
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433814446
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433814755
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Wed, 20 Dec 2023 23:10:29 GMT, Martin Doerr wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> improve error handling
>
> 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.
Originally my code worked for 32 & 64 Bit executables, but Thomas mentioned
that we have only 64 Bit executables. So I removed the 32 Bit implementation,
but this comment was an artefact. I removed the 32 Bit reference now.
> 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?
Done
> 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.
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433797348
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433801010
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433798833
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/f79c89da..7486ddb9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=06-07 Stats: 14 lines in 3 files changed: 1 ins; 6 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
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v7]
On Wed, 20 Dec 2023 11:16:03 GMT, Suchismith Roy wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Spaces fix
Only some minor suggestions.
src/hotspot/os/aix/os_aix.cpp line 1168:
> 1166: int extension_length = 3;
> 1167: char* file_path = NEW_C_HEAP_ARRAY(char, buffer_length +
> extension_length + 1, mtInternal);
> 1168: strncpy(file_path,filename, buffer_length + 1);
Why not using
`char* file_path = os::strdup (filename);`
which would replace lines 1167+1168
and use the corresponding
`os::free (file_path);`
at the end
src/hotspot/os/aix/os_aix.cpp line 1174:
> 1172: result = dll_load_library(file_path, ebuf, ebuflen);
> 1173: // If the load fails,we try to reload by changing the extension to .a
> for .so files only.
> 1174: if(result == nullptr) {
Space between if and (
also next line
-
Changes requested by jkern (Author).
PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1790895382
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1432716207
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1432738451
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 [v5]
On Mon, 18 Dec 2023 10:19:24 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - trailing whitespace
>> - Following most of Thomas proposals
>
> src/hotspot/os/aix/porting_aix.cpp line 1101:
>
>> 1099: for (i = 0; i < g_handletable_used; i++) {
>> 1100: if (g_handletable[i].handle == libhandle) {
>> 1101: // handle found, decrease refcount
>
> `assert(refcount > 0, "Sanity"))`
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429931831
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v5]
On Mon, 18 Dec 2023 10:25:50 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - trailing whitespace
>> - Following most of Thomas proposals
>
> src/hotspot/os/aix/os_aix.cpp line 30:
>
>> 28: #pragma alloca
>> 29:
>> 30:
>
> please remove whitespace change
Done
> src/hotspot/os/aix/os_aix.cpp line 193:
>
>> 191: // local variables
>> 192:
>> 193:
>
> please remove whitespace change
Done
> src/hotspot/os/aix/porting_aix.cpp line 1097:
>
>> 1095: }
>> 1096:
>> 1097: pthread_mutex_lock(&g_handletable_mutex);
>
> You can make your life a lot easier by defining an RAII object at the start
> of the file:
>
> struct TableLocker {
> TableLocker() { pthread_mutex_lock(&g_handletable_mutex); }
> ~TableLocker() { pthread_mutex_unlock(&g_handletable_mutex); }
> };
>
> and just place this at the beginning of your two functions
>
> TableLocker lock:
> ...
>
>
> no need to manually unlock then, with the danger of missing a return.
Great, thank you. This was one of the things I thought about, but was not sure,
because I did not fully understood the MutexLocker class and the difference
between Monitor and Mutex.
> src/hotspot/os/aix/porting_aix.cpp line 1143:
>
>> 1141: // entry of the array to the place of the entry we want to
>> remove and overwrite it
>> 1142: if (i < g_handletable_used) {
>> 1143: g_handletable[i] = g_handletable[g_handletable_used];
>
> To be super careful, I would zero out at least the handle of the moved item
> like this:
> `g_handletable[g_handletable_used].handle = nullptr`
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429950832
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429951237
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429946043
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429947950
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v5]
On Mon, 18 Dec 2023 10:16:07 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - trailing whitespace
>> - Following most of Thomas proposals
>
> src/hotspot/os/aix/porting_aix.cpp line 1005:
>
>> 1003: // LIBPATH or LD_LIBRARY_PATH and second with burned in libpath.
>> 1004: // No check against current working directory
>> 1005: Libpath.print("%s:%s", env, rtv_linkedin_libpath());
>
> Are you sure libpath env var has precedence over the baked-in libpath?
Yes, that was the outcome of my experiments, although the IBM docu says the
oposite:
_"Specifies that the library path used at process exec time should be prepended
to any library path specified in the load call (either as an argument or
environment variable). It is recommended that this flag be specified in all
calls to the load subroutine."_
My experiment showed: LIBPATH=libpath; baked-in-libpath=baked-in-libpath;
mylib.so is in both paths. After dlopen(mylib.so) a map call shows the library
was loaded from libpath.
Then I remove the LIBPATH envvar and repeat. Now after dlopen(mylib.so) a map
call shows the library was loaded from baked-in-libpath.
So the LIBPATH envvar has precedence over the baked-in-libpath.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429919510
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v5]
On Mon, 18 Dec 2023 10:54:31 GMT, Thomas Stuefe wrote: >> Yes it is, It's the fallback if LIBPATH is not defined > > In that case there may be errors in other places, since so far we assumed its > either one or the other, but not both. Example: > > https://github.com/openjdk/jdk/blob/a247d0c74bea50f11d24fb5f3576947c6901e567/src/java.base/unix/native/libjli/java_md.c#L43C1-L47 > > Maybe you need to take a look here, in case LD_LIBRARYPATH needs to be > handled in addition to LIBPATH? Yes, it's one or the other. If LIBPATH envvar exists (even empty string), LD_LIBRARY_PATH is ignored. So, no problems. - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1430106335
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v7]
> 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: cosmetic changes - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/978ed33c..f79c89da Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=05-06 Stats: 7 lines in 3 files changed: 1 ins; 4 del; 2 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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v6]
> 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 Thomas proposals - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/18d9d2b0..978ed33c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=04-05 Stats: 79 lines in 2 files changed: 19 ins; 21 del; 39 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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v5]
On Mon, 18 Dec 2023 10:14:42 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - trailing whitespace
>> - Following most of Thomas proposals
>
> src/hotspot/os/aix/porting_aix.cpp line 990:
>
>> 988: if (env == nullptr) {
>> 989: // no LIBPATH, try with LD_LIBRARY_PATH
>> 990: env = getenv("LD_LIBRARY_PATH");
>
> Is LD_LIBRARY_PATH a thing on AIX? I thought it is only used on non-AIX.
Yes it is, It's the fallback if LIBPATH is not defined
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429891049
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 [v5]
> 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 two additional commits since the last revision: - trailing whitespace - Following most of Thomas proposals - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/b7676822..18d9d2b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=03-04 Stats: 562 lines in 3 files changed: 272 ins; 290 del; 0 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
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*)&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.
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*)&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.. 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 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 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&pr=16920&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v3]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/0f6716db..2d32c43b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=01-02 Stats: 175 lines in 2 files changed: 90 ins; 82 del; 3 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
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v2]
> 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 handling of nonexisting files - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/e756f496..0f6716db Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=00-01 Stats: 16 lines in 1 file changed: 4 ins; 5 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
RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality
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. - Commit messages: - JDK-8320890 Changes: https://git.openjdk.org/jdk/pull/16920/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320890 Stats: 202 lines in 7 files changed: 122 ins; 70 del; 10 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
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v5]
On Fri, 15 Sep 2023 07:22:32 GMT, Joachim Kern wrote: >> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , >> the following test started to fail on AIX : >> com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; >> The problem was described in >> [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try >> of a fix. >> A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) >> was necessary. >> Both fixes just disable the specific subtest on AIX, without correction of >> the root cause. >> The root cause is, that dlopen() on AIX returns different handles every >> time, even if you load a library twice. There is no official AIX API >> available to get this information on a different way. >> My proposal is, to use the stat64x API with the fields st_device and >> st_inode. After a dlopen() the stat64x() API is called additionally to get >> this information which is then stored parallel to the library handle in the >> jvmtiAgent. For AIX we then can compare these values instead of the library >> handle and get the same functionality as on linux. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > adopt types If you have time, please call me for a short discussion. - PR Comment: https://git.openjdk.org/jdk/pull/15583#issuecomment-1828040131
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v5]
On Mon, 27 Nov 2023 13:23:42 GMT, Thomas Stuefe wrote: >> Joachim Kern has updated the pull request incrementally with one additional >> commit since the last revision: >> >> adopt types > > This now causes problems with > > https://github.com/openjdk/jdk/pull/16604#issuecomment-1827661214 > > since it removes the possibility of silently alternating the file path inside > os::dll_load, which would be the preferred way for AIX to handle *.a shared > objects. So this causes even more ifdef AIX to bloom up everywhere. > > A much better solution would have been to mimic stable-handle behavior inside > the AIX version of `os::dll_load`. > > Proposal for an alternate solution: Hold dlhandle-to-(inode, devid)tuple > mappings in a hash table. On dlopen, look up dl-handle by (inode, filename) > tupel. On dlclose, remove entry. Could have been done inside os_aix.cpp > without any changes to shared coding, and would have provided complete > coverage for all users of dll_load. @tstuefe: Hi Thomas, I'm not sure if I got it. We can make (inode, devid) to a hash, which replaces the dlhandle on return of os::dlload. This hash would of course be the same if the same library is loaded twice. But I do not know how to handle the two real dlhandles. - PR Comment: https://git.openjdk.org/jdk/pull/15583#issuecomment-1827954482
Integrated: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX
On Wed, 6 Sep 2023 08:18:45 GMT, Joachim Kern wrote: > After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , > the following test started to fail on AIX : > com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; > The problem was described in > [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try > of a fix. > A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) > was necessary. > Both fixes just disable the specific subtest on AIX, without correction of > the root cause. > The root cause is, that dlopen() on AIX returns different handles every time, > even if you load a library twice. There is no official AIX API available to > get this information on a different way. > My proposal is, to use the stat64x API with the fields st_device and > st_inode. After a dlopen() the stat64x() API is called additionally to get > this information which is then stored parallel to the library handle in the > jvmtiAgent. For AIX we then can compare these values instead of the library > handle and get the same functionality as on linux. This pull request has now been integrated. Changeset: 21c2dac1 Author:Joachim Kern Committer: Matthias Baesken URL: https://git.openjdk.org/jdk/commit/21c2dac15957e6d71e8f32a55f3825671da097a9 Stats: 114 lines in 7 files changed: 101 ins; 3 del; 10 mod 8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX Reviewed-by: dholmes, mbaesken - PR: https://git.openjdk.org/jdk/pull/15583
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v5]
> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , > the following test started to fail on AIX : > com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; > The problem was described in > [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try > of a fix. > A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) > was necessary. > Both fixes just disable the specific subtest on AIX, without correction of > the root cause. > The root cause is, that dlopen() on AIX returns different handles every time, > even if you load a library twice. There is no official AIX API available to > get this information on a different way. > My proposal is, to use the stat64x API with the fields st_device and > st_inode. After a dlopen() the stat64x() API is called additionally to get > this information which is then stored parallel to the library handle in the > jvmtiAgent. For AIX we then can compare these values instead of the library > handle and get the same functionality as on linux. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: adopt types - Changes: - all: https://git.openjdk.org/jdk/pull/15583/files - new: https://git.openjdk.org/jdk/pull/15583/files/a8c6e65b..c3852b38 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=03-04 Stats: 10 lines in 4 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/15583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15583/head:pull/15583 PR: https://git.openjdk.org/jdk/pull/15583
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v4]
On Fri, 15 Sep 2023 02:01:26 GMT, David Holmes wrote: >> Joachim Kern has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/JDK-8315706' into JDK-8315706 >> - Following the proposals > > src/hotspot/share/prims/jvmtiAgent.hpp line 48: > >> 46: #ifdef AIX >> 47: unsigned long _inode; >> 48: unsigned long _device; > > It is best, IMO, to use the actual types rather than something expected to be > "equivalent". OK, this would be ino64_t and dev64_t. I will do the change. - PR Review Comment: https://git.openjdk.org/jdk/pull/15583#discussion_r1326853499
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v4]
On Thu, 14 Sep 2023 12:32:18 GMT, Joachim Kern wrote: >> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , >> the following test started to fail on AIX : >> com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; >> The problem was described in >> [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try >> of a fix. >> A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) >> was necessary. >> Both fixes just disable the specific subtest on AIX, without correction of >> the root cause. >> The root cause is, that dlopen() on AIX returns different handles every >> time, even if you load a library twice. There is no official AIX API >> available to get this information on a different way. >> My proposal is, to use the stat64x API with the fields st_device and >> st_inode. After a dlopen() the stat64x() API is called additionally to get >> this information which is then stored parallel to the library handle in the >> jvmtiAgent. For AIX we then can compare these values instead of the library >> handle and get the same functionality as on linux. > > Joachim Kern has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/JDK-8315706' into JDK-8315706 > - Following the proposals Again, I followed the proposals. - PR Comment: https://git.openjdk.org/jdk/pull/15583#issuecomment-1719374035
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v4]
> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , > the following test started to fail on AIX : > com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; > The problem was described in > [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try > of a fix. > A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) > was necessary. > Both fixes just disable the specific subtest on AIX, without correction of > the root cause. > The root cause is, that dlopen() on AIX returns different handles every time, > even if you load a library twice. There is no official AIX API available to > get this information on a different way. > My proposal is, to use the stat64x API with the fields st_device and > st_inode. After a dlopen() the stat64x() API is called additionally to get > this information which is then stored parallel to the library handle in the > jvmtiAgent. For AIX we then can compare these values instead of the library > handle and get the same functionality as on linux. Joachim Kern has updated the pull request incrementally with two additional commits since the last revision: - Merge remote-tracking branch 'origin/JDK-8315706' into JDK-8315706 - Following the proposals - Changes: - all: https://git.openjdk.org/jdk/pull/15583/files - new: https://git.openjdk.org/jdk/pull/15583/files/f565f9a5..a8c6e65b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=02-03 Stats: 17 lines in 6 files changed: 0 ins; 1 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/15583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15583/head:pull/15583 PR: https://git.openjdk.org/jdk/pull/15583
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v3]
On Thu, 14 Sep 2023 09:40:54 GMT, Alan Bateman wrote: >> Joachim Kern has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into JDK-8315706 >> - try to improve code following Davids suggestions and do some cosmetic >> changes >> - JDK-8315706 > > src/hotspot/share/prims/jvmtiAgent.hpp line 48: > >> 46: #ifdef AIX >> 47: long _inode; >> 48: long _device; > > How are dev_t and ino_t defined on AIX, I'm wondering if long is okay here. They are defined as __ulong64_t which is unsigned long. So I can change it to unsigned long or even to dev_t and ino_t. - PR Review Comment: https://git.openjdk.org/jdk/pull/15583#discussion_r1325721375
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v3]
> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , > the following test started to fail on AIX : > com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; > The problem was described in > [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try > of a fix. > A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) > was necessary. > Both fixes just disable the specific subtest on AIX, without correction of > the root cause. > The root cause is, that dlopen() on AIX returns different handles every time, > even if you load a library twice. There is no official AIX API available to > get this information on a different way. > My proposal is, to use the stat64x API with the fields st_device and > st_inode. After a dlopen() the stat64x() API is called additionally to get > this information which is then stored parallel to the library handle in the > jvmtiAgent. For AIX we then can compare these values instead of the library > handle and get the same functionality as on linux. Joachim Kern has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8315706 - try to improve code following Davids suggestions and do some cosmetic changes - JDK-8315706 - Changes: - all: https://git.openjdk.org/jdk/pull/15583/files - new: https://git.openjdk.org/jdk/pull/15583/files/46a531b0..f565f9a5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=01-02 Stats: 25976 lines in 1433 files changed: 13398 ins; 8377 del; 4201 mod Patch: https://git.openjdk.org/jdk/pull/15583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15583/head:pull/15583 PR: https://git.openjdk.org/jdk/pull/15583
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v2]
On Wed, 13 Sep 2023 15:30:22 GMT, Joachim Kern wrote: >> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , >> the following test started to fail on AIX : >> com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; >> The problem was described in >> [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try >> of a fix. >> A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) >> was necessary. >> Both fixes just disable the specific subtest on AIX, without correction of >> the root cause. >> The root cause is, that dlopen() on AIX returns different handles every >> time, even if you load a library twice. There is no official AIX API >> available to get this information on a different way. >> My proposal is, to use the stat64x API with the fields st_device and >> st_inode. After a dlopen() the stat64x() API is called additionally to get >> this information which is then stored parallel to the library handle in the >> jvmtiAgent. For AIX we then can compare these values instead of the library >> handle and get the same functionality as on linux. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > try to improve code following Davids suggestions and do some cosmetic > changes I moved `stat64x_LIBPATH` to AIX code, and tried to use some `AIX_ONLY(...)` statements. I hope this is better. - PR Comment: https://git.openjdk.org/jdk/pull/15583#issuecomment-1717898238
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v2]
> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , > the following test started to fail on AIX : > com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; > The problem was described in > [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try > of a fix. > A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) > was necessary. > Both fixes just disable the specific subtest on AIX, without correction of > the root cause. > The root cause is, that dlopen() on AIX returns different handles every time, > even if you load a library twice. There is no official AIX API available to > get this information on a different way. > My proposal is, to use the stat64x API with the fields st_device and > st_inode. After a dlopen() the stat64x() API is called additionally to get > this information which is then stored parallel to the library handle in the > jvmtiAgent. For AIX we then can compare these values instead of the library > handle and get the same functionality as on linux. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: try to improve code following Davids suggestions and do some cosmetic changes - Changes: - all: https://git.openjdk.org/jdk/pull/15583/files - new: https://git.openjdk.org/jdk/pull/15583/files/e5b41fb0..46a531b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=00-01 Stats: 102 lines in 5 files changed: 32 ins; 48 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/15583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15583/head:pull/15583 PR: https://git.openjdk.org/jdk/pull/15583
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX [v2]
On Tue, 12 Sep 2023 04:59:13 GMT, David Holmes wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> try to improve code following Davids suggestions and do some cosmetic
>> changes
>
> src/hotspot/share/prims/jvmtiAgentList.cpp line 251:
>
>> 249: while (it.has_next()) {
>> 250: JvmtiAgent* const agent = it.next();
>> 251: if (!agent->is_static_lib() && device && inode &&
>
> Style nit: we don't use implicit booleans so check `device != 0` and `inode
> != 0` explicitly please.
I followed your suggestion
> test/jdk/com/sun/tools/attach/warnings/DynamicLoadWarningTest.java line 127:
>
>> 125:
>> 126: // test behavior on platforms that can detect if an agent
>> library was previously loaded
>> 127: if (!Platform.isAix()) {
>
> You need to fix the indentation of the old block.
I followed your suggestion here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15583#discussion_r1324696223
PR Review Comment: https://git.openjdk.org/jdk/pull/15583#discussion_r1324696651
