Revert^2 "Cleanup the code to determine instrumentation level"
This reverts commit cb8f8c12872d36304dbac80fbd08d8400a757fe5.
Reason for revert: Reland commit 21ef5a80704da06bdb2945be1c735ca86e8f1860 with fixes
Fixes:
In JITed code when we compare a bool variable
(instrumentation_stubs_installed_) we should use cmpb and not cmpw. This
wasn't caught earlier because the next three variables are bool which
are related to instrumentation are false when instrumentation is disabled.
Test: testrunner.py --host --redefine-stress -t 421-large-frame,
testrunner.py --host --redefine-stress -t 545-tracing-and-jit
Change-Id: Iba363fb62d0cb41bcbc86af202eae73a833ba267
diff --git a/compiler/optimizing/code_generator_arm64.cc b/compiler/optimizing/code_generator_arm64.cc
index fe97c7f..2cf2571 100644
--- a/compiler/optimizing/code_generator_arm64.cc
+++ b/compiler/optimizing/code_generator_arm64.cc
@@ -1159,7 +1159,7 @@
uint64_t address = reinterpret_cast64<uint64_t>(Runtime::Current()->GetInstrumentation());
int offset = instrumentation::Instrumentation::NeedsEntryExitHooksOffset().Int32Value();
__ Mov(temp, address + offset);
- __ Ldrh(value, MemOperand(temp, 0));
+ __ Ldrb(value, MemOperand(temp, 0));
__ Cbnz(value, slow_path->GetEntryLabel());
__ Bind(slow_path->GetExitLabel());
}
diff --git a/compiler/optimizing/code_generator_arm_vixl.cc b/compiler/optimizing/code_generator_arm_vixl.cc
index b58ef121..62c285d 100644
--- a/compiler/optimizing/code_generator_arm_vixl.cc
+++ b/compiler/optimizing/code_generator_arm_vixl.cc
@@ -2154,7 +2154,7 @@
int offset = instrumentation::Instrumentation::NeedsEntryExitHooksOffset().Int32Value();
uint32_t address = reinterpret_cast32<uint32_t>(Runtime::Current()->GetInstrumentation());
__ Mov(temp, address + offset);
- __ Ldrh(temp, MemOperand(temp, 0));
+ __ Ldrb(temp, MemOperand(temp, 0));
__ CompareAndBranchIfNonZero(temp, slow_path->GetEntryLabel());
__ Bind(slow_path->GetExitLabel());
}
diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc
index b5677e5..11c15d6 100644
--- a/compiler/optimizing/code_generator_x86.cc
+++ b/compiler/optimizing/code_generator_x86.cc
@@ -1166,8 +1166,8 @@
uint64_t address = reinterpret_cast64<uint64_t>(Runtime::Current()->GetInstrumentation());
int offset = instrumentation::Instrumentation::NeedsEntryExitHooksOffset().Int32Value();
- __ cmpw(Address::Absolute(address + offset), Immediate(0));
- __ j(kEqual, slow_path->GetEntryLabel());
+ __ cmpb(Address::Absolute(address + offset), Immediate(0));
+ __ j(kNotEqual, slow_path->GetEntryLabel());
__ Bind(slow_path->GetExitLabel());
}
diff --git a/compiler/optimizing/code_generator_x86_64.cc b/compiler/optimizing/code_generator_x86_64.cc
index a2faa43..6deb035 100644
--- a/compiler/optimizing/code_generator_x86_64.cc
+++ b/compiler/optimizing/code_generator_x86_64.cc
@@ -1534,7 +1534,7 @@
uint64_t address = reinterpret_cast64<uint64_t>(Runtime::Current()->GetInstrumentation());
int offset = instrumentation::Instrumentation::NeedsEntryExitHooksOffset().Int32Value();
__ movq(CpuRegister(TMP), Immediate(address + offset));
- __ cmpw(Address(CpuRegister(TMP), 0), Immediate(0));
+ __ cmpb(Address(CpuRegister(TMP), 0), Immediate(0));
__ j(kNotEqual, slow_path->GetEntryLabel());
__ Bind(slow_path->GetExitLabel());
}
diff --git a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
index b29da65..c14dee4 100644
--- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
@@ -2614,6 +2614,7 @@
<< "Enter instrumentation exit stub with pending exception " << self->GetException()->Dump();
instrumentation::Instrumentation* instr = Runtime::Current()->GetInstrumentation();
+ DCHECK(instr->AreExitStubsInstalled());
bool is_ref;
JValue return_value = instr->GetReturnValue(self, method, &is_ref, gpr_result, fpr_result);
bool deoptimize = false;
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 87db899..8ef5ef4 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -168,9 +168,7 @@
Instrumentation::Instrumentation()
: current_force_deopt_id_(0),
instrumentation_stubs_installed_(false),
- entry_exit_stubs_installed_(false),
- interpreter_stubs_installed_(false),
- interpret_only_(false),
+ instrumentation_level_(InstrumentationLevel::kInstrumentNothing),
forced_interpret_only_(false),
have_method_entry_listeners_(false),
have_method_exit_listeners_(false),
@@ -276,7 +274,7 @@
return;
}
const void* new_quick_code;
- bool uninstall = !entry_exit_stubs_installed_ && !interpreter_stubs_installed_;
+ bool uninstall = (instrumentation_level_ == InstrumentationLevel::kInstrumentNothing);
Runtime* const runtime = Runtime::Current();
ClassLinker* const class_linker = runtime->GetClassLinker();
bool is_class_initialized = method->GetDeclaringClass()->IsInitialized();
@@ -289,15 +287,14 @@
new_quick_code = GetQuickResolutionStub();
}
} else { // !uninstall
- if ((interpreter_stubs_installed_ || forced_interpret_only_ || IsDeoptimized(method)) &&
- !method->IsNative()) {
+ if ((InterpretOnly() || IsDeoptimized(method)) && !method->IsNative()) {
new_quick_code = GetQuickToInterpreterBridge();
} else {
// Do not overwrite resolution trampoline. When the trampoline initializes the method's
// class, all its static methods code will be set to the instrumentation entry point.
// For more details, see ClassLinker::FixupStaticTrampolines.
if (is_class_initialized || !method->IsStatic() || method->IsConstructor()) {
- if (entry_exit_stubs_installed_) {
+ if (EntryExitStubsInstalled()) {
// This needs to be checked first since the instrumentation entrypoint will be able to
// find the actual JIT compiled code that corresponds to this method.
const void* code = method->GetEntryPointFromQuickCompiledCodePtrSize(kRuntimePointerSize);
@@ -750,13 +747,7 @@
}
Instrumentation::InstrumentationLevel Instrumentation::GetCurrentInstrumentationLevel() const {
- if (interpreter_stubs_installed_) {
- return InstrumentationLevel::kInstrumentWithInterpreter;
- } else if (entry_exit_stubs_installed_) {
- return InstrumentationLevel::kInstrumentWithInstrumentationStubs;
- } else {
- return InstrumentationLevel::kInstrumentNothing;
- }
+ return instrumentation_level_;
}
bool Instrumentation::RequiresInstrumentationInstallation(InstrumentationLevel new_level) const {
@@ -798,6 +789,10 @@
UpdateStubs();
}
+void Instrumentation::UpdateInstrumentationLevel(InstrumentationLevel requested_level) {
+ instrumentation_level_ = requested_level;
+}
+
void Instrumentation::UpdateStubs() {
// Look for the highest required instrumentation level.
InstrumentationLevel requested_level = InstrumentationLevel::kInstrumentNothing;
@@ -810,9 +805,6 @@
<< "Use trampolines: " << can_use_instrumentation_trampolines_ << " level "
<< requested_level;
- interpret_only_ = (requested_level == InstrumentationLevel::kInstrumentWithInterpreter) ||
- forced_interpret_only_;
-
if (!RequiresInstrumentationInstallation(requested_level)) {
// We're already set.
return;
@@ -821,15 +813,8 @@
Runtime* runtime = Runtime::Current();
Locks::mutator_lock_->AssertExclusiveHeld(self);
Locks::thread_list_lock_->AssertNotHeld(self);
+ UpdateInstrumentationLevel(requested_level);
if (requested_level > InstrumentationLevel::kInstrumentNothing) {
- if (requested_level == InstrumentationLevel::kInstrumentWithInterpreter) {
- interpreter_stubs_installed_ = true;
- entry_exit_stubs_installed_ = true;
- } else {
- CHECK_EQ(requested_level, InstrumentationLevel::kInstrumentWithInstrumentationStubs);
- entry_exit_stubs_installed_ = true;
- interpreter_stubs_installed_ = false;
- }
InstallStubsClassVisitor visitor(this);
runtime->GetClassLinker()->VisitClasses(&visitor);
instrumentation_stubs_installed_ = true;
@@ -838,8 +823,6 @@
InstrumentThreadStack(thread, /* deopt_all_frames= */ false);
}
} else {
- interpreter_stubs_installed_ = false;
- entry_exit_stubs_installed_ = false;
InstallStubsClassVisitor visitor(this);
runtime->GetClassLinker()->VisitClasses(&visitor);
// Restore stack only if there is no method currently deoptimized.
@@ -947,14 +930,14 @@
if (LIKELY(!instrumentation_stubs_installed_)) {
new_quick_code = quick_code;
} else {
- if ((interpreter_stubs_installed_ || IsDeoptimized(method)) && !method->IsNative()) {
+ if ((InterpreterStubsInstalled() || IsDeoptimized(method)) && !method->IsNative()) {
new_quick_code = GetQuickToInterpreterBridge();
} else {
ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
if (class_linker->IsQuickResolutionStub(quick_code) ||
class_linker->IsQuickToInterpreterBridge(quick_code)) {
new_quick_code = quick_code;
- } else if (entry_exit_stubs_installed_ &&
+ } else if (EntryExitStubsInstalled() &&
// We need to make sure not to replace anything that InstallStubsForMethod
// wouldn't. Specifically we cannot stub out Proxy.<init> since subtypes copy the
// implementation directly and this will confuse the instrumentation trampolines.
@@ -976,7 +959,7 @@
// enter here on a soon-to-be deleted ArtMethod. Updating the entrypoint is OK though, as
// the ArtMethod is still in memory.
const void* new_quick_code = quick_code;
- if (UNLIKELY(instrumentation_stubs_installed_) && entry_exit_stubs_installed_) {
+ if (UNLIKELY(instrumentation_stubs_installed_) && EntryExitStubsInstalled()) {
new_quick_code = GetQuickInstrumentationEntryPoint();
}
UpdateEntrypoints(method, new_quick_code);
@@ -1047,7 +1030,7 @@
CHECK(has_not_been_deoptimized) << "Method " << ArtMethod::PrettyMethod(method)
<< " is already deoptimized";
}
- if (!interpreter_stubs_installed_) {
+ if (!InterpreterStubsInstalled()) {
UpdateEntrypoints(method, GetQuickInstrumentationEntryPoint());
// Install instrumentation exit stub and instrumentation frames. We may already have installed
@@ -1079,7 +1062,7 @@
}
// Restore code and possibly stack only if we did not deoptimize everything.
- if (!interpreter_stubs_installed_) {
+ if (!InterpreterStubsInstalled()) {
// Restore its code or resolution trampoline.
ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
if (method->IsStatic() && !method->IsConstructor() &&
@@ -1093,7 +1076,7 @@
}
// If there is no deoptimized method left, we can restore the stack of each thread.
- if (empty && !entry_exit_stubs_installed_) {
+ if (empty && !EntryExitStubsInstalled()) {
MutexLock mu(self, *Locks::thread_list_lock_);
Runtime::Current()->GetThreadList()->ForEach(InstrumentationRestoreStack, this);
instrumentation_stubs_installed_ = false;
@@ -1142,7 +1125,7 @@
if (!HasMethodEntryListeners() && !HasMethodExitListeners()) {
return false;
}
- return !deoptimization_enabled_ && !interpreter_stubs_installed_;
+ return !deoptimization_enabled_ && !InterpreterStubsInstalled();
}
void Instrumentation::DeoptimizeEverything(const char* key) {
@@ -1151,7 +1134,7 @@
}
void Instrumentation::UndeoptimizeEverything(const char* key) {
- CHECK(interpreter_stubs_installed_);
+ CHECK(InterpreterStubsInstalled());
CHECK(deoptimization_enabled_);
ConfigureStubs(key, InstrumentationLevel::kInstrumentNothing);
}
@@ -1174,7 +1157,7 @@
// This is called by instrumentation entry only and that should never be getting proxy methods.
DCHECK(!method->IsProxyMethod()) << method->PrettyMethod();
ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
- if (LIKELY(!instrumentation_stubs_installed_ && !interpreter_stubs_installed_)) {
+ if (LIKELY(!InterpreterStubsInstalled())) {
// In general we just return whatever the method thinks its entrypoint is here. The only
// exception is if it still has the instrumentation entrypoint. That means we are racing another
// thread getting rid of instrumentation which is unexpected but possible. In that case we want
@@ -1183,24 +1166,18 @@
DCHECK(code != nullptr);
if (code != GetQuickInstrumentationEntryPoint()) {
return code;
- } else if (method->IsNative()) {
- return class_linker->GetQuickOatCodeFor(method);
}
// We don't know what it is. Fallthough to try to find the code from the JIT or Oat file.
- } else if (method->IsNative()) {
+ }
+
+ if (method->IsNative()) {
// TODO We could have JIT compiled native entrypoints. It might be worth it to find these.
return class_linker->GetQuickOatCodeFor(method);
- } else if (UNLIKELY(interpreter_stubs_installed_)) {
+ } else if (!NeedDebugVersionFor(method) && !InterpreterStubsInstalled()) {
+ return class_linker->GetQuickOatCodeFor(method);
+ } else {
return GetQuickToInterpreterBridge();
}
- // Since the method cannot be native due to ifs above we can always fall back to interpreter
- // bridge.
- const void* result = GetQuickToInterpreterBridge();
- if (!NeedDebugVersionFor(method)) {
- // If we don't need a debug version we should see what the oat file/class linker has to say.
- result = class_linker->GetQuickOatCodeFor(method);
- }
- return result;
}
const void* Instrumentation::GetQuickCodeFor(ArtMethod* method, PointerSize pointer_size) const {
@@ -1543,7 +1520,7 @@
}
}
return (visitor.caller != nullptr) &&
- (interpreter_stubs_installed_ || IsDeoptimized(visitor.caller) ||
+ (InterpreterStubsInstalled() || IsDeoptimized(visitor.caller) ||
self->IsForceInterpreter() ||
// NB Since structurally obsolete compiled methods might have the offsets of
// methods/fields compiled in we need to go back to interpreter whenever we hit
diff --git a/runtime/instrumentation.h b/runtime/instrumentation.h
index 4f4bb42..a505ce3 100644
--- a/runtime/instrumentation.h
+++ b/runtime/instrumentation.h
@@ -207,6 +207,11 @@
Instrumentation();
static constexpr MemberOffset NeedsEntryExitHooksOffset() {
+ // Assert that instrumentation_stubs_installed_ is 8bits wide. If the size changes
+ // update the compare instructions in the code generator when generating checks for
+ // MethodEntryExitHooks.
+ static_assert(sizeof(instrumentation_stubs_installed_) == 1,
+ "instrumentation_stubs_installed_ isn't expected size");
return MemberOffset(OFFSETOF_MEMBER(Instrumentation, instrumentation_stubs_installed_));
}
@@ -231,7 +236,7 @@
REQUIRES(!GetDeoptimizedMethodsLock());
bool AreAllMethodsDeoptimized() const {
- return interpreter_stubs_installed_;
+ return InterpreterStubsInstalled();
}
bool ShouldNotifyMethodEnterExitEvents() const REQUIRES_SHARED(Locks::mutator_lock_);
@@ -327,13 +332,21 @@
REQUIRES_SHARED(Locks::mutator_lock_);
void ForceInterpretOnly() {
- interpret_only_ = true;
forced_interpret_only_ = true;
}
+ bool EntryExitStubsInstalled() const {
+ return instrumentation_level_ == InstrumentationLevel::kInstrumentWithInstrumentationStubs ||
+ instrumentation_level_ == InstrumentationLevel::kInstrumentWithInterpreter;
+ }
+
+ bool InterpreterStubsInstalled() const {
+ return instrumentation_level_ == InstrumentationLevel::kInstrumentWithInterpreter;
+ }
+
// Called by ArtMethod::Invoke to determine dispatch mechanism.
bool InterpretOnly() const {
- return interpret_only_;
+ return forced_interpret_only_ || InterpreterStubsInstalled();
}
bool IsForcedInterpretOnly() const {
@@ -573,6 +586,9 @@
// directly call entry / exit hooks and don't need the stub.
bool CodeNeedsEntryExitStub(const void* code, ArtMethod* method);
+ // Update the current instrumentation_level_.
+ void UpdateInstrumentationLevel(InstrumentationLevel level);
+
// Does the job of installing or removing instrumentation code within methods.
// In order to support multiple clients using instrumentation at the same time,
// the caller must pass a unique key (a string) identifying it so we remind which
@@ -665,14 +681,11 @@
// Have we hijacked ArtMethod::code_ so that it calls instrumentation/interpreter code?
bool instrumentation_stubs_installed_;
- // Have we hijacked ArtMethod::code_ to reference the enter/exit stubs?
- bool entry_exit_stubs_installed_;
-
- // Have we hijacked ArtMethod::code_ to reference the enter interpreter stub?
- bool interpreter_stubs_installed_;
-
- // Do we need the fidelity of events that we only get from running within the interpreter?
- bool interpret_only_;
+ // The required level of instrumentation. This could be one of the following values:
+ // kInstrumentNothing: no instrumentation support is needed
+ // kInstrumentWithInstrumentationStubs: needs support to call method entry/exit stubs.
+ // kInstrumentWithInterpreter: only execute with interpreter
+ Instrumentation::InstrumentationLevel instrumentation_level_;
// Did the runtime request we only run in the interpreter? ie -Xint mode.
bool forced_interpret_only_;
