Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 2505b6c

Browse files
mkustermanncommit-bot@chromium.org
authored andcommitted
[vm/concurrency] Ensure no races can occur when setting identity hash code
Usually the identity hashcode is implemented by returning if present and otherwise generating a random number, setting the hash code and returning it. This works fine in single-mutator environments, though when multiple mutators might race to install identity hash codes we should ensure only one of them wins and all others will obtain whatever hash code has been set by the winner. This will matter once we start sharing more objects across isolates. Issue dart-lang/sdk#36097 TEST=vm/cc/AsmIntrinsifier_SetHashIfNotSetYet Change-Id: Ie760ca9658e6ec0640255361544d6822b07574e2 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201827 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Slava Egorov <vegorov@google.com>
1 parent e3bc9b6 commit 2505b6c

File tree

14 files changed

+212
-52
lines changed

14 files changed

+212
-52
lines changed

runtime/lib/object.cc

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,19 +52,15 @@ DEFINE_NATIVE_ENTRY(Object_getHash, 0, 1) {
5252

5353
DEFINE_NATIVE_ENTRY(Object_setHashIfNotSetYet, 0, 2) {
5454
GET_NON_NULL_NATIVE_ARGUMENT(Smi, hash, arguments->NativeArgAt(1));
55-
const intptr_t current_hash = GetHash(isolate, arguments->NativeArgAt(0));
56-
if (current_hash != 0) {
57-
return Smi::New(current_hash);
58-
}
5955
#if defined(HASH_IN_OBJECT_HEADER)
60-
Object::SetCachedHash(arguments->NativeArgAt(0), hash.Value());
56+
return Smi::New(
57+
Object::SetCachedHashIfNotSet(arguments->NativeArgAt(0), hash.Value()));
6158
#else
6259
const Instance& instance =
6360
Instance::CheckedHandle(zone, arguments->NativeArgAt(0));
64-
Heap* heap = isolate->group()->heap();
65-
heap->SetHash(instance.ptr(), hash.Value());
61+
Heap* heap = thread->heap();
62+
return Smi::New(heap->SetHashIfNotSet(instance.ptr(), hash.Value()));
6663
#endif
67-
return hash.ptr();
6864
}
6965

7066
DEFINE_NATIVE_ENTRY(Object_toString, 0, 1) {

runtime/vm/compiler/asm_intrinsifier_arm64.cc

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1601,26 +1601,27 @@ void AsmIntrinsifier::Object_getHash(Assembler* assembler,
16011601

16021602
void AsmIntrinsifier::Object_setHashIfNotSetYet(Assembler* assembler,
16031603
Label* normal_ir_body) {
1604-
Label already_set;
1605-
__ ldr(R0, Address(SP, 1 * target::kWordSize)); // Object.
1606-
__ ldr(R1, FieldAddress(R0, target::String::hash_offset(), kFourBytes),
1607-
kUnsignedFourBytes);
1608-
__ cbnz(&already_set, R1, kFourBytes);
1609-
__ ldr(R1, Address(SP, 0 * target::kWordSize)); // Value.
1604+
__ ldp(/*Value=*/R1, /*Object=*/R0, Address(SP, 0, Address::PairOffset));
16101605
// R0: Untagged address of header word (ldxr/stxr do not support offsets).
16111606
__ sub(R0, R0, Operand(kHeapObjectTag));
16121607
__ SmiUntag(R1);
16131608
__ LslImmediate(R3, R1, target::UntaggedObject::kHashTagPos);
1614-
Label retry;
1609+
1610+
Label retry, already_set_in_r4;
16151611
__ Bind(&retry);
16161612
__ ldxr(R2, R0, kEightBytes);
1613+
__ LsrImmediate(R4, R2, target::UntaggedObject::kHashTagPos);
1614+
__ cbnz(&already_set_in_r4, R4);
16171615
__ orr(R2, R2, Operand(R3));
16181616
__ stxr(R4, R2, R0, kEightBytes);
16191617
__ cbnz(&retry, R4);
16201618
// Fall-through with R1 containing new hash value (untagged).
1621-
__ Bind(&already_set);
16221619
__ SmiTag(R0, R1);
16231620
__ ret();
1621+
__ Bind(&already_set_in_r4);
1622+
__ clrex();
1623+
__ SmiTag(R0, R4);
1624+
__ ret();
16241625
}
16251626

16261627
void GenerateSubstringMatchesSpecialization(Assembler* assembler,
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
#include "platform/assert.h"
6+
7+
#include "vm/globals.h"
8+
#include "vm/symbols.h"
9+
#include "vm/unit_test.h"
10+
11+
namespace dart {
12+
13+
static intptr_t GetHash(Isolate* isolate, const ObjectPtr obj) {
14+
#if defined(HASH_IN_OBJECT_HEADER)
15+
return Object::GetCachedHash(obj);
16+
#else
17+
Heap* heap = isolate->group()->heap();
18+
ASSERT(obj->IsDartInstance());
19+
return heap->GetHash(obj);
20+
#endif
21+
}
22+
23+
ISOLATE_UNIT_TEST_CASE(AsmIntrinsifier_SetHashIfNotSetYet) {
24+
auto I = Isolate::Current();
25+
const auto& corelib = Library::Handle(Library::CoreLibrary());
26+
const auto& name = String::Handle(String::New("_setHashIfNotSetYet"));
27+
const auto& symbol = String::Handle(Symbols::New(thread, name));
28+
29+
const auto& function =
30+
Function::Handle(corelib.LookupFunctionAllowPrivate(symbol));
31+
const auto& object_class =
32+
Class::Handle(corelib.LookupClass(Symbols::Object()));
33+
34+
auto& smi0 = Smi::Handle(Smi::New(0));
35+
auto& smi21 = Smi::Handle(Smi::New(21));
36+
auto& smi42 = Smi::Handle(Smi::New(42));
37+
const auto& obj = Object::Handle(Instance::New(object_class));
38+
const auto& args = Array::Handle(Array::New(2));
39+
40+
const auto& args_descriptor_array =
41+
Array::Handle(ArgumentsDescriptor::NewBoxed(0, 2, Array::empty_array()));
42+
43+
// Initialized to 0
44+
EXPECT_EQ(smi0.ptr(), Smi::New(GetHash(I, obj.ptr())));
45+
46+
// Lazily set to 42 on first call.
47+
args.SetAt(0, obj);
48+
args.SetAt(1, smi42);
49+
EXPECT_EQ(smi42.ptr(),
50+
DartEntry::InvokeFunction(function, args, args_descriptor_array));
51+
EXPECT_EQ(smi42.ptr(), Smi::New(GetHash(I, obj.ptr())));
52+
53+
// Stays at 42 on subsequent calls.
54+
args.SetAt(0, obj);
55+
args.SetAt(1, smi21);
56+
EXPECT_EQ(smi42.ptr(),
57+
DartEntry::InvokeFunction(function, args, args_descriptor_array));
58+
EXPECT_EQ(smi42.ptr(), Smi::New(GetHash(I, obj.ptr())));
59+
60+
const auto& obj2 = Object::Handle(Instance::New(object_class));
61+
const auto& smiMax = Smi::Handle(Smi::New(0xffffffff));
62+
63+
// Initialized to 0
64+
EXPECT_EQ(smi0.ptr(), Smi::New(GetHash(I, obj2.ptr())));
65+
66+
// Lazily set to smiMax first call.
67+
args.SetAt(0, obj2);
68+
args.SetAt(1, smiMax);
69+
EXPECT_EQ(smiMax.ptr(),
70+
DartEntry::InvokeFunction(function, args, args_descriptor_array));
71+
EXPECT_EQ(smiMax.ptr(), Smi::New(GetHash(I, obj2.ptr())));
72+
73+
// Stays at smiMax on subsequent calls.
74+
args.SetAt(0, obj2);
75+
args.SetAt(1, smi21);
76+
EXPECT_EQ(smiMax.ptr(),
77+
DartEntry::InvokeFunction(function, args, args_descriptor_array));
78+
EXPECT_EQ(smiMax.ptr(), Smi::New(GetHash(I, obj2.ptr())));
79+
}
80+
81+
} // namespace dart

runtime/vm/compiler/asm_intrinsifier_x64.cc

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1455,23 +1455,35 @@ void AsmIntrinsifier::Object_getHash(Assembler* assembler,
14551455

14561456
void AsmIntrinsifier::Object_setHashIfNotSetYet(Assembler* assembler,
14571457
Label* normal_ir_body) {
1458-
Label already_set;
1459-
__ movq(RAX, Address(RSP, +2 * target::kWordSize)); // Object.
1460-
__ movl(RDX, FieldAddress(RAX, target::String::hash_offset()));
1461-
__ testl(RDX, RDX);
1462-
__ j(NOT_ZERO, &already_set, AssemblerBase::kNearJump);
1463-
__ movq(RDX, Address(RSP, +1 * target::kWordSize)); // Value.
1464-
__ SmiUntag(RDX);
1465-
__ shlq(RDX, Immediate(target::UntaggedObject::kHashTagPos));
1466-
// lock+orq is an atomic read-modify-write.
1467-
__ lock();
1468-
__ orq(FieldAddress(RAX, target::Object::tags_offset()), RDX);
1469-
__ movq(RAX, Address(RSP, +1 * target::kWordSize));
1470-
__ ret();
1471-
__ Bind(&already_set);
1472-
__ movl(RAX, RDX);
1458+
ASSERT(target::String::hash_offset() == 4);
1459+
1460+
__ movq(RBX, Address(RSP, +2 * target::kWordSize)); // Object.
1461+
__ movq(RCX, Address(RSP, +1 * target::kWordSize)); // Value.
1462+
__ SmiUntag(RCX);
1463+
__ MoveRegister(RDX, RCX);
1464+
__ shlq(RDX, Immediate(32));
1465+
1466+
Label retry, success, already_in_rax;
1467+
__ Bind(&retry);
1468+
// RAX is used by "cmpxchgq" as comparison value (if comparison succeeds the
1469+
// store is performed).
1470+
__ movq(RAX, FieldAddress(RBX, 0));
1471+
__ TestImmediate(RAX, Immediate(0xffffffff00000000));
1472+
__ BranchIf(NOT_ZERO, &already_in_rax);
1473+
__ MoveRegister(RSI, RAX);
1474+
__ orq(RSI, RDX);
1475+
__ LockCmpxchgq(FieldAddress(RBX, 0), RSI);
1476+
__ BranchIf(NOT_ZERO, &retry);
1477+
// Fall-through with RCX containing new hash value (untagged)
1478+
__ Bind(&success);
1479+
__ SmiTag(RCX);
1480+
__ MoveRegister(RAX, RCX);
1481+
__ Ret();
1482+
1483+
__ Bind(&already_in_rax);
1484+
__ shrq(RAX, Immediate(32));
14731485
__ SmiTag(RAX);
1474-
__ ret();
1486+
__ Ret();
14751487
}
14761488

14771489
void GenerateSubstringMatchesSpecialization(Assembler* assembler,

runtime/vm/compiler/compiler_sources.gni

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ compiler_sources = [
154154
]
155155

156156
compiler_sources_tests = [
157+
"asm_intrinsifier_test.cc",
157158
"assembler/assembler_arm64_test.cc",
158159
"assembler/assembler_arm_test.cc",
159160
"assembler/assembler_ia32_test.cc",

runtime/vm/heap/become.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ void Become::ElementsForwardIdentity(const Array& before, const Array& after) {
297297
ForwardObjectTo(before_obj, after_obj);
298298
heap->ForwardWeakEntries(before_obj, after_obj);
299299
#if defined(HASH_IN_OBJECT_HEADER)
300-
Object::SetCachedHash(after_obj, Object::GetCachedHash(before_obj));
300+
Object::SetCachedHashIfNotSet(after_obj, Object::GetCachedHash(before_obj));
301301
#endif
302302
}
303303

runtime/vm/heap/heap.cc

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -893,6 +893,17 @@ void Heap::SetWeakEntry(ObjectPtr raw_obj, WeakSelector sel, intptr_t val) {
893893
}
894894
}
895895

896+
intptr_t Heap::SetWeakEntryIfNonExistent(ObjectPtr raw_obj,
897+
WeakSelector sel,
898+
intptr_t val) {
899+
if (!raw_obj->IsSmiOrOldObject()) {
900+
return new_weak_tables_[sel]->SetValueIfNonExistent(raw_obj, val);
901+
} else {
902+
ASSERT(raw_obj->IsSmiOrOldObject());
903+
return old_weak_tables_[sel]->SetValueIfNonExistent(raw_obj, val);
904+
}
905+
}
906+
896907
void Heap::ForwardWeakEntries(ObjectPtr before_object, ObjectPtr after_object) {
897908
const auto before_space =
898909
!before_object->IsSmiOrOldObject() ? Heap::kNew : Heap::kOld;

runtime/vm/heap/heap.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -211,8 +211,8 @@ class Heap {
211211
#if !defined(HASH_IN_OBJECT_HEADER)
212212
// Associate an identity hashCode with an object. An non-existent hashCode
213213
// is equal to 0.
214-
void SetHash(ObjectPtr raw_obj, intptr_t hash) {
215-
SetWeakEntry(raw_obj, kIdentityHashes, hash);
214+
intptr_t SetHashIfNotSet(ObjectPtr raw_obj, intptr_t hash) {
215+
return SetWeakEntryIfNonExistent(raw_obj, kIdentityHashes, hash);
216216
}
217217
intptr_t GetHash(ObjectPtr raw_obj) const {
218218
return GetWeakEntry(raw_obj, kIdentityHashes);
@@ -251,6 +251,9 @@ class Heap {
251251
// Used by the GC algorithms to propagate weak entries.
252252
intptr_t GetWeakEntry(ObjectPtr raw_obj, WeakSelector sel) const;
253253
void SetWeakEntry(ObjectPtr raw_obj, WeakSelector sel, intptr_t val);
254+
intptr_t SetWeakEntryIfNonExistent(ObjectPtr raw_obj,
255+
WeakSelector sel,
256+
intptr_t val);
254257

255258
WeakTable* GetWeakTable(Space space, WeakSelector selector) const {
256259
if (space == kNew) {

runtime/vm/heap/weak_table.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,16 @@ class WeakTable {
6262
return SetValueExclusive(key, val);
6363
}
6464

65+
intptr_t SetValueIfNonExistent(ObjectPtr key, intptr_t val) {
66+
MutexLocker ml(&mutex_);
67+
const auto old_value = GetValueExclusive(key);
68+
if (old_value == kNoValue) {
69+
SetValueExclusive(key, val);
70+
return val;
71+
}
72+
return old_value;
73+
}
74+
6575
// The following "exclusive" methods must only be called from call sites
6676
// which are known to have exclusive access to the weak table.
6777
//

runtime/vm/isolate_reload.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ InstancePtr InstanceMorpher::Morph(const Instance& instance) const {
224224
}
225225
#if defined(HASH_IN_OBJECT_HEADER)
226226
const uint32_t hash = Object::GetCachedHash(instance.ptr());
227-
Object::SetCachedHash(result.ptr(), hash);
227+
Object::SetCachedHashIfNotSet(result.ptr(), hash);
228228
#endif
229229

230230
// Morph the context from instance to result using mapping_.

0 commit comments

Comments
 (0)