8318696: Do not use LFS64 symbols on Linux

[thesamesam]

The LFS64 symbols provided by glibc are not part of any standard and were gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in 1.2.5). This commit replaces the usage of LFS64 symbols with their regular counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that functions will always act as their -64 variants on glibc.

---

Progress

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

Issue

• JDK-8318696: Do not use LFS64 symbols on Linux (Bug - P4)

Reviewers

• Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to 4432b9d8
• David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 4df24d81
• Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 5a66616f
• Matthias Baesken (@MBaesken - Reviewer) ⚠️ Review applies to 5a66616f

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16329

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @thesamesam, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user thesamesam" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

@thesamesam The following labels will be automatically applied to this pull request:

• build
• hotspot-runtime

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

[thesamesam]

I've already got an OCA processed for MySQL in the past.

/signed

[thesamesam]

I can't see a signup button for the bug tracker so I can't file an issue to include as a link.

[shipilev]

Submitted the bug: https://bugs.openjdk.org/browse/JDK-8318696
Please rename the PR to: 8318696: Do not use LFS64 symbols on Linux to get it hooked.

Please also go to https://github.com/thesamesam/jdk/actions and enable testing. The testing would normally trigger by new commit, but you can trigger it manually; select the "OpenJDK GHA Sanity Checks", then do "Run workflow" on the right, selecting the branch that corresponds to this PR.

[thesamesam]

Thanks!

[thesamesam]

I need to pipe config.h through to handle the x86 failure. I'll do that either today or the day after tomorrow.

[bridgekeeper]

You are already a known contributor!

[thesamesam]

OK, this should be better now. I've checked with static asserts that the definition propagates and it also errors out without those anyway on incompat. ptr. types.

[openjdk]

@thesamesam Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[mlbridge]

Webrevs

• 04: Full - Incremental (29e5c55f)
• 03: Full - Incremental (5a66616f)
• 02: Full - Incremental (e02b0715)
• 01: Full - Incremental (4df24d81)
• 00: Full (4432b9d8)

[dholmes-ora]

From what version of glibc can we be sure that the non-64 version is exactly equivalent to the 64 version for all these functions?

There are also uses of some 64 functions in the JDK code as well.

[thesamesam]

From what version of glibc can we be sure that the non-64 version is exactly equivalent to the 64 version for all these functions?

Fortunately, support for _FILE_OFFSET_BITS=64 making off_t 64-bit has been supported for quite a long time in glibc (unlike 64-bit time_t on 32-bit arches which is way more recent, like the last 2 years...).

It dates back to 1997. The exception is for fts.h which got wired up in 2015 (8b7b7f75d91f7bac323dd6a370aeb3e9c5c4a7d5) (glibc-2.23). OpenJDK doesn't use fts, so we're fine. It errored out before then anyway, it didn't silently fallback to 32-bit.

There are also uses of some 64 functions in the JDK code as well.

This requires closer inspection to determine how broken they are outside of these changes.

For some of the cases in the JDK code, foo64() functions are only used in some cases on AIX, leaving foo() used on e.g. 32-bit Linux. Some of them might be semantically broken already, not just at build-time with newer musl where off_t is always 64-bit.

My preference for moving forward is:

1. I add some static_assert for off_t to the modified JVM bits to be safe (that's what I tested with);
2. We keep this PR for the build-only fixes which are semantically identical to the previous code - this PR currently "preserves" bitness, it doesn't fix anything other than avoiding use of foo64() where it's unnecessary by converting to foo, but the foo64() use was right - just a glibcism - until now. From what I can tell, the JVM code touched here was correct, just relying on glibcisms;
3. In another PR, we look at the general 32-bit LFS situation which may involve runtime fixes if - as it appears - off_t is actually 32-bit on x86 systems right now and non foo64() functions are being used. Then we either port more stuff to use foo64() (not ideal), or consistently use foo() in more places with FILE_OFFSET_BITS.

My hope is to separate these semantic changes from the equivalent-behaviour stuff both to ease review but also because there's a bunch of projects which need noop fixes like this still that I need to look at.

I can't promise to be able to do 3) immediately though. I can probably start it soon, I just can't promise to get the whole thing done quickly.

What do you think?

[dholmes-ora]

I did a bit of archaeology here and have linked a number of existing JBS issues to JDK-8318696.

I don't know the exact history as to why we chose to use LFS64 vs FOB64 (perhaps glibc version limitations?) but it is clear (e.g. from JDK-8062658) that there has been confusion about when to use the 64-bit variants in the JDK. For Hotspot I don't see any issue switching to FOB64 in place of LFS64. So the outlined plan seems good.

[magicus]

From a build perspective, this looks sane.

It is okay to start with making sure Hotspot is fully FOB64 before looking at the rest of the JDK libraries, but it would be good if those too eventually were addressed.

This has been a neglected issue for some time, so I'm happy to see that it gets some attention.

Note that the hotspot parts still require two Hotspot reviewers.

/reviewers 3

[openjdk]

@thesamesam 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:

8318696: Do not use LFS64 symbols on Linux

Reviewed-by: ihse, dholmes, kbarrett, mbaesken

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@magicus, @dholmes-ora, @kimbarrett, @MBaesken) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@thesamesam This pull request has not yet been marked as ready for integration.

