8291067: macOS should use O_CLOEXEC instead of FD_CLOEXEC

[gerard-ziemski]

O_CLOEXEC is also available on macOS (__DARWIN_C_LEVEL >= 200809L), so use it same as on linux.

---

Progress

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

Issue

• JDK-8291067: macOS should use O_CLOEXEC instead of FD_CLOEXEC

Reviewers

• Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to 7628e09a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9663

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

Using diff file

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

[bridgekeeper[bot]]

:wave: Welcome back gziemski! 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.

[openjdk[bot]]

@gerard-ziemski The following label will be automatically applied to this pull request:

• hotspot-runtime

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.

[mlbridge[bot]]

Webrevs

• 06: Full (db9d11a3)
• 05: Full - Incremental (dd661815)
• 04: Full - Incremental (fe2c00f8)
• 03: Full - Incremental (42532c2c)
• 02: Full - Incremental (b3cb4f29)
• 01: Full - Incremental (82d1bbad)
• 00: Full (7628e09a)

[openjdk[bot]]

@gerard-ziemski 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:

8291067: macOS should use O_CLOEXEC instead of FD_CLOEXEC

Reviewed-by: dcubed, dholmes, stuefe

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 3 new commits pushed to the master branch:

• 392ac7055d4697c56fa85ac5572f5bc4dc431f1d: 8297211: Expensive fillInStackTrace operation in HttpURLConnection.getOutputStream0 when no content-length in response
• 5a45c25151b1da8e329ea2be21a0e4d2652f8b4a: 8297164: Update troff man pages and CheckManPageOptions.java
• f12710e938b36594623e9c82961d8aa0c0ef29c2: 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

[tstuefe]

What you could do and what would be helpful is a very primitive gtest that tests that files opened with os::open are correctly tagged with FD_CLOEXEC.

[bridgekeeper[bot]]

@gerard-ziemski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[gerard-ziemski]

What you could do and what would be helpful is a very primitive gtest that tests that files opened with os::open are correctly tagged with FD_CLOEXEC.

I like that.

[gerard-ziemski]

Followed feedback from David and Thomas.

We now assume that O_CLOEXEC is available on macOS, but we added __APPLE__ gtest that verifies that assumption via:

make run-test TEST=gtest:os.open_O_CLOEXEC

[bridgekeeper[bot]]

@gerard-ziemski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[gerard-ziemski]

Thank you Thomas for more feedback. Hopefully I have resolved all your comments.

[gerard-ziemski]

Thank you Dan, David and Thomas for your feedback!

[gerard-ziemski]

@dcubed-ojdk @dholmes-ora @tstuefe

Sorry guys, I need you to take a look at this one last time - turned out that the "gtest.cmdline" file does not exist in current working directory when ran in Mach5, so we now create our own test file.

[gerard-ziemski]

Hopefully this is it. Please take a look, thank you!

[tstuefe]

Looks good. Ship it!

[gerard-ziemski]

Having the open() call do the create is simpler. I don't know about using "test" for the file name.

Would you like me to change it? Would test_file.txt work or did you have something else in mind?

[dcubed-ojdk]

test_file.txt would be good (if you don't mind).

[gerard-ziemski]

Would love to ship this one.

[tstuefe]

Still good.

[openjdk[bot]]

@gerard-ziemski this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8291067
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[gerard-ziemski]

The fix had merge conflict, which I had to cleanup and now everyone who reviewed it is shown as "reviewer of previous version". Am I fine to go ahead with the integration, or do I need your final OK?

[tstuefe]

+1

[gerard-ziemski]

Thank you all very much for your patience on this one! I really do appreciate it.

[gerard-ziemski]

/integrate

[openjdk[bot]]

Going to push as commit ccc6e169188019dab008df5ffb11b4279e14f8fc. Since your change was applied there have been 10 commits pushed to the master branch:

• 0ac5b55311222d0531c9cfcec8c74932661b1750: 8297349: Parallel: Use correct claim value for CLD oop iteration in PSScavengeCLDClosure
• 932bf3539c21b1b53a2ff3a0b4af1997d5191fd3: 8297333: Parallel: Remove unused methods in PCIterateMarkAndPushClosure
• 42c2037429a8ee6f683bbbc99fb48c540519524c: 8297382: Test fails to compile after JDK-8288047
• 6d6046b3799217c281d077f12bce1ec590149849: 8252713: jtreg time out of CtrlASCII.java seems to hang the Xserver.
• 88957a7ce8932b95e3a18e6a7d1ceb3b7f60c781: 8297147: UnexpectedSourceImageSize test times out on slow machines when fastdebug is used
• 069685489afcea9b31491f0d9fec8cc52e210e99: 8297299: SequenceInputStream should not use Vector
• f0e99c634693fafc0c5d1103184e73c6669629db: 8297301: Cleanup unused methods in JavaUtilJarAccess
• 392ac7055d4697c56fa85ac5572f5bc4dc431f1d: 8297211: Expensive fillInStackTrace operation in HttpURLConnection.getOutputStream0 when no content-length in response
• 5a45c25151b1da8e329ea2be21a0e4d2652f8b4a: 8297164: Update troff man pages and CheckManPageOptions.java
• f12710e938b36594623e9c82961d8aa0c0ef29c2: 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@gerard-ziemski Pushed as commit ccc6e169188019dab008df5ffb11b4279e14f8fc.

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