Skip to content

Commit

Permalink
deps: V8: cherry-pick 94e8282325a1
Browse files Browse the repository at this point in the history
Original commit message:

    [symbol-as-weakmap-key] Fix DCHECKs and add CanBeHeldWeakly

    There are a few DCHECKs that weren't updated to allow for Symbols as
    weak collection keys. This CL updates those DCHECKs and also does the
    following refactors for clarity:

    - Add Object::CanBeHeldWeakly
    - Rename GotoIfCannotBeWeakKey -> GotoIfCannotBeHeldWeakly to align with
      spec AO name

    Bug: chromium:1370400, chromium:1370402, v8:12947
    Change-Id: I380840c8377497feae97e3fca37555dae0dcc255
    Fixed: chromium:1370400, chromium:1370402
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3928150
    Auto-Submit: Shu-yu Guo <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#83507}

Refs: v8/v8@94e8282
PR-URL: #51004
Reviewed-By: Chengzhong Wu <[email protected]>
  • Loading branch information
joyeecheung authored and richardlau committed Mar 15, 2024
1 parent fafbacd commit 6d50966
Show file tree
Hide file tree
Showing 13 changed files with 66 additions and 62 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.31',
'v8_embedder_string': '-node.32',

##### V8 defaults for Node.js #####

Expand Down
38 changes: 19 additions & 19 deletions deps/v8/src/builtins/builtins-collections-gen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -399,23 +399,23 @@ TNode<IntPtrT> BaseCollectionsAssembler::EstimatedInitialSize(
[=] { return IntPtrConstant(0); });
}

void BaseCollectionsAssembler::GotoIfCannotBeWeakKey(
const TNode<Object> obj, Label* if_cannot_be_weak_key) {
void BaseCollectionsAssembler::GotoIfCannotBeHeldWeakly(
const TNode<Object> obj, Label* if_cannot_be_held_weakly) {
Label check_symbol_key(this);
Label end(this);
GotoIf(TaggedIsSmi(obj), if_cannot_be_weak_key);
GotoIf(TaggedIsSmi(obj), if_cannot_be_held_weakly);
TNode<Uint16T> instance_type = LoadMapInstanceType(LoadMap(CAST(obj)));
GotoIfNot(IsJSReceiverInstanceType(instance_type), &check_symbol_key);
// TODO(v8:12547) Shared structs should only be able to point to shared values
// in weak collections. For now, disallow them as weak collection keys.
GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_weak_key);
GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_held_weakly);
Goto(&end);
Bind(&check_symbol_key);
GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_weak_key);
GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_weak_key);
GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_held_weakly);
GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_held_weakly);
TNode<Uint32T> flags = LoadSymbolFlags(CAST(obj));
GotoIf(Word32And(flags, Symbol::IsInPublicSymbolTableBit::kMask),
if_cannot_be_weak_key);
if_cannot_be_held_weakly);
Goto(&end);
Bind(&end);
}
Expand Down Expand Up @@ -2573,17 +2573,17 @@ TF_BUILTIN(WeakMapLookupHashIndex, WeakCollectionsBuiltinsAssembler) {
auto table = Parameter<EphemeronHashTable>(Descriptor::kTable);
auto key = Parameter<Object>(Descriptor::kKey);

Label if_cannot_be_weak_key(this);
Label if_cannot_be_held_weakly(this);

GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key);
GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly);

TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_weak_key);
TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_held_weakly);
TNode<IntPtrT> capacity = LoadTableCapacity(table);
TNode<IntPtrT> key_index = FindKeyIndexForKey(
table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key);
table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly);
Return(SmiTag(ValueIndexFromKeyIndex(key_index)));

BIND(&if_cannot_be_weak_key);
BIND(&if_cannot_be_held_weakly);
Return(SmiConstant(-1));
}

Expand Down Expand Up @@ -2638,22 +2638,22 @@ TF_BUILTIN(WeakCollectionDelete, WeakCollectionsBuiltinsAssembler) {
auto collection = Parameter<JSWeakCollection>(Descriptor::kCollection);
auto key = Parameter<Object>(Descriptor::kKey);

Label call_runtime(this), if_cannot_be_weak_key(this);
Label call_runtime(this), if_cannot_be_held_weakly(this);

GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key);
GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly);

TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_weak_key);
TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_held_weakly);
TNode<EphemeronHashTable> table = LoadTable(collection);
TNode<IntPtrT> capacity = LoadTableCapacity(table);
TNode<IntPtrT> key_index = FindKeyIndexForKey(
table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key);
table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly);
TNode<IntPtrT> number_of_elements = LoadNumberOfElements(table, -1);
GotoIf(ShouldShrink(capacity, number_of_elements), &call_runtime);

RemoveEntry(table, key_index, number_of_elements);
Return(TrueConstant());

BIND(&if_cannot_be_weak_key);
BIND(&if_cannot_be_held_weakly);
Return(FalseConstant());