[thesamesam]

/integrate

[openjdk]

@thesamesam
Your change (at version 29e5c55) is now ready to be sponsored by a Committer.

[AlanBateman]

I assume a separate issue will be needed for the JDK native libraries as there are quite a few LFS64 usages.

[thesamesam]

Yes, please. I don't think I'm able to file one myself.

[kimbarrett]

/sponsor

[openjdk]

Going to push as commit f4d08cc.

[openjdk]

@kimbarrett @thesamesam Pushed as commit f4d08cc.

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

[thesamesam]

We will need to backport this to at least OpenJDK 17 as well as OpenJDK 11, if possible. We had downstream reports of failure there in Gentoo. I haven't checked OpenJDK 8 although it's almost certainly needed there too.

/backport jdk17u-dev
/backport jdk11u-dev

[openjdk]

@thesamesam To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

[openjdk]

@thesamesam To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

[dholmes-ora]

Backport to 21u should also be done

[MBaesken]

/backport jdk22u

[openjdk]

@MBaesken the backport was successfully created on the branch backport-MBaesken-f4d08ccf in my personal fork of openjdk/jdk22u. To create a pull request with this backport targeting openjdk/jdk22u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit f4d08ccf from the openjdk/jdk repository.

The commit being backported was authored by Sam James on 20 Jan 2024 and was reviewed by Magnus Ihse Bursie, David Holmes, Kim Barrett and Matthias Baesken.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22u:

$ git fetch https://github.com/openjdk-bots/jdk22u.git backport-MBaesken-f4d08ccf:backport-MBaesken-f4d08ccf
$ git checkout backport-MBaesken-f4d08ccf
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22u.git backport-MBaesken-f4d08ccf

[thesamesam]

Could someone run the other commands again for older branches for me as well? I don't have access.

[MBaesken]

Could someone run the other commands again for older branches for me as well? I don't have access.

Will look into 17 as well. Might make sense to have some other minor backports to jdk17u-dev before, to make the backport easier (maybe [almost] clean) .

[magicus]

@AlanBateman @thesamesam I opened JDK-8324539 for the JDK libs. The implementation is trivial (#17538) but I am not sure how to understand the impact. My gut feeling is that if anything was wrong with this it would not even compile, but I need to understand this properly.

[AlanBateman]

@AlanBateman @thesamesam I opened JDK-8324539 for the JDK libs. The implementation is trivial (#17538) but I am not sure how to understand the impact. My gut feeling is that if anything was wrong with this it would not even compile, but I need to understand this properly.

Doesn't it mean going over the native code and replacing the LFS64 symbols with their regular counterparts?

[thesamesam]

I'll take a look, give my thoughts on the problem overall. Let's discuss it over on that side if that's OK.

[magicus]

Doesn't it mean going over the native code and replacing the LFS64 symbols with their regular counterparts?

That sounds reasonable. However, I expected functions like lseek64 to fail compilation if _LARGEFILE64_SOURCE was not defined, so it would be easy to spot those places.

To my surprise, this does not seem to be the case -- either we have no instances of these LFS64 symbols in JDK libs, or they are still defined even though I removed _LARGEFILE64_SOURCE. But let's continue that discussion in #17538 instead.