ART: Balanced locking
Change the verifier to check for balanced locking. When balanced
locking can't be guaranteed, use a new failure kind to punt to
the interpreter.
Add smali tests, with JNI code to check the balanced-locking result.
Bug: 23502994
Change-Id: Icd7db0be20ef2f69f0ac784de43dcba990035cd8
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 223268d..4f921bd 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -589,7 +589,7 @@
std::ostream& MethodVerifier::Fail(VerifyError error) {
// Mark the error type as encountered.
- encountered_failure_types_ |= (1U << static_cast<uint32_t>(error));
+ encountered_failure_types_ |= static_cast<uint32_t>(error);
switch (error) {
case VERIFY_ERROR_NO_CLASS:
@@ -601,6 +601,7 @@
case VERIFY_ERROR_INSTANTIATION:
case VERIFY_ERROR_CLASS_CHANGE:
case VERIFY_ERROR_FORCE_INTERPRETER:
+ case VERIFY_ERROR_LOCKING:
if (Runtime::Current()->IsAotCompiler() || !can_load_classes_) {
// If we're optimistically running verification at compile time, turn NO_xxx, ACCESS_xxx,
// class change and instantiation errors into soft verification errors so that we re-verify
@@ -631,12 +632,14 @@
}
}
break;
+
// Indication that verification should be retried at runtime.
case VERIFY_ERROR_BAD_CLASS_SOFT:
if (!allow_soft_failures_) {
have_pending_hard_failure_ = true;
}
break;
+
// Hard verification failures at compile time will still fail at runtime, so the class is
// marked as rejected to prevent it from being compiled.
case VERIFY_ERROR_BAD_CLASS_HARD: {
@@ -1657,6 +1660,33 @@
return DexFile::kDexNoIndex;
}
+// Setup a register line for the given return instruction.
+static void AdjustReturnLine(MethodVerifier* verifier,
+ const Instruction* ret_inst,
+ RegisterLine* line) {
+ Instruction::Code opcode = ret_inst->Opcode();
+
+ switch (opcode) {
+ case Instruction::RETURN_VOID:
+ case Instruction::RETURN_VOID_NO_BARRIER:
+ SafelyMarkAllRegistersAsConflicts(verifier, line);
+ break;
+
+ case Instruction::RETURN:
+ case Instruction::RETURN_OBJECT:
+ line->MarkAllRegistersAsConflictsExcept(verifier, ret_inst->VRegA_11x());
+ break;
+
+ case Instruction::RETURN_WIDE:
+ line->MarkAllRegistersAsConflictsExceptWide(verifier, ret_inst->VRegA_11x());
+ break;
+
+ default:
+ LOG(FATAL) << "Unknown return opcode " << opcode;
+ UNREACHABLE();
+ }
+}
+
bool MethodVerifier::CodeFlowVerifyInstruction(uint32_t* start_guess) {
// If we're doing FindLocksAtDexPc, check whether we're at the dex pc we care about.
// We want the state _before_ the instruction, for the case where the dex pc we're
@@ -3078,10 +3108,9 @@
} else if (have_pending_runtime_throw_failure_) {
/* checking interpreter will throw, mark following code as unreachable */
opcode_flags = Instruction::kThrow;
- have_any_pending_runtime_throw_failure_ = true;
- // Reset the pending_runtime_throw flag. The flag is a global to decouple Fail and is per
- // instruction.
- have_pending_runtime_throw_failure_ = false;
+ // Note: the flag must be reset as it is only global to decouple Fail and is semantically per
+ // instruction. However, RETURN checking may throw LOCKING errors, so we clear at the
+ // very end.
}
/*
* If we didn't just set the result register, clear it out. This ensures that you can only use
@@ -3250,16 +3279,7 @@
if (insn_flags_[next_insn_idx].IsReturn()) {
// For returns we only care about the operand to the return, all other registers are dead.
const Instruction* ret_inst = Instruction::At(code_item_->insns_ + next_insn_idx);
- Instruction::Code opcode = ret_inst->Opcode();
- if (opcode == Instruction::RETURN_VOID || opcode == Instruction::RETURN_VOID_NO_BARRIER) {
- SafelyMarkAllRegistersAsConflicts(this, work_line_.get());
- } else {
- if (opcode == Instruction::RETURN_WIDE) {
- work_line_->MarkAllRegistersAsConflictsExceptWide(this, ret_inst->VRegA_11x());
- } else {
- work_line_->MarkAllRegistersAsConflictsExcept(this, ret_inst->VRegA_11x());
- }
- }
+ AdjustReturnLine(this, ret_inst, work_line_.get());
}
RegisterLine* next_line = reg_table_.GetLine(next_insn_idx);
if (next_line != nullptr) {
@@ -3280,9 +3300,7 @@
/* If we're returning from the method, make sure monitor stack is empty. */
if ((opcode_flags & Instruction::kReturn) != 0) {
- if (!work_line_->VerifyMonitorStackEmpty(this)) {
- return false;
- }
+ work_line_->VerifyMonitorStackEmpty(this);
}
/*
@@ -3302,6 +3320,12 @@
DCHECK_LT(*start_guess, code_item_->insns_size_in_code_units_);
DCHECK(insn_flags_[*start_guess].IsOpcode());
+ if (have_pending_runtime_throw_failure_) {
+ have_any_pending_runtime_throw_failure_ = true;
+ // Reset the pending_runtime_throw flag now.
+ have_pending_runtime_throw_failure_ = false;
+ }
+
return true;
} // NOLINT(readability/fn_size)
@@ -4425,31 +4449,15 @@
* there's nothing to "merge". Copy the registers over and mark it as changed. (This is the
* only way a register can transition out of "unknown", so this is not just an optimization.)
*/
- if (!insn_flags_[next_insn].IsReturn()) {
- target_line->CopyFromLine(merge_line);
- } else {
+ target_line->CopyFromLine(merge_line);
+ if (insn_flags_[next_insn].IsReturn()) {
// Verify that the monitor stack is empty on return.
- if (!merge_line->VerifyMonitorStackEmpty(this)) {
- return false;
- }
+ merge_line->VerifyMonitorStackEmpty(this);
+
// For returns we only care about the operand to the return, all other registers are dead.
// Initialize them as conflicts so they don't add to GC and deoptimization information.
const Instruction* ret_inst = Instruction::At(code_item_->insns_ + next_insn);
- Instruction::Code opcode = ret_inst->Opcode();
- if (opcode == Instruction::RETURN_VOID || opcode == Instruction::RETURN_VOID_NO_BARRIER) {
- // Explicitly copy the this-initialized flag from the merge-line, as we didn't copy its
- // state. Must be done before SafelyMarkAllRegistersAsConflicts as that will do the
- // super-constructor-call checking.
- target_line->CopyThisInitialized(*merge_line);
- SafelyMarkAllRegistersAsConflicts(this, target_line);
- } else {
- target_line->CopyFromLine(merge_line);
- if (opcode == Instruction::RETURN_WIDE) {
- target_line->MarkAllRegistersAsConflictsExceptWide(this, ret_inst->VRegA_11x());
- } else {
- target_line->MarkAllRegistersAsConflictsExcept(this, ret_inst->VRegA_11x());
- }
- }
+ AdjustReturnLine(this, ret_inst, target_line);
}
} else {
std::unique_ptr<RegisterLine> copy(gDebugVerify ?
diff --git a/runtime/verifier/method_verifier.h b/runtime/verifier/method_verifier.h
index d0841f0..b57abf5 100644
--- a/runtime/verifier/method_verifier.h
+++ b/runtime/verifier/method_verifier.h
@@ -67,17 +67,17 @@
* to be rewritten to fail at runtime.
*/
enum VerifyError {
- VERIFY_ERROR_BAD_CLASS_HARD = 0, // VerifyError; hard error that skips compilation.
- VERIFY_ERROR_BAD_CLASS_SOFT = 1, // VerifyError; soft error that verifies again at runtime.
+ VERIFY_ERROR_BAD_CLASS_HARD = 1, // VerifyError; hard error that skips compilation.
+ VERIFY_ERROR_BAD_CLASS_SOFT = 2, // VerifyError; soft error that verifies again at runtime.
- VERIFY_ERROR_NO_CLASS = 2, // NoClassDefFoundError.
- VERIFY_ERROR_NO_FIELD = 3, // NoSuchFieldError.
- VERIFY_ERROR_NO_METHOD = 4, // NoSuchMethodError.
- VERIFY_ERROR_ACCESS_CLASS = 5, // IllegalAccessError.
- VERIFY_ERROR_ACCESS_FIELD = 6, // IllegalAccessError.
- VERIFY_ERROR_ACCESS_METHOD = 7, // IllegalAccessError.
- VERIFY_ERROR_CLASS_CHANGE = 8, // IncompatibleClassChangeError.
- VERIFY_ERROR_INSTANTIATION = 9, // InstantiationError.
+ VERIFY_ERROR_NO_CLASS = 4, // NoClassDefFoundError.
+ VERIFY_ERROR_NO_FIELD = 8, // NoSuchFieldError.
+ VERIFY_ERROR_NO_METHOD = 16, // NoSuchMethodError.
+ VERIFY_ERROR_ACCESS_CLASS = 32, // IllegalAccessError.
+ VERIFY_ERROR_ACCESS_FIELD = 64, // IllegalAccessError.
+ VERIFY_ERROR_ACCESS_METHOD = 128, // IllegalAccessError.
+ VERIFY_ERROR_CLASS_CHANGE = 256, // IncompatibleClassChangeError.
+ VERIFY_ERROR_INSTANTIATION = 512, // InstantiationError.
// For opcodes that don't have complete verifier support (such as lambda opcodes),
// we need a way to continue execution at runtime without attempting to re-verify
// (since we know it will fail no matter what). Instead, run as the interpreter
@@ -85,9 +85,11 @@
// on the fly.
//
// TODO: Once all new opcodes have implemented full verifier support, this can be removed.
- VERIFY_ERROR_FORCE_INTERPRETER = 10, // Skip the verification phase at runtime;
- // force the interpreter to do access checks.
- // (sets a soft fail at compile time).
+ VERIFY_ERROR_FORCE_INTERPRETER = 1024, // Skip the verification phase at runtime;
+ // force the interpreter to do access checks.
+ // (sets a soft fail at compile time).
+ VERIFY_ERROR_LOCKING = 2048, // Could not guarantee balanced locking. This should be
+ // punted to the interpreter with access checks.
};
std::ostream& operator<<(std::ostream& os, const VerifyError& rhs);
diff --git a/runtime/verifier/register_line-inl.h b/runtime/verifier/register_line-inl.h
index bee5834..1df2428 100644
--- a/runtime/verifier/register_line-inl.h
+++ b/runtime/verifier/register_line-inl.h
@@ -25,6 +25,10 @@
namespace art {
namespace verifier {
+// Should we dump a warning on failures to verify balanced locking? That would be an indication to
+// developers that their code will be slow.
+static constexpr bool kDumpLockFailures = true;
+
inline const RegType& RegisterLine::GetRegisterType(MethodVerifier* verifier, uint32_t vsrc) const {
// The register index was validated during the static pass, so we don't need to check it here.
DCHECK_LT(vsrc, num_regs_);
@@ -167,12 +171,14 @@
return true;
}
-inline bool RegisterLine::VerifyMonitorStackEmpty(MethodVerifier* verifier) const {
+inline void RegisterLine::VerifyMonitorStackEmpty(MethodVerifier* verifier) const {
if (MonitorStackDepth() != 0) {
- verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "expected empty monitor stack";
- return false;
- } else {
- return true;
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "expected empty monitor stack in "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
}
}
diff --git a/runtime/verifier/register_line.cc b/runtime/verifier/register_line.cc
index bb6df76..33c90e3 100644
--- a/runtime/verifier/register_line.cc
+++ b/runtime/verifier/register_line.cc
@@ -344,14 +344,22 @@
verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "monitor-enter on non-object ("
<< reg_type << ")";
} else if (monitors_.size() >= 32) {
- verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "monitor-enter stack overflow: "
- << monitors_.size();
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "monitor-enter stack overflow while verifying "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
} else {
if (SetRegToLockDepth(reg_idx, monitors_.size())) {
monitors_.push_back(insn_idx);
} else {
- verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "unexpected monitor-enter on register v" <<
- reg_idx;
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "unexpected monitor-enter on register v" << reg_idx << " in "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
}
}
}
@@ -361,16 +369,21 @@
if (!reg_type.IsReferenceTypes()) {
verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "monitor-exit on non-object (" << reg_type << ")";
} else if (monitors_.empty()) {
- verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "monitor-exit stack underflow";
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "monitor-exit stack underflow while verifying "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
} else {
monitors_.pop_back();
if (!IsSetLockDepth(reg_idx, monitors_.size())) {
- // Bug 3215458: Locks and unlocks are on objects, if that object is a literal then before
- // format "036" the constant collector may create unlocks on the same object but referenced
- // via different registers.
- ((verifier->DexFileVersion() >= 36) ? verifier->Fail(VERIFY_ERROR_BAD_CLASS_SOFT)
- : verifier->LogVerifyInfo())
- << "monitor-exit not unlocking the top of the monitor stack";
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "monitor-exit not unlocking the top of the monitor stack while verifying "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
} else {
// Record the register was unlocked
ClearRegToLockDepth(reg_idx, monitors_.size());
@@ -392,8 +405,13 @@
}
if (monitors_.size() > 0 || incoming_line->monitors_.size() > 0) {
if (monitors_.size() != incoming_line->monitors_.size()) {
- LOG(WARNING) << "mismatched stack depths (depth=" << MonitorStackDepth()
- << ", incoming depth=" << incoming_line->MonitorStackDepth() << ")";
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "mismatched stack depths (depth=" << MonitorStackDepth()
+ << ", incoming depth=" << incoming_line->MonitorStackDepth() << ") in "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
} else if (reg_to_lock_depths_ != incoming_line->reg_to_lock_depths_) {
for (uint32_t idx = 0; idx < num_regs_; idx++) {
size_t depths = reg_to_lock_depths_.count(idx);
@@ -402,14 +420,35 @@
if (depths == 0 || incoming_depths == 0) {
reg_to_lock_depths_.erase(idx);
} else {
- LOG(WARNING) << "mismatched stack depths for register v" << idx
- << ": " << depths << " != " << incoming_depths;
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "mismatched stack depths for register v" << idx
+ << ": " << depths << " != " << incoming_depths << " in "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
+ break;
+ }
+ } else if (depths > 0) {
+ // Check whether they're actually the same levels.
+ uint32_t locked_levels = reg_to_lock_depths_.find(idx)->second;
+ uint32_t incoming_locked_levels = incoming_line->reg_to_lock_depths_.find(idx)->second;
+ if (locked_levels != incoming_locked_levels) {
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "mismatched lock levels for register v" << idx << ": "
+ << std::hex << locked_levels << std::dec << " != "
+ << std::hex << incoming_locked_levels << std::dec << " in "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
break;
}
}
}
}
}
+
// Check whether "this" was initialized in both paths.
if (this_initialized_ && !incoming_line->this_initialized_) {
this_initialized_ = false;
diff --git a/runtime/verifier/register_line.h b/runtime/verifier/register_line.h
index 41f1e28..46db1c6 100644
--- a/runtime/verifier/register_line.h
+++ b/runtime/verifier/register_line.h
@@ -185,7 +185,9 @@
// Compare two register lines. Returns 0 if they match.
// Using this for a sort is unwise, since the value can change based on machine endianness.
int CompareLine(const RegisterLine* line2) const {
- DCHECK(monitors_ == line2->monitors_);
+ if (monitors_ != line2->monitors_) {
+ return 1;
+ }
// TODO: DCHECK(reg_to_lock_depths_ == line2->reg_to_lock_depths_);
return memcmp(&line_, &line2->line_, num_regs_ * sizeof(uint16_t));
}
@@ -298,8 +300,8 @@
}
// We expect no monitors to be held at certain points, such a method returns. Verify the stack
- // is empty, failing and returning false if not.
- bool VerifyMonitorStackEmpty(MethodVerifier* verifier) const;
+ // is empty, queueing a LOCKING error else.
+ void VerifyMonitorStackEmpty(MethodVerifier* verifier) const;
bool MergeRegisters(MethodVerifier* verifier, const RegisterLine* incoming_line)
SHARED_REQUIRES(Locks::mutator_lock_);