[vm] Fix race when setting identity hash codes.
We might interrupt Object._objectHashCode(obj) helper to serve a
vm-service request between checking for unset hash code and setting
a new hash code. While serving this request we might set the hash
code of [obj] (e.g. when generating heap snapshot or in
Instance::PrintJSONImpl). Later Object._objectHashCode will call
Object_setHash, which (on 64-bit platforms) assumes that
hash field in the object header is set to 0 and uses bitwise-or
to initialize it. This leads to a mismatch between hash code that
the first invocation of _objectHashCode will return and the
value stored in the header (because we OR non-zero value set
by vm-service with a value which _objectHashCode intended to
use as a hash).
This CL changes Object_setHash to avoid overwriting or mangling
the hash value if it was already set.
We also fix hash code generation in vm-service to ensure that we
only generate values which are valid Smis because the rest of the
code expects that.
TEST=pkg/front_end/test/incremental_compiler_leak_test.dart
Cq-Include-Trybots: luci.dart.try:vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-linux-release-simarm64-try
Change-Id: Ica913af8bc1cfef0ad60a9e7504531ee4de53015
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197400
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
diff --git a/pkg/front_end/testcases/general/invalid_operator.dart.weak.expect b/pkg/front_end/testcases/general/invalid_operator.dart.weak.expect
index c65d2ea..85568b7 100644
--- a/pkg/front_end/testcases/general/invalid_operator.dart.weak.expect
+++ b/pkg/front_end/testcases/general/invalid_operator.dart.weak.expect
@@ -564,35 +564,35 @@
// Try adding explicit types.
// operator ==<T>(a) => true;
// ^^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is one of the overridden members.
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is one of the overridden members.
// bool operator ==(Object other) native "Object_equals";
// ^^
//
// pkg/front_end/testcases/general/invalid_operator.dart:6:12: Error: The method 'Operators1.==' has fewer positional arguments than those of overridden method 'Object.=='.
// operator ==() => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
// pkg/front_end/testcases/general/invalid_operator.dart:27:12: Error: The method 'Operators2.==' has more required arguments than those of overridden method 'Object.=='.
// operator ==(a, b) => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
// pkg/front_end/testcases/general/invalid_operator.dart:71:12: Error: The method 'Operators4.==' has fewer positional arguments than those of overridden method 'Object.=='.
// operator ==({a}) => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
// pkg/front_end/testcases/general/invalid_operator.dart:137:12: Error: Declared type variables of 'Operators7.==' doesn't match those on overridden method 'Object.=='.
// operator ==<T>(a) => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
diff --git a/pkg/front_end/testcases/general/invalid_operator.dart.weak.outline.expect b/pkg/front_end/testcases/general/invalid_operator.dart.weak.outline.expect
index 63dfbeb..9474e68 100644
--- a/pkg/front_end/testcases/general/invalid_operator.dart.weak.outline.expect
+++ b/pkg/front_end/testcases/general/invalid_operator.dart.weak.outline.expect
@@ -564,35 +564,35 @@
// Try adding explicit types.
// operator ==<T>(a) => true;
// ^^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is one of the overridden members.
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is one of the overridden members.
// bool operator ==(Object other) native "Object_equals";
// ^^
//
// pkg/front_end/testcases/general/invalid_operator.dart:6:12: Error: The method 'Operators1.==' has fewer positional arguments than those of overridden method 'Object.=='.
// operator ==() => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
// pkg/front_end/testcases/general/invalid_operator.dart:27:12: Error: The method 'Operators2.==' has more required arguments than those of overridden method 'Object.=='.
// operator ==(a, b) => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
// pkg/front_end/testcases/general/invalid_operator.dart:71:12: Error: The method 'Operators4.==' has fewer positional arguments than those of overridden method 'Object.=='.
// operator ==({a}) => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
// pkg/front_end/testcases/general/invalid_operator.dart:137:12: Error: Declared type variables of 'Operators7.==' doesn't match those on overridden method 'Object.=='.
// operator ==<T>(a) => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
diff --git a/pkg/front_end/testcases/nnbd/issue42603.dart.strong.expect b/pkg/front_end/testcases/nnbd/issue42603.dart.strong.expect
index 12f52a0..734823f 100644
--- a/pkg/front_end/testcases/nnbd/issue42603.dart.strong.expect
+++ b/pkg/front_end/testcases/nnbd/issue42603.dart.strong.expect
@@ -9,7 +9,7 @@
// pkg/front_end/testcases/nnbd/issue42603.dart:18:17: Error: The method 'E.==' has fewer positional arguments than those of overridden method 'Object.=='.
// bool operator ==() => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
diff --git a/pkg/front_end/testcases/nnbd/issue42603.dart.weak.expect b/pkg/front_end/testcases/nnbd/issue42603.dart.weak.expect
index 12f52a0..734823f 100644
--- a/pkg/front_end/testcases/nnbd/issue42603.dart.weak.expect
+++ b/pkg/front_end/testcases/nnbd/issue42603.dart.weak.expect
@@ -9,7 +9,7 @@
// pkg/front_end/testcases/nnbd/issue42603.dart:18:17: Error: The method 'E.==' has fewer positional arguments than those of overridden method 'Object.=='.
// bool operator ==() => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
diff --git a/pkg/front_end/testcases/nnbd/issue42603.dart.weak.outline.expect b/pkg/front_end/testcases/nnbd/issue42603.dart.weak.outline.expect
index 926252c..7475e9f 100644
--- a/pkg/front_end/testcases/nnbd/issue42603.dart.weak.outline.expect
+++ b/pkg/front_end/testcases/nnbd/issue42603.dart.weak.outline.expect
@@ -9,7 +9,7 @@
// pkg/front_end/testcases/nnbd/issue42603.dart:18:17: Error: The method 'E.==' has fewer positional arguments than those of overridden method 'Object.=='.
// bool operator ==() => true;
// ^
-// sdk/lib/_internal/vm/lib/object_patch.dart:21:17: Context: This is the overridden method ('==').
+// sdk/lib/_internal/vm/lib/object_patch.dart:26:17: Context: This is the overridden method ('==').
// bool operator ==(Object other) native "Object_equals";
// ^
//
diff --git a/runtime/lib/object.cc b/runtime/lib/object.cc
index 507ce9d..6b0e86c 100644
--- a/runtime/lib/object.cc
+++ b/runtime/lib/object.cc
@@ -33,21 +33,29 @@
return Object::null();
}
-DEFINE_NATIVE_ENTRY(Object_getHash, 0, 1) {
-// Please note that no handle is created for the argument.
-// This is safe since the argument is only used in a tail call.
-// The performance benefit is more than 5% when using hashCode.
+static intptr_t GetHash(Isolate* isolate, const ObjectPtr obj) {
#if defined(HASH_IN_OBJECT_HEADER)
- return Smi::New(Object::GetCachedHash(arguments->NativeArgAt(0)));
+ return Object::GetCachedHash(obj);
#else
Heap* heap = isolate->group()->heap();
- ASSERT(arguments->NativeArgAt(0)->IsDartInstance());
- return Smi::New(heap->GetHash(arguments->NativeArgAt(0)));
+ ASSERT(obj->IsDartInstance());
+ return heap->GetHash(obj);
#endif
}
-DEFINE_NATIVE_ENTRY(Object_setHash, 0, 2) {
+DEFINE_NATIVE_ENTRY(Object_getHash, 0, 1) {
+ // Please note that no handle is created for the argument.
+ // This is safe since the argument is only used in a tail call.
+ // The performance benefit is more than 5% when using hashCode.
+ return Smi::New(GetHash(isolate, arguments->NativeArgAt(0)));
+}
+
+DEFINE_NATIVE_ENTRY(Object_setHashIfNotSetYet, 0, 2) {
GET_NON_NULL_NATIVE_ARGUMENT(Smi, hash, arguments->NativeArgAt(1));
+ const intptr_t current_hash = GetHash(isolate, arguments->NativeArgAt(0));
+ if (current_hash != 0) {
+ return Smi::New(current_hash);
+ }
#if defined(HASH_IN_OBJECT_HEADER)
Object::SetCachedHash(arguments->NativeArgAt(0), hash.Value());
#else
@@ -56,7 +64,7 @@
Heap* heap = isolate->group()->heap();
heap->SetHash(instance.ptr(), hash.Value());
#endif
- return Object::null();
+ return hash.ptr();
}
DEFINE_NATIVE_ENTRY(Object_toString, 0, 1) {
diff --git a/runtime/vm/bootstrap_natives.h b/runtime/vm/bootstrap_natives.h
index fe0f192..c17daf4 100644
--- a/runtime/vm/bootstrap_natives.h
+++ b/runtime/vm/bootstrap_natives.h
@@ -18,7 +18,7 @@
V(DartAsync_fatal, 1) \
V(Object_equals, 2) \
V(Object_getHash, 1) \
- V(Object_setHash, 2) \
+ V(Object_setHashIfNotSetYet, 2) \
V(Object_toString, 1) \
V(Object_runtimeType, 1) \
V(Object_haveSameRuntimeType, 2) \
diff --git a/runtime/vm/compiler/asm_intrinsifier_arm.cc b/runtime/vm/compiler/asm_intrinsifier_arm.cc
index 101ea94..770d456 100644
--- a/runtime/vm/compiler/asm_intrinsifier_arm.cc
+++ b/runtime/vm/compiler/asm_intrinsifier_arm.cc
@@ -1555,8 +1555,8 @@
UNREACHABLE();
}
-void AsmIntrinsifier::Object_setHash(Assembler* assembler,
- Label* normal_ir_body) {
+void AsmIntrinsifier::Object_setHashIfNotSetYet(Assembler* assembler,
+ Label* normal_ir_body) {
UNREACHABLE();
}
diff --git a/runtime/vm/compiler/asm_intrinsifier_arm64.cc b/runtime/vm/compiler/asm_intrinsifier_arm64.cc
index 91c8e54..167e089 100644
--- a/runtime/vm/compiler/asm_intrinsifier_arm64.cc
+++ b/runtime/vm/compiler/asm_intrinsifier_arm64.cc
@@ -1551,20 +1551,27 @@
__ ret();
}
-void AsmIntrinsifier::Object_setHash(Assembler* assembler,
- Label* normal_ir_body) {
+void AsmIntrinsifier::Object_setHashIfNotSetYet(Assembler* assembler,
+ Label* normal_ir_body) {
+ Label already_set;
__ ldr(R0, Address(SP, 1 * target::kWordSize)); // Object.
+ __ ldr(R1, FieldAddress(R0, target::String::hash_offset(), kFourBytes),
+ kUnsignedFourBytes);
+ __ cbnz(&already_set, R1, kFourBytes);
__ ldr(R1, Address(SP, 0 * target::kWordSize)); // Value.
// R0: Untagged address of header word (ldxr/stxr do not support offsets).
__ sub(R0, R0, Operand(kHeapObjectTag));
__ SmiUntag(R1);
- __ LslImmediate(R1, R1, target::UntaggedObject::kHashTagPos);
+ __ LslImmediate(R3, R1, target::UntaggedObject::kHashTagPos);
Label retry;
__ Bind(&retry);
__ ldxr(R2, R0, kEightBytes);
- __ orr(R2, R2, Operand(R1));
+ __ orr(R2, R2, Operand(R3));
__ stxr(R4, R2, R0, kEightBytes);
__ cbnz(&retry, R4);
+ // Fall-through with R1 containing new hash value (untagged).
+ __ Bind(&already_set);
+ __ SmiTag(R0, R1);
__ ret();
}
diff --git a/runtime/vm/compiler/asm_intrinsifier_ia32.cc b/runtime/vm/compiler/asm_intrinsifier_ia32.cc
index 9cfe941..a52f0ab 100644
--- a/runtime/vm/compiler/asm_intrinsifier_ia32.cc
+++ b/runtime/vm/compiler/asm_intrinsifier_ia32.cc
@@ -1544,8 +1544,8 @@
UNREACHABLE();
}
-void AsmIntrinsifier::Object_setHash(Assembler* assembler,
- Label* normal_ir_body) {
+void AsmIntrinsifier::Object_setHashIfNotSetYet(Assembler* assembler,
+ Label* normal_ir_body) {
UNREACHABLE();
}
diff --git a/runtime/vm/compiler/asm_intrinsifier_x64.cc b/runtime/vm/compiler/asm_intrinsifier_x64.cc
index c47d307..0bcfb90 100644
--- a/runtime/vm/compiler/asm_intrinsifier_x64.cc
+++ b/runtime/vm/compiler/asm_intrinsifier_x64.cc
@@ -1408,15 +1408,24 @@
__ ret();
}
-void AsmIntrinsifier::Object_setHash(Assembler* assembler,
- Label* normal_ir_body) {
+void AsmIntrinsifier::Object_setHashIfNotSetYet(Assembler* assembler,
+ Label* normal_ir_body) {
+ Label already_set;
__ movq(RAX, Address(RSP, +2 * target::kWordSize)); // Object.
+ __ movl(RDX, FieldAddress(RAX, target::String::hash_offset()));
+ __ testl(RDX, RDX);
+ __ j(NOT_ZERO, &already_set, AssemblerBase::kNearJump);
__ movq(RDX, Address(RSP, +1 * target::kWordSize)); // Value.
__ SmiUntag(RDX);
__ shlq(RDX, Immediate(target::UntaggedObject::kHashTagPos));
// lock+orq is an atomic read-modify-write.
__ lock();
__ orq(FieldAddress(RAX, target::Object::tags_offset()), RDX);
+ __ movq(RAX, Address(RSP, +1 * target::kWordSize));
+ __ ret();
+ __ Bind(&already_set);
+ __ movl(RAX, RDX);
+ __ SmiTag(RAX);
__ ret();
}
diff --git a/runtime/vm/compiler/intrinsifier.cc b/runtime/vm/compiler/intrinsifier.cc
index b4161bf..a1a0973 100644
--- a/runtime/vm/compiler/intrinsifier.cc
+++ b/runtime/vm/compiler/intrinsifier.cc
@@ -269,7 +269,8 @@
// therefore don't intrinsify them, falling back on the native C++
// implementations.
if (function.recognized_kind() == MethodRecognizer::kObject_getHash ||
- function.recognized_kind() == MethodRecognizer::kObject_setHash) {
+ function.recognized_kind() ==
+ MethodRecognizer::kObject_setHashIfNotSetYet) {
return false;
}
#endif
diff --git a/runtime/vm/compiler/recognized_methods_list.h b/runtime/vm/compiler/recognized_methods_list.h
index 80308087..7e54439 100644
--- a/runtime/vm/compiler/recognized_methods_list.h
+++ b/runtime/vm/compiler/recognized_methods_list.h
@@ -254,7 +254,7 @@
V(_FunctionType, get:hashCode, FunctionType_getHashCode, 0x75e0d454) \
V(_FunctionType, ==, FunctionType_equality, 0x465868ae) \
V(::, _getHash, Object_getHash, 0xc60ff758) \
- V(::, _setHash, Object_setHash, 0x8f2a5b0b) \
+ V(::, _setHashIfNotSetYet, Object_setHashIfNotSetYet, 0x4e17c2f5) \
#define CORE_INTEGER_LIB_INTRINSIC_LIST(V) \
V(_IntegerImplementation, >, Integer_greaterThan, 0x402b12df) \
diff --git a/runtime/vm/object_graph.cc b/runtime/vm/object_graph.cc
index 9df16ee..1ce7b76 100644
--- a/runtime/vm/object_graph.cc
+++ b/runtime/vm/object_graph.cc
@@ -1329,13 +1329,23 @@
return hash;
}
+// Generates a random value which can serve as an identity hash.
+// It must be a non-zero smi value (see also [Object._getObjectHash]).
+static uint32_t GenerateHash(Random* random) {
+ uint32_t hash;
+ do {
+ hash = random->NextUInt32();
+ } while (hash == 0 || (kSmiBits < 32 && !Smi::IsValid(hash)));
+ return hash;
+}
+
uint32_t HeapSnapshotWriter::GetHashHelper(Thread* thread, ObjectPtr obj) {
uint32_t hash;
#if defined(HASH_IN_OBJECT_HEADER)
hash = Object::GetCachedHash(obj);
if (hash == 0) {
ASSERT(!thread->heap()->old_space()->IsObjectFromImagePages(obj));
- hash = thread->random()->NextUInt32();
+ hash = GenerateHash(thread->random());
Object::SetCachedHash(obj, hash);
}
#else
@@ -1343,7 +1353,7 @@
hash = heap->GetHash(obj);
if (hash == 0) {
ASSERT(!heap->old_space()->IsObjectFromImagePages(obj));
- hash = thread->random()->NextUInt32();
+ hash = GenerateHash(thread->random());
heap->SetHash(obj, hash);
}
#endif
diff --git a/sdk/lib/_internal/vm/lib/object_patch.dart b/sdk/lib/_internal/vm/lib/object_patch.dart
index 0c20579..545d274 100644
--- a/sdk/lib/_internal/vm/lib/object_patch.dart
+++ b/sdk/lib/_internal/vm/lib/object_patch.dart
@@ -7,8 +7,13 @@
@pragma("vm:recognized", "asm-intrinsic")
@pragma("vm:exact-result-type", "dart:core#_Smi")
int _getHash(obj) native "Object_getHash";
+
+/// Set hash code associated with the object if it is not set yet
+/// and return the current hash code. See [Object._objectHashCode]
+/// for why this function needs to check for already set hash code.
@pragma("vm:recognized", "asm-intrinsic")
-void _setHash(obj, hash) native "Object_setHash";
+@pragma("vm:exact-result-type", "dart:core#_Smi")
+int _setHashIfNotSetYet(obj, int hash) native "Object_setHashIfNotSetYet";
@patch
@pragma("vm:entry-point")
@@ -29,11 +34,15 @@
var result = _getHash(obj);
if (result == 0) {
// We want the hash to be a Smi value greater than 0.
- result = _hashCodeRnd.nextInt(0x40000000);
do {
result = _hashCodeRnd.nextInt(0x40000000);
} while (result == 0);
- _setHash(obj, result);
+
+ // Caveat: we might be interrupted by vm-service which then
+ // can initialize [this] object's hash code, that is why we need to
+ // return the return value of [_setHashIfNotSetYet] rather than
+ // returning [result] itself.
+ return _setHashIfNotSetYet(obj, result);
}
return result;
}
