Do not hold breakpoint lock when running the verifier
When setting a breakpoint, we need to know whether the method may be
inlined. We run the method verifier but that may cause thread
suspension. Therefore we must not hold any lock at this time. The
issue is we do hold the breakpoint lock so we fails a check in debug
mode.
This CL ensures we don't hold the breakpoint lock when running the
method verifier to detect inlining.
Bug: 17562442
(cherry picked from commit 4d1e9ab57e4c822e8e678b1d0c1d6ad2abfa9b3e)
Change-Id: I6daae91e3e82f41006f29959a5c6c0ec0a88194e
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index d81f093..ce3172c 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -3053,6 +3053,8 @@
// should never be null. We could just check we never encounter this case.
return false;
}
+ // Note: method verifier may cause thread suspension.
+ self->AssertThreadSuspensionIsAllowable();
StackHandleScope<2> hs(self);
mirror::Class* declaring_class = m->GetDeclaringClass();
Handle<mirror::DexCache> dex_cache(hs.NewHandle(declaring_class->GetDexCache()));
@@ -3077,20 +3079,18 @@
// Sanity checks all existing breakpoints on the same method.
static void SanityCheckExistingBreakpoints(mirror::ArtMethod* m, bool need_full_deoptimization)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_, Locks::breakpoint_lock_) {
- if (kIsDebugBuild) {
- for (const Breakpoint& breakpoint : gBreakpoints) {
- CHECK_EQ(need_full_deoptimization, breakpoint.NeedFullDeoptimization());
- }
- if (need_full_deoptimization) {
- // We should have deoptimized everything but not "selectively" deoptimized this method.
- CHECK(Runtime::Current()->GetInstrumentation()->AreAllMethodsDeoptimized());
- CHECK(!Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
- } else {
- // We should have "selectively" deoptimized this method.
- // Note: while we have not deoptimized everything for this method, we may have done it for
- // another event.
- CHECK(Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
- }
+ for (const Breakpoint& breakpoint : gBreakpoints) {
+ CHECK_EQ(need_full_deoptimization, breakpoint.NeedFullDeoptimization());
+ }
+ if (need_full_deoptimization) {
+ // We should have deoptimized everything but not "selectively" deoptimized this method.
+ CHECK(Runtime::Current()->GetInstrumentation()->AreAllMethodsDeoptimized());
+ CHECK(!Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
+ } else {
+ // We should have "selectively" deoptimized this method.
+ // Note: while we have not deoptimized everything for this method, we may have done it for
+ // another event.
+ CHECK(Runtime::Current()->GetInstrumentation()->IsDeoptimized(m));
}
}
@@ -3101,12 +3101,17 @@
mirror::ArtMethod* m = FromMethodId(location->method_id);
DCHECK(m != nullptr) << "No method for method id " << location->method_id;
- WriterMutexLock mu(self, *Locks::breakpoint_lock_);
- const Breakpoint* const existing_breakpoint = FindFirstBreakpointForMethod(m);
+ const Breakpoint* existing_breakpoint;
+ {
+ ReaderMutexLock mu(self, *Locks::breakpoint_lock_);
+ existing_breakpoint = FindFirstBreakpointForMethod(m);
+ }
bool need_full_deoptimization;
if (existing_breakpoint == nullptr) {
// There is no breakpoint on this method yet: we need to deoptimize. If this method may be
// inlined, we deoptimize everything; otherwise we deoptimize only this method.
+ // Note: IsMethodPossiblyInlined goes into the method verifier and may cause thread suspension.
+ // Therefore we must not hold any lock when we call it.
need_full_deoptimization = IsMethodPossiblyInlined(self, m);
if (need_full_deoptimization) {
req->SetKind(DeoptimizationRequest::kFullDeoptimization);
@@ -3121,12 +3126,18 @@
req->SetMethod(nullptr);
need_full_deoptimization = existing_breakpoint->NeedFullDeoptimization();
- SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+ if (kIsDebugBuild) {
+ ReaderMutexLock mu(self, *Locks::breakpoint_lock_);
+ SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+ }
}
- gBreakpoints.push_back(Breakpoint(m, location->dex_pc, need_full_deoptimization));
- VLOG(jdwp) << "Set breakpoint #" << (gBreakpoints.size() - 1) << ": "
- << gBreakpoints[gBreakpoints.size() - 1];
+ {
+ WriterMutexLock mu(self, *Locks::breakpoint_lock_);
+ gBreakpoints.push_back(Breakpoint(m, location->dex_pc, need_full_deoptimization));
+ VLOG(jdwp) << "Set breakpoint #" << (gBreakpoints.size() - 1) << ": "
+ << gBreakpoints[gBreakpoints.size() - 1];
+ }
}
// Uninstalls a breakpoint at the specified location. Also indicates through the deoptimization
@@ -3161,7 +3172,9 @@
// There is at least one breakpoint for this method: we don't need to undeoptimize.
req->SetKind(DeoptimizationRequest::kNothing);
req->SetMethod(nullptr);
- SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+ if (kIsDebugBuild) {
+ SanityCheckExistingBreakpoints(m, need_full_deoptimization);
+ }
}
}
