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

8253015: Aarch64: Move linux code out from generic CPU feature detection #299

Closed
wants to merge 2 commits into from

Conversation

@rnkovacs
Copy link
Contributor

@rnkovacs rnkovacs commented Aug 27, 2021

Not a clean backport. Differences to the original commit:

  • Changes to files under src/hotspot/cpu/aarch64/ and src/hotspot/os_cpu/linux_aarch64/ are adjusted to cover hardware features supported in JDK 11.
  • As JDK 11 is built using an older C++ standard than tip, static_asserts need to be replaced with STATIC_ASSERT macros in vm_version_linux_aarch64.cpp.
  • The GraalVM related changes in vmStructs_jvmci.cpp, AArch64HotSpotLIRGenerator.java and GraalHotSpotVMConfig.java are skipped as they seem to be modifying code added in JDK 14 (JDK-8226222 and JDK-8231973).

This is part of the Windows/AArch64 port.


Progress

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

Issues

  • JDK-8253015: Aarch64: Move linux code out from generic CPU feature detection
  • JDK-8255716: AArch64: Regression: JVM crashes if manually offline a core

Reviewers

Contributors

  • Bernhard Urban-Forster <burban@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/299/head:pull/299
$ git checkout pull/299

Update a local copy of the PR:
$ git checkout pull/299
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/299/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 299

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/299.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 27, 2021

👋 Welcome back rnkovacs! 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 openjdk bot changed the title Backport ec9bee68660acd6abf0b4dd4023ae69514542256 8253015: Aarch64: Move linux code out from generic CPU feature detection Aug 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 27, 2021

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added the backport label Aug 27, 2021
@rnkovacs rnkovacs marked this pull request as ready for review Aug 27, 2021
@openjdk openjdk bot added the rfr label Aug 27, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 27, 2021

Webrevs

@rnkovacs rnkovacs force-pushed the 8253015-aarch64-refactoring branch from c55192d to 4884ef8 Aug 30, 2021
@rnkovacs rnkovacs changed the base branch from master to pr/274 Aug 30, 2021
Copy link
Member

@AntonKozlov AntonKozlov left a comment

Thanks for taking care of this.

Original change introduced two regressions which were fixed in:

  • JDK-8255975: Fix AArch64 OpenJDK build failure with gcc-5 -- but it is not a problem for this change, since STATIC_ASSERT is used
  • JDK-8255716: AArch64: Regression: JVM crashes if manually offline a core -- this is still a problem for this backport

I'm not sure is it better to backport JDK-8255716 separately or, considering that the follow-up change is small, to add that into this change (like adding another commit in the PR and do /issue add manually as described in https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/issue)

}
fclose(f);
}
guarantee(cpu_lines == os::processor_count(), "core count should be consistent");
Copy link
Member

@AntonKozlov AntonKozlov Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was later removed in https://bugs.openjdk.java.net/browse/JDK-8255716

Copy link
Contributor Author

@rnkovacs rnkovacs Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing this to my attention. I added that commit / issue here.

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 30, 2021

/issue add 8255716

@openjdk
Copy link

@openjdk openjdk bot commented Aug 30, 2021

@rnkovacs
Adding additional issue to issue list: 8255716: AArch64: Regression: JVM crashes if manually offline a core.

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 30, 2021

/contributor add Bernhard Urban-Forster burban@openjdk.org

@openjdk
Copy link

@openjdk openjdk bot commented Aug 30, 2021

@rnkovacs
Contributor Bernhard Urban-Forster <burban@openjdk.org> successfully added.

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 30, 2021

@AntonKozlov Thank you. Could you please tag these two JBS issues for me?

@AntonKozlov
Copy link
Member

@AntonKozlov AntonKozlov commented Aug 30, 2021

Could you please tag these two JBS issues for me?

