8340141: C1: rework ciMethod::equals following 8338471

[dean-long]

trafficstars

This PR changes ciMethod::equals() to a special-purpose debug helper method for the one place in C1 that uses it in an assert. The reason why making it general purpose is difficult is because JVMTI can add and delete methods. See the bug report and JDK-8338471 for more details. I'm open to suggestions for a better name than equals_ignore_version().

An alternative approach, which I think may actually be better, would be to check for old methods first, and bail out if we see any. Then we can change the assert back to how it was originally, using ==.

---

Progress

• [ ] 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-8340141: C1: rework ciMethod::equals following 8338471 (Bug - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) 🔄 Re-review required (review applies to 2c7fc099)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21148

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back dlong! 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]]

@dean-long 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:

8340141: C1: rework ciMethod::equals following 8338471

Reviewed-by: kvn, vlivanov

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

• 3f6c04247ff6ad69330bc219ed26944852954e85: 8345143: Remove uses of SecurityManager in the java.desktop module
• 5958463cadb04560ec85d9af972255bfe6dcc2f2: 8343377: Performance regression in reflective invocation of native methods
• 68b1b94d1be686037e2aaef57c0d9adc594fac7a: 8344904: Interned strings in old classes are not stored in CDS archive
• 1997e89ddf9fba7c6eea6c96bd0b5426576d4460: 8345346: Shenandoah: Description of ShenandoahGCMode still refers to incremental update mode
• 3a3bcd53d0b9aa55dcbc15de4d8278ce3258b31e: 8344800: Add W3C DTDs and XSDs to the JDK built-in Catalog
• 940aa7c4cf1bf770690660c8bb21fb3ddc5186e4: 8344397: Remove Security Manager dependencies from java.security and sun.security packages
• 3d0d0e62900653c4e395166a9ac48578b3dbc1f8: 8345012: os::build_agent_function_name potentially wastes a byte when allocating the buffer
• 525f33baaea2cc559ddd2396611a7734a64a9d66: 8345324: Update comment in SourceVersion for language evolution history for changes in 24
• d6a5f1bafb879258cf5f1d4cd89e9cc272b0c01f: 8344768: Consider removing "sun.security.krb5.autodeducerealm" system property
• 7c944ee6f4dda4f1626721d63ac6bc6d1b40d33b: 8345172: x86: Some CPU feature asserts are declared as 32-bit only
• ... and 108 more: https://git.openjdk.org/jdk/compare/4d4cef800a4b763ab00e93e7a76a5ca5040ab826...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[bot]]

@dean-long The following label will be automatically applied to this pull request:

• hotspot-compiler

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

• 08: Full (7a7bdb86)
• 07: Full - Incremental (fb3308c9)
• 06: Full (80024872)
• 05: Full - Incremental (701373f5)
• 04: Full - Incremental (2c7fc099)
• 03: Full - Incremental (55988fd3)
• 02: Full - Incremental (80c9ae67)
• 01: Full - Incremental (0705b33e)
• 00: Full (3b258664)

[dean-long]

The alternative approach would look something like this:

    if (target->get_Method()->is_old() || cha_monomorphic_target->get_Method()->is_old()) {
        BAILOUT("redefined method");
    }
    assert(!target->can_be_statically_bound() || target == cha_monomorphic_target, "");

[dean-long]

@coleenp, @matias9927, please take a look at this PR too.

[vnkozlov]

@dean-long why then you choose this change instead of alternative approach which you think is better?

[vnkozlov]

Instead of bailout in alternative approach we can change cha_monomorphic_target to nullptr in code which is looking for it in previous lines. target will be used for call and we will loose a little performance when JVMTI is used instead of skipping compilation. Am I missing something?

[dean-long]

@vnkozlov , I like the alternative approach better. I went with the current approach because I was thinking it would be simpler than the bailout, but I changed my mind after writing both out.

Yes, we can change cha_monomorphic_target to nullptr instead of bailing out. But my understanding is any use of old/redefined methods will cause the compilation to be thrown out when we try to create the nmethod, so we are avoiding wasted work by bailing out early.

[vnkozlov]

Yes, we can change cha_monomorphic_target to nullptr instead of bailing out. But my understanding is any use of old/redefined methods will cause the compilation to be thrown out when we try to create the nmethod, so we are avoiding wasted work by bailing out early.

Good point.

[vnkozlov]

@dean-long, may be I misunderstand your statement. Are you re-writing the fix or keep current?

[iwanowww]

JVMTI can add and delete methods

Can you elaborate on that point, please? JVMTI spec states that redefinition/retransformation "must not add, remove or rename fields or methods" [1] [2].

