Revert^4 "JVMTI PopFrame support"
This reverts commit 202b617acf477e8e8e11915f467120a0bd518e74.
This unreverts commit 202b617acf.
This unreverts commit 88a2a9d7a1.
There were several bugs with the implementation of pop-frame related
to interactions between the jit, exception handling, class-loading,
and deoptimization.
- We were instrumenting the target thread stack in cases where it was
unnecessary which caused the exception handler to incorrectly
determine that a method was not deoptimizable. This caused the
pop-frame to be ignored.
- We were incorrectly sending ExceptionCatch events if an exception
suppressed by pop-frame would have been caught in the current frame.
- We were allowing pop-frame to be used on threads suspended in the
ClassLoad or ClassPrepare events despite having surprising semantics
in that situation (see b/117615146).
Needed to modify test 1953 slightly for inclusion in CTS. I needed to
make the CTS entrypoint not run the class-load tests (since the cts
configuration means the classes are loaded by the verifier and not the
interpreter). I updated the expected.txt and check script to reflect
this.
Reason for revert: Fixed issue causing Exception events to sometimes
eat PopFrame and other issues.
Test: ./test.py --host
Test: ./art/tools/run-libjdwp-tests.sh --mode=host
Bug: 73255278
Bug: 111357976
Bug: 117533193
Bug: 117615146
Change-Id: I655c4fe769938cf41d7589f931d6710cf2001506
diff --git a/openjdkjvmti/OpenjdkJvmTi.cc b/openjdkjvmti/OpenjdkJvmTi.cc
index 3213bbe..48f326a 100644
--- a/openjdkjvmti/OpenjdkJvmTi.cc
+++ b/openjdkjvmti/OpenjdkJvmTi.cc
@@ -313,10 +313,10 @@
return StackUtil::GetFrameCount(env, thread, count_ptr);
}
- static jvmtiError PopFrame(jvmtiEnv* env, jthread thread ATTRIBUTE_UNUSED) {
+ static jvmtiError PopFrame(jvmtiEnv* env, jthread thread) {
ENSURE_VALID_ENV(env);
ENSURE_HAS_CAP(env, can_pop_frame);
- return ERR(NOT_IMPLEMENTED);
+ return StackUtil::PopFrame(env, thread);
}
static jvmtiError GetFrameLocation(jvmtiEnv* env,
diff --git a/openjdkjvmti/art_jvmti.h b/openjdkjvmti/art_jvmti.h
index 82f3866..1218e3b 100644
--- a/openjdkjvmti/art_jvmti.h
+++ b/openjdkjvmti/art_jvmti.h
@@ -249,7 +249,7 @@
.can_get_owned_monitor_info = 1,
.can_get_current_contended_monitor = 1,
.can_get_monitor_info = 1,
- .can_pop_frame = 0,
+ .can_pop_frame = 1,
.can_redefine_classes = 1,
.can_signal_thread = 1,
.can_get_source_file_name = 1,
@@ -291,6 +291,7 @@
// can_retransform_classes:
// can_redefine_any_class:
// can_redefine_classes:
+// can_pop_frame:
// We need to ensure that inlined code is either not present or can always be deoptimized. This
// is not guaranteed for non-debuggable processes since we might have inlined bootclasspath code
// on a threads stack.
@@ -303,7 +304,7 @@
.can_get_owned_monitor_info = 0,
.can_get_current_contended_monitor = 0,
.can_get_monitor_info = 0,
- .can_pop_frame = 0,
+ .can_pop_frame = 1,
.can_redefine_classes = 1,
.can_signal_thread = 0,
.can_get_source_file_name = 0,
diff --git a/openjdkjvmti/events-inl.h b/openjdkjvmti/events-inl.h
index e98517f..ca66556 100644
--- a/openjdkjvmti/events-inl.h
+++ b/openjdkjvmti/events-inl.h
@@ -26,7 +26,9 @@
#include "jni/jni_internal.h"
#include "nativehelper/scoped_local_ref.h"
#include "scoped_thread_state_change-inl.h"
+#include "stack.h"
#include "ti_breakpoint.h"
+#include "ti_thread.h"
#include "art_jvmti.h"
@@ -359,6 +361,7 @@
// have to deal with use-after-free or the frames being reallocated later.
art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
return env->notify_frames.erase(frame) != 0 &&
+ !frame->GetForcePopFrame() &&
ShouldDispatchOnThread<ArtJvmtiEvent::kFramePop>(env, thread);
}
@@ -418,6 +421,67 @@
ExecuteCallback<ArtJvmtiEvent::kFramePop>(event, jnienv, jni_thread, jmethod, is_exception);
}
+struct ScopedDisablePopFrame {
+ public:
+ explicit ScopedDisablePopFrame(art::Thread* thread) : thread_(thread) {
+ art::Locks::mutator_lock_->AssertSharedHeld(thread_);
+ art::MutexLock mu(thread_, *art::Locks::thread_list_lock_);
+ JvmtiGlobalTLSData* data = ThreadUtil::GetOrCreateGlobalTLSData(thread_);
+ current_top_frame_ = art::StackVisitor::ComputeNumFrames(
+ thread_, art::StackVisitor::StackWalkKind::kIncludeInlinedFrames);
+ old_disable_frame_pop_depth_ = data->disable_pop_frame_depth;
+ data->disable_pop_frame_depth = current_top_frame_;
+ DCHECK(old_disable_frame_pop_depth_ == JvmtiGlobalTLSData::kNoDisallowedPopFrame ||
+ current_top_frame_ > old_disable_frame_pop_depth_)
+ << "old: " << old_disable_frame_pop_depth_ << " current: " << current_top_frame_;
+ }
+
+ ~ScopedDisablePopFrame() {
+ art::Locks::mutator_lock_->AssertSharedHeld(thread_);
+ art::MutexLock mu(thread_, *art::Locks::thread_list_lock_);
+ JvmtiGlobalTLSData* data = ThreadUtil::GetGlobalTLSData(thread_);
+ DCHECK_EQ(data->disable_pop_frame_depth, current_top_frame_);
+ data->disable_pop_frame_depth = old_disable_frame_pop_depth_;
+ }
+
+ private:
+ art::Thread* thread_;
+ size_t current_top_frame_;
+ size_t old_disable_frame_pop_depth_;
+};
+// We want to prevent the use of PopFrame when reporting either of these events.
+template <ArtJvmtiEvent kEvent>
+inline void EventHandler::DispatchClassLoadOrPrepareEvent(art::Thread* thread,
+ JNIEnv* jnienv,
+ jthread jni_thread,
+ jclass klass) const {
+ ScopedDisablePopFrame sdpf(thread);
+ art::ScopedThreadStateChange stsc(thread, art::ThreadState::kNative);
+ std::vector<impl::EventHandlerFunc<kEvent>> events = CollectEvents<kEvent>(thread,
+ jnienv,
+ jni_thread,
+ klass);
+
+ for (auto event : events) {
+ ExecuteCallback<kEvent>(event, jnienv, jni_thread, klass);
+ }
+}
+
+template <>
+inline void EventHandler::DispatchEvent<ArtJvmtiEvent::kClassLoad>(art::Thread* thread,
+ JNIEnv* jnienv,
+ jthread jni_thread,
+ jclass klass) const {
+ DispatchClassLoadOrPrepareEvent<ArtJvmtiEvent::kClassLoad>(thread, jnienv, jni_thread, klass);
+}
+template <>
+inline void EventHandler::DispatchEvent<ArtJvmtiEvent::kClassPrepare>(art::Thread* thread,
+ JNIEnv* jnienv,
+ jthread jni_thread,
+ jclass klass) const {
+ DispatchClassLoadOrPrepareEvent<ArtJvmtiEvent::kClassPrepare>(thread, jnienv, jni_thread, klass);
+}
+
// Need to give a custom specialization for NativeMethodBind since it has to deal with an out
// variable.
template <>
@@ -553,6 +617,7 @@
: ArtJvmtiEvent::kClassFileLoadHookRetransformable;
return (added && caps.can_access_local_variables == 1) ||
caps.can_generate_breakpoint_events == 1 ||
+ caps.can_pop_frame == 1 ||
(caps.can_retransform_classes == 1 &&
IsEventEnabledAnywhere(event) &&
env->event_masks.IsEnabledAnywhere(event));
@@ -573,6 +638,11 @@
if (caps.can_generate_breakpoint_events == 1) {
HandleBreakpointEventsChanged(added);
}
+ if (caps.can_pop_frame == 1 && added) {
+ // TODO We should keep track of how many of these have been enabled and remove it if there are
+ // no more possible users. This isn't expected to be too common.
+ art::Runtime::Current()->SetNonStandardExitsEnabled();
+ }
}
}
diff --git a/openjdkjvmti/events.h b/openjdkjvmti/events.h
index bf12cb1..9f91a08 100644
--- a/openjdkjvmti/events.h
+++ b/openjdkjvmti/events.h
@@ -301,6 +301,13 @@
unsigned char** new_class_data) const
REQUIRES(!envs_lock_);
+ template <ArtJvmtiEvent kEvent>
+ ALWAYS_INLINE inline void DispatchClassLoadOrPrepareEvent(art::Thread* thread,
+ JNIEnv* jnienv,
+ jthread jni_thread,
+ jclass klass) const
+ REQUIRES(!envs_lock_);
+
void HandleEventType(ArtJvmtiEvent event, bool enable);
void HandleLocalAccessCapabilityAdded();
void HandleBreakpointEventsChanged(bool enable);
diff --git a/openjdkjvmti/ti_stack.cc b/openjdkjvmti/ti_stack.cc
index 220ad22..5a98755 100644
--- a/openjdkjvmti/ti_stack.cc
+++ b/openjdkjvmti/ti_stack.cc
@@ -112,6 +112,23 @@
size_t stop;
};
+art::ShadowFrame* FindFrameAtDepthVisitor::GetOrCreateShadowFrame(bool* created_frame) {
+ art::ShadowFrame* cur = GetCurrentShadowFrame();
+ if (cur == nullptr) {
+ *created_frame = true;
+ art::ArtMethod* method = GetMethod();
+ const uint16_t num_regs = method->DexInstructionData().RegistersSize();
+ cur = GetThread()->FindOrCreateDebuggerShadowFrame(GetFrameId(),
+ num_regs,
+ method,
+ GetDexPc());
+ DCHECK(cur != nullptr);
+ } else {
+ *created_frame = false;
+ }
+ return cur;
+}
+
template <typename FrameFn>
GetStackTraceVisitor<FrameFn> MakeStackTraceVisitor(art::Thread* thread_in,
size_t start,
@@ -1065,16 +1082,7 @@
// From here we are sure to succeed.
bool needs_instrument = false;
// Get/create a shadow frame
- art::ShadowFrame* shadow_frame = visitor.GetCurrentShadowFrame();
- if (shadow_frame == nullptr) {
- needs_instrument = true;
- const size_t frame_id = visitor.GetFrameId();
- const uint16_t num_regs = method->DexInstructionData().RegistersSize();
- shadow_frame = target->FindOrCreateDebuggerShadowFrame(frame_id,
- num_regs,
- method,
- visitor.GetDexPc());
- }
+ art::ShadowFrame* shadow_frame = visitor.GetOrCreateShadowFrame(&needs_instrument);
{
art::WriterMutexLock lk(self, tienv->event_info_mutex_);
// Mark shadow frame as needs_notify_pop_
@@ -1089,4 +1097,88 @@
} while (true);
}
+jvmtiError StackUtil::PopFrame(jvmtiEnv* env ATTRIBUTE_UNUSED, jthread thread) {
+ art::Thread* self = art::Thread::Current();
+ art::Thread* target;
+ do {
+ ThreadUtil::SuspendCheck(self);
+ art::MutexLock ucsl_mu(self, *art::Locks::user_code_suspension_lock_);
+ // Make sure we won't be suspended in the middle of holding the thread_suspend_count_lock_ by a
+ // user-code suspension. We retry and do another SuspendCheck to clear this.
+ if (ThreadUtil::WouldSuspendForUserCodeLocked(self)) {
+ continue;
+ }
+ // From now on we know we cannot get suspended by user-code.
+ // NB This does a SuspendCheck (during thread state change) so we need to make sure we don't
+ // have the 'suspend_lock' locked here.
+ art::ScopedObjectAccess soa(self);
+ art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
+ }
+ {
+ art::MutexLock tscl_mu(self, *art::Locks::thread_suspend_count_lock_);
+ if (target == self || target->GetUserCodeSuspendCount() == 0) {
+ // We cannot be the current thread for this function.
+ return ERR(THREAD_NOT_SUSPENDED);
+ }
+ }
+ JvmtiGlobalTLSData* tls_data = ThreadUtil::GetGlobalTLSData(target);
+ constexpr art::StackVisitor::StackWalkKind kWalkKind =
+ art::StackVisitor::StackWalkKind::kIncludeInlinedFrames;
+ if (tls_data != nullptr &&
+ tls_data->disable_pop_frame_depth != JvmtiGlobalTLSData::kNoDisallowedPopFrame &&
+ tls_data->disable_pop_frame_depth == art::StackVisitor::ComputeNumFrames(target,
+ kWalkKind)) {
+ LOG(WARNING) << "Disallowing frame pop due to in-progress class-load/prepare. Frame at depth "
+ << tls_data->disable_pop_frame_depth << " was marked as un-poppable by the "
+ << "jvmti plugin. See b/117615146 for more information.";
+ return ERR(OPAQUE_FRAME);
+ }
+ // We hold the user_code_suspension_lock_ so the target thread is staying suspended until we are
+ // done.
+ std::unique_ptr<art::Context> context(art::Context::Create());
+ FindFrameAtDepthVisitor final_frame(target, context.get(), 0);
+ FindFrameAtDepthVisitor penultimate_frame(target, context.get(), 1);
+ final_frame.WalkStack();
+ penultimate_frame.WalkStack();
+
+ if (!final_frame.FoundFrame() || !penultimate_frame.FoundFrame()) {
+ // Cannot do it if there is only one frame!
+ return ERR(NO_MORE_FRAMES);
+ }
+
+ art::ArtMethod* called_method = final_frame.GetMethod();
+ art::ArtMethod* calling_method = penultimate_frame.GetMethod();
+ if (calling_method->IsNative() || called_method->IsNative()) {
+ return ERR(OPAQUE_FRAME);
+ }
+ // From here we are sure to succeed.
+
+ // Get/create a shadow frame
+ bool created_final_frame = false;
+ bool created_penultimate_frame = false;
+ art::ShadowFrame* called_shadow_frame =
+ final_frame.GetOrCreateShadowFrame(&created_final_frame);
+ art::ShadowFrame* calling_shadow_frame =
+ penultimate_frame.GetOrCreateShadowFrame(&created_penultimate_frame);
+
+ CHECK_NE(called_shadow_frame, calling_shadow_frame)
+ << "Frames at different depths not different!";
+
+ // Tell the shadow-frame to return immediately and skip all exit events.
+ called_shadow_frame->SetForcePopFrame(true);
+ calling_shadow_frame->SetForceRetryInstruction(true);
+
+ // Make sure can we will go to the interpreter and use the shadow frames. The early return for
+ // the final frame will force everything to the interpreter so we only need to instrument if it
+ // was not present.
+ if (created_final_frame) {
+ DeoptManager::Get()->DeoptimizeThread(target);
+ }
+ return OK;
+ } while (true);
+}
+
} // namespace openjdkjvmti
diff --git a/openjdkjvmti/ti_stack.h b/openjdkjvmti/ti_stack.h
index b41fa4b..55c4269 100644
--- a/openjdkjvmti/ti_stack.h
+++ b/openjdkjvmti/ti_stack.h
@@ -81,6 +81,8 @@
jobject** owned_monitors_ptr);
static jvmtiError NotifyFramePop(jvmtiEnv* env, jthread thread, jint depth);
+
+ static jvmtiError PopFrame(jvmtiEnv* env, jthread thread);
};
struct FindFrameAtDepthVisitor : art::StackVisitor {
@@ -110,6 +112,9 @@
}
}
+ art::ShadowFrame* GetOrCreateShadowFrame(/*out*/bool* created_frame)
+ REQUIRES_SHARED(art::Locks::mutator_lock_);
+
private:
bool found_frame_;
size_t cnt_;
diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index b54c77d..a0e5b5c 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -623,18 +623,10 @@
return ERR(NONE);
}
-// The struct that we store in the art::Thread::custom_tls_ that maps the jvmtiEnvs to the data
-// stored with that thread. This is needed since different jvmtiEnvs are not supposed to share TLS
-// data but we only have a single slot in Thread objects to store data.
-struct JvmtiGlobalTLSData : public art::TLSData {
- std::unordered_map<jvmtiEnv*, const void*> data GUARDED_BY(art::Locks::thread_list_lock_);
-};
-
static void RemoveTLSData(art::Thread* target, void* ctx) REQUIRES(art::Locks::thread_list_lock_) {
jvmtiEnv* env = reinterpret_cast<jvmtiEnv*>(ctx);
art::Locks::thread_list_lock_->AssertHeld(art::Thread::Current());
- JvmtiGlobalTLSData* global_tls =
- reinterpret_cast<JvmtiGlobalTLSData*>(target->GetCustomTLS(kJvmtiTlsKey));
+ JvmtiGlobalTLSData* global_tls = ThreadUtil::GetGlobalTLSData(target);
if (global_tls != nullptr) {
global_tls->data.erase(env);
}
@@ -657,19 +649,27 @@
return err;
}
- JvmtiGlobalTLSData* global_tls =
- reinterpret_cast<JvmtiGlobalTLSData*>(target->GetCustomTLS(kJvmtiTlsKey));
- if (global_tls == nullptr) {
- // Synchronized using thread_list_lock_ to prevent racing sets.
- target->SetCustomTLS(kJvmtiTlsKey, new JvmtiGlobalTLSData);
- global_tls = reinterpret_cast<JvmtiGlobalTLSData*>(target->GetCustomTLS(kJvmtiTlsKey));
- }
+ JvmtiGlobalTLSData* global_tls = GetOrCreateGlobalTLSData(target);
global_tls->data[env] = data;
return ERR(NONE);
}
+JvmtiGlobalTLSData* ThreadUtil::GetOrCreateGlobalTLSData(art::Thread* thread) {
+ JvmtiGlobalTLSData* data = GetGlobalTLSData(thread);
+ if (data != nullptr) {
+ return data;
+ } else {
+ thread->SetCustomTLS(kJvmtiTlsKey, new JvmtiGlobalTLSData);
+ return GetGlobalTLSData(thread);
+ }
+}
+
+JvmtiGlobalTLSData* ThreadUtil::GetGlobalTLSData(art::Thread* thread) {
+ return reinterpret_cast<JvmtiGlobalTLSData*>(thread->GetCustomTLS(kJvmtiTlsKey));
+}
+
jvmtiError ThreadUtil::GetThreadLocalStorage(jvmtiEnv* env,
jthread thread,
void** data_ptr) {
@@ -686,8 +686,7 @@
return err;
}
- JvmtiGlobalTLSData* global_tls =
- reinterpret_cast<JvmtiGlobalTLSData*>(target->GetCustomTLS(kJvmtiTlsKey));
+ JvmtiGlobalTLSData* global_tls = GetGlobalTLSData(target);
if (global_tls == nullptr) {
*data_ptr = nullptr;
return OK;
diff --git a/openjdkjvmti/ti_thread.h b/openjdkjvmti/ti_thread.h
index c6b6af1..39f1f07 100644
--- a/openjdkjvmti/ti_thread.h
+++ b/openjdkjvmti/ti_thread.h
@@ -32,11 +32,14 @@
#ifndef ART_OPENJDKJVMTI_TI_THREAD_H_
#define ART_OPENJDKJVMTI_TI_THREAD_H_
+#include <unordered_map>
+
#include "jni.h"
#include "jvmti.h"
#include "base/macros.h"
#include "base/mutex.h"
+#include "thread.h"
namespace art {
class ArtField;
@@ -49,6 +52,18 @@
class EventHandler;
+// The struct that we store in the art::Thread::custom_tls_ that maps the jvmtiEnvs to the data
+// stored with that thread. This is needed since different jvmtiEnvs are not supposed to share TLS
+// data but we only have a single slot in Thread objects to store data.
+struct JvmtiGlobalTLSData : public art::TLSData {
+ std::unordered_map<jvmtiEnv*, const void*> data GUARDED_BY(art::Locks::thread_list_lock_);
+
+ // The depth of the last frame where popping using PopFrame it is not allowed. It is set to
+ // kNoDisallowedPopFrame if all frames can be popped. See b/117615146 for more information.
+ static constexpr size_t kNoDisallowedPopFrame = -1;
+ size_t disable_pop_frame_depth = kNoDisallowedPopFrame;
+};
+
class ThreadUtil {
public:
static void Register(EventHandler* event_handler);
@@ -134,6 +149,11 @@
REQUIRES(!art::Locks::user_code_suspension_lock_,
!art::Locks::thread_suspend_count_lock_);
+ static JvmtiGlobalTLSData* GetGlobalTLSData(art::Thread* thread)
+ REQUIRES(art::Locks::thread_list_lock_);
+ static JvmtiGlobalTLSData* GetOrCreateGlobalTLSData(art::Thread* thread)
+ REQUIRES(art::Locks::thread_list_lock_);
+
private:
// We need to make sure only one thread tries to suspend threads at a time so we can get the
// 'suspend-only-once' behavior the spec requires. Internally, ART considers suspension to be a