Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8317575

AArch64: C2_MacroAssembler::fast_lock uses rscratch1 for cmpxchg result

XMLWordPrintable

    • b21
    • aarch64

        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?

              stefank Stefan Karlsson
              stefank Stefan Karlsson
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Created:
                Updated:
                Resolved: