Skip to content
forked from v8/v8

Commit

Permalink
[compiler] fix bug in inlining of Array.At
Browse files Browse the repository at this point in the history
The inlined version of Array.At was only checking the kind of the
maps, rather than the maps themselves. When the feedback was
containing an array map that "supports_fast_array_iteration", then its
kind was added to the list of supported kinds. If this Array.at was
later called with a non-array map with the same kind, then the object
would be wrongly treated as an array.

This is now fixed: inlining Array.at checks the maps directly rather
than only their kinds.

Fixed: chromium:1377775
Change-Id: I6669ffdc04df04a7c9d00d6b9f8bac82dc9cd235
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3981554
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Commit-Queue: Darius Mercadier <dmercadier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83946}
  • Loading branch information
Darius M authored and V8 LUCI CQ committed Oct 27, 2022
1 parent f257ed5 commit 0ce2731
Showing 1 changed file with 22 additions and 26 deletions.
48 changes: 22 additions & 26 deletions src/compiler/js-call-reducer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -715,9 +715,8 @@ class IteratingArrayBuiltinReducerAssembler : public JSCallReducerAssembler {
MapInference* inference, const bool has_stability_dependency,
ElementsKind kind, const SharedFunctionInfoRef& shared,
const NativeContextRef& native_context, ArrayEverySomeVariant variant);
TNode<Object> ReduceArrayPrototypeAt(ZoneVector<ElementsKind> kinds,
bool needs_fallback_builtin_call,
Node* receiver_kind);
TNode<Object> ReduceArrayPrototypeAt(ZoneVector<const MapRef*> kinds,
bool needs_fallback_builtin_call);
TNode<Object> ReduceArrayPrototypeIndexOfIncludes(
ElementsKind kind, ArrayIndexOfIncludesVariant variant);
TNode<Number> ReduceArrayPrototypePush(MapInference* inference);
Expand Down Expand Up @@ -1391,24 +1390,25 @@ TNode<Object> JSCallReducerAssembler::ReduceJSCallMathMinMaxWithArrayLike(
}

TNode<Object> IteratingArrayBuiltinReducerAssembler::ReduceArrayPrototypeAt(
ZoneVector<ElementsKind> kinds, bool needs_fallback_builtin_call,
Node* receiver_kind) {
ZoneVector<const MapRef*> maps, bool needs_fallback_builtin_call) {
TNode<JSArray> receiver = ReceiverInputAs<JSArray>();
TNode<Object> index = ArgumentOrZero(0);

TNode<Number> index_num = CheckSmi(index);
TNode<FixedArrayBase> elements = LoadElements(receiver);

TNode<Map> receiver_map = LoadMap(receiver);

auto out = MakeLabel(MachineRepresentation::kTagged);

for (ElementsKind kind : kinds) {
for (const MapRef* map : maps) {
DCHECK(map->supports_fast_array_iteration());
auto correct_map_label = MakeLabel(), wrong_map_label = MakeLabel();
Branch(NumberEqual(TNode<Number>::UncheckedCast(receiver_kind),
NumberConstant(kind)),
&correct_map_label, &wrong_map_label);
TNode<Boolean> is_map_equal = ReferenceEqual(receiver_map, Constant(*map));
Branch(is_map_equal, &correct_map_label, &wrong_map_label);
Bind(&correct_map_label);

TNode<Number> length = LoadJSArrayLength(receiver, kind);
TNode<Number> length = LoadJSArrayLength(receiver, map->elements_kind());

// If index is less than 0, then subtract from length.
TNode<Boolean> cond = NumberLessThan(index_num, ZeroConstant());
Expand All @@ -1427,15 +1427,16 @@ TNode<Object> IteratingArrayBuiltinReducerAssembler::ReduceArrayPrototypeAt(

// Retrieving element at index.
TNode<Object> element = LoadElement<Object>(
AccessBuilder::ForFixedArrayElement(kind), elements, real_index_num);
if (IsHoleyElementsKind(kind)) {
AccessBuilder::ForFixedArrayElement(map->elements_kind()), elements,
real_index_num);
if (IsHoleyElementsKind(map->elements_kind())) {
// This case is needed in particular for HOLEY_DOUBLE_ELEMENTS: raw
// doubles are stored in the FixedDoubleArray, and need to be converted to
// HeapNumber or to Smi so that this function can return an Object. The
// automatic converstion performed by
// RepresentationChanger::GetTaggedRepresentationFor does not handle
// holes, so we convert manually a potential hole here.
element = TryConvertHoleToUndefined(element, kind);
element = TryConvertHoleToUndefined(element, map->elements_kind());
}
Goto(&out, element);

Expand Down Expand Up @@ -5831,25 +5832,22 @@ Reduction JSCallReducer::ReduceArrayPrototypeAt(Node* node) {
MapInference inference(broker(), receiver, effect);
if (!inference.HaveMaps()) return NoChange();

// Collecting kinds
ZoneVector<ElementsKind> kinds(broker()->zone());
// Collecting maps, and checking if a fallback builtin call will be required
// (it is required if at least one map doesn't support fast array iteration).
ZoneVector<const MapRef*> maps(broker()->zone());
bool needs_fallback_builtin_call = false;
for (const MapRef& map : inference.GetMaps()) {
if (map.supports_fast_array_iteration()) {
ElementsKind kind = map.elements_kind();
// Checking that |kind| isn't already in |kinds|. Using std::find should
// be fast enough since |kinds| can contain at most 4 items.
if (std::find(kinds.begin(), kinds.end(), kind) == kinds.end()) {
kinds.push_back(kind);
}
maps.push_back(&map);
} else {
needs_fallback_builtin_call = true;
}
}

inference.RelyOnMapsPreferStability(dependencies(), jsgraph(), &effect,
control, p.feedback());

if (kinds.empty()) {
if (maps.empty()) {
// No map in the feedback supports fast iteration. Keeping the builtin call.
return NoChange();
}
Expand All @@ -5858,13 +5856,11 @@ Reduction JSCallReducer::ReduceArrayPrototypeAt(Node* node) {
return NoChange();
}

Node* receiver_kind = LoadReceiverElementsKind(receiver, &effect, control);

IteratingArrayBuiltinReducerAssembler a(this, node);
a.InitializeEffectControl(effect, control);

TNode<Object> subgraph = a.ReduceArrayPrototypeAt(
kinds, needs_fallback_builtin_call, receiver_kind);
TNode<Object> subgraph =
a.ReduceArrayPrototypeAt(maps, needs_fallback_builtin_call);
return ReplaceWithSubgraph(&a, subgraph);
}

Expand Down

0 comments on commit 0ce2731

Please sign in to comment.