Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v11]
On Mon, 12 Feb 2024 08:01:23 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 with a new target base due to > a merge or a rebase. The pull request now contains 15 commits: > > - Merge branch 'master' into jdk-FOB64 > - Fix indentation > - Once more, remove AIX dirent64 et al defines > - Also fix fstatvfs on AIX > - Redefine statvfs as statvfs64 on AIX > - Add kludge to BufferedRenderPipe.c for AIX > - Merge branch 'master' into jdk-FOB64 > - Remove all system includes from debug_util.h > - Merge branch 'master' into jdk-FOB64 > - Move #include out of debug_util.h > - ... and 5 more: https://git.openjdk.org/jdk/compare/efa071dd...caccbf9b Thank you again for this! - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1938202537
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Tue, 6 Feb 2024 16:10:38 GMT, Magnus Ihse Bursie wrote: >> 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. > > @thesamesam Do you want a `static_assert`? As I said, please let me know > where you want to put it. I don't think it provides much, but if it makes you > feel more comfortable, I'm okay with adding it. Sorry! I think I'm okay with it as-is. - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1482529665
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 07:41:02 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 Marked as reviewed by thesame...@github.com (no known OpenJDK username). - PR Review: https://git.openjdk.org/jdk/pull/17538#pullrequestreview-1869449960
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 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
