8297235: ZGC: assert(regs[i] != regs[j]) failed: Multiple uses of register: rax

[xmas92]

Tests java/util/stream/test/org/openjdk/tests/java/util/* with -XX:+UseZGC -Xcomp -XX:-TieredCompilation crashes with assert(regs[i] != regs[j]) failed: Multiple uses of register: rax. More specifically compilation of java.util.concurrent.ForkJoinTask::awaitDone.

The reason seems to be that the compare value and the memory input ends up sharing a register. (Uses Unsafe CAS which CAS an object reference into a field of that object, oldval: rax and mem: [rax+offset]). The Z load barrier stub dispatch implementation require that the reference and reference address occupy distinct registers. In the loadP nodes this is established by marking all but the memory TEMP which results in no sharing.

This is not possible for the CompareAndSwapP / CompareAndExchangeP nodes as the compare value is an input node.

The solution proposed here is less than ideal as it makes the CAS nodes require one extra TEMP register, which in the common case is unused. This puts unnecessary extra strain on the register allocation. The problem is that there is no way currently (that I can find) to express in .ad that a memory input must not share registers with a specific other input.

There is an alternative solution for this specific crash which does not use a second TEMP register (see commit: cfd5ced). It accomplish this by using the same trick that the aarch64 Z CAS node uses which is to specify the memory as indirect which results in the address being LEA into a register. However from what I can see this does not guarantee that the address and the reference does not share a register (oldval: rax and mem: [rax]). So it is theoretically broken, (and so is the aarch64 implementation).

It is unclear to me if there is ever a way for C2 to generation a CAS which compares the address of the field with its content.

I call on anyone with more knowledge about adlc and C2 for feedback. And specifically I want to open up a discussion with these points:

• Is there some other way of expressing in the .ad file that a memory input should not share some register?
  • If not, is this a worthwhile RFE? As it seems to be a patterned used at least in other places in Z.
• Will the indirect input ever share a register with oldval and/or are the aarch64/riscv implementations broken because of this? How about ppc?

Testing: linux-x64 zgc tagged tests tier 1-7 and some specific crashing tests with -XX:+UseZGC -Xcomp -XX:-TieredCompilation (in: java/util/stream/, java/util/concurrent/)

---

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-8297235: ZGC: assert(regs[i] != regs[j]) failed: Multiple uses of register: rax

Reviewers

• Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to 42a72c1e
• Albert Mingkun Yang (@albertnetymk - Reviewer)
• Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11410

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

Using diff file

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

[bridgekeeper]

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

@xmas92 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]

Webrevs

• 02: Full - Incremental (0c715331)
• 01: Full - Incremental (42a72c1e)
• 00: Full (74f7567b)

[xmas92]

/label add hotspot-compiler

[openjdk]

@xmas92
The hotspot-compiler label was successfully added.

[fisk]

I think that changing memory to indirect in the mach node matching solves the conjoint register problem. The address then becomes a field, and the type of the expected value is an oop. For them to be the same register would be seemingly impossible.

[xmas92]

Changed all nodes to indirect memory inputs to ensure disjoint registers.

[fisk]

Looks good!

[openjdk]

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

8297235: ZGC: assert(regs[i] != regs[j]) failed: Multiple uses of register: rax

Reviewed-by: eosterlund, ayang, rcastanedalo

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

• 173778e: 8298568: Fastdebug build fails after JDK-8296389
• 829cbc2: 8292674: ReportJNIFatalError should print all java frames
• be69930: 8288287: Remove expired flags in JDK 21
• c3bc4fc: 8297505: Declare fields in some sun.security.pkcs11 classes as final
• c7aca73: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt())
• 781a2e0: 8298343: "Could not confirm if TargetJDK is hardened." warning for SA tests on macosx-aarch64-debug
• 9ff85f6: 8298589: java/net/SctpSanity.java fail with NoClassDefFoundError: sun/nio/ch/sctp/UnsupportedUtil
• 81f57d5: 8298567: Make field in RandomAccessFile final
• 56c438b: 8297822: De-duplicate code in module jdk.sctp
• fabda24: 8296389: C2: PhaseCFG::convert_NeverBranch_to_Goto must handle both orders of successors
• ... and 47 more: https://git.openjdk.org/jdk/compare/1166c8e2c0047869cd50b7ddc5355290ac2a695a...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.

[robcasloz]

I agree with @fisk's analysis above: the proposed solution should be safe as long as the first CompareAndSwap operand (mem) has a non-zero offset (which should at least be the case in the failing Unsafe-based patterns): the register allocator will then treat mem and oldval as two distinct, interfering values and assign them different registers.

I attached a minimal reproducer to the JBS issue, feel free to include it in this PR as a test case if you think it adds value.

I do not think there is a general way to express the constraint you want in .ad files, but I am not an expert in this area, maybe someone at Intel could comment on this (@sviswa7, @jatin-bhateja?). I also do not have a feeling for what would be the benefit vs. cost of implementing such construct. An alternative approach could be to enforce the constraint at the C2 IR level, by adding some kind of pseudo-node redefining the input to the first CompareAndSwap operand so that it always interferes with oldval.

Regarding the impact on other architectures, it seems they all follow the solution proposed here, so they should be as safe as in this case, that is, as long as C2 does not generate a CAS comparing the address of the field with its content. I cannot think how C2 could generate such pattern - which of course is not a guarantee that it will never do it ;).

[xmas92]

/integrate

[openjdk]

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

• 4081bba: 8297960: G1: Move Root Region Scan Waiting outside collection in logs
• 216775f: 8298144: Remove Space::new_dcto_cl
• c612f93: 8298576: Serial: Move some MarkSweep method definitions to cpp
• dbcfe7b: 8298521: Rename members in G1MonitoringSupport
• 3b1142a: 8298618: Typo in JPEGImageReader and JPEGImageWriter
• 173778e: 8298568: Fastdebug build fails after JDK-8296389
• 829cbc2: 8292674: ReportJNIFatalError should print all java frames
• be69930: 8288287: Remove expired flags in JDK 21
• c3bc4fc: 8297505: Declare fields in some sun.security.pkcs11 classes as final
• c7aca73: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt())
• ... and 52 more: https://git.openjdk.org/jdk/compare/1166c8e2c0047869cd50b7ddc5355290ac2a695a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@xmas92 Pushed as commit 042b706.

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

[xmas92]

/backport jdk20

[openjdk]

@xmas92 the backport was successfully created on the branch xmas92-backport-042b7062 in my personal fork of openjdk/jdk20. To create a pull request with this backport targeting openjdk/jdk20:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 042b7062 from the openjdk/jdk repository.

The commit being backported was authored by Axel Boldt-Christmas on 13 Dec 2022 and was reviewed by Erik Österlund, Albert Mingkun Yang and Roberto Castañeda Lozano.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk20:

$ git fetch https://github.com/openjdk-bots/jdk20 xmas92-backport-042b7062:xmas92-backport-042b7062
$ git checkout xmas92-backport-042b7062
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk20 xmas92-backport-042b7062

[dcubed-ojdk]

This PR removed all entries from test/jdk/ProblemList-zgc.txt including this entry:

jdk/internal/vm/Continuation/Fuzz.java#default 8298058 generic-x64

which has nothing to do with this bug fix. I'll restore that entry with a new bug shortly.