RFR: 8354523: runtime/Monitor/SyncOnValueBasedClassTest.java triggers SIGSEGV

[Axel Boldt-Christmas]

On Tue, 15 Apr 2025 12:47:47 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

> When DiagnoseSyncOnValueBasedClasses is != 0, then we can take the slow-path without having cleared the monitor cache in the BasicLock. This would later lead to a crash or other unexpected behaviour. This can happen with C1 or the interpreter, C2 has the DiagnoseSyncOnValueBasedClasses-block after clearing the cache, and the native-entry in sharedRuntime_x86_64.cpp does not have a DiagnoseSyncOnValueBasedClasses-block at all.
> 
> The proposed fix so far is a bit ugly because it repeats the clearing code in 3 places. The alternative would be to move the DiagnoseSyncOnValueBasedClasses-block into MA::lightweight_lock(), but this would bring DiagnoseSyncOnValueBasedClasses-handling into the native entry in sharedRuntime_x86_64.cpp, which is currently not the case. Also, we don't have enough regs for that, but we can probably use rscratch1 now that 32-bit is gone (as is already done in C1 and interpreter paths anyway).
> 
> I'd first settle on the structure, and then implement the same thing for aarch64.
> ping @xmas92

This seems fine.

I see pros with both approaches. If we go with this one I think MacroAssembler::lightweight_lock should document that it expects the BasicLock is cleared with UseObjectMonitorTable. 

Moving it into the `DiagnoseSyncOnValueBasedClasses != 0` condition keeps MacroAssembler::lightweight_lock  a bit more contained. But the comment about clearing just being about the fast path is not correct anymore. As we also read the value in the slow path.

Either way we should probably make sure the comments are up to date with what we exprect.

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

Marked as reviewed by aboldtch (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24660#pullrequestreview-2768899723