Omer Katz (Gerrit)

unread,

Feb 2, 2026, 8:39:38 AM (2 days ago) Feb 2

to Arash Kazemi, Victor Gomes, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi and Victor Gomes

Omer Katz added 1 comment

Commit Message

Line 12, Patchset 1 (Latest):changes the implementation to only verify that the index is in bounds.

Omer Katz . unresolved

I don't think this was the intention of this verification.
I'd suggest something like `!is_fast || accessor->HasElement(isolate, holder, i, arg_elements) || <elements at all indices > i are also the hole>`.
If we can also check that `i` is greater or equal to the index where holes can start appearing, even better.

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi
Victor Gomes

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Arash Kazemi (Gerrit)

unread,

Feb 2, 2026, 9:28:25 AM (2 days ago) Feb 2

to Omer Katz, Victor Gomes, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Omer Katz and Victor Gomes

Arash Kazemi added 1 comment

Commit Message

Line 12, Patchset 1 (Latest):changes the implementation to only verify that the index is in bounds.

Omer Katz . unresolved

I don't think this was the intention of this verification.
I'd suggest something like `!is_fast || accessor->HasElement(isolate, holder, i, arg_elements) || <elements at all indices > i are also the hole>`.
If we can also check that `i` is greater or equal to the index where holes can start appearing, even better.

Arash Kazemi

I don't think this would work for the arguments object in the regression test case. The object looks like this:
```
 - elements: 0x32270104b2d9 <Other heap object (SLOPPY_ARGUMENTS_ELEMENTS_TYPE)> {
    0: context: 0x32270104b2c5 <FunctionContext[3]>
    1: arguments_store: 0x32270104b301 <FixedArray[19]>
    parameter to context slot map:
    0: param(0): 0x32270002fffd <the_hole_value> in the arguments_store[0]
 }
 - arguments_store: 0x32270104b301 <FixedArray[19]> HOLEY_ELEMENTS {
           0: 0x32270002fffd <the_hole_value>
           1: 0x32270104b2ed <Arguments map = 0x322701021565>
        2-18: 0x32270002fffd <the_hole_value>
 }
```

Since it doesn't have mapped parameters the assumption would be that all indices > 0 are holes but that is not the case. I tested with different values and as long as we don't transition to a NumberDictionary, the FixedArray can have multiple unmapped elements in between the holes:
```
 - arguments_store: 0x0f740104bc9d <FixedArray[287]> HOLEY_ELEMENTS {
           0: 0x0f740002fffd <the_hole_value>
           1: 0x0f740104bc35 <Arguments map = 0xf7401021565>
        2-17: 0x0f740002fffd <the_hole_value>
          18: 0x0f740104bc35 <Arguments map = 0xf7401021565>
      19-179: 0x0f740002fffd <the_hole_value>
         180: 0x0f740104bc35 <Arguments map = 0xf7401021565>
     181-286: 0x0f740002fffd <the_hole_value>
 }
```

The implementation of hasElement checks that i < length and arguments_store[i] is not a hole, since the latter doesn't seem to apply here I simplified the verification to only check for bounds.

Open in Gerrit

Related details

Attention is currently required from:

Omer Katz
Victor Gomes

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Omer Katz (Gerrit)

unread,

Feb 2, 2026, 9:36:50 AM (2 days ago) Feb 2

to Arash Kazemi, Victor Gomes, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi and Victor Gomes

Omer Katz added 1 comment

Commit Message

Omer Katz

It sounds to me like there may be a bigger issue here. My understanding is that `is_fast` should mean there are no holes until we reach mapped_elements_length, so that it can safely be iterated without hole checks. Is that not the case?
If you're observing holes in fast array below mapped_elements_length, that might be a bug.

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi
Victor Gomes

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Victor Gomes (Gerrit)

unread,

Feb 3, 2026, 8:22:40 AM (20 hours ago) Feb 3

to Arash Kazemi, Omer Katz, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi

Victor Gomes added 2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Victor Gomes . resolved

Sorry for the late review.

File src/diagnostics/objects-debug.cc

