Skip to content

Commit 4cf9311

Browse files
LeszekSwirskiV8 LUCI CQ
LeszekSwirski
authored and
V8 LUCI CQ
committed
[compiler] Preserve field repr in property array extension
Walk the descriptor array in lockstep with the property array when extending the latter. Fixed: 460017370 Change-Id: If0b4fc3c5f62fc0cc373588cbddc3c0a95c7225c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7146166 Commit-Queue: Leszek Swirski <[email protected]> Reviewed-by: Nico Hartmann <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/main@{#103674}
1 parent 50d658f commit 4cf9311

7 files changed

Lines changed: 109 additions & 25 deletions

‎src/compiler/access-builder.cc‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
#include "src/compiler/access-builder.h"
66

7+
#include "src/codegen/machine-type.h"
8+
#include "src/compiler/property-access-builder.h"
79
#include "src/compiler/type-cache.h"
810
#include "src/handles/handles-inl.h"
911
#include "src/objects/arguments.h"
@@ -1070,12 +1072,16 @@ FieldAccess AccessBuilder::ForFeedbackVectorSlot(int index) {
10701072
}
10711073

10721074
// static
1073-
FieldAccess AccessBuilder::ForPropertyArraySlot(int index) {
1075+
FieldAccess AccessBuilder::ForPropertyArraySlot(int index,
1076+
Representation representation) {
10741077
int offset = PropertyArray::OffsetOfElementAt(index);
1075-
FieldAccess access = {kTaggedBase, offset,
1076-
Handle<Name>(), OptionalMapRef(),
1077-
Type::Any(), MachineType::AnyTagged(),
1078-
kFullWriteBarrier, "PropertyArraySlot"};
1078+
MachineType machine_type =
1079+
representation.IsHeapObject() || representation.IsDouble()
1080+
? MachineType::TaggedPointer()
1081+
: MachineType::AnyTagged();
1082+
FieldAccess access = {
1083+
kTaggedBase, offset, Handle<Name>(), OptionalMapRef(),
1084+
Type::Any(), machine_type, kFullWriteBarrier, "PropertyArraySlot"};
10791085
return access;
10801086
}
10811087

‎src/compiler/access-builder.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "src/compiler/write-barrier-kind.h"
1212
#include "src/objects/elements-kind.h"
1313
#include "src/objects/js-objects.h"
14+
#include "src/objects/property-details.h"
1415

1516
namespace v8 {
1617
namespace internal {
@@ -318,7 +319,8 @@ class V8_EXPORT_PRIVATE AccessBuilder final
318319
static FieldAccess ForFeedbackVectorSlot(int index);
319320

320321
// Provides access to PropertyArray slots.
321-
static FieldAccess ForPropertyArraySlot(int index);
322+
static FieldAccess ForPropertyArraySlot(int index,
323+
Representation representation);
322324

323325
// Provides access to ScopeInfo flags.
324326
static FieldAccess ForScopeInfoFlags();

‎src/compiler/js-native-context-specialization.cc‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
#include "src/objects/elements-kind.h"
3939
#include "src/objects/feedback-vector.h"
4040
#include "src/objects/heap-number.h"
41+
#include "src/objects/property-details.h"
4142
#include "src/objects/string.h"
4243

4344
namespace v8 {
@@ -4235,25 +4236,59 @@ Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore(
42354236
// for intermediate states of chains of property additions. That makes
42364237
// it unclear what the best approach is here.
42374238
DCHECK_EQ(map.UnusedPropertyFields(), 0);
4238-
int length = map.NextFreePropertyIndex() - map.GetInObjectProperties();
4239+
int in_object_length = map.GetInObjectProperties();
4240+
int length = map.NextFreePropertyIndex() - in_object_length;
42394241
// Under normal circumstances, NextFreePropertyIndex() will always be larger
42404242
// than GetInObjectProperties(). However, an attacker able to corrupt heap
42414243
// memory can break this invariant, in which case we'll get confused here,
42424244
// potentially causing a sandbox violation. This CHECK defends against that.
42434245
SBXCHECK_GE(length, 0);
42444246
int new_length = length + JSObject::kFieldsAdded;
4247+
4248+
// Find the descriptor index corresponding to the first out-of-object
4249+
// property.
4250+
DescriptorArrayRef descs = map.instance_descriptors(broker());
4251+
InternalIndex first_out_of_object_descriptor(in_object_length);
4252+
InternalIndex number_of_descriptors(descs.object()->number_of_descriptors());
4253+
for (InternalIndex i(in_object_length); i < number_of_descriptors; ++i) {
4254+
PropertyDetails details = descs.GetPropertyDetails(i);
4255+
// Skip over non-field properties.
4256+
if (details.location() != PropertyLocation::kField) {
4257+
continue;
4258+
}
4259+
// Skip over in-object fields.
4260+
// TODO(leszeks): We could make this smarter, like a binary search.
4261+
if (details.field_index() < in_object_length) {
4262+
continue;
4263+
}
4264+
first_out_of_object_descriptor = i;
4265+
break;
4266+
}
4267+
42454268
// Collect the field values from the {properties}.
4246-
ZoneVector<Node*> values(zone());
4269+
ZoneVector<std::pair<Node*, Representation>> values(zone());
42474270
values.reserve(new_length);
4248-
for (int i = 0; i < length; ++i) {
4271+
4272+
// Walk the property descriptors alongside the property values, to make
4273+
// sure to get and store them with the right machine type.
4274+
InternalIndex descriptor = first_out_of_object_descriptor;
4275+
for (int i = 0; i < length; ++i, ++descriptor) {
4276+
PropertyDetails details = descs.GetPropertyDetails(descriptor);
4277+
while (details.location() != PropertyLocation::kField) {
4278+
++descriptor;
4279+
details = descs.GetPropertyDetails(descriptor);
4280+
}
4281+
DCHECK_EQ(i, details.field_index() - in_object_length);
42494282
Node* value = effect = graph()->NewNode(
4250-
simplified()->LoadField(AccessBuilder::ForFixedArraySlot(i)),
4283+
simplified()->LoadField(
4284+
AccessBuilder::ForPropertyArraySlot(i, details.representation())),
42514285
properties, effect, control);
4252-
values.push_back(value);
4286+
values.push_back({value, details.representation()});
42534287
}
42544288
// Initialize the new fields to undefined.
42554289
for (int i = 0; i < JSObject::kFieldsAdded; ++i) {
4256-
values.push_back(jsgraph()->UndefinedConstant());
4290+
values.push_back(
4291+
{jsgraph()->UndefinedConstant(), Representation::Tagged()});
42574292
}
42584293

42594294
// Compute new length and hash.
@@ -4291,7 +4326,8 @@ Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore(
42914326
a.Store(AccessBuilder::ForMap(), jsgraph()->PropertyArrayMapConstant());
42924327
a.Store(AccessBuilder::ForPropertyArrayLengthAndHash(), new_length_and_hash);
42934328
for (int i = 0; i < new_length; ++i) {
4294-
a.Store(AccessBuilder::ForFixedArraySlot(i), values[i]);
4329+
a.Store(AccessBuilder::ForPropertyArraySlot(i, values[i].second),
4330+
values[i].first);
42954331
}
42964332
return a.Finish();
42974333
}

‎src/compiler/turboshaft/turbolev-early-lowering-reducer-inl.h‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "src/compiler/turboshaft/representations.h"
1515
#include "src/deoptimizer/deoptimize-reason.h"
1616
#include "src/objects/contexts.h"
17+
#include "src/objects/descriptor-array-inl.h"
1718
#include "src/objects/instance-type-inl.h"
1819

1920
namespace v8::internal::compiler::turboshaft {
@@ -318,8 +319,32 @@ class TurbolevEarlyLoweringReducer : public Next {
318319
}
319320

320321
V<PropertyArray> ExtendPropertiesBackingStore(
321-
V<PropertyArray> old_property_array, V<JSObject> object, int old_length,
322+
V<PropertyArray> old_property_array, V<JSObject> object,
323+
const compiler::MapRef& old_map, int old_length,
322324
V<FrameState> frame_state, const FeedbackSource& feedback) {
325+
int in_object_length = old_map.GetInObjectProperties();
326+
327+
// Find the descriptor index corresponding to the first out-of-object
328+
// property.
329+
DescriptorArrayRef descs = old_map.instance_descriptors(broker_);
330+
InternalIndex first_out_of_object_descriptor(in_object_length);
331+
InternalIndex number_of_descriptors(
332+
descs.object()->number_of_descriptors());
333+
for (InternalIndex i(in_object_length); i < number_of_descriptors; ++i) {
334+
PropertyDetails details = descs.GetPropertyDetails(i);
335+
// Skip over non-field properties.
336+
if (details.location() != PropertyLocation::kField) {
337+
continue;
338+
}
339+
// Skip over in-object fields.
340+
// TODO(leszeks): We could make this smarter, like a binary search.
341+
if (details.field_index() < in_object_length) {
342+
continue;
343+
}
344+
first_out_of_object_descriptor = i;
345+
break;
346+
}
347+
323348
// Allocate new PropertyArray.
324349
int new_length = old_length + JSObject::kFieldsAdded;
325350
Uninitialized<PropertyArray> new_property_array =
@@ -330,18 +355,28 @@ class TurbolevEarlyLoweringReducer : public Next {
330355
__ HeapConstant(factory_->property_array_map()));
331356

332357
// Copy existing properties over.
333-
for (int i = 0; i < old_length; i++) {
358+
InternalIndex descriptor = first_out_of_object_descriptor;
359+
for (int i = 0; i < old_length; ++i, ++descriptor) {
360+
PropertyDetails details = descs.GetPropertyDetails(descriptor);
361+
while (details.location() != PropertyLocation::kField) {
362+
++descriptor;
363+
details = descs.GetPropertyDetails(descriptor);
364+
}
365+
DCHECK_EQ(i, details.field_index() - in_object_length);
366+
Representation r = details.representation();
367+
334368
V<Object> old_value = __ template LoadField<Object>(
335-
old_property_array, AccessBuilder::ForPropertyArraySlot(i));
369+
old_property_array, AccessBuilder::ForPropertyArraySlot(i, r));
336370
__ InitializeField(new_property_array,
337-
AccessBuilder::ForPropertyArraySlot(i), old_value);
371+
AccessBuilder::ForPropertyArraySlot(i, r), old_value);
338372
}
339373

340374
// Initialize new properties to undefined.
341375
V<Undefined> undefined = __ HeapConstant(factory_->undefined_value());
342376
for (int i = 0; i < JSObject::kFieldsAdded; ++i) {
343377
__ InitializeField(new_property_array,
344-
AccessBuilder::ForPropertyArraySlot(old_length + i),
378+
AccessBuilder::ForPropertyArraySlot(
379+
old_length + i, Representation::Tagged()),
345380
undefined);
346381
}
347382

‎src/compiler/turboshaft/turbolev-graph-builder.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2793,10 +2793,11 @@ class GraphBuildingNodeProcessor {
27932793
maglev::ProcessResult Process(maglev::ExtendPropertiesBackingStore* node,
27942794
const maglev::ProcessingState& state) {
27952795
GET_FRAME_STATE_MAYBE_ABORT(frame_state, node->eager_deopt_info());
2796-
SetMap(node, __ ExtendPropertiesBackingStore(
2797-
Map(node->property_array_input()),
2798-
Map(node->object_input()), node->old_length(), frame_state,
2799-
node->eager_deopt_info()->feedback_to_update()));
2796+
SetMap(node,
2797+
__ ExtendPropertiesBackingStore(
2798+
Map(node->property_array_input()), Map(node->object_input()),
2799+
node->old_map(), node->old_length(), frame_state,
2800+
node->eager_deopt_info()->feedback_to_update()));
28002801
return maglev::ProcessResult::kContinue;
28012802
}
28022803

‎src/maglev/maglev-graph-builder.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5210,7 +5210,7 @@ ReduceResult MaglevGraphBuilder::BuildExtendPropertiesBackingStore(
52105210
// potentially causing a sandbox violation. This CHECK defends against that.
52115211
SBXCHECK_GE(length, 0);
52125212
return AddNewNode<ExtendPropertiesBackingStore>({property_array, receiver},
5213-
length);
5213+
map, length);
52145214
}
52155215

52165216
MaybeReduceResult MaglevGraphBuilder::TryBuildStoreField(

‎src/maglev/maglev-ir.h‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9215,8 +9215,10 @@ class ExtendPropertiesBackingStore
92159215
using Base = FixedInputValueNodeT<2, ExtendPropertiesBackingStore>;
92169216

92179217
public:
9218-
explicit ExtendPropertiesBackingStore(uint64_t bitfield, int old_length)
9219-
: Base(bitfield), old_length_(old_length) {}
9218+
explicit ExtendPropertiesBackingStore(uint64_t bitfield,
9219+
const compiler::MapRef& old_map,
9220+
int old_length)
9221+
: Base(bitfield), old_map_(old_map), old_length_(old_length) {}
92209222

92219223
static constexpr OpProperties kProperties =
92229224
OpProperties::CanAllocate() | OpProperties::CanRead() |
@@ -9236,9 +9238,11 @@ class ExtendPropertiesBackingStore
92369238
void GenerateCode(MaglevAssembler*, const ProcessingState&);
92379239
void PrintParams(std::ostream&) const;
92389240

9241+
const compiler::MapRef& old_map() const { return old_map_; }
92399242
int old_length() const { return old_length_; }
92409243

92419244
private:
9245+
const compiler::MapRef old_map_;
92429246
const int old_length_;
92439247
};
92449248

0 commit comments

Comments
 (0)