RFR: 8254263: Remove special_runtime_exit_condition() check from ~ThreadInVMForHandshake() [v2]

[Daniel D.Daugherty]

On Tue, 13 Oct 2020 17:48:26 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Hi all,
>> 
>> Please review the following patch that removes the call to handle_special_runtime_exit_condition() in
>> ~ThreadInVMForHandshake(). This check was added by 8223572 to prevent a JavaThread that was suspended while trying to
>> process a handshake to transition back to java without noticing it was suspended. Since 8238761, a JavaThread executing
>> HandshakeState::process_by_self() will never become safe. It comes from an unsafe state and remains unsafe, so it
>> cannot be suspended during that time. Removing this check also removes one of the reasons
>> SafepointMechanism::process_if_requested() is recursive (the other one remains SafepointSynchronize::block()). Tested
>> in mach5, tiers 1-7.  Thanks, Patricio
>
> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   added async exception check

I good with this change, but I do have some questions
buried in the code that you changed.

src/hotspot/share/runtime/safepoint.cpp line 947:

> 945: void ThreadSafepointState::handle_polling_page_exception() {
> 946:   JavaThread* self = thread();
> 947:   assert(self == Thread::current()->as_Java_thread(), "wrong thread");

Perhaps: s/wrong thread/must be self/  or  s/wrong thread/must be calling thread/

src/hotspot/share/runtime/safepoint.cpp line 992:

> 990:     // Process pending operation
> 991:     SafepointMechanism::process_if_requested(self);
> 992:     self->check_and_handle_async_exceptions();

I'm good with the addition of this call to check_and_handle_async_exceptions().

In particular because of this comment block in:

src/hotspot/share/runtime/thread.cpp:
void JavaThread::check_and_handle_async_exceptions(bool check_unsafe_error) {
  if (has_last_Java_frame() && has_async_condition()) {
    // If we are at a polling page safepoint (not a poll return)
    // then we must defer async exception because live registers
    // will be clobbered by the exception path. Poll return is
    // ok because the call we a returning from already collides
    // with exception handling registers and so there is no issue.
    // (The exception handling path kills call result registers but
    //  this is ok since the exception kills the result anyway).

so check_and_handle_async_exceptions() clearly was expecting to
be called when at a " poll return" which is the block that contains
this new call. That's all good!

What I can't quite figure out is how we "lost" the call to
check_and_handle_async_exceptions() that must have been
made somehow before. Clearly that function was expecting
to be called from a "poll return" site and that's what you're
putting back, but how was that call made before and what
change got rid of it?

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

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/577