RFR: 8305625: Stress test crashes with SEGV in Deoptimization::deoptimize_frame_internal(JavaThread*, long*, Deoptimization::DeoptReason)

[Robbin Ehn]

On Wed, 12 Apr 2023 15:48:53 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review this fix. The check to skip walking stacks of virtual threads will not identify a thread in a transition since it relies on the jvmti_vthread() which would have already changed at the very beginning of it. The crash happens because the anchor might have changed between walking the stack of the thread in a transition and executing the deopt handshake for a particular frame. The frame is never found and looping executing fr.sender() crashes. This scenario can happen if the initial EscapeBarrierSuspendHandshake executed to synchronize against all threads finds the thread blocked in the stackchunk allocation path. Because the thread will actually block on the next transition to Java, and not on a blocked->vm transition, it will continue executing and change its anchor while the requester is walking its stack. There are more details in the bug comments.
> The fix modifies the conditional to check if the continuation is mounted or not. This will identify the transition case too and won't face the anchor change issue since the continuation entry will be removed after returning from the freeze call.
> The fix was tested against a reproducer which I attached to the bug.
> 
> Thanks,
> Patricio

Thanks, seems good!

But question, I believe the query is wrong.
We actually don't care if it's a virtual thread or if it is a plain continuation, right?
It's a bit wired since some of the code is 'prepared' for plain continuations, while some is not.

The query does:

const ContinuationEntry* JavaThread::vthread_continuation() const {                                                   
  for (ContinuationEntry* c = last_continuation(); c != nullptr; c = c->parent()) {                                   
    if (c->is_virtual_thread())                                                  
      return c;                                                                 
  }                                                                                                                   
  return nullptr;                                                                                                     
}


But if we had a plain continuation, the same bug would happen AFAICT.

So I would like the question to be jt->have_continuation_mounted().

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

Marked as reviewed by rehn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13446#pullrequestreview-1386022701