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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f01f3dd
v1
pchilano Sep 28, 2021
922462f
revert to separate commits
pchilano Oct 5, 2021
79e7454
allow process of unsafe access errors in check_special_condition_for_…
pchilano Oct 5, 2021
4b1eb04
general cleanup
pchilano Oct 5, 2021
1dbbca9
async API cleanup
pchilano Oct 5, 2021
2d4bace
address David comments
pchilano Oct 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/hotspot/share/prims/jvm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/prims/jvmtiEnv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/runtime/safepoint.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
55 changes: 18 additions & 37 deletions src/hotspot/share/runtime/thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -1631,25 +1621,23 @@ 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");
// 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 */);
Comment on lines +1638 to +1640
Copy link
Member

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

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.

return;
}
case _thread_in_Java: {
Expand All @@ -1662,8 +1650,6 @@ void JavaThread::check_and_handle_async_exceptions() {
ShouldNotReachHere();
}
}

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

void JavaThread::handle_special_runtime_exit_condition(bool check_asyncs) {
Expand Down Expand Up @@ -1696,9 +1682,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);
}
Expand Down Expand Up @@ -1847,21 +1832,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

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

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
Expand Down
39 changes: 11 additions & 28 deletions src/hotspot/share/runtime/thread.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,37 +802,21 @@ 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;
bool has_async_exception_condition() {
return (_suspend_flags & _has_async_exception) != 0;
}
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);
}
}
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()
Expand Down Expand Up @@ -1177,8 +1161,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
Expand Down
15 changes: 11 additions & 4 deletions src/hotspot/share/runtime/thread.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,22 @@ inline void JavaThread::clear_obj_deopt_flag() {
clear_suspend_flag(_obj_deopt);
}

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_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.
set_suspend_flag(_has_async_exception);
}

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 {
#if defined(PPC64) || defined (AARCH64)
// Use membars when accessing volatile _thread_state. See
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/runtime/vmStructs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down Expand Up @@ -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**) \
Expand Down