Skip to content

V
V8
Commit d23dbf3b authored by Bill Budge's avatar Bill Budge Committed by V8 LUCI CQ
Browse files
Options

Revert "[compiler] Consider IsPendingAllocation in Ref construction" 

This reverts commit 5f0ac36c.

Reason for revert: Seems to be associated with multiple Sanitizer failures:

https://ci.chromium.org/p/v8/builders/ci/V8%20Linux64%20TSAN%20-%20stress-incremental-marking/3176

Original change's description:
> [compiler] Consider IsPendingAllocation in Ref construction
>
> The logic in JSHeapBroker::TryGetOrCreateData assumes that parts
> of the object are safe to read. In particular, the instance type
> must be readable for the chain of `Is##Name()` type checks.
>
> This is guaranteed if
>
>  - a global memory fence happened after object initialization and
>    prior to the read by the compiler; or
>  - the object was published through a release store and read through
>    an acquire read.
>
> The former is protected by the new call to ObjectMayBeUninitialized
> (which internally calls IsPendingAllocation) in TryGetOrCreateData.
>
> The latter must be marked explicitly by calling the new
> MakeRefAssumeMemoryFence variant.
>
> Note that support in this CL is expected to be incomplete and will
> have to be extended in the future as more cases show up in which
> MakeRef calls must be converted to MakeRefAssumeMemoryFence or to
> TryMakeRef.
>
> Bug: v8:7790,v8:11711
> Change-Id: Ic2f7d9fc46e4bfc3f6bbe42816f73fc5ec174337
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2874663
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Georg Neis <neis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#74474}

