JDK-8317575 : AArch64: C2_MacroAssembler::fast_lock uses rscratch1 for cmpxchg result
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 17.0.3,21,22
  • Priority: P5
  • Status: Resolved
  • Resolution: Fixed
  • CPU: aarch64
  • Submitted: 2023-10-05
  • Updated: 2024-09-05
  • Resolved: 2023-10-20
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 21 JDK 22
21.0.6Fixed 22 b21Fixed
Related Reports
Relates :  
Description
The fast_lock code CASes the owner field with the current thread and upon failure checks if the previous value was the current thread, which would indicate a recursive lock.
```
  add(tmp, disp_hdr, (in_bytes(ObjectMonitor::owner_offset())-markWord::monitor_value));
  cmpxchg(tmp, zr, rthread, Assembler::xword, /*acquire*/ true,
          /*release*/ true, /*weak*/ false, rscratch1); // Sets flags for result

  <snip>
  br(Assembler::EQ, cont); // CAS success means locking succeeded

  cmp(rscratch1, rthread);
  br(Assembler::NE, cont); // Check for recursive locking
```

The contract is that cmpxchg clobbers rscratch1, so this seems problematic.

The cmpxchg code looks like this:
```
void MacroAssembler::cmpxchg(Register addr, Register expected,
                             Register new_val,
                             enum operand_size size,
                             bool acquire, bool release,
                             bool weak,
                             Register result) {
  if (result == noreg)  result = rscratch1;
  BLOCK_COMMENT("cmpxchg {");
  if (UseLSE) {
    mov(result, expected);
    lse_cas(result, new_val, addr, size, acquire, release, /*not_pair*/ true);
    compare_eq(result, expected, size);
#ifdef ASSERT
    // Poison rscratch1 which is written on !UseLSE branch
    mov(rscratch1, 0x1f1f1f1f1f1f1f1f);
#endif
  } else {
    Label retry_load, done;
    prfm(Address(addr), PSTL1STRM);
    bind(retry_load);
    load_exclusive(result, addr, size, acquire);
    compare_eq(result, expected, size);
    br(Assembler::NE, done);
    store_exclusive(rscratch1, new_val, addr, size, release);
    if (weak) {
      cmpw(rscratch1, 0u);  // If the store fails, return NE to our caller.
    } else {
      cbnzw(rscratch1, retry_load);
    }
    bind(done);
  }
  BLOCK_COMMENT("} cmpxchg");
}
```

For -XX:-UseLSE this is a benign problem because when the owner value is set to non-null the cmpxchg doesn't take the clobbering path:
```
    store_exclusive(rscratch1, new_val, addr, size, release);
    if (weak) {
      cmpw(rscratch1, 0u);  // If the store fails, return NE to our caller.
    } else {
      cbnzw(rscratch1, retry_load);
    }
```

So, the code happens to work but it would be better to use another register for the result.

The debug clobbering in the -XX:+UseLSE path was recently added and this will lead to debug builds never taking the recursive fast-path. That clobbering should maybe be done also for -XX:-UseLSE, which doesn't always clobber the register?
Comments
[jdk21u-fix-request] Approval Request from Boris Let us do the backport for stability and code consistency. As Aleksey mentioned, the reasons are: a) we can have surprise interaction with other backports later; b) the code shape being similar in all currently maintained releases makes other backports cleaner, and thus less tedious. I have tested the change with jtreg tier1-3 on linux-aarch64.
21-08-2024

A pull request was submitted for review. Branch: master URL: https://git.openjdk.org/jdk21u-dev/pull/941 Date: 2024-08-21 22:00:46 +0000
21-08-2024

I would still consider backporting it, even if it does not fix any currently known bug, for two reasons: a) we can have surprise interaction with other backports later; b) the code shape being similar in all currently maintained releases makes other backports cleaner, and thus less tedious.
23-10-2023

Changeset: 387504c9 Author: Stefan Karlsson <stefank@openjdk.org> Date: 2023-10-20 07:05:30 +0000 URL: https://git.openjdk.org/jdk/commit/387504c9e4b93d162dcef7c90c57c27295858d2e
20-10-2023

Makes sense. I'm just adding the affects versions for future reference / completeness. Thanks.
05-10-2023

[~stefank] looks like this code was introduced by JDK-8277180. Is 17 affected as well?
05-10-2023

ILW = scratch register clobbered (not an issue with current code), AArch64 with LSE, no workaround = LMH = P5
05-10-2023

A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/16049 Date: 2023-10-05 08:08:31 +0000
05-10-2023

[~thartmann] 17 is affected as well, but since this isn't causing any bugs I'm not sure this needs to be updated in 17.
05-10-2023