[1] https://docs.oracle.com/en/java/javase/23/docs/specs/jvmti.html#RedefineClasses [2] https://docs.oracle.com/en/java/javase/23/docs/specs/jvmti.html#RetransformClasses

[iwanowww]

I like @vnkozlov suggestion to null out cha_monomorphic_target. Moreover, the validation can be performed inside ciMethod::find_monomorphic_target() which is used to compute cha_monomorphic_target.

[dean-long]

@dean-long, may be I misunderstand your statement. Are you re-writing the fix or keep current?

Either one is fine with me. I could make a separate draft PR with the alternative solution if that helps reviewers decide.

[dean-long]

JVMTI can add and delete methods

Can you elaborate on that point, please? JVMTI spec states that redefinition/retransformation "must not add, remove or rename fields or methods" [1] [2].

[1] https://docs.oracle.com/en/java/javase/23/docs/specs/jvmti.html#RedefineClasses [2] https://docs.oracle.com/en/java/javase/23/docs/specs/jvmti.html#RetransformClasses

It's because of the AllowRedefinitionToAddDeleteMethods flag:

https://github.com/openjdk/jdk/blob/7312eea382eed048b6abdb6409c006fc8e8f45b4/src/hotspot/share/prims/jvmtiRedefineClasses.cpp#L928

[dean-long]

I like @vnkozlov suggestion to null out cha_monomorphic_target. Moreover, the validation can be performed inside ciMethod::find_monomorphic_target() which is used to compute cha_monomorphic_target.

I like this idea. I pushed a new version.

[dean-long]

Thanks @vnkozlov.

[dean-long]

Hold off on re-reviews. I need to fix some errors introduced by moving the VM state transitions.

[dean-long]

OK, fixed version pushed. I moved the first group of is_old checks into resolve_invoke().

[openjdk[bot]]

@dean-long 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 8340141
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[dean-long]

Added bailouts. Because we record failure in CI layer, C1 and C2 need to check for failure there. C2 already did that, but C1 did not. For C1 I decided to delegate all failure recording to the CI layer. This is a step towards implementing JDK-8132354, and avoids string copying issues like those in JDK-8325095.

[dean-long]

I'm still working on this. It's surprisingly tricky to add new bailout failure points. We also have to add new bailout check locations so we don't try to continue while in an inconsistent state (same issues as exception handling). I'm simulating redefined methods randomly (similar to C2 StressBailout and fail_randomly()) to stress test it.

[dean-long]

New version. I was going to add dependencies for any old methods found, but then I realized we check jvmti_state_changed() at the end, which will throw out the compilation if any methods are redefined during the compilation.

[dean-long]

Thanks @iwanowww. @vnkozlov , please take another look when you get a chance.

[dean-long]

Thanks @vnkozlov.

[dean-long]

/integrate

[openjdk[bot]]

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

• 76e874c08e6434747ac4f4cb4d2e2edcde163b2a: 8345319: Fix the tag type in PoolEntry and AnnotationValue
• e9f6ba05264ecb2f1ca3983ea503778f301bf280: 8345293: Fix generational Shenandoah with compact headers
• e1910f2d19fce5cc78058154c7ddaaa8718973dc: 8345397: Remove from g1HeapRegionRemSet.cpp
• 3c60f0b2bb75150d49da9ab94d88b767275de5e2: 8345296: AArch64: VM crashes with SIGILL when prctl is disallowed
• 3eaa7615cd7dc67eb78fb0a8f89d4e6662a0db37: 8342086: FileInputStream.available() fails with "Incorrect function" for "nul" path (win)
• 60bd73a5957f26742e3c326cca0b45395b9470af: 8342089: Require --enable-native-access to be the same between CDS dump time and run time
• 2be27e1545a36628eef063d5a20c5e1f23e5c9ec: 8345393: ProblemList java/util/concurrent/locks/StampedLock/OOMEInStampedLock.java on generic-all JTREG_TEST_THREAD_FACTORY=Virtual
• ba5093935ddedfecaaa80d3107dc0d84d4d18756: 8341649: Regressions with large metaspace apps after 8338526
• caf053b3ad53e4ce86d07adee6d71ea1ff3e8965: 8337287: Update image in javax.swing.text.Document.insert
• 8647c00114385f74939bf705c9c07e709f41a18d: 8342602: Remove JButton/PressedButtonRightClickTest test
• ... and 138 more: https://git.openjdk.org/jdk/compare/4d4cef800a4b763ab00e93e7a76a5ca5040ab826...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@dean-long Pushed as commit 293323c3e210bc2a3e45a0a9bc99b55378be91d2.

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