Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8257620: Do not use objc_msgSend_stret to get macOS version #1569

Closed

Conversation

@AntonKozlov
Copy link
Member

@AntonKozlov AntonKozlov commented Dec 2, 2020

Please review a small change that replaces use of objc_msgSend_stret in macOS platform code with pure ObjC code. It's also a prerequisite for macOS/AArch64 support, where objc_msgSend_stret is not available.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8257620: Do not use objc_msgSend_stret to get macOS version

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1569/head:pull/1569
$ git checkout pull/1569

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 2, 2020

👋 Welcome back akozlov! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

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

  • core-libs

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.

Loading

@openjdk openjdk bot added the core-libs label Dec 2, 2020
@AntonKozlov AntonKozlov force-pushed the 8257620-msgsend-stret branch from e549569 to 2918b25 Dec 2, 2020
@openjdk openjdk bot added the rfr label Dec 2, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 2, 2020

Webrevs

Loading

Copy link
Contributor

@prrace prrace left a comment

Surely these days you can just call [NSProcessInfo operatingSystemVersion] directly ?
If I read the doc below it is in the 10.10 SDK and later.
https://developer.apple.com/documentation/foundation/nsprocessinfo/1410906-operatingsystemversion?language=occ

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Dec 2, 2020

Unfortunately, no. AFAIK, the minimum target version is 10.9 https://github.com/openjdk/jdk/blob/master/make/autoconf/flags.m4#L133, so I had to keep indirection.

Loading

@prrace
Copy link
Contributor

@prrace prrace commented Dec 2, 2020

/label add build

Loading

@openjdk openjdk bot added the build label Dec 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@prrace
The build label was successfully added.

Loading

@prrace
Copy link
Contributor

@prrace prrace commented Dec 2, 2020

I wonder if we should be "upping" that to something later.
10.9 is over 7 years old and has been out of support for what - 4 years ?

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 2, 2020

Mailing list message from Alan Snyder on core-libs-dev:

I know people running 10.10 and I try to keep my Java code running on 10.10, so I would suggest that.

However, my experience is that JDK 14 and later refuse to run on 10.10.

The metadata is conflicting:

The Info.plist has JVMMinimumSystemVersion 10.6.0

libjli.dylib has LC_VERSION_MIN_MACOSX 10.9

However, libjvm has LC_VERSION_MIN_MACOSX 10.13, and that is enough to prevent it from running.

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Dec 2, 2020

Interesting, I still able to run the build after this change on macOS 10.9.5. I use jdk image and there is no LC_VERSION_MIN_MACOSX in libjvm. libjli, libjava have one, and it's 10.9

Loading

@erikj79
Copy link
Member

@erikj79 erikj79 commented Dec 2, 2020

The current intention is to be consistent with the min system version and it's currently set to 10.9. If libjvm.dylib gets a different value, then that would be a bug, but note that this could also vary depending on how the build is configured and the compiler version used.

So far, I have only bumped this version once, and that was because the toolchain required it when switching to Clang from GCC. Keeping it low is nice for backwards compatibility. That said, I don't see a problem with increasing this value to 10.10 if it's needed for something. Even 10.10 was EOL a long time ago now. The current value is set in make/autoconf/flags.m4.

The discrepancy in Info.plist was fixed in JDK-8252145.

Loading

@erikj79
Copy link
Member

@erikj79 erikj79 commented Dec 2, 2020

We are indeed missing the macos-version-min argument when linking libjvm.dylib. This is a bug.

Loading

@erikj79
Copy link
Member

@erikj79 erikj79 commented Dec 2, 2020

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Dec 3, 2020

Thanks for taking care of those issues. To be clear, there is no real need to bump the version for this PR, 10.9 is fine. This PR just proposes another way to implement the workaround for 10.9.

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Dec 8, 2020

Hi, could I get review of the patch?

Loading

Copy link
Member

@magicus magicus left a comment

From a build PoV this sounds like a reasonable fix, but you need a review from someone in core-libs as well.

Loading

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Looks ok to me.
But it would be good if @prrace can have a look.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

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

8257620: Do not use objc_msgSend_stret to get macOS version

Reviewed-by: ihse, rriggs

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

  • 17ace83: 8258074: Move some flags related to compiler to compiler_globals.hpp
  • 47ba652: 8258455: problem list tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java
  • 1e77896: 8236225: Remove expired flags in JDK 17
  • 4d6f318: 8257726: Make -XX:+StressLdcRewrite option a diagnostic option
  • 8bf46c7: 8258438: build error in test/hotspot/gtest/runtime/test_os.cpp
  • 7977e38: 8254024: Enhance native libs for AWT and Swing to work with GraalVM Native Image
  • c37eabe: 8252148: vmError::controlled_crash should be #ifdef ASSERT and move tests to gtest
  • 2273f95: 8234930: Use MAP_JIT when allocating pages for code cache on macOS
  • da2415f: 8257457: Update --release 16 symbol information for JDK 16 build 28
  • 36e2097: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region"
  • ... and 268 more: https://git.openjdk.java.net/jdk/compare/4db05e991bd8ac6959d02e7993ed8c0e1e479e4f...master

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.

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 (@prrace, @magicus, @RogerRiggs) 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).

Loading

@openjdk openjdk bot added the ready label Dec 9, 2020
@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Dec 10, 2020

@prrace could you look at this?

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Dec 16, 2020

/integrate

Loading

@openjdk openjdk bot added the sponsor label Dec 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 16, 2020

@AntonKozlov
Your change (at version 2918b25) is now ready to be sponsored by a Committer.

Loading

@openjdk openjdk bot closed this Dec 21, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 21, 2020

@VladimirKempik @AntonKozlov Since your change was applied there have been 351 commits pushed to the master branch:

  • 8e42efa: 8257513: C2: assert((constant_addr - _masm.code()->consts()->start()) == con.offset())
  • fa50877: 8257853: Remove dependencies on JNF's JNI utility functions in AWT and 2D code
  • 580af49: 8258006: Replaces while cycles with iterator with enhanced for in java.desktop
  • dc7f01f: 8257584: [macos] NullPointerException originating from LWCToolkit.java
  • c7c53d0: 8258554: javax/swing/JTable/4235420/bug4235420.java fails in GTK L&F
  • c50b464: 8258715: [JVMCI] separate JVMCI code install timers for CompileBroker and hosted compilations
  • 64644a1: 8253881: Hotspot/Serviceability Terminology Refresh
  • 6a78b2a: 8258645: Bring Jemmy 1.3.11 to JDK test base
  • 7f92d18: 8258553: Limit number of fields in instance to be considered for scalar replacement
  • adf0e23: 8257800: CompileCommand TypedMethodOptionMatcher::parse_method_pattern() may over consume
  • ... and 341 more: https://git.openjdk.java.net/jdk/compare/4db05e991bd8ac6959d02e7993ed8c0e1e479e4f...master

Your commit was automatically rebased without conflicts.

Pushed as commit d4c7db5.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants