Revert "Allow threads to be marked as unsuspendable by kForUserCode"
This allows any thread (including jit-threads) to be suspended by
kForUserCode and the will stop executing at the next suspend-point.
This reverts commit 53570676750d74416cecdf5a8e01f3cf9a8d4169.
Reason for revert: This marking was not sufficient to prevent
deadlocks and there is a better solution in simply preventing the jit
thread from making this situation possible.
Test: ./test.py --host
Test: ./art/tools/run-libjdwp-tests.sh --mode=host
Test: ./art/tools/run-libjdwp-tests.sh \
--mode=host \
--variant=x64 \
--test org.apache.harmony.jpda.tests.jdwp.EventModifiers.InstanceOnlyModifierTest
Bug: 70838465
Bug: 111348762
Change-Id: I8314904cc35f66bdf287ac7b9ec69510310a3474
diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index 41ef6c2..b54c77d 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -818,37 +818,6 @@
return ERR(NONE);
}
-class ScopedSuspendByPeer {
- public:
- explicit ScopedSuspendByPeer(jthread jtarget)
- : thread_list_(art::Runtime::Current()->GetThreadList()),
- timeout_(false),
- target_(thread_list_->SuspendThreadByPeer(jtarget,
- /* suspend_thread */ true,
- art::SuspendReason::kInternal,
- &timeout_)) { }
- ~ScopedSuspendByPeer() {
- if (target_ != nullptr) {
- if (!thread_list_->Resume(target_, art::SuspendReason::kInternal)) {
- LOG(ERROR) << "Failed to resume " << target_ << "!";
- }
- }
- }
-
- art::Thread* GetTargetThread() const {
- return target_;
- }
-
- bool TimedOut() const {
- return timeout_;
- }
-
- private:
- art::ThreadList* thread_list_;
- bool timeout_;
- art::Thread* target_;
-};
-
jvmtiError ThreadUtil::SuspendOther(art::Thread* self,
jthread target_jthread) {
// Loop since we need to bail out and try again if we would end up getting suspended while holding
@@ -876,27 +845,29 @@
if (!GetAliveNativeThread(target_jthread, soa, &target, &err)) {
return err;
}
+ art::ThreadState state = target->GetState();
+ if (state == art::ThreadState::kStarting || target->IsStillStarting()) {
+ return ERR(THREAD_NOT_ALIVE);
+ } else {
+ art::MutexLock thread_suspend_count_mu(self, *art::Locks::thread_suspend_count_lock_);
+ if (target->GetUserCodeSuspendCount() != 0) {
+ return ERR(THREAD_SUSPENDED);
+ }
+ }
}
- // Get the actual thread in a suspended state so we can change the user-code suspend count.
- ScopedSuspendByPeer ssbp(target_jthread);
- if (ssbp.GetTargetThread() == nullptr && !ssbp.TimedOut()) {
+ bool timeout = true;
+ art::Thread* ret_target = art::Runtime::Current()->GetThreadList()->SuspendThreadByPeer(
+ target_jthread,
+ /* request_suspension */ true,
+ art::SuspendReason::kForUserCode,
+ &timeout);
+ if (ret_target == nullptr && !timeout) {
// TODO It would be good to get more information about why exactly the thread failed to
// suspend.
return ERR(INTERNAL);
- } else if (!ssbp.TimedOut()) {
- art::ThreadState state = ssbp.GetTargetThread()->GetState();
- if (state == art::ThreadState::kStarting || ssbp.GetTargetThread()->IsStillStarting()) {
- return ERR(THREAD_NOT_ALIVE);
- }
- // we didn't time out and got a result. Suspend the thread by usercode and return. It's
- // already suspended internal so we don't need to do anything but increment the count.
- art::MutexLock thread_suspend_count_mu(self, *art::Locks::thread_suspend_count_lock_);
- if (ssbp.GetTargetThread()->GetUserCodeSuspendCount() != 0) {
- return ERR(THREAD_SUSPENDED);
- }
- bool res = ssbp.GetTargetThread()->ModifySuspendCount(
- self, +1, nullptr, art::SuspendReason::kForUserCode);
- return res ? OK : ERR(INTERNAL);
+ } else if (!timeout) {
+ // we didn't time out and got a result.
+ return OK;
}
// We timed out. Just go around and try again.
} while (true);
@@ -905,17 +876,6 @@
jvmtiError ThreadUtil::SuspendSelf(art::Thread* self) {
CHECK(self == art::Thread::Current());
- if (!self->CanBeSuspendedByUserCode()) {
- // TODO This is really undesirable. As far as I can tell this is can only come about because of
- // class-loads in the jit-threads (through either VMObjectAlloc or the ClassLoad/ClassPrepare
- // events that we send). It's unlikely that anyone would be suspending themselves there since
- // it's almost guaranteed to cause a deadlock but it is technically allowed. Ideally we'd want
- // to put a CHECK here (or in the event-dispatch code) that we are only in this situation when
- // sending the GC callbacks but the jit causing events means we cannot do this.
- LOG(WARNING) << "Attempt to self-suspend on a thread without suspension enabled. Thread is "
- << *self;
- return ERR(INTERNAL);
- }
{
art::MutexLock mu(self, *art::Locks::user_code_suspension_lock_);
art::MutexLock thread_list_mu(self, *art::Locks::thread_suspend_count_lock_);
@@ -963,6 +923,7 @@
return ERR(NULL_POINTER);
}
art::Thread* self = art::Thread::Current();
+ art::Thread* target;
// Retry until we know we won't get suspended by user code while resuming something.
do {
SuspendCheck(self);
@@ -973,37 +934,36 @@
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);
- if (thread == nullptr) {
- // The thread is the current thread.
- return ERR(THREAD_NOT_SUSPENDED);
- } else if (!soa.Env()->IsInstanceOf(thread, art::WellKnownClasses::java_lang_Thread)) {
- // Not a thread object.
- return ERR(INVALID_THREAD);
- } else if (self->GetPeer() == soa.Decode<art::mirror::Object>(thread)) {
- // The thread is the current thread.
- return ERR(THREAD_NOT_SUSPENDED);
+ {
+ // 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 (!GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
+ } else if (target == self) {
+ // We would have paused until we aren't suspended anymore due to the ScopedObjectAccess so
+ // we can just return THREAD_NOT_SUSPENDED. Unfortunately we cannot do any real DCHECKs
+ // about current state since it's all concurrent.
+ return ERR(THREAD_NOT_SUSPENDED);
+ }
+ // The JVMTI spec requires us to return THREAD_NOT_SUSPENDED if it is alive but we really
+ // cannot tell why resume failed.
+ {
+ art::MutexLock thread_suspend_count_mu(self, *art::Locks::thread_suspend_count_lock_);
+ if (target->GetUserCodeSuspendCount() == 0) {
+ return ERR(THREAD_NOT_SUSPENDED);
+ }
+ }
}
- ScopedSuspendByPeer ssbp(thread);
- if (ssbp.TimedOut()) {
- // Unknown error. Couldn't suspend thread!
- return ERR(INTERNAL);
- } else if (ssbp.GetTargetThread() == nullptr) {
- // Thread must not be alive.
- return ERR(THREAD_NOT_ALIVE);
- }
- // We didn't time out and got a result. Check the thread is suspended by usercode, unsuspend it
- // and return. It's already suspended internal so we don't need to do anything but decrement the
- // count.
- art::MutexLock thread_list_mu(self, *art::Locks::thread_suspend_count_lock_);
- if (ssbp.GetTargetThread()->GetUserCodeSuspendCount() == 0) {
- return ERR(THREAD_NOT_SUSPENDED);
- } else if (!ssbp.GetTargetThread()->ModifySuspendCount(
- self, -1, nullptr, art::SuspendReason::kForUserCode)) {
+ // It is okay that we don't have a thread_list_lock here since we know that the thread cannot
+ // die since it is currently held suspended by a SuspendReason::kForUserCode suspend.
+ DCHECK(target != self);
+ if (!art::Runtime::Current()->GetThreadList()->Resume(target,
+ art::SuspendReason::kForUserCode)) {
// TODO Give a better error.
- // This should not really be possible and is probably some race.
+ // This is most likely THREAD_NOT_SUSPENDED but we cannot really be sure.
return ERR(INTERNAL);
} else {
return OK;
diff --git a/runtime/suspend_reason.h b/runtime/suspend_reason.h
index 4e75a4f..289a1a4 100644
--- a/runtime/suspend_reason.h
+++ b/runtime/suspend_reason.h
@@ -22,8 +22,6 @@
namespace art {
// The various reasons that we might be suspending a thread.
-// TODO Once kForDebugger is removed by removing the old debugger we should make the kForUserCode
-// just a basic count for bookkeeping instead of linking it as directly with internal suspends.
enum class SuspendReason {
// Suspending for internal reasons (e.g. GC, stack trace, etc.).
// TODO Split this into more descriptive sections.
diff --git a/runtime/thread.cc b/runtime/thread.cc
index fd19dac..4a3d8cb 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -1236,34 +1236,6 @@
LOG(FATAL) << ss.str();
}
-void Thread::SetCanBeSuspendedByUserCode(bool can_be_suspended_by_user_code) {
- CHECK_EQ(this, Thread::Current()) << "This function may only be called on the current thread. "
- << *Thread::Current() << " tried to modify the suspendability "
- << "of " << *this;
- // NB This checks the new value! This ensures that we can only set can_be_suspended_by_user_code
- // to false if !CanCallIntoJava().
- DCHECK(IsRuntimeThread() || can_be_suspended_by_user_code)
- << "Threads able to call into java may not be marked as unsuspendable!";
- if (can_be_suspended_by_user_code == CanBeSuspendedByUserCode()) {
- // Don't need to do anything if nothing is changing.
- return;
- }
- art::MutexLock mu(this, *Locks::user_code_suspension_lock_);
- art::MutexLock thread_list_mu(this, *Locks::thread_suspend_count_lock_);
-
- // We want to add the user-code suspend count if we are newly allowing user-code suspends and
- // remove them if we are disabling them.
- int adj = can_be_suspended_by_user_code ? GetUserCodeSuspendCount() : -GetUserCodeSuspendCount();
- // Adjust the global suspend count appropriately. Use kInternal to not change the ForUserCode
- // count.
- if (adj != 0) {
- bool suspend = ModifySuspendCountInternal(this, adj, nullptr, SuspendReason::kInternal);
- CHECK(suspend) << this << " was unable to modify it's own suspend count!";
- }
- // Mark thread as accepting user-code suspensions.
- can_be_suspended_by_user_code_ = can_be_suspended_by_user_code;
-}
-
bool Thread::ModifySuspendCountInternal(Thread* self,
int delta,
AtomicInteger* suspend_barrier,
@@ -1285,17 +1257,6 @@
LOG(ERROR) << "attempting to modify suspend count in an illegal way.";
return false;
}
- DCHECK(this == self || this->IsSuspended())
- << "Only self kForUserCode suspension on an unsuspended thread is allowed: " << this;
- if (UNLIKELY(!CanBeSuspendedByUserCode())) {
- VLOG(threads) << this << " is being requested to suspend for user code but that is disabled "
- << "the thread will not actually go to sleep.";
- // Having the user_code_suspend_count still be around is useful but we don't need to actually
- // do anything since we aren't going to 'really' suspend. Just adjust the
- // user_code_suspend_count and return.
- tls32_.user_code_suspend_count += delta;
- return true;
- }
}
if (UNLIKELY(delta < 0 && tls32_.suspend_count <= 0)) {
UnsafeLogFatalForSuspendCount(self, this);
@@ -2156,8 +2117,7 @@
Thread::Thread(bool daemon)
: tls32_(daemon),
wait_monitor_(nullptr),
- is_runtime_thread_(false),
- can_be_suspended_by_user_code_(true) {
+ is_runtime_thread_(false) {
wait_mutex_ = new Mutex("a thread wait mutex");
wait_cond_ = new ConditionVariable("a thread wait condition variable", *wait_mutex_);
tlsPtr_.instrumentation_stack = new std::deque<instrumentation::InstrumentationStackFrame>;
diff --git a/runtime/thread.h b/runtime/thread.h
index 4c9c38a..a915cd8 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -990,17 +990,6 @@
--tls32_.disable_thread_flip_count;
}
- // Returns true if the thread is subject to user_code_suspensions.
- bool CanBeSuspendedByUserCode() const {
- return can_be_suspended_by_user_code_;
- }
-
- // Sets CanBeSuspenededByUserCode and adjusts the suspend-count as needed. This may only be called
- // when running on the current thread. It is **absolutely required** that this be called only on
- // the Thread::Current() thread.
- void SetCanBeSuspendedByUserCode(bool can_be_suspended_by_user_code)
- REQUIRES(!Locks::thread_suspend_count_lock_, !Locks::user_code_suspension_lock_);
-
// Returns true if the thread is allowed to call into java.
bool IsRuntimeThread() const {
return is_runtime_thread_;
@@ -1590,9 +1579,8 @@
// critical section enter.
uint32_t disable_thread_flip_count;
- // If CanBeSuspendedByUserCode, how much of 'suspend_count_' is by request of user code, used to
- // distinguish threads suspended by the runtime from those suspended by user code. Otherwise
- // this is just a count of how many user-code suspends have been attempted (but were ignored).
+ // How much of 'suspend_count_' is by request of user code, used to distinguish threads
+ // suspended by the runtime from those suspended by user code.
// This should have GUARDED_BY(Locks::user_code_suspension_lock_) but auto analysis cannot be
// told that AssertHeld should be good enough.
int user_code_suspend_count GUARDED_BY(Locks::thread_suspend_count_lock_);
@@ -1818,10 +1806,6 @@
// True if the thread is some form of runtime thread (ex, GC or JIT).
bool is_runtime_thread_;
- // True if the thread is subject to user-code suspension. By default this is true. This can only
- // be false for threads where '!can_call_into_java_'.
- bool can_be_suspended_by_user_code_;
-
friend class Dbg; // For SetStateUnsafe.
friend class gc::collector::SemiSpace; // For getting stack traces.
friend class Runtime; // For CreatePeer.
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index cddc275..ec40716 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -902,8 +902,6 @@
bool request_suspension,
SuspendReason reason,
bool* timed_out) {
- CHECK_NE(reason, SuspendReason::kForUserCode) << "Cannot suspend for user-code by peer. Must be "
- << "done directly on the thread.";
const uint64_t start_time = NanoTime();
useconds_t sleep_us = kThreadSuspendInitialSleepUs;
*timed_out = false;
diff --git a/runtime/thread_pool.cc b/runtime/thread_pool.cc
index fc9067d..f1c808b 100644
--- a/runtime/thread_pool.cc
+++ b/runtime/thread_pool.cc
@@ -104,13 +104,8 @@
worker->thread_ = Thread::Current();
// Mark thread pool workers as runtime-threads.
worker->thread_->SetIsRuntimeThread(true);
- // Thread pool workers should not be getting paused by user-code.
- worker->thread_->SetCanBeSuspendedByUserCode(false);
// Do work until its time to shut down.
worker->Run();
- // Thread pool worker is finished. We want to allow suspension during shutdown.
- worker->thread_->SetCanBeSuspendedByUserCode(true);
- // Thread shuts down.
runtime->DetachCurrentThread();
return nullptr;
}