BIND(&call_runtime);
Expand Down Expand Up @@ -2735,7 +2735,7 @@ TF_BUILTIN(WeakMapPrototypeSet, WeakCollectionsBuiltinsAssembler) {
"WeakMap.prototype.set");

Label throw_invalid_key(this);
GotoIfCannotBeWeakKey(key, &throw_invalid_key);
GotoIfCannotBeHeldWeakly(key, &throw_invalid_key);

Return(
CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, key, value));
Expand All @@ -2753,7 +2753,7 @@ TF_BUILTIN(WeakSetPrototypeAdd, WeakCollectionsBuiltinsAssembler) {
"WeakSet.prototype.add");

Label throw_invalid_value(this);
GotoIfCannotBeWeakKey(value, &throw_invalid_value);
GotoIfCannotBeHeldWeakly(value, &throw_invalid_value);

Return(CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, value,
TrueConstant()));
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/src/builtins/builtins-collections-gen.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ class BaseCollectionsAssembler : public CodeStubAssembler {

virtual ~BaseCollectionsAssembler() = default;

void GotoIfCannotBeWeakKey(const TNode<Object> obj,
Label* if_cannot_be_weak_key);
void GotoIfCannotBeHeldWeakly(const TNode<Object> obj,
Label* if_cannot_be_held_weakly);

protected:
enum Variant { kMap, kSet, kWeakMap, kWeakSet };
Expand Down
21 changes: 4 additions & 17 deletions deps/v8/src/builtins/builtins-weak-refs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,10 @@ BUILTIN(FinalizationRegistryUnregister) {

// 4. If CanBeHeldWeakly(unregisterToken) is false, throw a TypeError
// exception.
if (FLAG_harmony_symbol_as_weakmap_key) {
if (!unregister_token->IsJSReceiver() &&
(!unregister_token->IsSymbol() ||
Handle<Symbol>::cast(unregister_token)->is_in_public_symbol_table())) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate,
NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
unregister_token));
}
} else {
// 4. If Type(unregisterToken) is not Object, throw a TypeError exception.
if (!unregister_token->IsJSReceiver()) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate,
NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
unregister_token));
}
if (!unregister_token->CanBeHeldWeakly()) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
unregister_token));
}

