Revert "Revert "Move rewritten StringFactory call results into dex registers for deopt""
Potential gc points can make the result value stale. We now set the result value
to null proactively once it's moved to shadow frame registers. IsStringInit()
is written in a way that does string comparison instead of requiring method
resolution so that it doesn't have a gc point. Also we don't cache the callee
method during frame unwinding since the method may be rewritten already.
Bug: 28555675
Change-Id: Ic51511a4a0fc84a852d8d907f91e7835f49ac478
diff --git a/runtime/interpreter/interpreter.cc b/runtime/interpreter/interpreter.cc
index 1d0e600..8c42b3a 100644
--- a/runtime/interpreter/interpreter.cc
+++ b/runtime/interpreter/interpreter.cc
@@ -484,6 +484,36 @@
self->PopShadowFrame();
}
+static bool IsStringInit(const Instruction* instr, ArtMethod* caller)
+ SHARED_REQUIRES(Locks::mutator_lock_) {
+ if (instr->Opcode() == Instruction::INVOKE_DIRECT ||
+ instr->Opcode() == Instruction::INVOKE_DIRECT_RANGE) {
+ // Instead of calling ResolveMethod() which has suspend point and can trigger
+ // GC, look up the callee method symbolically.
+ uint16_t callee_method_idx = (instr->Opcode() == Instruction::INVOKE_DIRECT_RANGE) ?
+ instr->VRegB_3rc() : instr->VRegB_35c();
+ const DexFile* dex_file = caller->GetDexFile();
+ const DexFile::MethodId& method_id = dex_file->GetMethodId(callee_method_idx);
+ const char* class_name = dex_file->StringByTypeIdx(method_id.class_idx_);
+ const char* method_name = dex_file->GetMethodName(method_id);
+ // Compare method's class name and method name against string init.
+ // It's ok since it's not allowed to create your own java/lang/String.
+ // TODO: verify that assumption.
+ if ((strcmp(class_name, "Ljava/lang/String;") == 0) &&
+ (strcmp(method_name, "<init>") == 0)) {
+ return true;
+ }
+ }
+ return false;
+}
+
+static int16_t GetReceiverRegisterForStringInit(const Instruction* instr) {
+ DCHECK(instr->Opcode() == Instruction::INVOKE_DIRECT_RANGE ||
+ instr->Opcode() == Instruction::INVOKE_DIRECT);
+ return (instr->Opcode() == Instruction::INVOKE_DIRECT_RANGE) ?
+ instr->VRegC_3rc() : instr->VRegC_35c();
+}
+
void EnterInterpreterFromDeoptimize(Thread* self,
ShadowFrame* shadow_frame,
bool from_code,
@@ -519,22 +549,38 @@
// TODO: should be tested more once b/17586779 is fixed.
const Instruction* instr = Instruction::At(&code_item->insns_[dex_pc]);
if (instr->IsInvoke()) {
+ if (IsStringInit(instr, shadow_frame->GetMethod())) {
+ uint16_t this_obj_vreg = GetReceiverRegisterForStringInit(instr);
+ // Move the StringFactory.newStringFromChars() result into the register representing
+ // "this object" when invoking the string constructor in the original dex instruction.
+ // Also move the result into all aliases.
+ DCHECK(value.GetL()->IsString());
+ SetStringInitValueToAllAliases(shadow_frame, this_obj_vreg, value);
+ // Calling string constructor in the original dex code doesn't generate a result value.
+ value.SetJ(0);
+ }
new_dex_pc = dex_pc + instr->SizeInCodeUnits();
} else if (instr->Opcode() == Instruction::NEW_INSTANCE) {
// It's possible to deoptimize at a NEW_INSTANCE dex instruciton that's for a
// java string, which is turned into a call into StringFactory.newEmptyString();
+ // Move the StringFactory.newEmptyString() result into the destination register.
+ DCHECK(value.GetL()->IsString());
+ shadow_frame->SetVRegReference(instr->VRegA_21c(), value.GetL());
+ // new-instance doesn't generate a result value.
+ value.SetJ(0);
+ // Skip the dex instruction since we essentially come back from an invocation.
+ new_dex_pc = dex_pc + instr->SizeInCodeUnits();
if (kIsDebugBuild) {
ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
+ // This is a suspend point. But it's ok since value has been set into shadow_frame.
mirror::Class* klass = class_linker->ResolveType(
instr->VRegB_21c(), shadow_frame->GetMethod());
DCHECK(klass->IsStringClass());
}
- // Skip the dex instruction since we essentially come back from an invocation.
- new_dex_pc = dex_pc + instr->SizeInCodeUnits();
} else {
- DCHECK(false) << "Unexpected instruction opcode " << instr->Opcode()
- << " at dex_pc " << dex_pc
- << " of method: " << PrettyMethod(shadow_frame->GetMethod(), false);
+ CHECK(false) << "Unexpected instruction opcode " << instr->Opcode()
+ << " at dex_pc " << dex_pc
+ << " of method: " << PrettyMethod(shadow_frame->GetMethod(), false);
}
} else {
// Nothing to do, the dex_pc is the one at which the code requested
diff --git a/runtime/interpreter/interpreter_common.cc b/runtime/interpreter/interpreter_common.cc
index 12d70c5..53d5e43 100644
--- a/runtime/interpreter/interpreter_common.cc
+++ b/runtime/interpreter/interpreter_common.cc
@@ -540,6 +540,30 @@
result, method->GetInterfaceMethodIfProxy(sizeof(void*))->GetShorty());
}
+void SetStringInitValueToAllAliases(ShadowFrame* shadow_frame,
+ uint16_t this_obj_vreg,
+ JValue result)
+ SHARED_REQUIRES(Locks::mutator_lock_) {
+ Object* existing = shadow_frame->GetVRegReference(this_obj_vreg);
+ if (existing == nullptr) {
+ // If it's null, we come from compiled code that was deoptimized. Nothing to do,
+ // as the compiler verified there was no alias.
+ // Set the new string result of the StringFactory.
+ shadow_frame->SetVRegReference(this_obj_vreg, result.GetL());
+ return;
+ }
+ // Set the string init result into all aliases.
+ for (uint32_t i = 0, e = shadow_frame->NumberOfVRegs(); i < e; ++i) {
+ if (shadow_frame->GetVRegReference(i) == existing) {
+ DCHECK_EQ(shadow_frame->GetVRegReference(i),
+ reinterpret_cast<mirror::Object*>(shadow_frame->GetVReg(i)));
+ shadow_frame->SetVRegReference(i, result.GetL());
+ DCHECK_EQ(shadow_frame->GetVRegReference(i),
+ reinterpret_cast<mirror::Object*>(shadow_frame->GetVReg(i)));
+ }
+ }
+}
+
template <bool is_range,
bool do_assignability_check,
size_t kVarArgMax>
@@ -739,24 +763,7 @@
}
if (string_init && !self->IsExceptionPending()) {
- mirror::Object* existing = shadow_frame.GetVRegReference(string_init_vreg_this);
- if (existing == nullptr) {
- // If it's null, we come from compiled code that was deoptimized. Nothing to do,
- // as the compiler verified there was no alias.
- // Set the new string result of the StringFactory.
- shadow_frame.SetVRegReference(string_init_vreg_this, result->GetL());
- } else {
- // Replace the fake string that was allocated with the StringFactory result.
- for (uint32_t i = 0; i < shadow_frame.NumberOfVRegs(); ++i) {
- if (shadow_frame.GetVRegReference(i) == existing) {
- DCHECK_EQ(shadow_frame.GetVRegReference(i),
- reinterpret_cast<mirror::Object*>(shadow_frame.GetVReg(i)));
- shadow_frame.SetVRegReference(i, result->GetL());
- DCHECK_EQ(shadow_frame.GetVRegReference(i),
- reinterpret_cast<mirror::Object*>(shadow_frame.GetVReg(i)));
- }
- }
- }
+ SetStringInitValueToAllAliases(&shadow_frame, string_init_vreg_this, *result);
}
return !self->IsExceptionPending();
diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h
index 69376fd..cc470f3 100644
--- a/runtime/interpreter/interpreter_common.h
+++ b/runtime/interpreter/interpreter_common.h
@@ -1021,6 +1021,12 @@
ShadowFrame* shadow_frame,
JValue* result);
+// Set string value created from StringFactory.newStringFromXXX() into all aliases of
+// StringFactory.newEmptyString().
+void SetStringInitValueToAllAliases(ShadowFrame* shadow_frame,
+ uint16_t this_obj_vreg,
+ JValue result);
+
// Explicitly instantiate all DoInvoke functions.
#define EXPLICIT_DO_INVOKE_TEMPLATE_DECL(_type, _is_range, _do_check) \
template SHARED_REQUIRES(Locks::mutator_lock_) \
diff --git a/test/597-deopt-new-string/src/Main.java b/test/597-deopt-new-string/src/Main.java
index 1224e40..e78f0d3 100644
--- a/test/597-deopt-new-string/src/Main.java
+++ b/test/597-deopt-new-string/src/Main.java
@@ -48,7 +48,12 @@
throw new Error();
}
char[] arr = {'a', 'b', 'c'};
- return new String(arr, 0, arr.length);
+ String str = new String(arr, 0, arr.length);
+ if (!str.equals("abc")) {
+ System.out.println("Failure 1! " + str);
+ System.exit(0);
+ }
+ return str;
}
public void run() {
@@ -68,7 +73,11 @@
} else {
// This thread keeps doing new String() from a char array.
while (!done) {
- $noinline$run0();
+ String str = $noinline$run0();
+ if (!str.equals("abc")) {
+ System.out.println("Failure 2! " + str);
+ System.exit(0);
+ }
}
}
}