[vm, gc] Remove data race updating TypedDataView::pointer_ during parallel scavenge.
TEST=tsan
Bug: https://github.com/dart-lang/sdk/issues/45217
Change-Id: I0045b793767b8397c62ce992cc775eff989e8642
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/189800
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/heap/scavenger.cc b/runtime/vm/heap/scavenger.cc
index e4807d5..5f89dfd 100644
--- a/runtime/vm/heap/scavenger.cc
+++ b/runtime/vm/heap/scavenger.cc
@@ -124,12 +124,28 @@
virtual void VisitTypedDataViewPointers(TypedDataViewPtr view,
ObjectPtr* first,
ObjectPtr* last) {
- // First we forward all fields of the typed data view.
+ // TypedDataViews require extra processing to update their
+ // PointerBase::data_ pointer. If the underlying typed data is external, no
+ // update is needed. If the underlying typed data is internal, the pointer
+ // must be updated if the typed data was copied or promoted. We cannot
+ // safely dereference the underlying typed data to make this distinction.
+ // It may have been forwarded by a different scavanger worker, so the access
+ // could have a data race. Rather than checking the CID of the underlying
+ // typed data, which requires dereferencing the copied/promoted header, we
+ // compare the view's internal pointer to what it should be if the
+ // underlying typed data was internal, and assume that external typed data
+ // never points into the Dart heap. We must do this before VisitPointers
+ // because we want to compare the old pointer and old typed data.
+ const bool is_external =
+ view->untag()->data_ != view->untag()->DataFieldForInternalTypedData();
+
+ // Forward all fields of the typed data view.
VisitPointers(first, last);
if (view->untag()->data_ == nullptr) {
ASSERT(RawSmiValue(view->untag()->offset_in_bytes_) == 0 &&
RawSmiValue(view->untag()->length_) == 0);
+ ASSERT(is_external);
return;
}
@@ -146,17 +162,21 @@
*reinterpret_cast<uword*>(UntaggedObject::ToAddr(td));
ASSERT(!IsForwarding(td_header) || td->IsOldObject());
- // We can always obtain the class id from the forwarded backing store.
- const classid_t cid = td->GetClassId();
+ if (!parallel) {
+ // When there is only one worker, there is no data race.
+ ASSERT_EQUAL(IsExternalTypedDataClassId(td->GetClassId()), is_external);
+ }
// If we have external typed data we can simply return since the backing
// store lives in C-heap and will not move.
- if (IsExternalTypedDataClassId(cid)) {
+ if (is_external) {
return;
}
// Now we update the inner pointer.
- ASSERT(IsTypedDataClassId(cid));
+ if (!parallel) {
+ ASSERT(IsTypedDataClassId(td->GetClassId()));
+ }
view->untag()->RecomputeDataFieldForInternalTypedData();
}
diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h
index 56a71b7..82c1998 100644
--- a/runtime/vm/raw_object.h
+++ b/runtime/vm/raw_object.h
@@ -2646,18 +2646,22 @@
data_ = payload + offset_in_bytes;
}
- // Recopute [data_] based on internal [typed_data_] - needs to be called by GC
- // whenever the backing store moved.
+ // Recompute [data_] based on internal [typed_data_] - needs to be called by
+ // GC whenever the backing store moved.
//
// NOTICE: This method assumes [this] is the forwarded object and the
// [typed_data_] pointer points to the new backing store. The backing store's
// fields don't need to be valid - only it's address.
void RecomputeDataFieldForInternalTypedData() {
+ data_ = DataFieldForInternalTypedData();
+ }
+
+ uint8_t* DataFieldForInternalTypedData() const {
const intptr_t offset_in_bytes = RawSmiValue(offset_in_bytes_);
uint8_t* payload =
reinterpret_cast<uint8_t*>(UntaggedObject::ToAddr(typed_data()) +
UntaggedTypedData::payload_offset());
- data_ = payload + offset_in_bytes;
+ return payload + offset_in_bytes;
}
void ValidateInnerPointer() {