bool success = JSFinalizationRegistry::Unregister(
Expand Down
6 changes: 3 additions & 3 deletions deps/v8/src/builtins/finalization-registry.tq
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ extern transitioning macro
RemoveFinalizationRegistryCellFromUnregisterTokenMap(
JSFinalizationRegistry, WeakCell): void;

extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeWeakKey(JSAny):
extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeHeldWeakly(JSAny):
void labels NotWeakKey;

macro SplitOffTail(weakCell: WeakCell): WeakCell|Undefined {
Expand Down Expand Up @@ -140,7 +140,7 @@ FinalizationRegistryRegister(
MessageTemplate::kIncompatibleMethodReceiver,
'FinalizationRegistry.prototype.register', receiver);
// 3. If CanBeHeldWeakly(target) is false, throw a TypeError exception.
GotoIfCannotBeWeakKey(arguments[0])
GotoIfCannotBeHeldWeakly(arguments[0])
otherwise ThrowTypeError(MessageTemplate::kInvalidWeakRefsRegisterTarget);

const target = UnsafeCast<(JSReceiver | Symbol)>(arguments[0]);
Expand All @@ -159,7 +159,7 @@ FinalizationRegistryRegister(
if (IsUndefined(unregisterTokenRaw)) {
unregisterToken = Undefined;
} else {
GotoIfCannotBeWeakKey(unregisterTokenRaw)
GotoIfCannotBeHeldWeakly(unregisterTokenRaw)
otherwise ThrowTypeError(
MessageTemplate::kInvalidWeakRefsUnregisterToken, unregisterTokenRaw);
unregisterToken = UnsafeCast<(JSReceiver | Symbol)>(unregisterTokenRaw);
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/src/builtins/weak-ref.tq
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ WeakRefConstructor(
}

// 2. If CanBeHeldWeakly(weakTarget) is false, throw a TypeError exception.
GotoIfCannotBeWeakKey(weakTarget) otherwise ThrowTypeError(
GotoIfCannotBeHeldWeakly(weakTarget) otherwise ThrowTypeError(
MessageTemplate::kInvalidWeakRefsWeakRefConstructorTarget);

// 3. Let weakRef be ? OrdinaryCreateFromConstructor(NewTarget,
Expand Down
8 changes: 2 additions & 6 deletions deps/v8/src/diagnostics/objects-debug.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1240,9 +1240,7 @@ void JSSharedStruct::JSSharedStructVerify(Isolate* isolate) {
void WeakCell::WeakCellVerify(Isolate* isolate) {
CHECK(IsWeakCell());

CHECK(target().IsJSReceiver() || target().IsUndefined(isolate) ||
(target().IsSymbol() &&
!Symbol::cast(target()).is_in_public_symbol_table()));
CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly());

CHECK(prev().IsWeakCell() || prev().IsUndefined(isolate));
if (prev().IsWeakCell()) {
Expand Down Expand Up @@ -1270,9 +1268,7 @@ void WeakCell::WeakCellVerify(Isolate* isolate) {
void JSWeakRef::JSWeakRefVerify(Isolate* isolate) {
CHECK(IsJSWeakRef());
JSObjectVerify(isolate);
CHECK(target().IsUndefined(isolate) || target().IsJSReceiver() ||
(target().IsSymbol() &&
!Symbol::cast(target()).is_in_public_symbol_table()));
CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly());
}

void JSFinalizationRegistry::JSFinalizationRegistryVerify(Isolate* isolate) {
Expand Down
6 changes: 2 additions & 4 deletions deps/v8/src/objects/js-weak-refs-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,7 @@ void WeakCell::Nullify(Isolate* isolate,
// only called for WeakCells which haven't been unregistered yet, so they will
// be in the active_cells list. (The caller must guard against calling this
// for unregistered WeakCells by checking that the target is not undefined.)
DCHECK(target().IsJSReceiver() ||
(target().IsSymbol() &&
!Symbol::cast(target()).is_in_public_symbol_table()));
DCHECK(target().CanBeHeldWeakly());
set_target(ReadOnlyRoots(isolate).undefined_value());

JSFinalizationRegistry fr =
Expand Down Expand Up @@ -218,7 +216,7 @@ void WeakCell::RemoveFromFinalizationRegistryCells(Isolate* isolate) {

// It's important to set_target to undefined here. This guards that we won't
// call Nullify (which assumes that the WeakCell is in active_cells).
DCHECK(target().IsUndefined() || target().IsJSReceiver());
DCHECK(target().IsUndefined() || target().CanBeHeldWeakly());
set_target(ReadOnlyRoots(isolate).undefined_value());

JSFinalizationRegistry fr =
Expand Down
17 changes: 17 additions & 0 deletions deps/v8/src/objects/objects-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,23 @@ MaybeHandle<Object> Object::Share(Isolate* isolate, Handle<Object> value,
throw_if_cannot_be_shared);
}

// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation
bool Object::CanBeHeldWeakly() const {
if (IsJSReceiver()) {
// TODO(v8:12547) Shared structs and arrays should only be able to point
// to shared values in weak collections. For now, disallow them as weak
// collection keys.
if (FLAG_harmony_struct) {
return !IsJSSharedStruct();
}
return true;
}
if (FLAG_harmony_symbol_as_weakmap_key) {
return IsSymbol() && !Symbol::cast(*this).is_in_public_symbol_table();
}
return false;
}

Handle<Object> ObjectHashTableShape::AsHandle(Handle<Object> key) {
return key;
}
Expand Down
5 changes: 5 additions & 0 deletions deps/v8/src/objects/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,11 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
Handle<HeapObject> value,
ShouldThrow throw_if_cannot_be_shared);

// Whether this Object can be held weakly, i.e. whether it can be used as a
// key in WeakMap, as a key in WeakSet, as the target of a WeakRef, or as a
// target or unregister token of a FinalizationRegistry.
inline bool CanBeHeldWeakly() const;

protected:
inline Address field_address(size_t offset) const {
return ptr() + offset - kHeapObjectTag;
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/src/runtime/runtime-collections.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionDelete) {
int hash = args.smi_value_at(2);

#ifdef DEBUG
DCHECK(key->IsJSReceiver());
DCHECK(key->CanBeHeldWeakly());
DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key));
Handle<EphemeronHashTable> table(
EphemeronHashTable::cast(weak_collection->table()), isolate);
Expand All @@ -105,7 +105,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionSet) {
int hash = args.smi_value_at(3);

#ifdef DEBUG
DCHECK(key->IsJSReceiver());
DCHECK(key->CanBeHeldWeakly());
DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key));
Handle<EphemeronHashTable> table(
EphemeronHashTable::cast(weak_collection->table()), isolate);
Expand Down
8 changes: 1 addition & 7 deletions deps/v8/src/runtime/runtime-weak-refs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,7 @@ RUNTIME_FUNCTION(Runtime_JSWeakRefAddToKeptObjects) {
HandleScope scope(isolate);
DCHECK_EQ(1, args.length());
Handle<HeapObject> object = args.at<HeapObject>(0);
if (FLAG_harmony_symbol_as_weakmap_key) {
DCHECK(object->IsJSReceiver() ||
(object->IsSymbol() &&
!(Handle<Symbol>::cast(object))->is_in_public_symbol_table()));
} else {
DCHECK(object->IsJSReceiver());
}
DCHECK(object->CanBeHeldWeakly());

isolate->heap()->KeepDuringJob(object);

Expand Down
7 changes: 7 additions & 0 deletions deps/v8/test/mjsunit/harmony/symbol-as-weakmap-key.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,10 @@
assertFalse(set.delete(key));
assertFalse(set.has(key));
})();

(function TestFinalizationRegistryUnregister() {
const fr = new FinalizationRegistry(function() {});
const key = {};
fr.register(Symbol('foo'), "holdings", key);
fr.unregister(key);
})();

0 comments on commit 6d50966

Please sign in to comment.