RFR: 8298371: monitors_on_stack extracts unprocessed oops

[Patricio Chilano Mateo]

On Thu, 8 Dec 2022 10:27:31 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> While investigating JDK-8298058 we've found that monitors_on_stack extracts oops without setting up the RegisterMap to process oops. This then leaks out stale oops.
> 
> A simple fix is to change the RegisterMap to perform oops processing. However, monitors_on_stack is only used in an assert, so this means that we'll get a difference in behavior between release builds and debug builds. This has the potential to hide bugs in debug builds. It has been suggested to me that it might be better to simply remove the assert:
> 
>   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());
> 
> 
> The suggested patch just turns on processing, but I'm also fine with removing the assert if that seems to be a better solution.
> 
> Tested with reproducer using Fuzz.java + ZGC

Looks good. Although I think I would rather avoid the possible hiding of bugs in debug builds issue. Maybe better instead of just removing it completely would be to move the assert after the flush_stack_processing() call which already processes the frames anyways. That would cover the successful freeze cases but not the pinned ones.
Another possibility would be for monitors_on_stack() to just return the same expected held_monitor_count() for ZGC, i.e. skip it. If we have a locking mismatch bug it would likely be independent of which GC we are using so we would still be checking it for the other GCs.

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

Marked as reviewed by pchilanomate (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11582