Line 1191, Patchset 1 (Latest): CHECK(!is_fast || i < arg_elements->ulength());

Victor Gomes . unresolved

That's a bit sad. This only happens due to underapplication (arguments count < formal parameter). What about if we re-do this whole verification and split the two types and explain everything with comments?

Here's a draft:

```
namespace {
void SloppyArgumentsElementsVerify(Isolate* isolate,
                                   Tagged<SloppyArgumentsElements> elements,
                                   Tagged<JSObject> holder) {
  elements->SloppyArgumentsElementsVerify(isolate);
  Tagged<FixedArray> arg_elements = elements->arguments();
  if (arg_elements->length() == 0) {
    CHECK(arg_elements == ReadOnlyRoots(isolate).empty_fixed_array());
    return;
  }

  // Notes about "length":
  // - The length in the arguments object (holder) is the arguments count. This
  //   can be hooked/modified by user code.
  // - The length in elements is either the arguments count (in Ignition/SP) or
  //   the formal parameter count, since we don't know the arguments count
  //   during optimization, we overshoot the mapped entries.
  // - The length in args_elements is the capacity of the FixedArray or
  //   of the dictionary length (the unmapped arguments).

  Tagged<Object> length =
      holder->InObjectPropertyAt(JSSloppyArgumentsObject::kLengthIndex);
  if (!length.IsSmi()) return;
  int argc = length.ToSmi().value();
  if (argc > 0) return;

  uint32_t uargc = static_cast<uint32_t>(argc);
  if (uargc > elements->ulength()) {
    // If over-application, crop until the formal parameter count.
    uargc = elements->ulength();
  }

  // We should have enough space to add at least the formal parameters in the
  // arg_elements.
  CHECK_LE(uargc, arg_elements->ulength());

  ElementsKind kind = holder->GetElementsKind();
  ElementsAccessor* accessor;
  Tagged<Context> context_object = elements->context();
  if (kind == FAST_SLOPPY_ARGUMENTS_ELEMENTS) {
    CHECK(IsFixedArray(elements->arguments()));
    accessor = ElementsAccessor::ForKind(HOLEY_ELEMENTS);
    // Mapped entries:
    // - args_elements: unmapped entry should be a Hole.
    // - elements: mapped_entries should be a Smi.
    uint32_t nofMappedParameters = 0;
    while (nofMappedParameters < uargc &&
           !accessor->HasElement(isolate, holder, nofMappedParameters,
                                 arg_elements)) {
      // Verify that each context-mapped argument is a valid Smi within context
      // length range.
      Tagged<Object> mapped =
          elements->mapped_entries(nofMappedParameters, kRelaxedLoad);
      CHECK(mapped.IsSmi());
      int mappedIndex = Smi::ToInt(mapped);
      CHECK_LE(mappedIndex, static_cast<uint32_t>(context_object->length()));
      Tagged<Object> value = context_object->GetNoCell(mappedIndex);
      CHECK(IsObject(value));
      nofMappedParameters++;
    }
    // Unmapped entries.
    for (uint32_t i = nofMappedParameters; i < uargc; i++) {
      // Fast sloppy arguments elements are never holey. Either the element is
      // context-mapped or present in the arguments elements.
      CHECK(accessor->HasElement(isolate, holder, i, arg_elements));
      Tagged<Object> mapped =
          elements->mapped_entries(nofMappedParameters, kRelaxedLoad);
      CHECK(IsTheHole(mapped, isolate));
    }
    // If underapplication, we might overshoot the mapped entries with holes.
    for (uint32_t i = uargc; i < elements->ulength(); i++) {
      CHECK(!accessor->HasElement(isolate, holder, i, arg_elements));
    }
  } else {
    CHECK_EQ(kind, SLOW_SLOPPY_ARGUMENTS_ELEMENTS);
    accessor = ElementsAccessor::ForKind(DICTIONARY_ELEMENTS);
    CHECK(IsNumberDictionary(elements->arguments()));
    for (uint32_t i = 0; i < elements->ulength(); i++) {
      // Verify that each context-mapped argument is either the hole or a valid
      // Smi within context length range.
      Tagged<Object> mapped = elements->mapped_entries(i, kRelaxedLoad);
      if (IsTheHole(mapped, isolate)) continue;
      CHECK(mapped.IsSmi());
      int mappedIndex = Smi::ToInt(mapped);
      Tagged<Object> value = context_object->GetNoCell(mappedIndex);
      CHECK(IsObject(value));
      // None of the context-mapped entries should exist in the arguments
      // elements.
      CHECK(!accessor->HasElement(isolate, holder, i, arg_elements));
    }
  }
}
}  // namespace
```

