8314078: HotSpotConstantPool.lookupField() asserts due to field changes in ConstantPool.cpp

[iklam]

This PR updates Java code in JVMCI to match the C code changes in JDK-8301996 that modified the constant pool layout. Essentially, the indices after a getfield/putfield/getstatic/putstatic bytecode is no longer a CpCacheIndex, but an index for ConstantPool::resolved_field_entry_at(int field_index).

The assertion (and subsequent crash) happen only in debug builds. Out of pure luck, in product build JVMCI produces the correct output even after JDK-8301996, although the code was doing the wrong thing.

This PR has (so far) two commits:

• 6527e67 The actual functional change to use the rawIndex that follows a field bytecode.
• c322b8e Fixed incorrectly named parameters and variables in the JVMCI code and JavaDoc. In most cases, cpi needs to be changed to rawIndex to reflect the correct type of the index.

To help reviewing, I am limiting the renaming to just those affected by the field changes (without the renames, it's hard to validate that I am actually doing the right thing).

There are still some cases of cpi that need to be changed to rawIndex. I will fix those in a separate RFE. E.g. in ConstantPool.java:

default JavaMethod lookupMethod(int cpi, int opcode) {
    return lookupMethod(cpi, opcode, null);
}

---

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-8314078: HotSpotConstantPool.lookupField() asserts due to field changes in ConstantPool.cpp (Bug - P2)

Reviewers

• Doug Simon (@dougxc - Reviewer) ⚠️ Review applies to c322b8e7
• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to c322b8e7

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15237

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back iklam! 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]

@iklam The following labels will be automatically applied to this pull request:

• graal
• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[iklam]

/label add hotspot-runtime

[openjdk]

@iklam
The hotspot-runtime label was successfully added.

[mlbridge]

Webrevs

• 01: Full - Incremental (21976c06)
• 00: Full (c322b8e7)

[dougxc]

Thanks for doing this Ioi.

In this PR or the follow-up renaming RFE, could you please add a "decoder ring" comment to the javadoc for ConstantPool. An incomplete example:

 * The following terminology is used when indexing a constant pool entry:
 * <ul>
 * <li>rawIndex - index in the bytecode stream after the opcode (could be rewritten for some bytecodes)</li>
 * <li>cpi - the class file constant pool index</li>
 * <li>cpci - a constant pool cache index</li>
 * </ul>

[openjdk]

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

8314078: HotSpotConstantPool.lookupField() asserts due to field changes in ConstantPool.cpp

Reviewed-by: dnsimon, 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 27 new commits pushed to the master branch:

• 8f1c134: 8313798: [aarch64] sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java sometimes times out on aarch64
• 1232677: 8313244: NM flags handling in configure process
• 6ffc032: 8314113: G1: Remove unused G1CardSetInlinePtr::card_at
• 62adeb0: 8311648: Refactor the Arena/Chunk/ChunkPool interface
• 43462a3: 8313224: Avoid calling JavaThread::current() in MemAllocator::Allocation constructor
• 9abb2a5: 8312461: JNI warnings in SunMSCApi provider
• 42758cb: 8312882: Update the CONTRIBUTING.md with pointers to lifecycle of a PR
• 88b4e3b: 8304292: Memory leak related to ClassLoader::update_class_path_entry_list
• 6f5c903: 8313899: JVMCI exception Translation can fail in TranslatedException.
• d97de82: 8313633: [macOS] java/awt/dnd/NextDropActionTest/NextDropActionTest.java fails with java.lang.RuntimeException: wrong next drop action!
• ... and 17 more: https://git.openjdk.org/jdk/compare/c307391ab1f071b1473cd5f4c12437b8d5e0ca93...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.

[coleenp]

This looks good. I don't know how we missed it so thank you for fixing this. Thank you for fixing the variable names, which could have been part of the confusion.

[iklam]

Thanks for doing this Ioi.

In this PR or the follow-up renaming RFE, could you please add a "decoder ring" comment to the javadoc for ConstantPool. An incomplete example:

 * The following terminology is used when indexing a constant pool entry:
 * <ul>
 * <li>rawIndex - index in the bytecode stream after the opcode (could be rewritten for some bytecodes)</li>
 * <li>cpi - the class file constant pool index</li>
 * <li>cpci - a constant pool cache index</li>
 * </ul>

Hi Doug, thanks for the review. I added the comments into ConstantPool.java. I omitted cpci as it's not part of the API.

I hope to add more details in the comments when fixing the other incorrectly named variables in JDK-8314172

[iklam]

Thanks @dougxc and @coleenp for the review.
/integrate

[openjdk]

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

• 6574dd7: 8314025: Remove JUnit-based test in java/lang/invoke from problem list
• 207bd00: 8313756: [BACKOUT] 8308682: Enhance AES performance
• 823f5b9: 8308850: Change JVM options with small ranges that get -Wconversion warnings to 32 bits
• 5bfb82e: 8314119: G1: Fix -Wconversion warnings in G1CardSetInlinePtr::card_pos_for
• 06aa3c5: 8314118: Update JMH devkit to 1.37
• 4164693: 8313372: [JVMCI] Export vmIntrinsics::is_intrinsic_available results to JVMCI compilers.
• 049b55f: 8314019: Add gc logging to jdk/jfr/event/gc/detailed/TestZAllocationStallEvent.java
• a39ed10: 8314116: C2: assert(false) failed: malformed control flow after JDK-8305636
• 1de5bf1: 8314106: C2: assert(is_valid()) failed: must be valid after JDK-8305636
• 5c91622: 8314117: RISC-V: Incorrect VMReg encoding in RISCV64Frame.java
• ... and 31 more: https://git.openjdk.org/jdk/compare/c307391ab1f071b1473cd5f4c12437b8d5e0ca93...master

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 911d1db.

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

[shipilev]

I have the new warning here: cpi is not used. I guess it is correct, seeing how methods are accepting rawIndex now, right?

[iklam]

You're right. cpi is not used, as the next line uses rawIndex to obtain information about the field bytecode.

final int nameAndTypeIndex = getNameAndTypeRefIndexAt(rawIndex, opcode);

I'll remove the cpi line in my next JVMCI cleanup PR.