Revert^4 "Remove Global deopt requirement for several jvmti events"
It was possible for the top frame of a thread to enter the
artQuickToInterpreterBridge during runtime shutdown. This could be
caused by the libjdwp agent. If this happens the caller of the frame
will be 'null', which could cause a segv if not detected correctly.
This reverts commit 939798e6a565a92e597136c589428e89c28bffd5.
Reason for revert: Fixed issue that could cause crash during process
shutdown.
Test: ./art/tools/run-libjdwp-tests.sh --mode=host
Test: ./test.py --host
Change-Id: I2aad1705c761edb4ed788cec4fc8a3068d67aee5
diff --git a/openjdkjvmti/OpenjdkJvmTi.cc b/openjdkjvmti/OpenjdkJvmTi.cc
index 7a2b638..39e49d7 100644
--- a/openjdkjvmti/OpenjdkJvmTi.cc
+++ b/openjdkjvmti/OpenjdkJvmTi.cc
@@ -1053,22 +1053,9 @@
jthread event_thread,
...) {
ENSURE_VALID_ENV(env);
- art::Thread* art_thread = nullptr;
- if (event_thread != nullptr) {
- // TODO The locking around this call is less then what we really want.
- art::ScopedObjectAccess soa(art::Thread::Current());
- art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
- jvmtiError err = ERR(INTERNAL);
- if (!ThreadUtil::GetAliveNativeThread(event_thread, soa, &art_thread, &err)) {
- return err;
- } else if (art_thread->IsStillStarting()) {
- return ERR(THREAD_NOT_ALIVE);
- }
- }
-
ArtJvmTiEnv* art_env = ArtJvmTiEnv::AsArtJvmTiEnv(env);
return gEventHandler->SetEvent(art_env,
- art_thread,
+ event_thread,
GetArtJvmtiEvent(art_env, event_type),
mode);
}
diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index d456d83..ee77b7b 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -49,6 +49,7 @@
#include "nativehelper/scoped_local_ref.h"
#include "runtime_callbacks.h"
#include "scoped_thread_state_change-inl.h"
+#include "scoped_thread_state_change.h"
#include "thread-current-inl.h"
#include "thread_list.h"
#include "ti_phase.h"
@@ -356,6 +357,47 @@
kDeoptManagerInstrumentationKey);
}
+jvmtiError DeoptManager::AddDeoptimizeThreadMethods(art::ScopedObjectAccessUnchecked& soa, jthread jtarget) {
+ art::Locks::thread_list_lock_->ExclusiveLock(soa.Self());
+ art::Thread* target = nullptr;
+ jvmtiError err = OK;
+ if (!ThreadUtil::GetNativeThread(jtarget, soa, &target, &err)) {
+ art::Locks::thread_list_lock_->ExclusiveUnlock(soa.Self());
+ return err;
+ }
+ // We don't need additional locking here because we hold the Thread_list_lock_.
+ target->SetForceInterpreterCount(target->ForceInterpreterCount() + 1);
+ if (target->ForceInterpreterCount() == 1) {
+ struct DeoptClosure : public art::Closure {
+ public:
+ explicit DeoptClosure(DeoptManager* man) : man_(man) {}
+ void Run(art::Thread* self) override REQUIRES_SHARED(art::Locks::mutator_lock_) {
+ man_->DeoptimizeThread(self);
+ }
+
+ private:
+ DeoptManager* man_;
+ };
+ DeoptClosure c(this);
+ target->RequestSynchronousCheckpoint(&c);
+ } else {
+ art::Locks::thread_list_lock_->ExclusiveUnlock(soa.Self());
+ }
+ return OK;
+}
+
+jvmtiError DeoptManager::RemoveDeoptimizeThreadMethods(art::ScopedObjectAccessUnchecked& soa, jthread jtarget) {
+ art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
+ art::Thread* target = nullptr;
+ jvmtiError err = OK;
+ if (!ThreadUtil::GetNativeThread(jtarget, soa, &target, &err)) {
+ return err;
+ }
+ // We don't need additional locking here because we hold the Thread_list_lock_.
+ DCHECK_GT(target->ForceInterpreterCount(), 0u);
+ target->DecrementForceInterpreterCount();
+ return OK;
+}
void DeoptManager::RemoveDeoptimizationRequester() {
art::Thread* self = art::Thread::Current();
diff --git a/openjdkjvmti/deopt_manager.h b/openjdkjvmti/deopt_manager.h
index 856f3f4..4c4a774 100644
--- a/openjdkjvmti/deopt_manager.h
+++ b/openjdkjvmti/deopt_manager.h
@@ -38,8 +38,11 @@
#include "base/mutex.h"
#include "runtime_callbacks.h"
+#include <jvmti.h>
+
namespace art {
class ArtMethod;
+class ScopedObjectAccessUnchecked;
namespace mirror {
class Class;
} // namespace mirror
@@ -98,6 +101,14 @@
REQUIRES(!deoptimization_status_lock_, !art::Roles::uninterruptible_)
REQUIRES_SHARED(art::Locks::mutator_lock_);
+ jvmtiError AddDeoptimizeThreadMethods(art::ScopedObjectAccessUnchecked& soa, jthread thread)
+ REQUIRES(!deoptimization_status_lock_, !art::Roles::uninterruptible_)
+ REQUIRES_SHARED(art::Locks::mutator_lock_);
+
+ jvmtiError RemoveDeoptimizeThreadMethods(art::ScopedObjectAccessUnchecked& soa, jthread thread)
+ REQUIRES(!deoptimization_status_lock_, !art::Roles::uninterruptible_)
+ REQUIRES_SHARED(art::Locks::mutator_lock_);
+
void DeoptimizeThread(art::Thread* target) REQUIRES_SHARED(art::Locks::mutator_lock_);
void DeoptimizeAllThreads() REQUIRES_SHARED(art::Locks::mutator_lock_);
diff --git a/openjdkjvmti/events.cc b/openjdkjvmti/events.cc
index 22c622a..b41edc5 100644
--- a/openjdkjvmti/events.cc
+++ b/openjdkjvmti/events.cc
@@ -794,6 +794,12 @@
void WatchedFramePop(art::Thread* self, const art::ShadowFrame& frame)
REQUIRES_SHARED(art::Locks::mutator_lock_) override {
art::JNIEnvExt* jnienv = self->GetJniEnv();
+ // Remove the force-interpreter added by the WatchFrame.
+ {
+ art::MutexLock mu(self, *art::Locks::thread_list_lock_);
+ CHECK_GT(self->ForceInterpreterCount(), 0u);
+ self->DecrementForceInterpreterCount();
+ }
jboolean is_exception_pending = self->IsExceptionPending();
RunEventCallback<ArtJvmtiEvent::kFramePop>(
event_handler_,
@@ -965,45 +971,78 @@
}
}
-static bool EventNeedsFullDeopt(ArtJvmtiEvent event) {
+enum class DeoptRequirement {
+ // Limited/no deopt required.
+ kLimited,
+ // A single thread must be put into interpret only.
+ kThread,
+ // All methods and all threads deopted.
+ kFull,
+};
+
+static DeoptRequirement GetDeoptRequirement(ArtJvmtiEvent event, jthread thread) {
switch (event) {
case ArtJvmtiEvent::kBreakpoint:
case ArtJvmtiEvent::kException:
- return false;
- // TODO We should support more of these or at least do something to make them discriminate by
- // thread.
+ return DeoptRequirement::kLimited;
+ // TODO MethodEntry is needed due to inconsistencies between the interpreter and the trampoline
+ // in how to handle exceptions.
case ArtJvmtiEvent::kMethodEntry:
case ArtJvmtiEvent::kExceptionCatch:
+ return DeoptRequirement::kFull;
case ArtJvmtiEvent::kMethodExit:
case ArtJvmtiEvent::kFieldModification:
case ArtJvmtiEvent::kFieldAccess:
case ArtJvmtiEvent::kSingleStep:
case ArtJvmtiEvent::kFramePop:
- return true;
+ return thread == nullptr ? DeoptRequirement::kFull : DeoptRequirement::kThread;
default:
LOG(FATAL) << "Unexpected event type!";
UNREACHABLE();
}
}
-void EventHandler::SetupTraceListener(JvmtiMethodTraceListener* listener,
- ArtJvmtiEvent event,
- bool enable) {
- bool needs_full_deopt = EventNeedsFullDeopt(event);
+jvmtiError EventHandler::SetupTraceListener(JvmtiMethodTraceListener* listener,
+ ArtJvmtiEvent event,
+ jthread thread,
+ bool enable) {
+ DeoptRequirement deopt_req = GetDeoptRequirement(event, thread);
// Make sure we can deopt.
{
art::ScopedObjectAccess soa(art::Thread::Current());
DeoptManager* deopt_manager = DeoptManager::Get();
+ jvmtiError err = OK;
if (enable) {
deopt_manager->AddDeoptimizationRequester();
- if (needs_full_deopt) {
- deopt_manager->AddDeoptimizeAllMethods();
+ switch (deopt_req) {
+ case DeoptRequirement::kFull:
+ deopt_manager->AddDeoptimizeAllMethods();
+ break;
+ case DeoptRequirement::kThread:
+ err = deopt_manager->AddDeoptimizeThreadMethods(soa, thread);
+ break;
+ default:
+ break;
+ }
+ if (err != OK) {
+ deopt_manager->RemoveDeoptimizationRequester();
+ return err;
}
} else {
- if (needs_full_deopt) {
- deopt_manager->RemoveDeoptimizeAllMethods();
+ switch (deopt_req) {
+ case DeoptRequirement::kFull:
+ deopt_manager->RemoveDeoptimizeAllMethods();
+ break;
+ case DeoptRequirement::kThread:
+ err = deopt_manager->RemoveDeoptimizeThreadMethods(soa, thread);
+ break;
+ default:
+ break;
}
deopt_manager->RemoveDeoptimizationRequester();
+ if (err != OK) {
+ return err;
+ }
}
}
@@ -1019,7 +1058,7 @@
if (IsEventEnabledAnywhere(other)) {
// The event needs to be kept around/is already enabled by the other jvmti event that uses the
// same instrumentation event.
- return;
+ return OK;
}
}
art::ScopedThreadStateChange stsc(art::Thread::Current(), art::ThreadState::kNative);
@@ -1030,6 +1069,7 @@
} else {
instr->RemoveListener(listener, new_events);
}
+ return OK;
}
// Makes sure that all compiled methods are AsyncDeoptimizable so we can deoptimize (and force to
@@ -1085,41 +1125,42 @@
return false;
}
-void EventHandler::SetupFramePopTraceListener(bool enable) {
+jvmtiError EventHandler::SetupFramePopTraceListener(jthread thread, bool enable) {
if (enable) {
frame_pop_enabled = true;
- SetupTraceListener(method_trace_listener_.get(), ArtJvmtiEvent::kFramePop, enable);
+ return SetupTraceListener(
+ method_trace_listener_.get(), ArtJvmtiEvent::kFramePop, thread, enable);
} else {
// remove the listener if we have no outstanding frames.
{
art::ReaderMutexLock mu(art::Thread::Current(), envs_lock_);
- for (ArtJvmTiEnv* env : envs) {
+ for (ArtJvmTiEnv *env : envs) {
art::ReaderMutexLock event_mu(art::Thread::Current(), env->event_info_mutex_);
if (!env->notify_frames.empty()) {
// Leaving FramePop listener since there are unsent FramePop events.
- return;
+ return OK;
}
}
frame_pop_enabled = false;
}
- SetupTraceListener(method_trace_listener_.get(), ArtJvmtiEvent::kFramePop, enable);
+ return SetupTraceListener(
+ method_trace_listener_.get(), ArtJvmtiEvent::kFramePop, thread, enable);
}
}
// Handle special work for the given event type, if necessary.
-void EventHandler::HandleEventType(ArtJvmtiEvent event, bool enable) {
+jvmtiError EventHandler::HandleEventType(ArtJvmtiEvent event, jthread thread, bool enable) {
switch (event) {
case ArtJvmtiEvent::kDdmPublishChunk:
SetupDdmTracking(ddm_listener_.get(), enable);
- return;
+ return OK;
case ArtJvmtiEvent::kVmObjectAlloc:
SetupObjectAllocationTracking(alloc_listener_.get(), enable);
- return;
-
+ return OK;
case ArtJvmtiEvent::kGarbageCollectionStart:
case ArtJvmtiEvent::kGarbageCollectionFinish:
SetupGcPauseTracking(gc_pause_listener_.get(), event, enable);
- return;
+ return OK;
// FramePop can never be disabled once it's been turned on if it was turned off with outstanding
// pop-events since we would either need to deal with dangling pointers or have missed events.
case ArtJvmtiEvent::kFramePop:
@@ -1127,8 +1168,7 @@
// The frame-pop event was held on by pending events so we don't need to do anything.
break;
} else {
- SetupFramePopTraceListener(enable);
- break;
+ return SetupFramePopTraceListener(thread, enable);
}
case ArtJvmtiEvent::kMethodEntry:
case ArtJvmtiEvent::kMethodExit:
@@ -1138,8 +1178,7 @@
case ArtJvmtiEvent::kExceptionCatch:
case ArtJvmtiEvent::kBreakpoint:
case ArtJvmtiEvent::kSingleStep:
- SetupTraceListener(method_trace_listener_.get(), event, enable);
- return;
+ return SetupTraceListener(method_trace_listener_.get(), event, thread, enable);
case ArtJvmtiEvent::kMonitorContendedEnter:
case ArtJvmtiEvent::kMonitorContendedEntered:
case ArtJvmtiEvent::kMonitorWait:
@@ -1147,10 +1186,11 @@
if (!OtherMonitorEventsEnabledAnywhere(event)) {
SetupMonitorListener(monitor_listener_.get(), park_listener_.get(), enable);
}
- return;
+ return OK;
default:
break;
}
+ return OK;
}
// Checks to see if the env has the capabilities associated with the given event.
@@ -1212,21 +1252,9 @@
}
jvmtiError EventHandler::SetEvent(ArtJvmTiEnv* env,
- art::Thread* thread,
+ jthread thread,
ArtJvmtiEvent event,
jvmtiEventMode mode) {
- if (thread != nullptr) {
- art::ThreadState state = thread->GetState();
- if (state == art::ThreadState::kStarting ||
- state == art::ThreadState::kTerminated ||
- thread->IsStillStarting()) {
- return ERR(THREAD_NOT_ALIVE);
- }
- if (!IsThreadControllable(event)) {
- return ERR(ILLEGAL_ARGUMENT);
- }
- }
-
if (mode != JVMTI_ENABLE && mode != JVMTI_DISABLE) {
return ERR(ILLEGAL_ARGUMENT);
}
@@ -1239,6 +1267,28 @@
return ERR(MUST_POSSESS_CAPABILITY);
}
+ art::Thread* art_thread = nullptr;
+ if (thread != nullptr) {
+ if (!IsThreadControllable(event)) {
+ return ERR(ILLEGAL_ARGUMENT);
+ }
+ art::ScopedObjectAccess soa(art::Thread::Current());
+ art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &art_thread, &err)) {
+ return err;
+ } else if (art_thread->IsStillStarting()) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
+ art::ThreadState state = art_thread->GetState();
+ if (state == art::ThreadState::kStarting || state == art::ThreadState::kTerminated) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
+ }
+
+ // TODO We use art_thread simply as a global unique identifier here. It is not safe to actually
+ // use it without holding the thread_list_lock_.
+
bool old_state;
bool new_state;
@@ -1249,13 +1299,14 @@
art::WriterMutexLock mu_env_info(self, env->event_info_mutex_);
old_state = global_mask.Test(event);
if (mode == JVMTI_ENABLE) {
- env->event_masks.EnableEvent(env, thread, event);
+ env->event_masks.EnableEvent(env, art_thread, event);
global_mask.Set(event);
new_state = true;
} else {
DCHECK_EQ(mode, JVMTI_DISABLE);
- env->event_masks.DisableEvent(env, thread, event);
+ // TODO Replace art_thread with a uintptr_t or something to indicate we cannot read from it.
+ env->event_masks.DisableEvent(env, art_thread, event);
RecalculateGlobalEventMaskLocked(event);
new_state = global_mask.Test(event);
}
@@ -1263,7 +1314,7 @@
// Handle any special work required for the event type.
if (new_state != old_state) {
- HandleEventType(event, mode == JVMTI_ENABLE);
+ return HandleEventType(event, thread, mode == JVMTI_ENABLE);
}
return ERR(NONE);
diff --git a/openjdkjvmti/events.h b/openjdkjvmti/events.h
index abb15cc..e48772c 100644
--- a/openjdkjvmti/events.h
+++ b/openjdkjvmti/events.h
@@ -198,7 +198,7 @@
}
jvmtiError SetEvent(ArtJvmTiEnv* env,
- art::Thread* thread,
+ jthread thread,
ArtJvmtiEvent event,
jvmtiEventMode mode)
REQUIRES(!envs_lock_);
@@ -246,10 +246,13 @@
REQUIRES(!envs_lock_);
private:
- void SetupTraceListener(JvmtiMethodTraceListener* listener, ArtJvmtiEvent event, bool enable);
+ jvmtiError SetupTraceListener(JvmtiMethodTraceListener* listener,
+ ArtJvmtiEvent event,
+ jthread thread,
+ bool enable);
// Specifically handle the FramePop event which it might not always be possible to turn off.
- void SetupFramePopTraceListener(bool enable);
+ jvmtiError SetupFramePopTraceListener(jthread thread, bool enable);
template <ArtJvmtiEvent kEvent, typename ...Args>
ALWAYS_INLINE
@@ -309,7 +312,7 @@
jclass klass) const
REQUIRES(!envs_lock_);
- void HandleEventType(ArtJvmtiEvent event, bool enable);
+ jvmtiError HandleEventType(ArtJvmtiEvent event, jthread thread, bool enable);
void HandleLocalAccessCapabilityAdded();
void HandleBreakpointEventsChanged(bool enable);
diff --git a/openjdkjvmti/ti_stack.cc b/openjdkjvmti/ti_stack.cc
index 385ac45..5e934df 100644
--- a/openjdkjvmti/ti_stack.cc
+++ b/openjdkjvmti/ti_stack.cc
@@ -1061,8 +1061,14 @@
art::ShadowFrame* shadow_frame = visitor.GetOrCreateShadowFrame(&needs_instrument);
{
art::WriterMutexLock lk(self, tienv->event_info_mutex_);
- // Mark shadow frame as needs_notify_pop_
- shadow_frame->SetNotifyPop(true);
+ if (LIKELY(!shadow_frame->NeedsNotifyPop())) {
+ // Ensure we won't miss exceptions being thrown if we get jit-compiled. We only do this for
+ // the first NotifyPopFrame.
+ target->IncrementForceInterpreterCount();
+
+ // Mark shadow frame as needs_notify_pop_
+ shadow_frame->SetNotifyPop(true);
+ }
tienv->notify_frames.insert(shadow_frame);
}
// Make sure can we will go to the interpreter and use the shadow frames.