8324539: Do not use LFS64 symbols in JDK libs

[magicus]

Similar to JDK-8318696, we should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK native libraries.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8324539: Do not use LFS64 symbols in JDK libs (Bug - P4)

Reviewers

• Julian Waters (@TheShermanTanker - Committer) ⚠️ Review applies to 134cf66f
• Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 134cf66f
• Matthias Baesken (@MBaesken - Reviewer) ⚠️ Review applies to 1fd34b10
• @thesamesam (no known openjdk.org user name / role) ⚠️ Review applies to 8a71e3b6
• Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 8a71e3b6

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17538/head:pull/17538
$ git checkout pull/17538

Update a local copy of the PR:
$ git checkout pull/17538
$ git pull https://git.openjdk.org/jdk.git pull/17538/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17538

View PR using the GUI difftool:
$ git pr show -t 17538

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17538.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[magicus]

I'm keeping this as draft until I've figured out how what tests are needed to confirm that this fix is unproblematic.

[openjdk]

@magicus The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[thesamesam]

(I'll try look at this during this week.)

[mlbridge]

Webrevs

• 11: Full - Incremental (7f673ef6)
• 10: Full (caccbf9b)
• 09: Full - Incremental (8a71e3b6)
• 08: Full - Incremental (1fd34b10)
• 07: Full - Incremental (2de377cd)
• 06: Full - Incremental (3c404183)
• 05: Full (eb92119e)
• 04: Full - Incremental (6e9ec631)
• 03: Full - Incremental (d6c64bc4)
• 02: Full - Incremental (25c691df)
• 01: Full - Incremental (0b26268e)
• 00: Full (134cf66f)

[magicus]

I have searched the code base to the extend of my ability, looking for all <foo>64 functions that are affected by _LARGEFILE64_SOURCE. (At one point, I also tried looking for [a-z]+64 or something like that, but there was a myriad of hits...)

Also note that this will only make a change on 32-bit platforms -- all 64-bit platforms already use the 64-bit version of the file functions/structs. This significantly lowers the risk of this patch, since 32-bit support is not as critical as 64-bit support.

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.)

[TheShermanTanker]

Not that it means much coming from me, but the build changes look ok

[erikj79]

Build changes look good.

[openjdk]

@magicus This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8324539: Do not use LFS64 symbols in JDK libs

Reviewed-by: jwaters, erikj, mbaesken, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[MBaesken]

I put it into our build/test patch list to see how it behaves on our environments (especially AIX).

[MBaesken]

AIX fastdebug build fails with the patch, build error is

/priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-dbg/jdk/src/java.desktop/share/native/libawt/java2d/pipe/BufferedRenderPipe.c:101:24: error: no member named 'open64' in 'SpanIteratorFuncs'; did you mean 'open'?
    srData = (*pFuncs->open)(env, si);
                       ^~~~
                       open
/usr/include/fcntl.h:115:14: note: expanded from macro 'open'
#define open    open64
                ^
/priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-dbg/jdk/src/java.desktop/share/native/libawt/java2d/pipe/SpanIterator.h:37:17: note: 'open' declared here
    void     *(*open)(JNIEnv *env, jobject iterator);

[magicus]

@MBaesken You gotta be kidding me... They just put in a #define open open64 in a convenient place? 😞

But why do only slowdebug fail? Weird.

[MBaesken]

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.

[MBaesken]

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 *);

[magicus]

Yeah, I missed fstatvfs. 😩

Now, then maybe nth time's a charm?

[MBaesken]

AIX build is fixed now after latest commit, thanks for handling the AIX special cases.

[JoKern65]

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.

[magicus]

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+1th time. @MBaesken Can you rerun AIX testing with the latest commit?

[JoKern65]

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.

[MBaesken]

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+1th time. @MBaesken Can you rerun AIX testing with the latest commit?

Latest commit looks still good on AIX.

[magicus]

@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.

[JoKern65]

@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.

[magicus]

And the smaller file system ID is not a problem, I guess?

Do you want me to remove the redefines?

[AlanBateman]

I can't comment on AIX but the changes look okay overall. I assume you'll bump the copyright header date on all the updated files before integrating.

[magicus]

/integrate

[openjdk]

Going to push as commit e5cb78c.

[openjdk]

@magicus Pushed as commit e5cb78c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[magicus]

@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.

[thesamesam]

Thank you again for this!

[JoKern65]

@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.