RFR: 8317575: AArch64: C2_MacroAssembler::fast_lock uses rscratch1 for cmpxchg result

[Stefan Karlsson]

On Thu, 5 Oct 2023 08:08:31 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> Please review this removal a potentially problematic usage of rscratch1 in C2_MacroAssembler::fast_lock.

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?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16049#issuecomment-1748336425