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

AArch64: C2_MacroAssembler::fast_lock uses rscratch1 for cmpxchg result

    XMLWordPrintable

Details

    • b21
    • aarch64

    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?

      Attachments

        Issue Links

          Activity

            People

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

              Dates

                Created:
                Updated:
                Resolved: