8341444: Unnecessary check for JSRs in CDS #21330

matias9927 · 2024-10-03T17:45:42Z

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-8341444: Unnecessary check for JSRs in CDS (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Coleen Phillimore (@coleenp - Reviewer) Review applies to 8b599317

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21330

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-03T17:46:49Z

👋 Welcome back matsaave! 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 · 2024-10-03T17:47:39Z

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

8341444: Unnecessary check for JSRs in CDS

Reviewed-by: dholmes, coleenp

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

7a16906: 8341134: Deprecate for removal the jrunscript tool
ffe6091: 8173970: jar tool should have a way to extract to a directory
2b03dbd: 8311530: Deprecate jdk.jsobject module for removal
f7e8f50: 8341292: Open some TextArea awt tests 3
fe83b7d: 8339341: SurfaceManager cacheMap retains strong references
12551ae: 8339975: Open some dialog awt tests 2
60af907: 8340494: Open some dialog awt tests 4
40a44e1: 8340851: Open some TextArea awt tests
b993227: 8342119: Shenandoah: Remove extra ShenandoahUpdateRefsClosure
83e9e48: 8325937: runtime/handshake/HandshakeDirectTest.java causes "monitor end should be strictly below the frame pointer" assertion failure on AArch64
... and 349 more: https://git.openjdk.org/jdk/compare/212e32931cafe446d94219d6c3ffd92261984dff...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.

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

openjdk · 2024-10-03T17:48:31Z

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

hotspot

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 · 2024-10-03T20:16:54Z

Webrevs

dholmes-ora

Again apologies for commenting and running off on vacation, but this one does not look right to me based on the description.

dholmes-ora · 2024-10-04T05:50:01Z

src/hotspot/share/interpreter/bytecodeStream.hpp

      if (len == 0) len = Bytecodes::length_at(_method(), bcp);
      if (len <= 0 || (_bci > _end_bci - len) || (_bci - len >= _next_bci)) {
-        raw_code = code = Bytecodes::_illegal;
+        fatal("Should have been caught by verifier");


Verification may be disabled, This may be a risky change in behaviour.

If verification is disabled, this fatal is better than what might come next. But there may be callers now or in the future that check for Bytecodes::_illegal, so I don't think we should have this assert even though we don't have any code that hits it.

dholmes-ora · 2024-10-04T05:52:20Z

src/hotspot/share/oops/instanceKlass.cpp

  // don't have any methods share the Universe::_the_empty_method_array which is in the RO region.
-  if (_methods != nullptr && _methods->length() > 0 &&
-      !can_be_verified_at_dumptime() && methods_contain_jsr_bytecode()) {
+  if (_methods != nullptr && _methods->length() > 0 && !can_be_verified_at_dumptime()) {
    // To handle jsr bytecode, new Method* maybe stored into _methods


Based on the comment we only need to do this if the method does contain a JSR bytecode - now we will do it for all cases where verification can't be done at dumptime. ??

Yes, we will put these in RW space the time. Using the BytecodeStream to look for JSRs in unverified code is risky, so we avoid doing this optimization.

So shouldn't the comment be updated to indicate that we cannot safely check if the jsr bytecode exists and so all methods that can't be verified are made writeable?

coleenp

I think you should take out the 'fatal' error in bytecodeStream.

coleenp · 2024-10-04T17:31:13Z

src/hotspot/share/interpreter/bytecodeStream.hpp

      if (len == 0) len = Bytecodes::length_at(_method(), bcp);
      if (len <= 0 || (_bci > _end_bci - len) || (_bci - len >= _next_bci)) {
-        raw_code = code = Bytecodes::_illegal;
+        fatal("Should have been caught by verifier");


If verification is disabled, this fatal is better than what might come next. But there may be callers now or in the future that check for Bytecodes::_illegal, so I don't think we should have this assert even though we don't have any code that hits it.

coleenp · 2024-10-04T17:33:50Z

src/hotspot/share/oops/instanceKlass.cpp

  // don't have any methods share the Universe::_the_empty_method_array which is in the RO region.
-  if (_methods != nullptr && _methods->length() > 0 &&
-      !can_be_verified_at_dumptime() && methods_contain_jsr_bytecode()) {
+  if (_methods != nullptr && _methods->length() > 0 && !can_be_verified_at_dumptime()) {
    // To handle jsr bytecode, new Method* maybe stored into _methods


Yes, we will put these in RW space the time. Using the BytecodeStream to look for JSRs in unverified code is risky, so we avoid doing this optimization.

coleenp

This looks good.

dholmes-ora

Thank you!

matias9927 · 2024-10-18T13:22:51Z

Thanks for the reviews @dholmes-ora and @coleenp!
/integrate

openjdk · 2024-10-18T13:23:52Z

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

8174cbd: 8341978: Improve JButton/bug4490179.java
9201e9f: 8342409: [s390x] C1 unwind_handler fails to unlock synchronized methods with LM_MONITOR
0963b9e: 8341664: ReferenceClassDescImpl cache internalName
c51a086: 8339694: ciTypeFlow does not correctly handle unresolved constant dynamic of array type
7f4ed50: 8341020: Error handler crashes when Metaspace is not fully initialized
f50bd0d: 8341513: Remove the unused thread_type field from OSThread
7a16906: 8341134: Deprecate for removal the jrunscript tool
ffe6091: 8173970: jar tool should have a way to extract to a directory
2b03dbd: 8311530: Deprecate jdk.jsobject module for removal
f7e8f50: 8341292: Open some TextArea awt tests 3
... and 355 more: https://git.openjdk.org/jdk/compare/212e32931cafe446d94219d6c3ffd92261984dff...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-18T13:24:04Z

@matias9927 Pushed as commit 28252bb.

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

8341444: Unnecessary check for JSRs in CDS

9268ef4

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 3, 2024

matias9927 marked this pull request as ready for review October 3, 2024 20:12

openjdk bot added the rfr Pull request is ready for review label Oct 3, 2024

dholmes-ora reviewed Oct 4, 2024

View reviewed changes

coleenp suggested changes Oct 4, 2024

View reviewed changes

Removed fatal()

8b59931

coleenp approved these changes Oct 14, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 14, 2024

matias9927 added 2 commits October 16, 2024 17:22

Updated comment

a4cbde8

Further changed comment

d7cdb70

openjdk bot removed the ready Pull request is ready to be integrated label Oct 17, 2024

dholmes-ora approved these changes Oct 18, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 18, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 18, 2024

openjdk bot closed this Oct 18, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 18, 2024

8341444: Unnecessary check for JSRs in CDS #21330

8341444: Unnecessary check for JSRs in CDS #21330

Uh oh!

Conversation

matias9927 commented Oct 3, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 3, 2024

Uh oh!

openjdk bot commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 3, 2024

Uh oh!

mlbridge bot commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

matias9927 commented Oct 18, 2024

Uh oh!

openjdk bot commented Oct 18, 2024

Uh oh!

openjdk bot commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

matias9927 commented Oct 3, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 3, 2024 •

edited

Loading

mlbridge bot commented Oct 3, 2024 •

edited

Loading