-
Bug
-
Resolution: Fixed
-
P5
-
17.0.3, 21, 22
-
b21
-
aarch64
Issue | Fix Version | Assignee | Priority | Status | Resolution | Resolved In Build |
---|---|---|---|---|---|---|
JDK-8339624 | 21.0.6 | Boris Ulasevich | P5 | Resolved | Fixed | b01 |
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?
```
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?
- backported by
-
JDK-8339624 AArch64: C2_MacroAssembler::fast_lock uses rscratch1 for cmpxchg result
- Resolved
- relates to
-
JDK-8277180 Intrinsify recursive ObjectMonitor locking for C2 x64 and A64
- Resolved
- links to
-
Commit openjdk/jdk/387504c9
-
Commit(master) openjdk/jdk21u-dev/a7e928c9
-
Review openjdk/jdk/16049
-
Review(master) openjdk/jdk21u-dev/941
(1 links to)