Closed Bug 1502035 Opened 1 year ago Closed 1 year ago

Implement DataCount section and verify memory.init / data.drop with this info

Categories

(Core :: Javascript: WebAssembly, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

(This is vague because I'm not 100% sure what changes are needed.)

Currently we check statically that the data/element indices in mem.init/table.init are within range (and point to the correct type of data/element segment?), this requires a small amount of machinery to do the check when the module has been parsed completely, since the data segments come after the code.  A spec change requires the indices to be checked at run-time since mem.drop requires that anyway.  The spec change /may/ allow the indices to target active segments; if so, this too may have to change.

The new DataCount section figures into this: https://github.com/WebAssembly/bulk-memory-operations/pull/42

Summary: Bulk memory: Some static checks should be dynamic checks → Bulk memory: Some static checks should be dynamic checks + DataCount section

This bug is all about validation, not about run-time semantics, we'll do those elsewhere:

  • text format support for datacount section
  • make use of datacount when verifying memory.init and data.drop
  • remove the deferred validation state, as it's not needed
  • update test cases that use memory.init and data.drop to include the datacount section
  • update error messages in test cases
  • test cases for when datacount section is missing, does not contain the correct data segment count, or contains a correct count s.t. memory.init and data.drop are out of bounds statically
Summary: Bulk memory: Some static checks should be dynamic checks + DataCount section → Implement DataCount section and verify memory.init / data.drop with this info
Attached patch bug1502035-wip-datacount.patch (obsolete) — Splinter Review

Almost complete, just needs some more test cases.

Assignee: nobody → lhansen
Status: NEW → ASSIGNED

Thanks! Quite happy to see the whole deferred stuff go away.

Is (datacount x) being proposed anywhere else for addition to the text format? In my last comment I mentioned my expectation that the presence of the datacount section could be kept purely a binary format detail and that, from an AST and text format pov, there is only the vector of data segments that are validated against indices in mem.init et al and the validation that "if a mem.init is present, the datacount section must be present" is done purely as part of binary format (just like the check that the func/code section counts are the same, since this distinction is not present in the AST or text format).

Thus, my expectation is that wasmTextToBinary() would either (1) always add a datacount binary section, (2) only add it when a mem.init et al per present (which works b/c of the two-pass translation).

It is of course important to test the not-present error cases, but I think this could be done in tests/wasm/binary.js, just like we test func/code section count mismatches.

(In reply to Luke Wagner [:luke] from comment #4)

Is (datacount x) being proposed anywhere else for addition to the text format?

I don't know; I added it for testing in this case. It's not like we follow the standard text format very well in any case :)

In my last comment I mentioned my expectation that the presence of the datacount section could be kept purely a binary format detail and that, from an AST and text format pov, there is only the vector of data segments that are validated against indices in mem.init et al and the validation that "if a mem.init is present, the datacount section must be present" is done purely as part of binary format (just like the check that the func/code section counts are the same, since this distinction is not present in the AST or text format).

I'm probably happiest to infer a datacount section if one is not present in the text but data segments and memory.init instructions both are, but to accept it as given if it is present.

Thus, my expectation is that wasmTextToBinary() would either (1) always add a datacount binary section, (2) only add it when a mem.init et al per present (which works b/c of the two-pass translation).

I think (1) is probably undesirable; (2) is certainly appropriate; my solution above would be (3). LMK what you think or if you want to review (the default is otherwise to burden Julian further).

Note, this sits on top of the following patch stack:

Bug 1513499 - Update encodings for table.copy and memory.copy. r?jseward
Bug 1522082 - make encoding of memory.fill standard-compliant. r?jseward
Bug 1522081 - Update instruction encodings for memory.init / table.init. r?jseward
Bug 1521780 - Rename memory.drop and table.drop. r?jseward

Attachment #9038518 - Attachment is obsolete: true
Attachment #9038790 - Flags: review?(luke)
Comment on attachment 9038790 [details] [diff] [review]
bug1502035-datacount.patch

Review of attachment 9038790 [details] [diff] [review]:
-----------------------------------------------------------------

Good points.  Really nice patch!

::: js/src/wasm/WasmAST.h
@@ +1338,4 @@
>    }
>    uint32_t gcFeatureOptIn() const { return gcFeatureOptIn_; }
>  #endif
> +  bool addDataCount(uint32_t dataCount) {

How about 'initDataCount()'?
Attachment #9038790 - Flags: review?(luke) → review+
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
See Also: → 1523081
You need to log in before you can comment on or make changes to this bug.