On Thu, 27 May 2021 09:56:22 GMT, Martin Doerr <[email protected]> wrote:
> We need a fix for crashes in get_current_contended_monitor due to concurrent
> modification of memory locations which are not declared volatile. See bug for
> details.
Thumbs up.
The code that you're fixing is indeed a special case and I wrote the new test
(serviceability/monitoring/ThreadInfo/GetLockOwnerName/GetLockOwnerName.java)
specifically to stress the crashing code path. Unfortunately, I have never seen
the
new test fail in our test in Mach5 or in my testing in my lab. We did get a
single closed
test failure back on 2021.03.20 which is what started me down the path of
creating
the new test. That single failure has never reproduced in the original closed
test nor
in the targeted test that I wrote (GetLockOwnerName.java).
The similar usage in JVM/TI is safe for exactly the reasons explained in the
comment so a
general Atomic::load() solution in current_waiting_monitor() or
current_pending_monitor()
is not necessary.
I think your solution of adding `volatile` to `wait_obj` and `enter_obj` is a
good solution.
I would like to see a comment added to explain the need for the `volatile`.
I'll add an
embedded comment in the other PR view.
Using either `wait_obj` or `enter_obj` after it has been cleared from the
JavaThread is
safe. The ObjectMonitor can only have gone through the first stage of async
deflation.
The ObjectMonitor's memory cannot be freed until after a handshake with all
threads
is completed and that cannot happen while this thread is executing the code
that is
using `wait_obj` or `enter_obj`.
src/hotspot/share/services/threadService.cpp line 232:
> 230: // ObjectMonitor by the time this object reference is processed
> 231: // by the caller.
> 232: ObjectMonitor* volatile wait_obj = thread->current_waiting_monitor();
Please add a comment above this declaration. Something like:
// Using 'volatile' to prevent the compiler from generating code that
// reloads 'wait_obj' from memory when used below.
src/hotspot/share/services/threadService.cpp line 239:
> 237: obj = wait_obj->object();
> 238: } else {
> 239: ObjectMonitor* volatile enter_obj =
> thread->current_pending_monitor();
Please add a comment above this declaration. Something like:
// Using 'volatile' to prevent the compiler from generating code that
// reloads 'enter_obj' from memory when used below.
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4224
