cppgc: Rework Visitor ephemeron handling

Fixes an issue with tracing empty ephemeron values of mixin types.

Bug: chromium:1056170
Change-Id: I0089df29943ba7670ec4bdfa5592a01b0ec6de04
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2732025
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73185}
This commit is contained in:
Michael Lippautz 2021-03-04 10:41:56 +01:00 committed by Commit Bot
parent 30dd7b462c
commit ec741dbd7d
2 changed files with 43 additions and 33 deletions

View File

@ -158,48 +158,52 @@ class V8_EXPORT Visitor {
}
/**
* Trace method for ephemerons. Used for tracing raw ephemeron in which the
* key and value are kept separately.
* Trace method for a single ephemeron. Used for tracing a raw ephemeron in
* which the `key` and `value` are kept separately.
*
* \param key WeakMember reference weakly retaining a key object.
* \param value Member reference weakly retaining a value object.
* \param weak_member_key WeakMember reference weakly retaining a key object.
* \param member_value Member reference with ephemeron semantics.
*/
template <typename KeyType, typename ValueType>
void TraceEphemeron(const WeakMember<KeyType>& key,
const Member<ValueType>* value) {
const KeyType* k = key.GetRawAtomic();
if (!k) return;
void TraceEphemeron(const WeakMember<KeyType>& weak_member_key,
const Member<ValueType>* member_value) {
const KeyType* key = weak_member_key.GetRawAtomic();
if (!key) return;
// `value` must always be non-null.
CPPGC_DCHECK(value);
TraceDescriptor value_desc =
TraceTrait<Member<ValueType>>::GetTraceDescriptor(value);
if (!value_desc.base_object_payload) return;
CPPGC_DCHECK(member_value);
const ValueType* value = member_value->GetRawAtomic();
if (!value) return;
// KeyType might be a GarbageCollectedMixin.
// KeyType and ValueType may refer to GarbageCollectedMixin.
TraceDescriptor value_desc =
TraceTrait<ValueType>::GetTraceDescriptor(value);
CPPGC_DCHECK(value_desc.base_object_payload);
const void* key_base_object_payload =
TraceTrait<KeyType>::GetTraceDescriptor(k).base_object_payload;
TraceTrait<KeyType>::GetTraceDescriptor(key).base_object_payload;
CPPGC_DCHECK(key_base_object_payload);
// `value_desc.base_object_payload` must also be non-null because empty
// values are filtered above.
CPPGC_DCHECK(value_desc.base_object_payload);
VisitEphemeron(key_base_object_payload, value, value_desc);
}
/**
* Trace method for ephemerons. Used for tracing raw ephemeron in which the
* key and value are kept separately.
* Trace method for a single ephemeron. Used for tracing a raw ephemeron in
* which the `key` and `value` are kept separately. Note that this overload
* is for non-GarbageCollected `value`s that can be traced though.
*
* \param key WeakMember reference weakly retaining a key object.
* \param value Traceable reference weakly retaining a value object.
* \param key `WeakMember` reference weakly retaining a key object.
* \param value Reference weakly retaining a value object. Note that
* `ValueType` here should not be `Member`. It is expected that
* `TraceTrait<ValueType>::GetTraceDescriptor(value)` returns a
* `TraceDescriptor` with a null base pointer but a valid trace method.
*/
template <typename KeyType, typename ValueType>
void TraceEphemeron(const WeakMember<KeyType>& key, const ValueType* value) {
void TraceEphemeron(const WeakMember<KeyType>& weak_member_key,
const ValueType* value) {
static_assert(!IsGarbageCollectedOrMixinTypeV<ValueType>,
"garbage-collected types must use WeakMember and Member");
const KeyType* k = key.GetRawAtomic();
if (!k) return;
const KeyType* key = weak_member_key.GetRawAtomic();
if (!key) return;
// `value` must always be non-null.
CPPGC_DCHECK(value);
@ -211,8 +215,9 @@ class V8_EXPORT Visitor {
// KeyType might be a GarbageCollectedMixin.
const void* key_base_object_payload =
TraceTrait<KeyType>::GetTraceDescriptor(k).base_object_payload;
TraceTrait<KeyType>::GetTraceDescriptor(key).base_object_payload;
CPPGC_DCHECK(key_base_object_payload);
VisitEphemeron(key_base_object_payload, value, value_desc);
}
@ -367,14 +372,6 @@ class V8_EXPORT Visitor {
friend class internal::VisitorBase;
};
template <typename T>
struct TraceTrait<Member<T>> {
static TraceDescriptor GetTraceDescriptor(const void* self) {
return TraceTrait<T>::GetTraceDescriptor(
static_cast<const Member<T>*>(self)->GetRawAtomic());
}
};
} // namespace cppgc
#endif // INCLUDE_CPPGC_VISITOR_H_

View File

@ -226,5 +226,18 @@ TEST_F(EphemeronPairTest, EphemeronPairWithMixinKey) {
EXPECT_TRUE(HeapObjectHeader::FromPayload(value).IsMarked());
}
TEST_F(EphemeronPairTest, EphemeronPairWithEmptyMixinValue) {
GCedWithMixin* key =
MakeGarbageCollected<GCedWithMixin>(GetAllocationHandle());
Persistent<EphemeronHolderWithMixins> holder =
MakeGarbageCollected<EphemeronHolderWithMixins>(GetAllocationHandle(),
key, nullptr);
EXPECT_NE(static_cast<void*>(key), holder->ephemeron_pair().key.Get());
EXPECT_TRUE(HeapObjectHeader::FromPayload(key).TryMarkAtomic());
InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get());
FinishSteps();
FinishMarking();
}
} // namespace internal
} // namespace cppgc