8354668: Missing REX2 prefix accounting in ZGC barriers leads to incorrect encoding

[jatin-bhateja]

ZGC bookkeeps multiple place holders in barrier code snippets through relocations, these are later used to patch appropriate contents (mostly immediate values) in instruction encoding. While most of the relocation records the patching offsets from the end of the instruction, SHL instruction, which is used for pointer coloring, computes the patching offset from the starting address of the instruction.

Thus, in case the destination register operand of SHL instruction is an extended GPR register, we miss accounting additional REX2 prefix byte in patch offset, thereby corrupting the encoding since runtime patches the primary opcode byte resulting into ILLEGAL instruction exception.

This patch fixes reported failures by computing the relocation offset of SHL instruction from end of instruction, thereby making the patch offset agnostic to REX/REX2 prefix.

Please review and share your feedback.

Best Regards,
Jatin

PS: Validation were performed using latest Intel Software Development Emulator after modifying static register allocation order in x86_64.ad file giving preference to EGPRs.

---

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-8354668: Missing REX2 prefix accounting in ZGC barriers leads to incorrect encoding (Bug - P3)

Reviewers

• Axel Boldt-Christmas (@xmas92 - Reviewer)
• Sandhya Viswanathan (@sviswa7 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24664

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[jatin-bhateja]

/label add hotspot-compiler-dev

[bridgekeeper]

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

[jatin-bhateja]

/label add hotspot-gc-dev

[openjdk]

@jatin-bhateja 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:

8354668: Missing REX2 prefix accounting in ZGC barriers leads to incorrect encoding

Reviewed-by: aboldtch, sviswanathan

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

• d61765f: 8353488: Open some JComboBox bugs 3
• 486a664: 8353486: Open source Swing Tests - Set 4
• f98af0a: 8354701: Open source few JToolTip tests
• ... and 118 more: https://git.openjdk.org/jdk/compare/36864a2a08b5b64e63a9265d595ba7fb608994cb...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]

@jatin-bhateja
The hotspot-compiler label was successfully added.

[openjdk]

@jatin-bhateja
The hotspot-gc label was successfully added.

[mlbridge]

Webrevs

• 02: Full - Incremental (dc2b2b16)
• 01: Full - Incremental (ffd92c37)
• 00: Full (1a5a73c0)

[xmas92]

Looks good but need to communicate with JVMCI implementors.

Also pre-exisiting but maybe ZBarrierRelocationFormatLoadGoodAfterShl should be called ZBarrierRelocationFormatLoadGoodAfterShX as we use it for both shr and shl.

[xmas92]

Should probably communicate with the JVMCI / Graal @dougxc so we can both update this exported symbol name to reflect the new behaviour, and give them the opportunity to adapt to the new relocation patching.

[dean-long]

This looks OK, but we could do better. Instead of making the relocation point to the end of the instruction and then looking up the offset with patch_barrier_relocation_offset(), why not make the offset always 0 and have the relocation point to the data offset inside the instruction?

[jatin-bhateja]

This looks OK, but we could do better. Instead of making the relocation point to the end of the instruction and then looking up the offset with patch_barrier_relocation_offset(), why not make the offset always 0 and have the relocation point to the data offset inside the instruction?

Hi @dean-long ,
As of now, barrier relocations are placed either before[1] or after[2] the instructions, offset is then added to compute the effective address of the patch site. For this bug fix PR I intend to make the patch offset agnostic to REX/REX2 prefix without disturbing the existing implimentation.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp#L394
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp#L397

[dean-long]

Yes, I am suggesting doing something like:

      __ relocate(__ pc() - 4, barrier_Relocation::spec(), ZBarrierRelocationFormatStoreGoodAfterOr);

which would be a bigger change to the implementation.

[jatin-bhateja]

Yes, I am suggesting doing something like:

      __ relocate(__ pc() - 4, barrier_Relocation::spec(), ZBarrierRelocationFormatStoreGoodAfterOr);

which would be a bigger change to the implementation.

Hi @dean-long ,
Intent of this bug fix PR is not to improve upon the infrastructure but to align the fix with the current scheme. Do you suggest doing that in a seperate PR ?

[dean-long]

When I made my suggestions, I didn't realize it would also require changes on the Graal side. So I would suggest a separate PR only if the Graal team agrees.

[xmas92]

Changes looks good. But coordinate with the Graal team before pushing anything.

I think @dean-long's suggestion is good. But it should be done for all relocations in a separate PR.

[sviswa7]

Looks good to me as well.

[jatin-bhateja]

Hi @dean-long ,
I have created a follow up JBS (JDK-8355341) to capture your suggestion.

Thanks for reviews @xmas92 and @sviswa7

[jatin-bhateja]

/integrate

[openjdk]

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

• 5392488: 8355332: Fix failing semi-manual test EDT issue
• b7e8952: 8355071: Fix nsk/jdi test to not require lookup of main thread in order to set the breakpoint used for communication
• cc9148d: 8354695: Open source several swing tests batch7
• ... and 123 more: https://git.openjdk.org/jdk/compare/36864a2a08b5b64e63a9265d595ba7fb608994cb...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jatin-bhateja Pushed as commit 4c37370.

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

[TobiHartmann]

Backed out again with #24815 due to failures in our testing.