JDK-8320310 : CompiledMethod::has_monitors flag can be incorrect
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 21,22,23
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2023-11-17
  • Updated: 2024-01-16
  • Resolved: 2024-01-08
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 23
23 b05Fixed
Related Reports
Relates :  
JDK-8267532 - C2: Profile and prune untaken exception handlers
Relates :  
JDK-8322743 - assert(held_monitor_count() == jni_monitor_count()) failed
Description
During the development of: JDK-8267532 which attempts to remove extra dead code during compilation, it was discovered that the nmethod::has_monitors flag may not be correct.

The current code sets the has_monitors flag based on whether monitorenter and monitorexit bytecodes are parsed during compilation. However, not every bytecode of a method is necessarily parsed. For instance, if a branch is dead, an uncommon trap may be emitted, and then the bytecode of the branch is not parsed further.

A situation like that may make this assert in 'freeze_internal' in continuationFreezeThaw.cpp fail:

  assert(monitors_on_stack(current) == ((current->held_monitor_count() - current->jni_monitor_count()) > 0),
         "Held monitor count and locks on stack invariant: " INT64_FORMAT " JNI: " INT64_FORMAT, (int64_t)current->held_monitor_count(), (int64_t)current->jni_monitor_count());

We already track whether a method has monitor byte codes while rewriting byte codes. This information is available in (ci)Method::has_monitor_bytecodes(). We could simply reuse this information either to set the nmethod::has_monitor flag (patch attached). This would avoid any issues in case not every byte code is parsed.
Comments
Talking about OSR and locking - see JDK-8322743
10-01-2024
Thanks for explanation. I would then still consider it as low-priority backport to make sure we source our "fiddles with monitors" information from a single proper place, in case our safety assessment is too optimistic.
09-01-2024
[~shade] I couldn't find a reproducer without JDK-8267532. An OSR compilation for a loop inside a synchronized block might not see the monitorenter, but should always see the monitorexit in the exception handler for the synchronized block (since, AFAIU, an OSR compilation always includes the rest of the method starting from a certain point). Prior to JDK-8267532, we already drop exception handlers that have unloaded exception types, so theoretically the synthetic exception handler of a synchronized block could be dropper as well. But, at least javac always catches 'any' exception in the exception handler that it generates, so this handler will never be dropped in practice. I don't see why another java source compiler would do it differently, so I don't think this is really a problem before JDK-8267532
09-01-2024
Understanding question: Does this happen without JDK-8267532, e.g. during parsing that emits uncommon traps? If so, it should affect JDK versions prior to 22?
09-01-2024
Changeset: c8fa3e21 Author: Jorn Vernee <jvernee@openjdk.org> Date: 2024-01-08 14:55:17 +0000 URL: https://git.openjdk.org/jdk/commit/c8fa3e21e6a4fd7846932b545a1748cc1dc6d9f1
08-01-2024
A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/16799 Date: 2023-11-23 15:55:07 +0000
24-11-2023
[~jvernee] thanks for clarifying.
21-11-2023
[~dholmes] The has_monitors flag I'm talking about is actually in CompiledMethod (the super class of nmethod). That is also the one set by JVMCIRuntime::register_method. There's another has_monitors flag on Compile, which is transferred to the nmethod/CompiledMethod when the compiled code is registered (in ciEnv::register_method called from PhaseOutput::install_code). Hence, any errors in how the Compile::has_monitors flag is computed, eventually result in the CompiledMethod::has_monitors flag being incorrect.
20-11-2023
ILW = Possibly incorrect nmethod::has_monitors flag, not observed with mainline, no workaround = MLH = P4
20-11-2023
We have way too many `set_has_monitors()` methods but AFAICS the NMethod one is only set from JVMCIRuntime::register_method. Your patch seems to be changing the Compiler code not the NMethod. ??
20-11-2023
Note that during the review of JDK-8267532, it was requested to split this into a separate issue.
17-11-2023