ART: Change lock dumping
Return dex registers as well as dex PCs from FindLocksAtDexPc.
It is necessary to return the registers as the register used for
a monitor-enter operation may be aliased and overwritten by the
time the requested dex PC is reached.
It is at this point necessary to return a set of registers as
optimizations may have eliminated dead registers, and the verifier
has no notion of liveness.
A client of FindLocksAtDexPc should not assume that all registers
can be successfully retrieved from a stack frame, with the possibility
of none. Only a properly verified method (i.e., lock verification succeeded)
implies that at least one register should work for all locks held at
any point in the program.
Bug: 68703210
Test: art/test/testrunner/testrunner.py -b --host -t 167
Change-Id: I9027787e395cf8df0e7699a606665edb2ecb5136
diff --git a/runtime/monitor.cc b/runtime/monitor.cc
index cdc55bd..d5520d9 100644
--- a/runtime/monitor.cc
+++ b/runtime/monitor.cc
@@ -1401,26 +1401,38 @@
// Ask the verifier for the dex pcs of all the monitor-enter instructions corresponding to
// the locks held in this stack frame.
- std::vector<uint32_t> monitor_enter_dex_pcs;
+ std::vector<verifier::MethodVerifier::DexLockInfo> monitor_enter_dex_pcs;
verifier::MethodVerifier::FindLocksAtDexPc(m, dex_pc, &monitor_enter_dex_pcs);
- for (uint32_t monitor_dex_pc : monitor_enter_dex_pcs) {
- // The verifier works in terms of the dex pcs of the monitor-enter instructions.
- // We want the registers used by those instructions (so we can read the values out of them).
- const Instruction* monitor_enter_instruction =
- Instruction::At(&code_item->insns_[monitor_dex_pc]);
+ for (verifier::MethodVerifier::DexLockInfo& dex_lock_info : monitor_enter_dex_pcs) {
+ // As a debug check, check that dex PC corresponds to a monitor-enter.
+ if (kIsDebugBuild) {
+ const Instruction* monitor_enter_instruction =
+ Instruction::At(&code_item->insns_[dex_lock_info.dex_pc]);
+ CHECK_EQ(monitor_enter_instruction->Opcode(), Instruction::MONITOR_ENTER)
+ << "expected monitor-enter @" << dex_lock_info.dex_pc << "; was "
+ << reinterpret_cast<const void*>(monitor_enter_instruction);
+ }
- // Quick sanity check.
- CHECK_EQ(monitor_enter_instruction->Opcode(), Instruction::MONITOR_ENTER)
- << "expected monitor-enter @" << monitor_dex_pc << "; was "
- << reinterpret_cast<const void*>(monitor_enter_instruction);
-
- uint16_t monitor_register = monitor_enter_instruction->VRegA();
- uint32_t value;
- bool success = stack_visitor->GetVReg(m, monitor_register, kReferenceVReg, &value);
- CHECK(success) << "Failed to read v" << monitor_register << " of kind "
- << kReferenceVReg << " in method " << m->PrettyMethod();
- mirror::Object* o = reinterpret_cast<mirror::Object*>(value);
- callback(o, callback_context);
+ // Iterate through the set of dex registers, as the compiler may not have held all of them
+ // live.
+ bool success = false;
+ for (uint32_t dex_reg : dex_lock_info.dex_registers) {
+ uint32_t value;
+ success = stack_visitor->GetVReg(m, dex_reg, kReferenceVReg, &value);
+ if (success) {
+ mirror::Object* o = reinterpret_cast<mirror::Object*>(value);
+ callback(o, callback_context);
+ break;
+ }
+ }
+ DCHECK(success) << "Failed to find/read reference for monitor-enter at dex pc "
+ << dex_lock_info.dex_pc
+ << " in method "
+ << m->PrettyMethod();
+ if (!success) {
+ LOG(WARNING) << "Had a lock reported for dex pc " << dex_lock_info.dex_pc
+ << " but was not able to fetch a corresponding object!";
+ }
}
}
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index a75157d..c6ba2f7 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -592,8 +592,10 @@
STLDeleteElements(&failure_messages_);
}
-void MethodVerifier::FindLocksAtDexPc(ArtMethod* m, uint32_t dex_pc,
- std::vector<uint32_t>* monitor_enter_dex_pcs) {
+void MethodVerifier::FindLocksAtDexPc(
+ ArtMethod* m,
+ uint32_t dex_pc,
+ std::vector<MethodVerifier::DexLockInfo>* monitor_enter_dex_pcs) {
StackHandleScope<2> hs(Thread::Current());
Handle<mirror::DexCache> dex_cache(hs.NewHandle(m->GetDexCache()));
Handle<mirror::ClassLoader> class_loader(hs.NewHandle(m->GetClassLoader()));
@@ -2036,8 +2038,20 @@
// for a thread to be suspended).
if (monitor_enter_dex_pcs_ != nullptr && work_insn_idx_ == interesting_dex_pc_) {
monitor_enter_dex_pcs_->clear(); // The new work line is more accurate than the previous one.
- for (size_t i = 0; i < work_line_->GetMonitorEnterCount(); ++i) {
- monitor_enter_dex_pcs_->push_back(work_line_->GetMonitorEnterDexPc(i));
+
+ std::map<uint32_t, DexLockInfo> depth_to_lock_info;
+ auto collector = [&](uint32_t dex_reg, uint32_t depth) {
+ auto insert_pair = depth_to_lock_info.emplace(depth, DexLockInfo(depth));
+ auto it = insert_pair.first;
+ auto set_insert_pair = it->second.dex_registers.insert(dex_reg);
+ DCHECK(set_insert_pair.second);
+ };
+ work_line_->IterateRegToLockDepths(collector);
+ for (auto& pair : depth_to_lock_info) {
+ monitor_enter_dex_pcs_->push_back(pair.second);
+ // Map depth to dex PC.
+ (*monitor_enter_dex_pcs_)[monitor_enter_dex_pcs_->size() - 1].dex_pc =
+ work_line_->GetMonitorEnterDexPc(pair.second.dex_pc);
}
}
diff --git a/runtime/verifier/method_verifier.h b/runtime/verifier/method_verifier.h
index 813ce87..c885914 100644
--- a/runtime/verifier/method_verifier.h
+++ b/runtime/verifier/method_verifier.h
@@ -149,10 +149,21 @@
void Dump(std::ostream& os) REQUIRES_SHARED(Locks::mutator_lock_);
void Dump(VariableIndentationOutputStream* vios) REQUIRES_SHARED(Locks::mutator_lock_);
+ // Information structure for a lock held at a certain point in time.
+ struct DexLockInfo {
+ // The registers aliasing the lock.
+ std::set<uint32_t> dex_registers;
+ // The dex PC of the monitor-enter instruction.
+ uint32_t dex_pc;
+
+ explicit DexLockInfo(uint32_t dex_pc_in) {
+ dex_pc = dex_pc_in;
+ }
+ };
// Fills 'monitor_enter_dex_pcs' with the dex pcs of the monitor-enter instructions corresponding
// to the locks held at 'dex_pc' in method 'm'.
static void FindLocksAtDexPc(ArtMethod* m, uint32_t dex_pc,
- std::vector<uint32_t>* monitor_enter_dex_pcs)
+ std::vector<DexLockInfo>* monitor_enter_dex_pcs)
REQUIRES_SHARED(Locks::mutator_lock_);
// Returns the accessed field corresponding to the quick instruction's field
@@ -750,7 +761,7 @@
uint32_t interesting_dex_pc_;
// The container into which FindLocksAtDexPc should write the registers containing held locks,
// null if we're not doing FindLocksAtDexPc.
- std::vector<uint32_t>* monitor_enter_dex_pcs_;
+ std::vector<DexLockInfo>* monitor_enter_dex_pcs_;
// The types of any error that occurs.
std::vector<VerifyError> failures_;
diff --git a/runtime/verifier/register_line.h b/runtime/verifier/register_line.h
index 221aa80..71eb4d6 100644
--- a/runtime/verifier/register_line.h
+++ b/runtime/verifier/register_line.h
@@ -353,6 +353,23 @@
return monitors_[i];
}
+ // We give access to the lock depth map to avoid an expensive poll loop for FindLocksAtDexPC.
+ template <typename T>
+ void IterateRegToLockDepths(T fn) const {
+ for (const auto& pair : reg_to_lock_depths_) {
+ const uint32_t reg = pair.first;
+ uint32_t depths = pair.second;
+ uint32_t depth = 0;
+ while (depths != 0) {
+ if ((depths & 1) != 0) {
+ fn(reg, depth);
+ }
+ depths >>= 1;
+ depth++;
+ }
+ }
+ }
+
private:
void CopyRegToLockDepth(size_t dst, size_t src) {
auto it = reg_to_lock_depths_.find(src);
diff --git a/test/167-visit-locks/expected.txt b/test/167-visit-locks/expected.txt
new file mode 100644
index 0000000..5157c64
--- /dev/null
+++ b/test/167-visit-locks/expected.txt
@@ -0,0 +1,3 @@
+JNI_OnLoad called
+First
+Second
diff --git a/test/167-visit-locks/info.txt b/test/167-visit-locks/info.txt
new file mode 100644
index 0000000..d849bc3
--- /dev/null
+++ b/test/167-visit-locks/info.txt
@@ -0,0 +1 @@
+Regression test for b/68703210
diff --git a/test/167-visit-locks/run b/test/167-visit-locks/run
new file mode 100644
index 0000000..9365411
--- /dev/null
+++ b/test/167-visit-locks/run
@@ -0,0 +1,18 @@
+#!/bin/bash
+#
+# Copyright (C) 2017 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Use a smaller heap so it's easier to potentially fill up.
+exec ${RUN} $@ --runtime-option -Xmx2m
diff --git a/test/167-visit-locks/smali/TestSync.smali b/test/167-visit-locks/smali/TestSync.smali
new file mode 100644
index 0000000..5e68ad7
--- /dev/null
+++ b/test/167-visit-locks/smali/TestSync.smali
@@ -0,0 +1,119 @@
+.class LTestSync;
+.super Ljava/lang/Object;
+.source "Main.java"
+
+
+# direct methods
+.method constructor <init>()V
+ .registers 1
+
+ .prologue
+ .line 6
+ invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+
+ return-void
+.end method
+
+.method public static run()V
+ # v0-v2 were generated by javac+dx for the original src code, keeping them.
+ # v10..v19 are for tracking, aliasing and manipulating the first lock.
+ # v20..v29 are for tracking, aliasing and manipulating the second lock.
+ .registers 30
+
+ .prologue
+ .line 8
+ const-string v1, "First"
+
+ .line 9
+ const-string v2, "Second"
+
+ move-object v10, v1
+ const v1, 0x1
+
+ .line 10
+ monitor-enter v10
+
+ # Introduce a range of dead copies.
+ move-object v11, v10
+ move-object v12, v10
+ move-object v13, v10
+ move-object v14, v10
+ move-object v15, v10
+ move-object/16 v16, v10
+ move-object/16 v17, v10
+ move-object/16 v18, v10
+
+ # Introduce a copy that we'll use for unlock.
+ move-object/16 v19, v10
+
+ # Clobber the original alias.
+ const v10, 0x3
+
+ move-object/16 v20, v2
+ const v2, 0x2
+
+ .line 11
+ :try_start_b
+ monitor-enter v20
+ :try_end_c
+
+ # Introduce a range of dead copies.
+ move-object/16 v21, v20
+ move-object/16 v22, v20
+ move-object/16 v23, v20
+ move-object/16 v24, v20
+ move-object/16 v25, v20
+ move-object/16 v26, v20
+ move-object/16 v27, v20
+
+ # Introduce another copy that we will hold live.
+ move-object/16 v28, v20
+
+ # Clobber the original alias.
+ const v20, 0x5
+
+ # Introduce another copy that we'll use for unlock.
+ move-object/16 v29, v28
+
+ .catchall {:try_start_b .. :try_end_c} :catchall_15
+
+ .line 12
+ :try_start_c
+ invoke-static/range { v28 }, LMain;->run(Ljava/lang/Object;)V
+
+ .line 13
+ monitor-exit v29
+ :try_end_10
+ .catchall {:try_start_c .. :try_end_10} :catchall_12
+
+ .line 14
+ :try_start_10
+ monitor-exit v19
+ :try_end_11
+ .catchall {:try_start_10 .. :try_end_11} :catchall_15
+
+ .line 15
+ return-void
+
+ .line 13
+ :catchall_12
+ move-exception v0
+
+ :try_start_13
+ monitor-exit v29
+ :try_end_14
+ .catchall {:try_start_13 .. :try_end_14} :catchall_12
+
+ :try_start_14
+ throw v0
+
+ .line 14
+ :catchall_15
+ move-exception v0
+
+ monitor-exit v19
+ :try_end_17
+ .catchall {:try_start_14 .. :try_end_17} :catchall_15
+
+ throw v0
+.end method
diff --git a/test/167-visit-locks/src/Main.java b/test/167-visit-locks/src/Main.java
new file mode 100644
index 0000000..d8da927
--- /dev/null
+++ b/test/167-visit-locks/src/Main.java
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class Main {
+ public static void main(String[] args) throws Exception {
+ System.loadLibrary(args[0]);
+
+ Class.forName("TestSync").getMethod("run").invoke(null);
+ }
+
+ public static void run(Object o) {
+ testVisitLocks();
+ }
+
+ public static native void testVisitLocks();
+}
diff --git a/test/167-visit-locks/visit_locks.cc b/test/167-visit-locks/visit_locks.cc
new file mode 100644
index 0000000..e79c880
--- /dev/null
+++ b/test/167-visit-locks/visit_locks.cc
@@ -0,0 +1,74 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "jni.h"
+
+#include <iostream>
+
+#include "android-base/logging.h"
+
+#include "arch/context.h"
+#include "art_method.h"
+#include "base/macros.h"
+#include "base/mutex.h"
+#include "mirror/object-inl.h"
+#include "mirror/string.h"
+#include "monitor.h"
+#include "scoped_thread_state_change-inl.h"
+#include "stack.h"
+#include "thread-current-inl.h"
+
+namespace art {
+
+extern "C" JNIEXPORT void JNICALL Java_Main_testVisitLocks(JNIEnv*, jclass) {
+ ScopedObjectAccess soa(Thread::Current());
+
+ class VisitLocks : public StackVisitor {
+ public:
+ VisitLocks(Thread* thread, Context* context)
+ : StackVisitor(thread, context, StackWalkKind::kIncludeInlinedFrames) {
+ }
+
+ bool VisitFrame() OVERRIDE REQUIRES_SHARED(Locks::mutator_lock_) {
+ ArtMethod* m = GetMethod();
+
+ // Ignore runtime methods.
+ if (m == nullptr || m->IsRuntimeMethod()) {
+ return true;
+ }
+
+ if (m->PrettyMethod() == "void TestSync.run()") {
+ // Interesting frame.
+ Monitor::VisitLocks(this, Callback, nullptr);
+ return false;
+ }
+
+ return true;
+ }
+
+ static void Callback(mirror::Object* obj, void*) REQUIRES_SHARED(Locks::mutator_lock_) {
+ CHECK(obj != nullptr);
+ CHECK(obj->IsString());
+ std::cerr << obj->AsString()->ToModifiedUtf8() << std::endl;
+ }
+ };
+ Context* context = Context::Create();
+ VisitLocks vl(soa.Self(), context);
+ vl.WalkStack();
+ delete context;
+}
+
+} // namespace art
diff --git a/test/Android.bp b/test/Android.bp
index 2d526d2..01e424d 100644
--- a/test/Android.bp
+++ b/test/Android.bp
@@ -364,8 +364,9 @@
"141-class-unload/jni_unload.cc",
"148-multithread-gc-annotations/gc_coverage.cc",
"149-suspend-all-stress/suspend_all.cc",
- "203-multi-checkpoint/multi_checkpoint.cc",
"154-gc-loop/heap_interface.cc",
+ "167-visit-locks/visit_locks.cc",
+ "203-multi-checkpoint/multi_checkpoint.cc",
"454-get-vreg/get_vreg_jni.cc",
"457-regs/regs_jni.cc",
"461-get-reference-vreg/get_reference_vreg_jni.cc",