There are multiple threads that can end up calling clearStep(), so we need to be careful about races. There are two main paths into clearStep.
- The first is through stepControl_endStep. It always grabs the handler_lock and stepControl_lock.
- The 2nd is through through stepControl_clearRequest, which grabs no locks itself, but there are some already grabbed when it is called. It should really grab the stepControl_lock, but it can't because that lock is suppose to be grabbed before threadControl_lock, which has already been grabbed. The proper order is handler_lock -> stepControl_lock -> threadControl_lock. So we need to fix locking for stepControl_clearRequest somehow.
There are 3 paths into stepControl_clearRequest().
- The first goes through clearThread via removeThread via removeResumed. This path always holds the handler_lock and threadControl_lock.
- The 2nd goes through clearThread via removeThread via threadControl_onEventHandlerExit. This also holds the handler_lock and threadControl_lock.
- The 3rd goes through clearThread via removeVThreads via threadControl_reset. This path always holds the handler_lock, but not threadControl_lock
Holding the threadControl_lock here does not help us since stepControl_endStep does not grab that lock. Holding the eventHandler_lock does help since stepControl_endStep does grab it, but the 3rd path above does not grab the eventHandler_lock. I think the simplest fix here is to just make it do so. But that's using the eventHandler_lock to make stepControl code thread safe, when stepControl_lock should be used. Not exactly clean.
So that means we need to require the caller of stepControl_endStep to do all the needed locking first. Basically add locking of stepControl_lock somewhere. Adding locking of stepControl_lock in clearThread is too late since we already hold the threadLock. It really needs to be pushed up the call chain of each of the above 3 paths to the point just before grabbing the threadLock. For example for the 2nd call path above we have:
if (ei == EI_THREAD_END) {
eventHandler_lock(); /* for proper lock order - see removeThread() call below */
debugMonitorEnter(threadLock);
So we need to add stepControl_lock() in between those too lines. We need to do similar for the 1st and 3rd paths also.
It would be good to resolve this as part of or just after the rank locking support is done (JDK-8328866) since this is very lock order sensitive code, and the rank locking support will help detect errors.
- The first is through stepControl_endStep. It always grabs the handler_lock and stepControl_lock.
- The 2nd is through through stepControl_clearRequest, which grabs no locks itself, but there are some already grabbed when it is called. It should really grab the stepControl_lock, but it can't because that lock is suppose to be grabbed before threadControl_lock, which has already been grabbed. The proper order is handler_lock -> stepControl_lock -> threadControl_lock. So we need to fix locking for stepControl_clearRequest somehow.
There are 3 paths into stepControl_clearRequest().
- The first goes through clearThread via removeThread via removeResumed. This path always holds the handler_lock and threadControl_lock.
- The 2nd goes through clearThread via removeThread via threadControl_onEventHandlerExit. This also holds the handler_lock and threadControl_lock.
- The 3rd goes through clearThread via removeVThreads via threadControl_reset. This path always holds the handler_lock, but not threadControl_lock
Holding the threadControl_lock here does not help us since stepControl_endStep does not grab that lock. Holding the eventHandler_lock does help since stepControl_endStep does grab it, but the 3rd path above does not grab the eventHandler_lock. I think the simplest fix here is to just make it do so. But that's using the eventHandler_lock to make stepControl code thread safe, when stepControl_lock should be used. Not exactly clean.
So that means we need to require the caller of stepControl_endStep to do all the needed locking first. Basically add locking of stepControl_lock somewhere. Adding locking of stepControl_lock in clearThread is too late since we already hold the threadLock. It really needs to be pushed up the call chain of each of the above 3 paths to the point just before grabbing the threadLock. For example for the 2nd call path above we have:
if (ei == EI_THREAD_END) {
eventHandler_lock(); /* for proper lock order - see removeThread() call below */
debugMonitorEnter(threadLock);
So we need to add stepControl_lock() in between those too lines. We need to do similar for the 1st and 3rd paths also.
It would be good to resolve this as part of or just after the rank locking support is done (JDK-8328866) since this is very lock order sensitive code, and the rank locking support will help detect errors.
- relates to
-
JDK-8229012 When single stepping, the debug agent can cause the thread to remain in interpreter mode after single stepping completes
-
- Resolved
-
-
JDK-8328866 Add raw monitor rank support to the debug agent
-
- In Progress
-