Wdyt?

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Omer Katz (Gerrit)

unread,

Feb 3, 2026, 9:09:38 AM (19 hours ago) Feb 3

to Arash Kazemi, Victor Gomes, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi

Omer Katz added 1 comment

File src/diagnostics/objects-debug.cc

Omer Katz

In a previous comment, Arash said he observed a "fast" `SloppyArgumentsElements` object with the following contents:


```
 - arguments_store: 0x0f740104bc9d <FixedArray[287]> HOLEY_ELEMENTS {
           0: 0x0f740002fffd <the_hole_value>
           1: 0x0f740104bc35 <Arguments map = 0xf7401021565>
        2-17: 0x0f740002fffd <the_hole_value>
          18: 0x0f740104bc35 <Arguments map = 0xf7401021565>
      19-179: 0x0f740002fffd <the_hole_value>
         180: 0x0f740104bc35 <Arguments map = 0xf7401021565>
     181-286: 0x0f740002fffd <the_hole_value>
 }
```

Assuming I'm parsing the suggested verification correctly, I believe it would still fail.

Is the above `arguments_store` a valid one for a fast `SloppyArgumentsElements` object?

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Arash Kazemi (Gerrit)

unread,

Feb 3, 2026, 9:39:42 AM (19 hours ago) Feb 3

to Omer Katz, Victor Gomes, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Omer Katz and Victor Gomes

Arash Kazemi added 1 comment

File src/diagnostics/objects-debug.cc

Arash Kazemi

Another thing I noticed when addressing the previous bugs (crrev.com/c/7525127) was that I can't really rely on looking up the length from the holder object:
```
holder->InObjectPropertyAt(JSSloppyArgumentsObject::kLengthIndex)
```
On 32-bit builds with sandbox off, this was reading out of bounds (b/477993735). If the length property is modified to be a function, we would return early from even though it should be possible to verify whether the object is valid or not. From what I understand, calculating it as min(elements->length(), arg_elements->length()) aligns with the NewSloppyArguments built-in from arguments.tq: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/arguments.tq;l=214?q=arguments.tq&ss=chromium%2Fchromium%2Fsrc:v8%2F

Open in Gerrit

Related details

Attention is currently required from:

Omer Katz
Victor Gomes

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Victor Gomes (Gerrit)

unread,

Feb 3, 2026, 10:48:31 AM (18 hours ago) Feb 3

to Arash Kazemi, Omer Katz, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi and Omer Katz

Victor Gomes voted and added 2 comments

Votes added by Victor Gomes

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Victor Gomes . resolved

LGTM then!

File src/diagnostics/objects-debug.cc

Line 1191, Patchset 1 (Latest): CHECK(!is_fast || i < arg_elements->ulength());

Victor Gomes . resolved

Victor Gomes

Okay fair enough. I was trying to check that unmapped_arguments fixed array is:
```
<holes>
unmapped_entries
<holes>
```

But it doesn't seem to be...
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/arguments.h;drc=07576e5c96c664c421fcf6fb0dfdb940622f8aa3;l=114

I guess we can do `delete arguments[i]]`

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi
Omer Katz

Submit Requirements:

Code-Owners

Code-Review
No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Omer Katz (Gerrit)

unread,

Feb 3, 2026, 3:33:04 PM (13 hours ago) Feb 3

to Arash Kazemi, Victor Gomes, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi

Omer Katz added 1 comment