Bug: v8:7790
Bug: v8:11711
Change-Id: Ia736cd1143da30ca25fdc2c3c1a2056ebf18d596
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2883245
Auto-Submit: Bill Budge <bbudge@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#74484}
parent f779fba4
Show whitespace changes
Inline Side-by-side
Showing with 77 additions and 128 deletions
src/compiler/heap-refs.cc
View file @ d23dbf3b
......@@ -71,15 +71,13 @@ enum ObjectDataKind {
namespace {
bool IsReadOnlyHeapObjectForCompiler(HeapObject object) {
bool IsReadOnlyHeapObject(Object object) {
DisallowGarbageCollection no_gc;
// HeapNumber is excluded because we have a uniform background
// serialization treatment for it.
// TODO(jgruber): Remove this compiler-specific predicate and use the plain
// heap predicate instead. This would involve removing the special cases for
// builtins and heap numbers.
return (object.IsCode() && Code::cast(object).is_builtin()) ||
(!object.IsHeapNumber() && ReadOnlyHeap::Contains(object));
(object.IsHeapObject() && !object.IsHeapNumber() &&
ReadOnlyHeap::Contains(HeapObject::cast(object)));
}
} // namespace
......@@ -118,8 +116,7 @@ class ObjectData : public ZoneObject {
kind == kNeverSerializedHeapObject ||
kind == kBackgroundSerializedHeapObject);
CHECK_IMPLIES(kind == kUnserializedReadOnlyHeapObject,
object->IsHeapObject() && IsReadOnlyHeapObjectForCompiler(
HeapObject::cast(*object)));
IsReadOnlyHeapObject(*object));
}
#define DECLARE_IS(Name, ...) bool Is##Name() const;
......@@ -340,7 +337,7 @@ bool PropertyCellData::Serialize(JSHeapBroker* broker) {
}
}
ObjectData* value_data = broker->TryGetOrCreateData(value);
ObjectData* value_data = broker->TryGetOrCreateData(value, false);
if (value_data == nullptr) {
DCHECK(!broker->IsMainThread());
return false;
......@@ -2135,7 +2132,7 @@ base::Optional<PropertyCellRef> GetPropertyCellFromHeap(JSHeapBroker* broker,
it.TryLookupCachedProperty();
if (it.state() == LookupIterator::DATA &&
it.GetHolder<JSObject>()->IsJSGlobalObject()) {
return TryMakeRef(broker, it.GetPropertyCell());
return MakeRef(broker, it.GetPropertyCell());
}
return base::nullopt;
}
......@@ -2807,14 +2804,15 @@ void JSHeapBroker::ClearReconstructibleData() {
}
}
ObjectData* JSHeapBroker::TryGetOrCreateData(Handle<Object> object,
GetOrCreateDataFlags flags) {
ObjectData* JSHeapBroker::TryGetOrCreateData(
Handle<Object> object, bool crash_on_error,
ObjectRef::BackgroundSerialization background_serialization) {
RefsMap::Entry* entry = refs_->Lookup(object.address());
if (entry != nullptr) return entry->value;
if (mode() == JSHeapBroker::kDisabled) {
entry = refs_->LookupOrInsert(object.address());
ObjectData** storage = &entry->value;
ObjectData** storage = &(entry->value);
if (*storage == nullptr) {
entry->value = zone()->New<ObjectData>(
this, storage, object,
......@@ -2829,43 +2827,24 @@ ObjectData* JSHeapBroker::TryGetOrCreateData(Handle<Object> object,
ObjectData* object_data;
if (object->IsSmi()) {
entry = refs_->LookupOrInsert(object.address());
return zone()->New<ObjectData>(this, &entry->value, object, kSmi);
}
DCHECK(!object->IsSmi());
const bool crash_on_error = (flags & kCrashOnError) != 0;
if ((flags & kAssumeMemoryFence) == 0 &&
ObjectMayBeUninitialized(HeapObject::cast(*object))) {
TRACE_BROKER_MISSING(this, "Object may be uninitialized " << *object);
CHECK_WITH_MSG(!crash_on_error, "Ref construction failed");
return nullptr;
}
if (IsReadOnlyHeapObjectForCompiler(HeapObject::cast(*object))) {
object_data = zone()->New<ObjectData>(this, &(entry->value), object, kSmi);
} else if (IsReadOnlyHeapObject(*object)) {
entry = refs_->LookupOrInsert(object.address());
return zone()->New<ObjectData>(this, &entry->value, object,
object_data = zone()->New<ObjectData>(this, &(entry->value), object,
kUnserializedReadOnlyHeapObject);
}
const ObjectRef::BackgroundSerialization background_serialization =
(flags & kAllowBackgroundSerialization) != 0
? ObjectRef::BackgroundSerialization::kAllowed
: ObjectRef::BackgroundSerialization::kDisallowed;
#define CREATE_DATA(Name, Kind) \
if (object->Is##Name()) { \
} \
/* NOLINTNEXTLINE(readability/braces) */ \
else if (object->Is##Name()) { \
CreateDataFunctor<Kind, Name##Data, Name> f; \
if (!f(this, refs_, background_serialization, object, &entry, \
&object_data)) { \
CHECK_WITH_MSG(!crash_on_error, "Ref construction failed"); \
CHECK(!crash_on_error); \
return nullptr; \
} \
/* NOLINTNEXTLINE(readability/braces) */ \
} else
}
HEAP_BROKER_OBJECT_LIST(CREATE_DATA)
#undef CREATE_DATA
{
} else {
UNREACHABLE();
}
// At this point the entry pointer is not guaranteed to be valid as
......@@ -3411,7 +3390,7 @@ base::Optional<CallHandlerInfoRef> FunctionTemplateInfoRef::call_code() const {
if (data_->should_access_heap()) {
HeapObject call_code = object()->call_code(kAcquireLoad);
if (call_code.IsUndefined()) return base::nullopt;
return TryMakeRef(broker(), CallHandlerInfo::cast(call_code));
return MakeRef(broker(), CallHandlerInfo::cast(call_code));
}
ObjectData* call_code = data()->AsFunctionTemplateInfo()->call_code();
if (!call_code) return base::nullopt;
......@@ -4012,7 +3991,7 @@ base::Optional<ObjectRef> JSArrayRef::GetOwnCowElement(
base::Optional<CellRef> SourceTextModuleRef::GetCell(int cell_index) const {
if (data_->should_access_heap()) {
return TryMakeRef(broker(), object()->GetCell(cell_index));
return MakeRef(broker(), object()->GetCell(cell_index));
}
ObjectData* cell =
data()->AsSourceTextModule()->GetCell(broker(), cell_index);
......@@ -4028,26 +4007,17 @@ base::Optional<ObjectRef> SourceTextModuleRef::import_meta() const {
data()->AsSourceTextModule()->GetImportMeta(broker()));
}
namespace {
GetOrCreateDataFlags FlagsForBackgroundSerialization(
ObjectRef::BackgroundSerialization v) {
if (v == ObjectRef::BackgroundSerialization::kAllowed) {
return kAllowBackgroundSerialization;
}
return {};
}
} // namespace
ObjectRef::ObjectRef(JSHeapBroker* broker, Handle<Object> object,
BackgroundSerialization background_serialization,
bool check_type)
: broker_(broker) {
CHECK_NE(broker->mode(), JSHeapBroker::kRetired);
data_ = broker->GetOrCreateData(
object, FlagsForBackgroundSerialization(background_serialization));
data_ = broker->GetOrCreateData(object, background_serialization);
if (!data_) { // TODO(mslekova): Remove once we're on the background thread.
object->Print();
}
CHECK_WITH_MSG(data_ != nullptr, "Object is not known to the heap broker");
}
namespace {
......@@ -4094,10 +4064,9 @@ HeapObjectType HeapObjectRef::GetHeapObjectType() const {
if (map().is_callable()) flags |= HeapObjectType::kCallable;
return HeapObjectType(map().instance_type(), flags, map().oddball_type());
}
base::Optional<JSObjectRef> AllocationSiteRef::boilerplate() const {
if (data_->should_access_heap()) {
return TryMakeRef(broker(), object()->boilerplate(kAcquireLoad));
return MakeRef(broker(), object()->boilerplate(kAcquireLoad));
}
ObjectData* boilerplate = data()->AsAllocationSite()->boilerplate();
if (boilerplate) {
......@@ -4113,7 +4082,7 @@ ElementsKind JSObjectRef::GetElementsKind() const {
base::Optional<FixedArrayBaseRef> JSObjectRef::elements() const {
if (data_->should_access_heap()) {
return TryMakeRef(broker(), object()->elements());
return MakeRef(broker(), object()->elements());
}
const JSObjectData* d = data()->AsJSObject();
if (!d->serialized_elements()) {
......@@ -4319,8 +4288,7 @@ void NativeContextData::SerializeOnBackground(JSHeapBroker* broker) {
constexpr auto kAllowed = ObjectRef::BackgroundSerialization::kAllowed;
#define SERIALIZE_MEMBER(type, name) \
DCHECK_NULL(name##_); \
name##_ = broker->GetOrCreateData( \
context->name(), FlagsForBackgroundSerialization(kAllowed)); \
name##_ = broker->GetOrCreateData(context->name(), kAllowed); \
if (!name##_->should_access_heap()) { \
DCHECK(!name##_->IsJSFunction()); \
}
......@@ -4335,8 +4303,8 @@ void NativeContextData::SerializeOnBackground(JSHeapBroker* broker) {
int const last = Context::LAST_FUNCTION_MAP_INDEX;
function_maps_.reserve(last + 1 - first);
for (int i = first; i <= last; ++i) {
function_maps_.push_back(broker->GetOrCreateData(
context->get(i), FlagsForBackgroundSerialization(kAllowed)));
function_maps_.push_back(
broker->GetOrCreateData(context->get(i), kAllowed));
}
}
......@@ -4373,7 +4341,7 @@ bool JSFunctionRef::serialized_code_and_feedback() const {
CodeRef JSFunctionRef::code() const {
if (data_->should_access_heap() || broker()->is_concurrent_inlining()) {
return MakeRefAssumeMemoryFence(broker(), object()->code(kAcquireLoad));
return MakeRef(broker(), object()->code(kAcquireLoad));
}
return CodeRef(broker(), ObjectRef::data()->AsJSFunction()->code());
......
src/compiler/js-heap-broker.cc
View file @ d23dbf3b
......@@ -227,21 +227,27 @@ bool JSHeapBroker::IsArrayOrObjectPrototype(Handle<JSObject> object) const {
array_and_object_prototypes_.end();
}
ObjectData* JSHeapBroker::TryGetOrCreateData(Object object,
GetOrCreateDataFlags flags) {
return TryGetOrCreateData(CanonicalPersistentHandle(object), flags);
}
ObjectData* JSHeapBroker::GetOrCreateData(Handle<Object> object,
GetOrCreateDataFlags flags) {
ObjectData* return_value = TryGetOrCreateData(object, flags | kCrashOnError);
ObjectData* JSHeapBroker::TryGetOrCreateData(
Object object, bool crash_on_error,
ObjectRef::BackgroundSerialization background_serialization) {
return TryGetOrCreateData(CanonicalPersistentHandle(object), crash_on_error,
background_serialization);
}
ObjectData* JSHeapBroker::GetOrCreateData(
Handle<Object> object,
ObjectRef::BackgroundSerialization background_serialization) {
ObjectData* return_value =
TryGetOrCreateData(object, true, background_serialization);
DCHECK_NOT_NULL(return_value);
return return_value;
}
ObjectData* JSHeapBroker::GetOrCreateData(Object object,
GetOrCreateDataFlags flags) {
return GetOrCreateData(CanonicalPersistentHandle(object), flags);
ObjectData* JSHeapBroker::GetOrCreateData(
Object object,
ObjectRef::BackgroundSerialization background_serialization) {
return GetOrCreateData(CanonicalPersistentHandle(object),
background_serialization);
}
bool JSHeapBroker::StackHasOverflowed() const {
......@@ -253,12 +259,8 @@ bool JSHeapBroker::StackHasOverflowed() const {
}
bool JSHeapBroker::ObjectMayBeUninitialized(Handle<Object> object) const {
if (!object->IsHeapObject()) return false;
return ObjectMayBeUninitialized(HeapObject::cast(*object));
}
bool JSHeapBroker::ObjectMayBeUninitialized(HeapObject object) const {
return !IsMainThread() && isolate()->heap()->IsPendingAllocation(object);
return !IsMainThread() && object->IsHeapObject() &&
isolate()->heap()->IsPendingAllocation(HeapObject::cast(*object));
}
bool CanInlineElementAccess(MapRef const& map) {
......
src/compiler/js-heap-broker.h
View file @ d23dbf3b
......@@ -79,21 +79,6 @@ struct PropertyAccessTarget {
};
};
enum GetOrCreateDataFlag {
// If set, a failure to create the data object results in a crash.
kCrashOnError = 1 << 0,
// If set, data construction assumes that the given object is protected by
// a memory fence (e.g. acquire-release) and thus fields required for
// construction (like Object::map) are safe to read. The protection can
// extend to some other situations as well.
kAssumeMemoryFence = 1 << 1,
// Whether background serialization is allowed (only used for
// kPossiblyBackgroundSerialized refs).
kAllowBackgroundSerialization = 1 << 2,
};
using GetOrCreateDataFlags = base::Flags<GetOrCreateDataFlag>;
DEFINE_OPERATORS_FOR_FLAGS(GetOrCreateDataFlags)
class V8_EXPORT_PRIVATE JSHeapBroker {
public:
JSHeapBroker(Isolate* isolate, Zone* broker_zone, bool tracing_enabled,
......@@ -166,16 +151,25 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
Handle<Object> GetRootHandle(Object object);
// Never returns nullptr.
ObjectData* GetOrCreateData(Handle<Object> object,
GetOrCreateDataFlags flags = {});
ObjectData* GetOrCreateData(Object object, GetOrCreateDataFlags flags = {});
ObjectData* GetOrCreateData(
Handle<Object>,
ObjectRef::BackgroundSerialization background_serialization =
ObjectRef::BackgroundSerialization::kDisallowed);
// Like the previous but wraps argument in handle first (for convenience).
ObjectData* GetOrCreateData(
Object, ObjectRef::BackgroundSerialization background_serialization =
ObjectRef::BackgroundSerialization::kDisallowed);
// Gets data only if we have it. However, thin wrappers will be created for
// smis, read-only objects and never-serialized objects.
ObjectData* TryGetOrCreateData(Handle<Object> object,
GetOrCreateDataFlags flags = {});
ObjectData* TryGetOrCreateData(Object object,
GetOrCreateDataFlags flags = {});
ObjectData* TryGetOrCreateData(
Handle<Object>, bool crash_on_error = false,
ObjectRef::BackgroundSerialization background_serialization =
ObjectRef::BackgroundSerialization::kDisallowed);
ObjectData* TryGetOrCreateData(
Object object, bool crash_on_error = false,
ObjectRef::BackgroundSerialization background_serialization =
ObjectRef::BackgroundSerialization::kDisallowed);
// Check if {object} is any native context's %ArrayPrototype% or
// %ObjectPrototype%.
......@@ -397,7 +391,6 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
// thus safe to read from a memory safety perspective. The converse does not
// necessarily hold.
bool ObjectMayBeUninitialized(Handle<Object> object) const;
bool ObjectMayBeUninitialized(HeapObject object) const;
bool CanUseFeedback(const FeedbackNexus& nexus) const;
const ProcessedFeedback& NewInsufficientFeedback(FeedbackSlotKind kind) const;
......@@ -587,8 +580,8 @@ class V8_NODISCARD UnparkedScopeIfNeeded {
template <class T,
typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
JSHeapBroker* broker, T object, GetOrCreateDataFlags flags = {}) {
ObjectData* data = broker->TryGetOrCreateData(object, flags);
JSHeapBroker* broker, T object) {
ObjectData* data = broker->TryGetOrCreateData(object);
if (data == nullptr) {
TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(object));
return {};
......@@ -599,8 +592,8 @@ base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
template <class T,
typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
JSHeapBroker* broker, Handle<T> object, GetOrCreateDataFlags flags = {}) {
ObjectData* data = broker->TryGetOrCreateData(object, flags);
JSHeapBroker* broker, Handle<T> object) {
ObjectData* data = broker->TryGetOrCreateData(object);
if (data == nullptr) {
TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(*object));
return {};
......@@ -621,20 +614,6 @@ typename ref_traits<T>::ref_type MakeRef(JSHeapBroker* broker,
return TryMakeRef(broker, object).value();
}
template <class T,
typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
typename ref_traits<T>::ref_type MakeRefAssumeMemoryFence(JSHeapBroker* broker,
T object) {
return TryMakeRef(broker, object, kAssumeMemoryFence).value();
}
template <class T,
typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
typename ref_traits<T>::ref_type MakeRefAssumeMemoryFence(JSHeapBroker* broker,
Handle<T> object) {
return TryMakeRef(broker, object, kAssumeMemoryFence).value();
}
} // namespace compiler
} // namespace internal
} // namespace v8
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment