Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Tue, 6 Feb 2024 08:05:14 GMT, Matthias Baesken wrote: >>> I hope finally the AIX part of this PR is done. >> >> Thanks for the AIX related effort ; I put it again into our internal >> build/test queue. > >> >> Thanks for the AIX related effort ; I put it again into our internal >> build/test queue. > > With the latest commit the build again fails on AIX with this error > > /jdk/src/java.base/unix/native/libnio/ch/UnixFileDispatcherImpl.c:381:27: > error: incompatible pointer types passing 'struct statvfs64 *' to parameter > of type 'struct statvfs *' [-Werror,-Wincompatible-pointer-types] > result = fstatvfs(fd, &file_stat); > ^~ > /usr/include/sys/statvfs.h:102:42: note: passing argument to parameter here > extern int fstatvfs(int, struct statvfs *); > Well, well... The code at least looks cleaner without these AIX defines, so I > really hope that this is the end of the AIX saga, at the `n+1`th time. > @MBaesken Can you rerun AIX testing with the latest commit? Latest commit looks still good on AIX. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1933652124
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Tue, 6 Feb 2024 08:05:14 GMT, Matthias Baesken wrote: >>> I hope finally the AIX part of this PR is done. >> >> Thanks for the AIX related effort ; I put it again into our internal >> build/test queue. > >> >> Thanks for the AIX related effort ; I put it again into our internal >> build/test queue. > > With the latest commit the build again fails on AIX with this error > > /jdk/src/java.base/unix/native/libnio/ch/UnixFileDispatcherImpl.c:381:27: > error: incompatible pointer types passing 'struct statvfs64 *' to parameter > of type 'struct statvfs *' [-Werror,-Wincompatible-pointer-types] > result = fstatvfs(fd, &file_stat); > ^~ > /usr/include/sys/statvfs.h:102:42: note: passing argument to parameter here > extern int fstatvfs(int, struct statvfs *); Well, well... The code at least looks cleaner without these AIX defines, so I really hope that this is the end of the AIX saga, at the `n+1`th time. @MBaesken Can you rerun AIX testing with the latest commit? - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1933513366
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 15:49:59 GMT, Magnus Ihse Bursie wrote: >> I wrote earlier: >> >>> There is one change that merit highlighting: In >>> src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c, I kept the >>> dlsym lookup for openat64, fstatat64 and fdopendir64, on non-BSD OSes (i.e. >>> Linux and AIX), and on AIX, respectively. This seems to me to be the safest >>> choice, as we do not need to know if a lookup of openat would yield a >>> 32-bit or a 64-bit version. (I frankly don't know, but I'm guessing it >>> would yield the 32-bit version.) > > Basically, my understanding is that a call to "openat" in the source file > will be converted into a call to openat64 on 32-bit platforms. When we look > up the function using dlsym, I assume we need to find the 64-bit version > directly. > > Even if this is incorrect, it seems at least *safe* to do it this way. The redirection is done via aliases in the headers, so you're right. Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1482403071
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Mon, 5 Feb 2024 14:15:44 GMT, Matthias Baesken wrote: > > Thanks for the AIX related effort ; I put it again into our internal > build/test queue. With the latest commit the build again fails on AIX with this error /jdk/src/java.base/unix/native/libnio/ch/UnixFileDispatcherImpl.c:381:27: error: incompatible pointer types passing 'struct statvfs64 *' to parameter of type 'struct statvfs *' [-Werror,-Wincompatible-pointer-types] result = fstatvfs(fd, &file_stat); ^~ /usr/include/sys/statvfs.h:102:42: note: passing argument to parameter here extern int fstatvfs(int, struct statvfs *); - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1928972961
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Mon, 5 Feb 2024 14:03:45 GMT, Magnus Ihse Bursie wrote: > I hope finally the AIX part of this PR is done. Thanks for the AIX related effort ; I put it again into our internal build/test queue. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1927105669
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX I hope finally the AIX part of this PR is done. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1927082091
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX RIght, my bad. I apologize, you are completely correct, I turned the defines around in my head. I will add a redefine for AIX that turns statvfs into statvfs64. Thanks for noticing! - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1927065583
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Mon, 5 Feb 2024 12:38:03 GMT, Magnus Ihse Bursie wrote: > It seems like the statvfs64 is a pre-existing bug in AIX in that case. I have > not removed any statvfs64 for AIX; all such changes are guarded by `#ifdef > _ALLBSD_SOURCE`, which I presume is not defined on AIX. > > I recommend that I push this PR as-is first, and then you can do a follow-up > fix to define statvfs to statvfs64 on AIX. Java_sun_nio_fs_UnixNativeDispatcher_statvfs0 is changed from statvfs64 to statvfs, did I miss something ? - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1926925394
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX It seems like the statvfs64 is a pre-existing bug in AIX in that case. I have not removed any statvfs64 for AIX; all such changes are guarded by `#ifdef _ALLBSD_SOURCE`, which I presume is not defined on AIX. I recommend that I push this PR as-is first, and then you can do a follow-up fix to define statvfs to statvfs64 on AIX. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1926903203
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Mon, 5 Feb 2024 12:17:33 GMT, Joachim Kern wrote: > 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! Thanks for confirming. I think we do not want to fallback on AIX, so the <*>64 variant needs to stay on AIX. Seems the other symbols are covered by _LARGE_FILES according to the table in the linked IBM AIX doc (table in https://www.ibm.com/docs/en/aix/7.1?topic=volumes-writing-programs-that-access-large-files ) , is that correct (Joachim?) or did I miss something ? - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1926874075
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: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX 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 . - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1926848063
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX zlib is 3rd party source, I did not touch those. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1926532998
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX I noticed that in the file src/java.base/share/native/libzip/zlib/zconf.h we seem to still use `off64_t` , is this okay (at most other locations it was replaced) https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libzip/zlib/zconf.h#L541 - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1926448109
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 07:01:33 GMT, Sam James wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add kludge to BufferedRenderPipe.c for AIX > > make/modules/jdk.hotspot.agent/Lib.gmk line 31: > >> 29: >> 30: ifeq ($(call isTargetOs, linux), true) >> 31: SA_CFLAGS := -D_FILE_OFFSET_BITS=64 > > We have two choices to feel a bit more comfortable: > 1) We unconditionally `static_assert` in a few places for large `off_t`, or > 2) We only do it for platforms we know definitely support F_O_B and aren't > AIX (which we've handled separately). > > Not sure that's strictly necessary though. Also, if something understands > LARGEFILE*_SOURCE, then it probably understood F_O_B, or at least has some > macro to control it. Just thinking aloud. I'm guessing your comment was really more general, and just happened to be placed here? The reason I am removing `-D_FILE_OFFSET_BITS=64` here is that it is always set for all JDK compilations. 1. I don't know why you would not trust that compiler flags in the build system would work, but if you really want to add a static_assert, let me know where you want it. 2. No, AIX is not handled separately. It is handled as part of this PR. You are probably thinking about JDK-8324834, but that was only about Hotspot. This PR is about all the other JDK native libraries. - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476236956
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 15:48:04 GMT, Magnus Ihse Bursie wrote: >> src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c line 365: >> >>> 363: #else >>> 364: // Make sure we link to the 64-bit version of the functions >>> 365: my_openat_func = (openat_func*) dlsym(RTLD_DEFAULT, "openat64"); >> >> Explain this part to me, if you wouldn't mind? (Why do we keep the `64` >> variants?) > > I wrote earlier: > >> There is one change that merit highlighting: In >> src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c, I kept the dlsym >> lookup for openat64, fstatat64 and fdopendir64, on non-BSD OSes (i.e. Linux >> and AIX), and on AIX, respectively. This seems to me to be the safest >> choice, as we do not need to know if a lookup of openat would yield a 32-bit >> or a 64-bit version. (I frankly don't know, but I'm guessing it would yield >> the 32-bit version.) Basically, my understanding is that a call to "openat" in the source file will be converted into a call to openat64 on 32-bit platforms. When we look up the function using dlsym, I assume we need to find the 64-bit version directly. Even if this is incorrect, it seems at least *safe* to do it this way. - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476231574
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 07:02:43 GMT, Sam James wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add kludge to BufferedRenderPipe.c for AIX > > src/java.base/linux/native/libnio/ch/FileDispatcherImpl.c line 94: > >> 92: return IOS_UNSUPPORTED_CASE; >> 93: >> 94: loff_t offset = (loff_t)position; > > Is this `loff_t` for the benefit of `copy_file_range`? That is how copy_file_range is defined. The cast to off64_t was technically incorrect (but they ended up being the same type). > src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c line 365: > >> 363: #else >> 364: // Make sure we link to the 64-bit version of the functions >> 365: my_openat_func = (openat_func*) dlsym(RTLD_DEFAULT, "openat64"); > > Explain this part to me, if you wouldn't mind? (Why do we keep the `64` > variants?) I wrote earlier: > There is one change that merit highlighting: In > src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c, I kept the dlsym > lookup for openat64, fstatat64 and fdopendir64, on non-BSD OSes (i.e. Linux > and AIX), and on AIX, respectively. This seems to me to be the safest choice, > as we do not need to know if a lookup of openat would yield a 32-bit or a > 64-bit version. (I frankly don't know, but I'm guessing it would yield the > 32-bit version.) - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476232283 PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476229335
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c line 365: > 363: #else > 364: // Make sure we link to the 64-bit version of the functions > 365: my_openat_func = (openat_func*) dlsym(RTLD_DEFAULT, "openat64"); Explain this part to me, if you wouldn't mind? (Why do we keep the `64` variants?) - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1475642841
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX make/modules/jdk.hotspot.agent/Lib.gmk line 31: > 29: > 30: ifeq ($(call isTargetOs, linux), true) > 31: SA_CFLAGS := -D_FILE_OFFSET_BITS=64 We have two choices to feel a bit more comfortable: 1) We unconditionally `static_assert` in a few places for large `off_t`, or 2) We only do it for platforms we know definitely support F_O_B and aren't AIX (which we've handled separately). Not sure that's strictly necessary though. Also, if something understands LARGEFILE*_SOURCE, then it probably understood F_O_B, or at least has some macro to control it. Just thinking aloud. src/java.base/linux/native/libnio/ch/FileDispatcherImpl.c line 94: > 92: return IOS_UNSUPPORTED_CASE; > 93: > 94: loff_t offset = (loff_t)position; Is this `loff_t` for the benefit of `copy_file_range`? - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1475635336 PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1475636686
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 06:55:19 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: > > Add kludge to BufferedRenderPipe.c for AIX First, huge thanks for doing this. I did have a very rough cut of this locally which I'd put to one side, and you and I have done essentially the same thing (but yours with more tact). That's a positive sign. > > Can you confirm that you've run tier1-4 at least? Some of the library code > > that is changed here is not tested in the lower tiers. > > I have run tier1-4 now, and it passes (bar the tests that are currently > failing in mainline). However, this only tests 64-bit builds, and these > changes do not affect 64-bit builds, only 32-bit linux. So the tier1-4 is > more of a sanity check that I did not inadvertenly broke any 64-bit code. > > To really test that this works properly, a 32-bit linux with an assortment of > operations on > 2GB files would be needed. To the best of my knowledge, we > have no such test environment available, and I could not even try to think of > how to create such a test setup that does anything useful. (That is, if I > even were to spend any time on creating new tests for 32-bit platforms...) Yeah, let's not, I think. The only way of doing this is with libc shims and a huge mess. As long as we have tests which handle > 2GB files in general, and then also we can get someone to run this on a 32-bit system and tell us if the test suite passes as-is, then we're fine. Really, even if it builds on a 32-bit system with strict `-Werror=x` for pointer conversions and such, then we're OK. I'll leave comments inline for the rest. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1923093781
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
> 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: Add kludge to BufferedRenderPipe.c for AIX - Changes: - all: https://git.openjdk.org/jdk/pull/17538/files - new: https://git.openjdk.org/jdk/pull/17538/files/eb92119e..3c404183 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17538&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17538&range=05-06 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17538.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17538/head:pull/17538 PR: https://git.openjdk.org/jdk/pull/17538
