Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274379: Allow process of unsafe access errors in check_special_condition_for_native_trans #5741

Closed
wants to merge 6 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -2961,7 +2961,7 @@ JVM_ENTRY(void, JVM_StopThread(JNIEnv* env, jobject jthread, jobject throwable))
THROW_OOP(java_throwable);
} else {
// Use a VM_Operation to throw the exception.
JavaThread::send_async_exception(java_thread, java_throwable);
JavaThread::send_async_exception(receiver, java_throwable);
}
} else {
// Either:
@@ -1072,7 +1072,7 @@ JvmtiEnv::StopThread(JavaThread* java_thread, jobject exception) {
oop e = JNIHandles::resolve_external_guard(exception);
NULL_CHECK(e, JVMTI_ERROR_NULL_POINTER);

JavaThread::send_async_exception(java_thread->threadObj(), e);
JavaThread::send_async_exception(java_thread, e);

return JVMTI_ERROR_NONE;

@@ -131,7 +131,6 @@ class SafepointSynchronize : AllStatic {
switch(state) {
case _thread_in_vm:
case _thread_in_Java: // From compiled code
case _thread_in_native_trans:
case _thread_blocked_trans:
return true;
default:
@@ -1001,8 +1001,10 @@ JavaThread::JavaThread() :
_monitor_chunks(nullptr),

_suspend_flags(0),
_async_exception_condition(_no_async_condition),
_pending_async_exception(nullptr),
#ifdef ASSERT
_is_unsafe_access_error(false),
#endif

_thread_state(_thread_new),
_saved_exception_pc(nullptr),
@@ -1572,9 +1574,6 @@ void JavaThread::remove_monitor_chunk(MonitorChunk* chunk) {

// Asynchronous exceptions support
//
// Note: this function shouldn't block if it's called in
// _thread_in_native_trans state (such as from
// check_special_condition_for_native_trans()).
void JavaThread::check_and_handle_async_exceptions() {
if (has_last_Java_frame() && has_async_exception_condition()) {
// If we are at a polling page safepoint (not a poll return)
@@ -1600,21 +1599,12 @@ void JavaThread::check_and_handle_async_exceptions() {
}
}

AsyncExceptionCondition condition = clear_async_exception_condition();
if (condition == _no_async_condition) {
// Conditions have changed since has_special_runtime_exit_condition()
// was called:
// - if we were here only because of an external suspend request,
// then that was taken care of above (or cancelled) so we are done
// - if we were here because of another async request, then it has
// been cleared between the has_special_runtime_exit_condition()
// and now so again we are done
if (!clear_async_exception_condition()) {
return;
}

// Check for pending async. exception
if (_pending_async_exception != NULL) {
// Only overwrite an already pending exception, if it is not a threadDeath.
// Only overwrite an already pending exception if it is not a threadDeath.
if (!has_pending_exception() || !pending_exception()->is_a(vmClasses::ThreadDeath_klass())) {

// We cannot call Exceptions::_throw(...) here because we cannot block
@@ -1631,38 +1621,35 @@ void JavaThread::check_and_handle_async_exceptions() {
}
ls.print_cr(" of type: %s", _pending_async_exception->klass()->external_name());
}
_pending_async_exception = NULL;
// Clear condition from _suspend_flags since we have finished processing it.
clear_suspend_flag(_has_async_exception);
}
}
// Always null out the _pending_async_exception oop here since the async condition was
// already cleared above and thus considered handled.
_pending_async_exception = NULL;
} else {
assert(_is_unsafe_access_error, "must be");
DEBUG_ONLY(_is_unsafe_access_error = false);

if (condition == _async_unsafe_access_error && !has_pending_exception()) {
// We may be at method entry which requires we save the do-not-unlock flag.
UnlockFlagSaver fs(this);
switch (thread_state()) {
case _thread_in_vm: {
JavaThread* THREAD = this;
Exceptions::throw_unsafe_access_internal_error(THREAD, __FILE__, __LINE__, "a fault occurred in an unsafe memory access operation");
return;
}
case _thread_in_native: {
ThreadInVMfromNative tiv(this);
JavaThread* THREAD = this;
Exceptions::throw_unsafe_access_internal_error(THREAD, __FILE__, __LINE__, "a fault occurred in an unsafe memory access operation");
return;
// We might have blocked in a ThreadBlockInVM wrapper in the call above so make sure we process pending
// suspend requests and object reallocation operations if any since we might be going to Java after this.
SafepointMechanism::process_if_requested_with_exit_check(this, true /* check asyncs */);
Copy link
Member

@dholmes-ora dholmes-ora Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to wonder why this isn't handled in the TBIVM?

Copy link
Contributor Author

@pchilano pchilano Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 8270085 we had to disable by default processing of suspend requests due to possible deadlocks. So although suspend requests are implemented with handshakes, calls to process_if_requested() from ~TBIVM will not process them. As for object reallocation I'm not sure but I think it's just because there is no need to process them until we are about to go back to Java.

break;
Copy link
Member

@dholmes-ora dholmes-ora Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than now including the final assertion to be executed, I don't see why we need to change from "return" to "break". That final assertion seems unnecessary as it basically checks that Exceptions::throw... actually threw. I'd be tempted to just delete the final assertion and keep the return statements. But not a major concern either way.

Copy link
Contributor Author

@pchilano pchilano Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
case _thread_in_Java: {
ThreadInVMfromJava tiv(this);
JavaThread* THREAD = this;
Exceptions::throw_unsafe_access_internal_error(THREAD, __FILE__, __LINE__, "a fault occurred in an unsafe memory access operation in compiled Java code");
return;
break;
}
default:
ShouldNotReachHere();
}
}

assert(has_pending_exception(), "must have handled the async condition if no exception");
}

@@ -1696,9 +1683,8 @@ class InstallAsyncExceptionClosure : public HandshakeClosure {
}
};

void JavaThread::send_async_exception(oop java_thread, oop java_throwable) {
void JavaThread::send_async_exception(JavaThread* target, oop java_throwable) {
Handle throwable(Thread::current(), java_throwable);
JavaThread* target = java_lang_Thread::thread(java_thread);
InstallAsyncExceptionClosure vm_stop(throwable);
Handshake::execute(&vm_stop, target);
}
@@ -1732,7 +1718,8 @@ void JavaThread::send_thread_stop(oop java_throwable) {
}

// Set async. pending exception in thread.
set_pending_async_exception(java_throwable);
_pending_async_exception = java_throwable;
set_suspend_flag(_has_async_exception);
Copy link
Member

@dholmes-ora dholmes-ora Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear why we didn't just simplify set_pending_async_exception.

Copy link
Contributor Author

@pchilano pchilano Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, maybe it's better to do that. I restored set_pending_async_exception() and simplified it instead.


if (log_is_enabled(Info, exceptions)) {
ResourceMark rm;
@@ -1847,21 +1834,17 @@ void JavaThread::check_special_condition_for_native_trans(JavaThread *thread) {
assert(thread->thread_state() == _thread_in_native_trans, "wrong state");
assert(!thread->has_last_Java_frame() || thread->frame_anchor()->walkable(), "Unwalkable stack in native->Java transition");

thread->set_thread_state(_thread_in_vm);
Copy link
Member

@dholmes-ora dholmes-ora Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems odd to change the thread-state explicitly here when this logic is being processed in the middle of a transition from native->java. So what we end up doing is native->native_trans->in_vm->in_java (and possibly some additional changes from in_vm to blocked and back). I understand why we need to be "in vm" before calling process_if_requested... but it makes the thread state transition story rather messy (just imagine trying to draw a state machine model for this :) ).

Copy link
Contributor Author

@pchilano pchilano Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we call JavaThread::check_special_condition_for_native_trans() we are calling into the vm so to me it actually makes more sense to set the state to _thread_in_vm there. Specially given that this is not some leaf method, but we actually process safepoint/handshakes/stackwatermarks and all other operations we check for in handle_special_runtime_exit_condition().
Staying in native_trans also doesn't simplify the state machine in my opinion and actually makes it more complex because inside process_if_requested_with_exit_check() we have to keep track of all these states different than _thread_in_vm: switch case with valid states for safepoint code, transition wrapper for state bookkeeping in handshake code (so we still have the native_trans->vm change), and having to manually switch to _thread_blocked and back because we cannot use ThreadBlockInVM in wait_for_object_deoptimization().

Note: As a future change I think we could just avoid the native_trans state altogether and just poll in a state of _thread_in_vm, since the only purpose of it is to poll in an unsafe state. Or we could even set the state to _thread_in_Java already, so that when there is nothing to process there are no extra changes of state needed, and when there are pending operations we can use a ThreadInVMfromJava wrapper here.


// Enable WXWrite: called directly from interpreter native wrapper.
MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));

SafepointMechanism::process_if_requested_with_exit_check(thread, false /* check asyncs */);
SafepointMechanism::process_if_requested_with_exit_check(thread, true /* check asyncs */);

// After returning from native, it could be that the stack frames are not
// yet safe to use. We catch such situations in the subsequent stack watermark
// barrier, which will trap unsafe stack frames.
StackWatermarkSet::before_unwind(thread);

if (thread->has_async_exception_condition(false /* check unsafe access error */)) {
// We are in _thread_in_native_trans state, don't handle unsafe
// access error since that may block.
thread->check_and_handle_async_exceptions();
}
}

#ifndef PRODUCT
@@ -802,37 +802,20 @@ class JavaThread: public Thread {

// Asynchronous exceptions support
private:
enum AsyncExceptionCondition {
_no_async_condition = 0,
_async_exception,
_async_unsafe_access_error
};
AsyncExceptionCondition _async_exception_condition;
oop _pending_async_exception;

void set_async_exception_condition(AsyncExceptionCondition aec) { _async_exception_condition = aec; }
AsyncExceptionCondition clear_async_exception_condition() {
AsyncExceptionCondition x = _async_exception_condition;
_async_exception_condition = _no_async_condition;
return x;
}
oop _pending_async_exception;
#ifdef ASSERT
bool _is_unsafe_access_error;
#endif

inline bool clear_async_exception_condition();
public:
bool has_async_exception_condition(bool check_unsafe_access_error = true) {
return check_unsafe_access_error ? _async_exception_condition != _no_async_condition
: _async_exception_condition == _async_exception;
}
inline void set_pending_async_exception(oop e);
void set_pending_unsafe_access_error() {
// Don't overwrite an asynchronous exception sent by another thread
if (_async_exception_condition == _no_async_condition) {
set_async_exception_condition(_async_unsafe_access_error);
}
bool has_async_exception_condition() {
return (_suspend_flags & _has_async_exception) != 0;
}
void check_and_handle_async_exceptions();
// Installs a pending exception to be inserted later
static void send_async_exception(oop thread_oop, oop java_throwable);
inline void set_pending_unsafe_access_error();
static void send_async_exception(JavaThread* jt, oop java_throwable);
void send_thread_stop(oop throwable);
void check_and_handle_async_exceptions();

// Safepoint support
public: // Expose _thread_state for SafeFetchInt()
@@ -1177,8 +1160,7 @@ class JavaThread: public Thread {
// Return true if JavaThread has an asynchronous condition or
// if external suspension is requested.
bool has_special_runtime_exit_condition() {
return (_async_exception_condition != _no_async_condition) ||
(_suspend_flags & (_obj_deopt JFR_ONLY(| _trace_flag))) != 0;
return (_suspend_flags & (_has_async_exception | _obj_deopt JFR_ONLY(| _trace_flag))) != 0;
}

// Fast-locking support
@@ -122,13 +122,15 @@ inline void JavaThread::clear_obj_deopt_flag() {
clear_suspend_flag(_obj_deopt);
}

inline void JavaThread::set_pending_async_exception(oop e) {
_pending_async_exception = e;
set_async_exception_condition(_async_exception);
// Set _suspend_flags too so we save a comparison in the transition from native to Java
// in the native wrappers. It will be cleared in check_and_handle_async_exceptions()
// when we actually install the exception.
inline bool JavaThread::clear_async_exception_condition() {
bool ret = has_async_exception_condition();
clear_suspend_flag(_has_async_exception);
return ret;
}

inline void JavaThread::set_pending_unsafe_access_error() {
set_suspend_flag(_has_async_exception);
DEBUG_ONLY(_is_unsafe_access_error = true);
}

inline JavaThreadState JavaThread::thread_state() const {
@@ -717,7 +717,6 @@
nonstatic_field(JavaThread, _current_pending_monitor_is_from_java, bool) \
volatile_nonstatic_field(JavaThread, _current_waiting_monitor, ObjectMonitor*) \
volatile_nonstatic_field(JavaThread, _suspend_flags, uint32_t) \
nonstatic_field(JavaThread, _async_exception_condition, JavaThread::AsyncExceptionCondition) \
nonstatic_field(JavaThread, _pending_async_exception, oop) \
volatile_nonstatic_field(JavaThread, _exception_oop, oop) \
volatile_nonstatic_field(JavaThread, _exception_pc, address) \
@@ -1951,7 +1950,6 @@
declare_toplevel_type(JavaThread*) \
declare_toplevel_type(JavaThread *const *const) \
declare_toplevel_type(java_lang_Class) \
declare_integer_type(JavaThread::AsyncExceptionCondition) \
declare_integer_type(JavaThread::TerminatedTypes) \
declare_toplevel_type(jbyte*) \
declare_toplevel_type(jbyte**) \