Should partial memory/table initializations after instantiation traps still be visible?

[fitzgen]

Thinking particularly about these tests:

• bulk-memory-operations/test/core/linking.wast

  Line 227 in 404b7e7

  ;; Unlike in the v1 spec, the elements stored before an out-of-bounds access

• bulk-memory-operations/test/core/linking.wast

  Line 336 in 404b7e7

  ;; Unlike in v1 spec, bytes written before an out-of-bounds access persist

IIRC memory.init and table.init didn't used to do up front bounds checks, which would explain the behavior prescribed by those tests.

However, the spec now has bounds checks before any mutations for memory.init and table.init. Since instantiation of active table segments and data segments is described in terms of memory.init and table.init, I would expect the bounds checks to happen before any mutations to the memory and table.

Am I overlooking something?

[fitzgen]

Basically, these tests look out of date to me, but I want to make sure my understanding is correct before I open PRs to fix them.

Thanks!

[binji]

I think these are still correct. The MVP requires that all data segments and element segments are in bounds before writing any of them. If any of them are out of bounds, then no changes are made.

The bulk memory proposal loosens this so that segments are written in order, first element segments and then data segments. An individual segment is bounds-checked, so it will never be partially updated. But previously initialized segments are persisted.

It used to be that a segment would also be partially written if it were out-of-bounds, but we rolled back that part of the change.

[fitzgen]

Ah yes, of course! Thanks for the explanation.

It seems that while the linking.wast test checks that an in bounds segment is initialized into the table before an out-of-bounds trap happens when initializing a following segment, it is missing a check that none of a partially out of bounds segment's elements end up in the table. And similar for data segments.

(module $m
  (table (export "table") funcref (elem $zero $zero $zero $zero $zero $zero $zero $zero $zero $zero))

  (func $zero (result i32)
    (i32.const 0))

  (func (export "indirect-call") (param i32) (result i32)
    local.get 0
    call_indirect (result i32)))

(register "m" $m)

(assert_trap
  (module
    (table (import "m" "table") 10 funcref)

    (func $one (result i32)
      (i32.const 1))

    ;; An in-bounds segment that should get initialized in the table.
    (elem (i32.const 7) $one)

    ;; Part of this segment is out of bounds, so none of its elements should be
    ;; initialized into the table, and it should trap.
    (elem (i32.const 9) $one $one $one)
  )
  "out of bounds"
)

;; The first `$one` segment *was* initialized OK.
;;
;; We already have tests for this case.
(assert_return (invoke "indirect-call" (i32.const 7)) (i32.const 1))

;; The second `$one` segment is partially out of bounds, and therefore none of
;; its elements were written into the table.
;;
;; We do *not* have tests for this case.
(assert_return (invoke "indirect-call" (i32.const 9)) (i32.const 0))

Should I make a PR to add this second assertion?

[binji]

Yes, thanks!