I would like to, but my review does not count -- I'm not a formal reviewer in the JDK Updates project.
I think this should be possible after a formal review (per step 4 of https://wiki.openjdk.java.net/display/JDKUpdates/How+to+contribute+a+fix).

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Aug 30, 2021

Could you please tag these two JBS issues for me?

I would like to, but my review does not count -- I'm not a formal reviewer in the JDK Updates project.
I think this should be possible after a formal review (per step 4 of https://wiki.openjdk.java.net/display/JDKUpdates/How+to+contribute+a+fix).

Right, I'll check out the formal Reviewers list. Thanks.

@VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Oct 2, 2021

Can we get a formal review here please ?
Original Author of the fix reviewed it already ( but has no reviewer status) and improvements were made.

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Oct 4, 2021

Can we get a formal review here please ? Original Author of the fix reviewed it already ( but has no reviewer status) and improvements were made.

There's progress on #274 now. I suppose we can get that integrated shortly and then come back here.

@rnkovacs rnkovacs force-pushed the 8253015-aarch64-refactoring branch from 1b4247f to 08e454f Oct 4, 2021
@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Oct 4, 2021

Rebase (dependency has been updated).

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/274 to master Oct 5, 2021
@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented Oct 5, 2021

The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout 8250810-push-missing
git fetch https://git.openjdk.java.net/jdk11u-dev master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@rnkovacs rnkovacs force-pushed the 8253015-aarch64-refactoring branch from 08e454f to f4b7e65 Oct 5, 2021
@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Oct 5, 2021

Another rebase.

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Oct 5, 2021

@theRealAph @TheRealMDoerr Any chance to review this? Would be the next one in the chain for Windows AArch64 port (and I believe required as a prerequisite for Mac Arm64 as well?)

@lewurm
Copy link
Member

@lewurm lewurm commented Oct 12, 2021

@theRealAph @TheRealMDoerr ping. This change is needed for the Windows/AArch64 backport can go in.

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

I don't know which hardware features are supported in 11, but the backports looks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 2021

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

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

8253015: Aarch64: Move linux code out from generic CPU feature detection
8255716: AArch64: Regression: JVM crashes if manually offline a core

Co-authored-by: Bernhard Urban-Forster <burban@openjdk.org>
Reviewed-by: akozlov, mdoerr

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

  • 2a028c4: 8180568: Refactor javax/crypto shell tests to plain java tests
  • 7410b74: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
  • fc2646b: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
  • aa5d5f3: 8245147: Refactor and improve utility of test/langtools/tools/javac/versions/Versions.java
  • 9512c47: 8273826: Correct Manifest file name and NPE checks
  • bcee700: 8273961: jdk/nio/zipfs/ZipFSTester.java fails if file path contains '+' character
  • 4e63adb: 8242793: Incorrect copyright header in ContinuousCallSiteTargetChange.java
  • 82491fd: 7105119: [TEST_BUG] [macosx] In test UIDefaults.toString() must be called with the invokeLater()
  • d66464e: 8237589: Fix copyright header formatting
  • 75b513f: 8243543: jtreg test security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java fails
  • ... and 31 more: https://git.openjdk.java.net/jdk11u-dev/compare/ddc3288b8cde25001244fee81896c98b1237affc...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 (@TheRealMDoerr) 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 openjdk bot added the ready label Oct 12, 2021
@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Oct 12, 2021

Thanks! Tagged both issues.

@rnkovacs
Copy link
Contributor Author

@rnkovacs rnkovacs commented Oct 12, 2021

/integrate

@openjdk openjdk bot added the sponsor label Oct 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 2021

@rnkovacs
Your change (at version f4b7e65) is now ready to be sponsored by a Committer.

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Oct 12, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 2021

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

  • 2a028c4: 8180568: Refactor javax/crypto shell tests to plain java tests
  • 7410b74: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
  • fc2646b: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
  • aa5d5f3: 8245147: Refactor and improve utility of test/langtools/tools/javac/versions/Versions.java
  • 9512c47: 8273826: Correct Manifest file name and NPE checks
  • bcee700: 8273961: jdk/nio/zipfs/ZipFSTester.java fails if file path contains '+' character
  • 4e63adb: 8242793: Incorrect copyright header in ContinuousCallSiteTargetChange.java
  • 82491fd: 7105119: [TEST_BUG] [macosx] In test UIDefaults.toString() must be called with the invokeLater()
  • d66464e: 8237589: Fix copyright header formatting
  • 75b513f: 8243543: jtreg test security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java fails
  • ... and 31 more: https://git.openjdk.java.net/jdk11u-dev/compare/ddc3288b8cde25001244fee81896c98b1237affc...master

Your commit was automatically rebased without conflicts.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 2021

@RealCLanger @rnkovacs Pushed as commit d336b24.

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

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