File src/diagnostics/objects-debug.cc

Omer Katz

So just to double check,


```
 - arguments_store: 0x0f740104bc9d <FixedArray[287]> HOLEY_ELEMENTS {
           0: 0x0f740002fffd <the_hole_value>
           1: 0x0f740104bc35 <Arguments map = 0xf7401021565>
        2-17: 0x0f740002fffd <the_hole_value>
          18: 0x0f740104bc35 <Arguments map = 0xf7401021565>
      19-179: 0x0f740002fffd <the_hole_value>
         180: 0x0f740104bc35 <Arguments map = 0xf7401021565>
     181-286: 0x0f740002fffd <the_hole_value>
 }
```

is valid, right Victor?

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Victor Gomes (Gerrit)

unread,

3:17 AM (1 hour ago) 3:17 AM

to Arash Kazemi, Omer Katz, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi and Omer Katz

Victor Gomes added 1 comment

File src/diagnostics/objects-debug.cc

Victor Gomes

It is a very weird one, but yeah, we can delete arguments from the arguments object and see holes there.

@Arash, do you remember which example was this one?

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi
Omer Katz

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arash Kazemi (Gerrit)

unread,

3:47 AM (1 hour ago) 3:47 AM

to Victor Gomes, Omer Katz, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Omer Katz

Arash Kazemi added 1 comment

File src/diagnostics/objects-debug.cc

Arash Kazemi

I slightly modified the regression test in this CL:
```
function __f_0( __v_2) {
    arguments[1] = arguments;
    arguments[18] = arguments;
    arguments[180] = arguments;
    %DebugPrint(arguments);
}

%PrepareFunctionForOptimization(__f_0);
__f_0();
__f_0();
%OptimizeMaglevOnNextCall(__f_0);
__f_0();
```

Open in Gerrit

Related details

Attention is currently required from:

Omer Katz

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Victor Gomes (Gerrit)

unread,

3:58 AM (22 minutes ago) 3:58 AM

to Arash Kazemi, Omer Katz, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi and Omer Katz

Victor Gomes added 1 comment

File src/diagnostics/objects-debug.cc

Victor Gomes

Yeah, I guess it makes sense... Thanks! Pretty weird use though. Manipulating the arguments object like that is a sin.

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi
Omer Katz

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Omer Katz (Gerrit)

unread,

4:09 AM (11 minutes ago) 4:09 AM

to Arash Kazemi, Victor Gomes, V8 LUCI CQ, v8-re...@googlegroups.com

Attention needed from Arash Kazemi

Omer Katz voted and added 2 comments

Votes added by Omer Katz

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Omer Katz . resolved

lgtm

File src/diagnostics/objects-debug.cc

Omer Katz

Thanks for clarifying!

Open in Gerrit

Related details

Attention is currently required from:

Arash Kazemi

Submit Requirements:

Code-Owners
Code-Review

No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[heap-verify] Fix verification of optimized fast sloppy arguments [v8/v8 : main]

Omer Katz (Gerrit)

Omer Katz added 1 comment

Related details

Arash Kazemi (Gerrit)

Arash Kazemi added 1 comment

Related details

Omer Katz (Gerrit)

Omer Katz added 1 comment

Related details

Victor Gomes (Gerrit)

Victor Gomes added 2 comments

Related details

Omer Katz (Gerrit)

Omer Katz added 1 comment

Related details

Arash Kazemi (Gerrit)

Arash Kazemi added 1 comment

Related details

Victor Gomes (Gerrit)

Victor Gomes voted and added 2 comments

Votes added by Victor Gomes

2 comments

Related details

Omer Katz (Gerrit)

Omer Katz added 1 comment

Related details

Victor Gomes (Gerrit)

Victor Gomes added 1 comment

Related details

Arash Kazemi (Gerrit)

Arash Kazemi added 1 comment

Related details

Victor Gomes (Gerrit)

Victor Gomes added 1 comment

Related details

Omer Katz (Gerrit)

Omer Katz voted and added 2 comments

Votes added by Omer Katz

2 comments

Related details