Don't allow suspension from FindLocksAtDexPc
Transitioning to suspended from runnable sometimes runs dump
checkpoints in ThreadStress which can cause deadlocks. This happens
since FindLocksAtDexPC runs the verifier which calls
AllowThreadSuspension. This results in a blocked thread which holds
the monitor lock, and if another thread tries to do a monitor enter,
it deadlocks while holding the mutator lock (assuming the GC is
suspending all).
The fix for avoiding this deadlock is not calling
AllowThreadSuspension from FindLocksAtDexPc.
Bug: 18576985
Change-Id: I7e5faaf3bbbd5b5f680de95d53c33b5106705b0c
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 556f2f8..5f5d3f7 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -3192,7 +3192,7 @@
Handle<mirror::ArtMethod> method(hs.NewHandle(m));
verifier::MethodVerifier verifier(self, dex_cache->GetDexFile(), dex_cache, class_loader,
&m->GetClassDef(), code_item, m->GetDexMethodIndex(), method,
- m->GetAccessFlags(), false, true, false);
+ m->GetAccessFlags(), false, true, false, true);
// Note: we don't need to verify the method.
return InlineMethodAnalyser::AnalyseMethodCode(&verifier, nullptr);
}
diff --git a/runtime/quick_exception_handler.cc b/runtime/quick_exception_handler.cc
index 90c9fe7..3517848 100644
--- a/runtime/quick_exception_handler.cc
+++ b/runtime/quick_exception_handler.cc
@@ -214,7 +214,7 @@
Handle<mirror::ArtMethod> h_method(hs.NewHandle(m));
verifier::MethodVerifier verifier(self_, h_dex_cache->GetDexFile(), h_dex_cache, h_class_loader,
&m->GetClassDef(), code_item, m->GetDexMethodIndex(),
- h_method, m->GetAccessFlags(), false, true, true);
+ h_method, m->GetAccessFlags(), false, true, true, true);
verifier.Verify();
const std::vector<int32_t> kinds(verifier.DescribeVRegs(dex_pc));
for (uint16_t reg = 0; reg < num_regs; ++reg) {
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 5ff7490..23c6557 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -1339,7 +1339,6 @@
}
mirror::Object* Thread::DecodeJObject(jobject obj) const {
- Locks::mutator_lock_->AssertSharedHeld(this);
if (obj == nullptr) {
return nullptr;
}
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 66846b5..88944d7 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -286,7 +286,7 @@
MethodVerifier verifier(self, dex_file, dex_cache, class_loader, class_def, code_item,
method_idx, method, method_access_flags, true, allow_soft_failures,
- need_precise_constants);
+ need_precise_constants, true);
if (verifier.Verify()) {
// Verification completed, however failures may be pending that didn't cause the verification
// to hard fail.
@@ -352,7 +352,8 @@
const DexFile::CodeItem* code_item, uint32_t dex_method_idx,
Handle<mirror::ArtMethod> method, uint32_t method_access_flags,
bool can_load_classes, bool allow_soft_failures,
- bool need_precise_constants, bool verify_to_dump)
+ bool need_precise_constants, bool verify_to_dump,
+ bool allow_thread_suspension)
: self_(self),
reg_types_(can_load_classes),
work_insn_idx_(-1),
@@ -377,7 +378,8 @@
need_precise_constants_(need_precise_constants),
has_check_casts_(false),
has_virtual_or_interface_invokes_(false),
- verify_to_dump_(verify_to_dump) {
+ verify_to_dump_(verify_to_dump),
+ allow_thread_suspension_(allow_thread_suspension) {
Runtime::Current()->AddMethodVerifier(this);
DCHECK(class_def != nullptr);
}
@@ -396,7 +398,7 @@
Handle<mirror::ArtMethod> method(hs.NewHandle(m));
MethodVerifier verifier(self, m->GetDexFile(), dex_cache, class_loader, &m->GetClassDef(),
m->GetCodeItem(), m->GetDexMethodIndex(), method, m->GetAccessFlags(),
- false, true, false);
+ false, true, false, false);
verifier.interesting_dex_pc_ = dex_pc;
verifier.monitor_enter_dex_pcs_ = monitor_enter_dex_pcs;
verifier.FindLocksAtDexPc();
@@ -443,7 +445,7 @@
Handle<mirror::ArtMethod> method(hs.NewHandle(m));
MethodVerifier verifier(self, m->GetDexFile(), dex_cache, class_loader, &m->GetClassDef(),
m->GetCodeItem(), m->GetDexMethodIndex(), method, m->GetAccessFlags(),
- true, true, false);
+ true, true, false, true);
return verifier.FindAccessedFieldAtDexPc(dex_pc);
}
@@ -475,7 +477,7 @@
Handle<mirror::ArtMethod> method(hs.NewHandle(m));
MethodVerifier verifier(self, m->GetDexFile(), dex_cache, class_loader, &m->GetClassDef(),
m->GetCodeItem(), m->GetDexMethodIndex(), method, m->GetAccessFlags(),
- true, true, false);
+ true, true, false, true);
return verifier.FindInvokedMethodAtDexPc(dex_pc);
}
@@ -1402,7 +1404,9 @@
/* Continue until no instructions are marked "changed". */
while (true) {
- self_->AllowThreadSuspension();
+ if (allow_thread_suspension_) {
+ self_->AllowThreadSuspension();
+ }
// Find the first marked one. Use "start_guess" as a way to find one quickly.
uint32_t insn_idx = start_guess;
for (; insn_idx < insns_size; insn_idx++) {
diff --git a/runtime/verifier/method_verifier.h b/runtime/verifier/method_verifier.h
index 15a09c5..b83e647 100644
--- a/runtime/verifier/method_verifier.h
+++ b/runtime/verifier/method_verifier.h
@@ -207,10 +207,11 @@
const DexFile::CodeItem* code_item, uint32_t method_idx,
Handle<mirror::ArtMethod> method,
uint32_t access_flags, bool can_load_classes, bool allow_soft_failures,
- bool need_precise_constants) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+ bool need_precise_constants, bool allow_thread_suspension)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
: MethodVerifier(self, dex_file, dex_cache, class_loader, class_def, code_item, method_idx,
method, access_flags, can_load_classes, allow_soft_failures,
- need_precise_constants, false) {}
+ need_precise_constants, false, allow_thread_suspension) {}
~MethodVerifier();
@@ -260,7 +261,7 @@
const DexFile::CodeItem* code_item, uint32_t method_idx,
Handle<mirror::ArtMethod> method, uint32_t access_flags,
bool can_load_classes, bool allow_soft_failures, bool need_precise_constants,
- bool verify_to_dump)
+ bool verify_to_dump, bool allow_thread_suspension)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
// Adds the given string to the beginning of the last failure message.
@@ -729,6 +730,11 @@
// VerifyMethodAndDump.
const bool verify_to_dump_;
+ // Whether or not we call AllowThreadSuspension periodically, we want a way to disable this for
+ // thread dumping checkpoints since we may get thread suspension at an inopportune time due to
+ // FindLocksAtDexPC, resulting in deadlocks.
+ const bool allow_thread_suspension_;
+
DISALLOW_COPY_AND_ASSIGN(MethodVerifier);
};
std::ostream& operator<<(std::ostream& os, const MethodVerifier::FailureKind& rhs);