1471500 - Complete initial implementation of the bulk-memory proposal

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Julian Seward [:jseward]]

Bug 1446930 provided a first implementation of memory.{fill,copy}.
There still remain 5 unimplemented opcodes, per the proposal as of
27 June 2018, at [1].  This bug is for their implementation:

  memory.init  copy from a passive data segment to linear memory
  memory.drop  prevent further use of passive data segment
  table.init   copy from a passive element segment to a table
  table.drop   prevent further use of a passive element segment
  table.copy   copy from one region of a table to another region

[1] https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md

Comment 1

[Lars T Hansen [:lth]]

Also there's an ongoing discussion about range checking semantics for memory.init (see discussion at CG 2018-06-26) but that has not concluded yet and we can probably just do that as follow-up work.  Salient points here: https://github.com/WebAssembly/spec/pull/820.

Comment 2

[Julian Seward [:jseward]]

Attachment: Initial implementation (WIP) of memory.init only.

Also contains loads of plumbing/parsing/printing/refactoring stuff
to do with passive data segments.  No tests in the patch though.

Comment 3

[Julian Seward [:jseward]]

Attachment: Implements memory.{init,drop} (WIP)

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1471500-allbm-10.diff

First fully functional implementation.  Implements:
memory.{init,drop} and table.{copy,init,drop}.
No test cases in the patch.

Comment 5

[Julian Seward [:jseward]]

Attachment: bug1471500-allbm-14.diff

Rollup patch.  Complete functionality, with a smoke test, rebased
to current m-c.

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-1-prelim-fixes.diff

Comment 7

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-2-memFillCopy-tidying.diff

Comment 8

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-3-move-Segs-to-Code.diff

Comment 9

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-4-add-LazyInit.diff

Comment 10

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-5-add-passive-Segs.diff

Comment 11

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-6-changes-to-Table.diff

Comment 12

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-7-new-insns-pre-codegen.diff

Comment 13

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-8-new-insns-cg-and-run.diff

Comment 14

[Julian Seward [:jseward]]

Attachment: bug1471500-r17-9-smoketest.diff

Comment 15

[Julian Seward [:jseward]]

First reviewable version, split into 9 patches (comments 6 .. 14).
The implementation is mostly straightforward, with the following 
architectural changes:

* |const DataSegmentVector dataSegments_| and
  |const ElemSegmentVector elemSegments_| have been moved from
  class Module to class Code, so as to make their lifetimes at least as
  long as that of any instance.  That is because they need to be reachable
  from an Instance because they need to be consulted at runtime by the
  implementation of {memory,table}.{init,drop}.

* New types |LazyInit<T>| and |LazyInitVec<T>|.  These are used to store the
  required initialisation data for {memory,table}.init.  They are computed
  at instantiation time and are then owned by the new instance.  This avoids
  having to extend or alter the lifetime of Module contents.

* There is a new |struct WasmCallee| that holds entry point data,
  being simply a pair (code pointer, instance pointer).

Each patch contains a commit message with more details.  The 9 patches are,
in summary:

1-prelim-fixes.diff
  Some preliminary bug fixes to existing mem.{copy, fill}

2-memFillCopy-tidying.diff
  No functional change.  Some style/format changes to Instance::mem{Fill,Copy}
  to make them in line with new implementations in patch 8.

3-move-Segs-to-Code.diff
  Move dataSegments_ and elemSegments_ from Module to Code per comments above.

4-add-LazyInit.diff
  Add LazyInit<T> and LazyInitVec<T>, and initialisation thereof,
  per comments above.

5-add-passive-Segs.diff
  Change the representation of DataSegment and ElemSegment so as to allow
  passive segments.  Also parsing changes.

6-changes-to-Table.diff
  Minor changes to the signature of Table::set to facilitate calling it from
  Instance::tableInit.

7-new-insns-pre-codegen.diff
  AST, parsing, validation, etc stuff for new insns: {memory,table}.{init,drop}
  and table.copy.

8-new-insns-cg-and-run.diff
  Ion/Baseline code generation, and Instance:: implementations, for the new
  instructions.

9-smoketest.diff
  Tests {mem,table}.{init,drop} "normal" (non-boundary) cases enough to be
  fairly convinced that the implementation works.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990921 [details] [diff] [review]
bug1471500-r17-1-prelim-fixes.diff

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

The test fix is a bit strange to me, because now we could argue that we're testing the absence of the final End in the function's body. But I don't think there's a proper way to test for invalid prefixes without being affected by the next value, so I'm fine with either the current test or the way you've modified it.

Was the incorrect ExprType causing any issues at some point in the codebase? If so, adding a test case would be nice. Looking at uses of type(), it seems they're used only for Block and block-like things, as the comment suggests, which could justify a refactoring here...

Anyway, LGTM as is, thanks for the patch.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990922 [details] [diff] [review]
bug1471500-r17-2-memFillCopy-tidying.diff

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

Can you please unfold the "CSE" acronym in the changeset message (I don't know what it means).

Would like to have another look, since I've got many comments here, thanks.

::: js/src/wasm/WasmInstance.cpp
@@ +423,5 @@
>          uint8_t* rawBuf = arrBuf.dataPointerEither().unwrap();
>  
>          // Here, we know that |len - 1| cannot underflow.
>          typedef CheckedInt<uint32_t> CheckedU32;
> +        CheckedU32 lenMinus1 = CheckedU32(len - 1);

Why to use a CheckedU32 if len-1 can't underflow? Could it just be an uint32 instead?

@@ +425,5 @@
>          // Here, we know that |len - 1| cannot underflow.
>          typedef CheckedInt<uint32_t> CheckedU32;
> +        CheckedU32 lenMinus1 = CheckedU32(len - 1);
> +        CheckedU32 highest_dstOffset = CheckedU32(dstByteOffset) + lenMinus1;
> +        CheckedU32 highest_srcOffset = CheckedU32(srcByteOffset) + lenMinus1;

pre-existing: we don't use underscore in the code base for naming, but camelCasingEverywhere. Can you rename those highestDstOffset and highestSrcOffset, instead?

@@ +429,5 @@
> +        CheckedU32 highest_srcOffset = CheckedU32(srcByteOffset) + lenMinus1;
> +        if (highest_dstOffset.isValid() &&         // wraparound check
> +            highest_srcOffset.isValid() &&         // wraparound check
> +            highest_dstOffset.value() < memLen &&  // range check
> +            highest_srcOffset.value() < memLen)    // range check

pre-existing: not sure these comments add much value to the code, we could remove them if you agree.

@@ +436,3 @@
>              return 0;
>          }
>          // else fall through to failure case

pre-existing: not sure this comment and the parallel one in the `if (len == 0)` bring much either?

@@ +461,5 @@
>          uint8_t* rawBuf = arrBuf.dataPointerEither().unwrap();
>  
>          // Here, we know that |len - 1| cannot underflow.
>          typedef CheckedInt<uint32_t> CheckedU32;
> +        CheckedU32 lenMinus1 = CheckedU32(len - 1);

same question as above

@@ +464,5 @@
>          typedef CheckedInt<uint32_t> CheckedU32;
> +        CheckedU32 lenMinus1 = CheckedU32(len - 1);
> +        CheckedU32 highest_offset = CheckedU32(byteOffset) + lenMinus1;
> +        if (highest_offset.isValid() &&       // wraparound check
> +            highest_offset.value() < memLen)  // range check

same remarks for naming temporaries and comments

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990923 [details] [diff] [review]
bug1471500-r17-3-move-Segs-to-Code.diff

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

Looks good, we could have simpler code for cloning I think.

::: js/src/wasm/WasmModule.cpp
@@ +817,5 @@
>  }
>  
> +// Clone a Uint32Vector, doing so fallibly.
> +static bool
> +cloneUint32Vector(Uint32Vector* out, const Uint32Vector& orig)

nit: static functions' names start with an uppercase letter, so CloneUint32Vector

also nit: out parameters tend to go last, in the codebase

also: can we just use Vector::appendAll(const Vector&) instead of this function?

@@ +825,5 @@
> +        return false;
> +    for (size_t i = 0; i < orig.length(); i++)
> +        clone[i] = orig[i];
> +
> +    *out = std::move(clone);

We could directly operate on `out` instead of using a temporary that we move at the end.

@@ +834,5 @@
> +// constructor for it, because there's no way to make that fallible.
> +// Unfortunately, manually cloning an ElemSegment also implies manually
> +// cloning an Uint32Vector.
> +static bool
> +cloneElemSegment(ElemSegment* out, const ElemSegment& orig)

ditto for naming

@@ +1261,5 @@
>              JumpTables jumpTables;
>              if (!jumpTables.init(CompileMode::Once, codeTier->segment(), metadata(tier).codeRanges))
>                  return false;
>  
> +            DataSegmentVector dataSegmentsCloned;

naming suggestion: maybe just dataSegments (ditto for elemSegmentsCloned)? That's what we use around (for codeTier, jumpTables, etc) so it would be more consistent.

@@ +1263,5 @@
>                  return false;
>  
> +            DataSegmentVector dataSegmentsCloned;
> +            for (auto& dseg : code_->dataSegments()) {
> +                if (!dataSegmentsCloned.append(dseg))

Maybe we can use appendAll here too?

@@ +1276,5 @@
> +                if (!clone)
> +                    return false;
> +                if (!cloneElemSegment(clone, eseg))
> +                    return false;
> +                if (!elemSegmentsCloned.append(std::move(*clone)))

I think the `clone` memory allocation leaks here. Instead, what we could do is:

- first fallibly copy all the vectors
- then use elemSegmentsClone.emplaceBack to construct in place the new elem segment
- then set fields we haven't set yet (elemCodeRangeIndices1_ and 2)

This will imply some changes in the code (notably, the CloneElemSegment function would probably be removed), but is simpler and avoids one malloc.

Also, possible suggestion (to be MOZ_ASSERTed), elemCodeRangeIndices2_ should be empty at this point. (it's there for tier 2, and this code path is called only for tier 1)

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990924 [details] [diff] [review]
bug1471500-r17-4-add-LazyInit.diff

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

nit in commit message: "have the computed data owned by the instantiation." => "by the instance", if I understand correctly.

Many comments, mostly nits, and uncertainties about naming. Would like to have another look, thanks. Looks good in principle!

::: js/src/wasm/WasmInstance.cpp
@@ +694,5 @@
>                  lockedFuncTypeIdSet->deallocateFuncTypeId(funcType, funcTypeId);
>          }
>      }
> +
> +    for (const LazyInit<uint8_t>* li : dataSegLIV_) {

Could the LazyInitVector be a vector of UniquePtr<T> instead of T, so that deletion automatically happens?

::: js/src/wasm/WasmInstance.h
@@ +55,5 @@
>      const UniqueTlsData             tlsData_;
>      GCPtrWasmMemoryObject           memory_;
>      SharedTableVector               tables_;
> +    LazyInitVector<uint8_t>         dataSegLIV_;
> +    LazyInitVector<WasmCallee>      elemSegLIV_;

general naming suggestion for this patch: how about lazyDataSegments_ / lazyElemSegments_ instead of dataSegLIV_/elemSegLIV_?

::: js/src/wasm/WasmJS.cpp
@@ +1089,5 @@
>                             Handle<FunctionVector> funcImports,
>                             const GlobalDescVector& globals,
>                             HandleValVector globalImportValues,
>                             const WasmGlobalObjectVector& globalObjs,
> +                           const ShareableBytes* bytecode,

Could it be SharedBytes?

@@ +1129,5 @@
>  
> +    // Create a vector the same length as the data segment vector, holding a
> +    // copy of the initialising data for passive segments and |nullptr| for
> +    // active ones.
> +    LazyInitVector<uint8_t> dataLIV;

(as suggested elsewhere, how about lazyDataSegments as a name?)

@@ +1130,5 @@
> +    // Create a vector the same length as the data segment vector, holding a
> +    // copy of the initialising data for passive segments and |nullptr| for
> +    // active ones.
> +    LazyInitVector<uint8_t> dataLIV;
> +    for (auto& dseg : code->dataSegments()) {

dseg could be simply renamed seg here. Also, I personally like explicit types instead of auto, when the type doesn't explicitly already appear in the expression.

@@ +1131,5 @@
> +    // copy of the initialising data for passive segments and |nullptr| for
> +    // active ones.
> +    LazyInitVector<uint8_t> dataLIV;
> +    for (auto& dseg : code->dataSegments()) {
> +        LazyInit<uint8_t>* li = nullptr;

Use a UniquePtr here to make sure we're not leaking memory in failure paths.

@@ +1132,5 @@
> +    // active ones.
> +    LazyInitVector<uint8_t> dataLIV;
> +    for (auto& dseg : code->dataSegments()) {
> +        LazyInit<uint8_t>* li = nullptr;
> +        if (dseg.offset.isNothing()) {

nit: if (!dset.offset)

@@ +1140,5 @@
> +                ReportOutOfMemory(cx);
> +                return nullptr;
> +            }
> +            memcpy((char*)li->begin(),
> +                   (char*)bytecode->begin() + dseg.bytecodeOffset, dseg.length);

Where are the length checks? If they're done in another patch during validation, I think it'd be nice to assert that the offsets are in range.

also nit: how about putting dseg.length on its own line to improve readability?

@@ +1142,5 @@
> +            }
> +            memcpy((char*)li->begin(),
> +                   (char*)bytecode->begin() + dseg.bytecodeOffset, dseg.length);
> +        }
> +        if (!dataLIV.append(li)) {

(here the UniquePtr will need to be moved)

@@ +1149,5 @@
> +        }
> +    }
> +
> +    // And similarly for the elem segment vector.
> +    LazyInitVector<WasmCallee> elemLIV;

(All same remarks apply here)

@@ +1169,5 @@
> +            MOZ_ASSERT(eseg.elemCodeRangeIndices(tier).length() ==
> +                       eseg.elemFuncIndices.length());
> +
> +            for (uint32_t i = 0; i < eseg.elemCodeRangeIndices(tier).length(); i++) {
> +                (*li)[i] = ComputeWasmCallee(i, funcImports, code->metadata(),

Since ComputeWasmCallee is only used in this file, could it be defined here as a static function?

::: js/src/wasm/WasmModule.cpp
@@ +734,5 @@
> +        Instance& exportInstance = exportInstanceObj->instance();
> +        Tier exportTier = exportInstance.code().bestTier();
> +        const CodeRange& cr = exportInstanceObj->getExportedFunctionCodeRange(f, exportTier);
> +        return WasmCallee(&exportInstance, exportInstance.codeBase(exportTier) + cr.funcTableEntry());
> +    } else {

nit: no else after return

@@ +1346,5 @@
>      auto debug = cx->make_unique<DebugState>(code, maybeBytecode, binarySource);
>      if (!debug)
>          return false;
>  
> +    // We pass ::create a pointer to the bytecode, so it can copy parts of

typo on this line?

::: js/src/wasm/WasmModule.h
@@ +249,5 @@
>  
> +// Compute the (entry point, instance pointer) pair for an entry in the
> +// function-indices vector of an element segment.  If the function is local
> +// then the returned instance pointer is simply the parameter |instance|.  In
> +// such a case it is allowable to pass nullptr for |instance| if its value is

allowable -> allowed?

@@ +260,5 @@
> +ComputeWasmCallee(uint32_t funcIndexIndex,
> +                  Handle<FunctionVector> funcImports,
> +                  const Metadata& metadata, const Table& table,
> +                  const CodeRangeVector& codeRanges, const ElemSegment& seg,
> +                  Tier tier, const Instance* instance, uint8_t* codeBase);

I think most things passed to this function could be retrieved from the Code, which would make the signature much smaller and nicer.

Also, it'd be nicer to provide an out-parameter WasmCallee*, as we do most of the time.

::: js/src/wasm/WasmTypes.h
@@ +1305,5 @@
> +// available at run time in the instance, in particular to
> +// Instance::{mem,table}{Init,Drop}.
> +//
> +// The final structures have type LazyInitVec<uint8_t> for data segments
> +// and LazyInitVec<WasmCallee> for elem segments.

This comment should probably be closer to the definition of LazyInit/LazyInitVector.

@@ +1309,5 @@
> +// and LazyInitVec<WasmCallee> for elem segments.
> +
> +// A pairing of entry point and instance pointer, used for lazy table
> +// initialisation.
> +struct WasmCallee {

nit: brace on the next line, to follow current style

+ \n after multiline comment

@@ +1312,5 @@
> +// initialisation.
> +struct WasmCallee {
> +    WasmCallee(const Instance* instance, void* entry)
> +        : instance_(instance)
> +        , entry_(entry)

nit: list initializers get half-indent (sorry about Spidermonkey styling ¯\_(ツ)_/¯), comma gets on the previous line

@@ +1316,5 @@
> +        , entry_(entry)
> +    {}
> +    // The instance associated with the code address.  If this is null,
> +    // it denotes a reference to the instance that owns this structure.
> +    const Instance* instance_;

nit: public fields don't get the trailing _

@@ +1322,5 @@
> +    void* entry_;
> +};
> +
> +template <typename T>
> +using LazyInit = Vector<T, 0, SystemAllocPolicy>;

I'm not sure we need templates here just for two uses, and I can't see any future usage of this template in a near future. How about:

using LazyBytecodeData = Vector<uint8_t, 0, SystemAllocPolicy>;
using LazyElemData = Vector<WasmCallee, ...>

And two more definitions which get Vector at the end.

I'm not even sure about my suggestions. The LazyInit structures contain the data used during passive segment initialization. Other names that came to mind:
- instead of LazyInit<uint8_t>: PassiveBytecodeData, PassiveBytecode, PassiveDataSegmentData
- instead of LazyInit<WasmCallee>: PassiveElemData, PassiveWasmCalleeData, PassiveElem, PassiveWasmCallee, PassiveElemSegmentData

Anyway, if there's a name that sounds right, or you have any other idea or a strong justification for the current names, I'd be happy to discuss.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990925 [details] [diff] [review]
bug1471500-r17-5-add-passive-Segs.diff

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

Looking good. Many small nits requiring a second read in my opinion. Thanks.

::: js/src/wasm/WasmModule.cpp
@@ +763,5 @@
> +        // If this is a passive segment, there's nothing we can check at
> +        // this point.  We'll have to perform the relevant checks later, if
> +        // and when the segment is used, that is, as an argument to the
> +        // table.init or table.drop instructions.
> +        if (seg.offset.isNothing())

nit: for conciseness here and below, if (!seg.offset)

@@ +771,4 @@
>          uint32_t numElems = seg.elemCodeRangeIndices(tier).length();
>  
>          uint32_t tableLength = tables[seg.tableIndex]->length();
> +        uint32_t offset = EvaluateInitExpr(globalImportValues, seg.offset.value());

is the call to value() needed here?

@@ +786,5 @@
> +            // As with element segments just above, skip passive ones for now.
> +            if (seg.offset.isNothing())
> +                continue;
> +
> +            uint32_t offset = EvaluateInitExpr(globalImportValues, seg.offset.value());

same remarks for isNothing() and value()

@@ +804,5 @@
>  
>      for (const ElemSegment& seg : code_->elemSegments()) {
> +        // Skip passive segments.  Those may get applied later, under
> +        // program control, per comments above.
> +        if (seg.offset.isNothing())

ditto

@@ +818,3 @@
>  
> +        for (uint32_t i = 0; i < seg.elemCodeRangeIndices(tier).length(); i++) {
> +            WasmCallee cee = ComputeWasmCallee(i, funcImports, metadata(),

I think this change should be in one of the previous patches.

+nit: rename cee into callee

@@ +832,5 @@
>              MOZ_ASSERT(seg.bytecodeOffset <= bytecode_->length());
>              MOZ_ASSERT(seg.length <= bytecode_->length() - seg.bytecodeOffset);
> +            // Skip passive segments.  Those may get applied later, under
> +            // program control, per comments above.
> +            if (seg.offset.isNothing())

ditto

@@ +835,5 @@
> +            // program control, per comments above.
> +            if (seg.offset.isNothing())
> +                continue;
> +            // But apply active segments right now.
> +            uint32_t offset = EvaluateInitExpr(globalImportValues, seg.offset.value());

ditto

::: js/src/wasm/WasmTextToBinary.cpp
@@ +3509,5 @@
>  }
>  
> +// Returns a pair, in which the bool indicates success/failure.  In case of
> +// success, the AstExpr* is nullptr for "passive" or non-nullptr otherwise.
> +static pair<bool, AstExpr*>

We usually use out params in this case.

Looking at the two uses, we could even do:

bool
ParseInitializerExpressionOrPassive(WasmParseContext& c, Maybe<AstExpr*>* maybeInitExpr)

and the returned bool just indicates success or failure?

Then we can even remove the std::pair import and it would simplify call sites.

@@ +3534,3 @@
>          return nullptr;
>  
> +    AstExpr* offset = ok_and_offset.second;

Call site will look like this:

Maybe<AstExpr*> offsetIfActive;
if (!Parse...(..., &offsetIfActive))
    return nullptr;

@@ +3628,5 @@
>              if (!offset)
>                  return false;
>  
> +            AstDataSegment* segment
> +                = new(c.lifo) AstDataSegment(Some(offset), std::move(fragments));

nit: to fit more on the line, consider using auto* on the LHS, since the type is already written on the line. Then we can probably fit it all on one line

@@ +4876,5 @@
>              return false;
>      }
>  
>      for (AstDataSegment* segment : module->dataSegments()) {
> +        if (segment->offset().isSome() && !ResolveExpr(r, *segment->offset().value()))

nit: for more conciness, you can test segment->offset() instead of segment->offset().isSome()

Is the call to value() also needed on the RHS? It should be deduced by the compiler, but in some cases it can't be avoided.

@@ +4881,5 @@
>              return false;
>      }
>  
>      for (AstElemSegment* segment : module->elemSegments()) {
> +        if (segment->offset().isSome() && !ResolveExpr(r, *segment->offset().value()))

same remarks

@@ +5794,5 @@
>  
>  static bool
>  EncodeDataSegment(Encoder& e, const AstDataSegment& segment)
>  {
> +    if (segment.offset().isSome()) {

nit: ditto about isSome()

@@ +5855,5 @@
>  
>  static bool
>  EncodeElemSegment(Encoder& e, AstElemSegment& segment)
>  {
> +    if (segment.offset().isSome()) {

ditto

::: js/src/wasm/WasmTypes.h
@@ +1237,5 @@
>  
>  struct ElemSegment
>  {
>      uint32_t tableIndex;
> +    Maybe<InitExpr> offset; // Nothing denotes a passive segment.

Maybe here (and actually everywhere in this patch), we could call them offsetIfActive, which would allow us to remove the comment then?

@@ +1243,5 @@
>      Uint32Vector elemCodeRangeIndices1_;
>      mutable Uint32Vector elemCodeRangeIndices2_;
>  
>      ElemSegment() = default;
> +    ElemSegment(uint32_t tableIndex, Maybe<InitExpr> offset, Uint32Vector&& elemFuncIndices)

nit: I think the static analysis will forbid you to pass Maybe<T> by value.

::: js/src/wasm/WasmValidate.cpp
@@ +1931,5 @@
> +        uint32_t initializerKind;
> +        if (!d.readVarU32(&initializerKind))
> +            return d.fail("expected elem initializer-kind field");
> +
> +        if (initializerKind != uint32_t(InitializerKind::Active) &&

I think these (and below in the DecodeDataSection function) should be ifdef'd, otherwise it's not Nightly-only.

@@ +1933,5 @@
> +            return d.fail("expected elem initializer-kind field");
> +
> +        if (initializerKind != uint32_t(InitializerKind::Active) &&
> +            initializerKind != uint32_t(InitializerKind::Passive))
> +            return d.fail("invalid elem initializer-kind field");

nit: multi-line condition implies {} for the if body (unless there's a clear whitespace separation between condition and body, which is not the case here)

@@ +1938,4 @@
>  
>          MOZ_ASSERT(env->tables.length() <= 1);
> +        if (env->tables.length() == 0)
> +            return d.fail("elem segment requires a table section");

What happens to the table index here? Unless I missed something, there's still a future we can have several tables per module.

@@ +1942,2 @@
>  
> +        Maybe<InitExpr> mb_offset = Nothing();

nit: rename to offsetIfActive here too?

also (maybe) nit: isn't Nothing() the default value at construction? if so, we could omit it on the RHS.

@@ -2103,3 @@
>  
> -        if (linearMemoryIndex != 0)
> -            return d.fail("linear memory index must currently be 0");

(Same question for memory index)

@@ +2111,4 @@
>  
> +        if (initializerKind != uint32_t(InitializerKind::Active) &&
> +            initializerKind != uint32_t(InitializerKind::Passive))
> +            return d.fail("invalid data initializer-kind field");

Maybe these few lines could be factored out into a static function DecodeInitializerKind?

@@ +2116,5 @@
>          if (!env->usesMemory())
>              return d.fail("data segment requires a memory section");
>  
>          DataSegment seg;
> +        seg.offset = Nothing();

nit: as said above, probably not needed

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990928 [details] [diff] [review]
bug1471500-r17-8-new-insns-cg-and-run.diff

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

Makes sense. I think there are many renamings due here, suggesting another round of review would be useful. Thanks for the patch.

::: js/src/js.msg
@@ +398,5 @@
>  MSG_DEF(JSMSG_WASM_TEXT_FAIL,          1, JSEXN_SYNTAXERR,   "wasm text error: {0}")
>  MSG_DEF(JSMSG_WASM_MISSING_MAXIMUM,    0, JSEXN_TYPEERR,     "'shared' is true but maximum is not specified")
>  MSG_DEF(JSMSG_WASM_GLOBAL_IMMUTABLE,   0, JSEXN_TYPEERR,     "can't set value of immutable global")
> +MSG_DEF(JSMSG_WASM_INVALID_PASSIVE_DATA_SEG, 0, JSEXN_WASMRUNTIMEERROR, "use of invalid passive data segment")
> +MSG_DEF(JSMSG_WASM_INVALID_PASSIVE_ELEM_SEG, 0, JSEXN_WASMRUNTIMEERROR, "use of invalid passive element segment")

nit: move next to the other wasm runtime errors

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +9361,1 @@
>          return false;

nit: I'd prefer the longer

if (isMem) {
  if (!iter_.readMemCopy())
    return false;
} else {
  if (!iter_.readTableCopy())
    return false;
}

@@ +9379,5 @@
> +BaseCompiler::emitMemOrTableDrop(bool isMem)
> +{
> +    uint32_t lineOrBytecode = readCallSiteLineOrBytecode();
> +
> +    uint32_t segIx = 0;

nit: rename to segIndex or segmentIndex

@@ +9383,5 @@
> +    uint32_t segIx = 0;
> +    bool iterOk = isMem ? iter_.readMemDrop(&segIx)
> +                        : iter_.readTableDrop(&segIx);
> +    if (!iterOk)
> +        return false;

ditto

@@ +9434,5 @@
> +    Nothing nothing;
> +    bool iterOk = isMem ? iter_.readMemInit(&segIx, &nothing, &nothing, &nothing)
> +                        : iter_.readTableInit(&segIx, &nothing, &nothing, &nothing);
> +    if (!iterOk)
> +        return false;

ditto for control flow

::: js/src/wasm/WasmInstance.cpp
@@ +446,5 @@
>  /* static */ int32_t
> +Instance::memDrop(Instance* instance, uint32_t segIx)
> +{
> +    const DataSegmentVector& dataSegs = instance->code().dataSegments();
> +    LazyInitVector<uint8_t>& dataSegLIV = instance->dataSegLIV();

nit: I think I suggested another renaming instead of the "LIV"/"liv" naming, it'd be nice to use it here too.

@@ +452,5 @@
> +    size_t livLen = dataSegLIV.length();
> +    MOZ_ASSERT(dataSegs.length() == livLen);
> +
> +    // Range-check the segment index.
> +    if ((size_t)segIx >= livLen) {

nit: C++ style conversion: size_t(segIndex)

@@ +471,5 @@
> +    }
> +
> +    const DataSegment& seg = dataSegs[segIx];
> +
> +    // If this is an active initialiser, something is badly wrong.

(american) nit: initializer? (to be more consistent with the proposal and rest of the code)

There are a few instances below. (We can also decide to ignore this)

@@ +472,5 @@
> +
> +    const DataSegment& seg = dataSegs[segIx];
> +
> +    // If this is an active initialiser, something is badly wrong.
> +    MOZ_RELEASE_ASSERT(seg.offset.isNothing());

nit: !seg.offsetIfActive, for concision

@@ +476,5 @@
> +    MOZ_RELEASE_ASSERT(seg.offset.isNothing());
> +
> +    // Free the initialiser, and update the initialiser vector accordingly.
> +    // This makes the initialiser unavailable for future usage.
> +    js_delete(dataSegLIV[segIx]);

With my suggestion to use a vector of uniqueptr in the other patch, no need to explicitly delete here, I think!

@@ +529,5 @@
> +    MOZ_ASSERT(dataSegs.length() == livLen);
> +
> +    // Range-check the segment index.
> +    if ((size_t)segIx >= livLen) {
> +        JSContext* cx = TlsContext.get();

nit: indentation is wrong here

@@ +569,5 @@
> +    // Checks when len > 0:
> +    //   dstOffset + len does not wrap around
> +    //   dstOffset + len <= memLen
> +    //   srcOffset + len does not wrap around
> +    //   srcOffset + len <= seg.length

I think the list of checks is quite explicit from the code below and doesn't need a comment. Maybe we can keep only "We are proposing ... " + len - 1]"?

@@ +577,5 @@
> +        // Even though the length is zero, we must check for valid offsets.
> +        if (dstOffset < memLen && srcOffset < seg.length)
> +            return 0;
> +
> +        // else fall through to failure case

I think this comment can be removed.

@@ +580,5 @@
> +
> +        // else fall through to failure case
> +    } else {
> +        ArrayBufferObjectMaybeShared& arrBuf = mem->buffer();
> +        uint8_t* memoryBase = arrBuf.dataPointerEither().unwrap();

These two definitions can be put closer to their uses, in the innermost if body.

@@ +583,5 @@
> +        ArrayBufferObjectMaybeShared& arrBuf = mem->buffer();
> +        uint8_t* memoryBase = arrBuf.dataPointerEither().unwrap();
> +
> +        // Here, we know that |len - 1| cannot underflow.
> +        typedef CheckedInt<uint32_t> CheckedU32;

since this is used in other functions too, iirc, maybe do a unique `using` declaration at the global scope above these functions?

@@ +586,5 @@
> +        // Here, we know that |len - 1| cannot underflow.
> +        typedef CheckedInt<uint32_t> CheckedU32;
> +        CheckedU32 lenMinus1 = CheckedU32(len - 1);
> +        CheckedU32 highest_dstOffset = CheckedU32(dstOffset) + lenMinus1;
> +        CheckedU32 highest_srcOffset = CheckedU32(srcOffset) + lenMinus1;

nit: lastDstOffset / lastSrcOffset? (or whatever was suggested beforehands in previous patches)

@@ +590,5 @@
> +        CheckedU32 highest_srcOffset = CheckedU32(srcOffset) + lenMinus1;
> +        if (highest_dstOffset.isValid() &&         // wraparound check
> +            highest_srcOffset.isValid() &&         // wraparound check
> +            highest_dstOffset.value() < memLen &&  // range check
> +            highest_srcOffset.value() < segLen)    // range check

nit: i don't think these comments add much value here

@@ +597,5 @@
> +            memcpy(memoryBase + dstOffset,
> +                   (const char*)segLI->begin() + srcOffset, len);
> +            return 0;
> +        }
> +        // else fall through to failure case

nit: i'd remove this comment too

@@ +607,5 @@
> +    return -1;
> +}
> +
> +/* static */ int32_t
> +Instance::tableCopy(Instance* instance, uint32_t dstOffset, uint32_t srcOffset, uint32_t len)

Most comments apply to this function too.

@@ +609,5 @@
> +
> +/* static */ int32_t
> +Instance::tableCopy(Instance* instance, uint32_t dstOffset, uint32_t srcOffset, uint32_t len)
> +{
> +    const SharedTableVector& tables = instance->tables();

nit: tables is used only once, maybe inline it in its use?

@@ +632,5 @@
> +            highest_srcOffset.isValid() &&           // wraparound check
> +            highest_dstOffset.value() < tableLen &&  // range check
> +            highest_srcOffset.value() < tableLen)    // range check
> +        {
> +            // Actually do the copy.

Maybe explain that the ordering of srcOffset/dstOffset justifies to copy in one direction or the other to avoid aliasing?

@@ +636,5 @@
> +            // Actually do the copy.
> +            if (dstOffset > srcOffset) {
> +                for (uint32_t i = 0; i < len; i++)
> +                    table->copy(dstOffset + (len - 1 - i),
> +                                srcOffset + (len - 1 - i));

doesn't it fit on one line? also, why not iterating from i = len - 1 down to 0 instead?

@@ +639,5 @@
> +                    table->copy(dstOffset + (len - 1 - i),
> +                                srcOffset + (len - 1 - i));
> +            }
> +            else
> +            if (dstOffset < srcOffset) {

nit: `else if` on the same line

@@ +655,5 @@
> +    return -1;
> +}
> +
> +/* static */ int32_t
> +Instance::tableDrop(Instance* instance, uint32_t segIx)

Any chance we could templatize the implementations of Drop? Or at least share the code that does the checks in a static function? (less crazy)

@@ +730,5 @@
> +
> +    const uint32_t segLen = seg.elemFuncIndices.length();
> +    MOZ_ASSERT(segLen == segLI->length());
> +
> +    const SharedTableVector& tables = instance->tables();

nit: tables used only once, please inline

@@ +739,5 @@
> +    // attached, it is marked as "external".  Hence we can assume here
> +    // it is "external".  Assured by:
> +    //   DecodeElemSection(Decoder&, ModuleEnvironment*)
> +    //   WasmTableObject::create(JSContext*, const Limits&)
> +    MOZ_ASSERT(table->external());

I'd remove these references in the comments, since they can become very easily stall. We could even remove the entire comment and just have a comment in the MOZ_ASSERT:

MOZ_ASSERT(table->external, "ensured by validation because of passive segments");

@@ +776,5 @@
> +            highest_srcOffset.value() < segLen)      // range check
> +        {
> +            MOZ_RELEASE_ASSERT(srcOffset + (len - 1) < segLen);
> +            for (uint32_t i = 0; i < len; i++) {
> +                WasmCallee cee = (*segLI)[srcOffset + i];

nit: cee -> callee

@@ +779,5 @@
> +            for (uint32_t i = 0; i < len; i++) {
> +                WasmCallee cee = (*segLI)[srcOffset + i];
> +                // If |cee| specifies .instance_, use that.  Otherwise, the
> +                // meaning of its null-ness is that we should use the
> +                // instance that owns this WasmCallee.

Not sure I've suggested it in the past, but just in case: could we have the WasmCallee.instance always filled with the correct instance, so we don't need a particular case here?

::: js/src/wasm/WasmInstance.h
@@ +183,5 @@
>      static int32_t wait_i32(Instance* instance, uint32_t byteOffset, int32_t value, int64_t timeout);
>      static int32_t wait_i64(Instance* instance, uint32_t byteOffset, int64_t value, int64_t timeout);
>      static int32_t wake(Instance* instance, uint32_t byteOffset, int32_t count);
>      static int32_t memCopy(Instance* instance, uint32_t destByteOffset, uint32_t srcByteOffset, uint32_t len);
> +    static int32_t memDrop(Instance* instance, uint32_t segIx);

nit: rename segIx into segIndex or segmentIndex everywhere in the patch

@@ +188,3 @@
>      static int32_t memFill(Instance* instance, uint32_t byteOffset, uint32_t value, uint32_t len);
> +    static int32_t memInit(Instance* instance, uint32_t dstOffset, uint32_t srcOffset, uint32_t len, uint32_t segIx);
> +    static int32_t tableCopy(Instance* instance, uint32_t dstOffset, uint32_t srcOffset, uint32_t nItemsToCopy);

nItemsToCopy could be named len here too, for consistency with memCopy

::: js/src/wasm/WasmIonCompile.cpp
@@ +3606,5 @@
>  {
>      MDefinition *dest, *src, *len;
> +    bool iterOk = isMem ? f.iter().readMemCopy(&dest, &src, &len)
> +                        : f.iter().readTableCopy(&dest, &src, &len);
> +    if (!iterOk)

same remark for control flow

@@ +3646,5 @@
> +
> +static bool
> +EmitMemOrTableDrop(FunctionCompiler& f, bool isMem)
> +{
> +    uint32_t segIx = 0;

nit: rename to segIndex or segmentIndex

@@ +3647,5 @@
> +static bool
> +EmitMemOrTableDrop(FunctionCompiler& f, bool isMem)
> +{
> +    uint32_t segIx = 0;
> +    MDefinition* segIxM;

nit: move this next to initialization

@@ +3651,5 @@
> +    MDefinition* segIxM;
> +    bool iterOk = isMem ? f.iter().readMemDrop(&segIx)
> +                        : f.iter().readTableDrop(&segIx);
> +    if (!iterOk)
> +        return false;

ditto

@@ +3652,5 @@
> +    bool iterOk = isMem ? f.iter().readMemDrop(&segIx)
> +                        : f.iter().readTableDrop(&segIx);
> +    if (!iterOk)
> +        return false;
> +    segIxM = f.constant(Int32Value(int32_t(segIx)), MIRType::Int32);

nit: rename to segIndexDef or segmentIndexDef?

also move this statement after the `f.inDeadCode()` check

@@ +3672,5 @@
> +
> +    if (!f.finishCall(&args))
> +        return false;
> +
> +    MDefinition* ret;

nit: move this line just before the f.builtinInstanceMethodCall call

@@ +3726,5 @@
> +
> +static bool
> +EmitMemOrTableInit(FunctionCompiler& f, bool isMem)
> +{
> +    uint32_t segIx = 0;

nit: rename etc.

@@ +3727,5 @@
> +static bool
> +EmitMemOrTableInit(FunctionCompiler& f, bool isMem)
> +{
> +    uint32_t segIx = 0;
> +    MDefinition *dstOffM, *srcOffM, *lenM, *segIxM;

nit: first * goes next to MDefinition

@@ +3731,5 @@
> +    MDefinition *dstOffM, *srcOffM, *lenM, *segIxM;
> +    bool iterOk = isMem ? f.iter().readMemInit(&segIx, &dstOffM, &srcOffM, &lenM)
> +                        : f.iter().readTableInit(&segIx, &dstOffM, &srcOffM, &lenM);
> +    if (!iterOk)
> +        return false;

nit: control flow

@@ +3732,5 @@
> +    bool iterOk = isMem ? f.iter().readMemInit(&segIx, &dstOffM, &srcOffM, &lenM)
> +                        : f.iter().readTableInit(&segIx, &dstOffM, &srcOffM, &lenM);
> +    if (!iterOk)
> +        return false;
> +    segIxM = f.constant(Int32Value(int32_t(segIx)), MIRType::Int32);

(most comments in the previous function apply here too)

@@ +3758,5 @@
> +
> +    if (!f.finishCall(&args))
> +        return false;
> +
> +    MDefinition* ret;

(here too)

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990930 [details] [diff] [review]
bug1471500-r17-9-smoketest.diff

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

We'll need many more test cases, for all the exceptional and failure cases at least.

I'd also want simpler, more unit tests that just do one thing at a time, in a small module (i.e. one unit test per opcode, with a single small module, tested on a few interesting combinations of inputs). Some code can be shared for sure. This test harness might take more time to understand than just writing the simpler tests it's actually performing, as simple unit tests. Don't get me wrong, I understand that these tests are good enough to make sure most things work as expected, but I wouldn't put an r+ for it and having them checked in.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990926 [details] [diff] [review]
bug1471500-r17-6-changes-to-Table.diff

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

::: js/src/wasm/WasmTable.cpp
@@ +146,5 @@
> +        ExternalTableElem& dst = externalArray()[dstIndex];
> +        if (dst.tls)
> +            JSObject::writeBarrierPre(dst.tls->instance->objectUnbarriered());
> +
> +        dst = externalArray()[srcIndex];

maybe do it explicitly field by field, to avoid unwanted C++ copy behavior? It would also make the above prebarrier look more legit.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990927 [details] [diff] [review]
bug1471500-r17-7-new-insns-pre-codegen.diff

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

::: js/src/wasm/WasmAST.h
@@ +979,5 @@
>  };
>  
> +class AstMemDrop : public AstExpr
> +{
> +    uint32_t segIx_;

nit: everywhere in this patch, please rename to segmentIndex_ or segIndex_ (local variables, members, parameters, accessors, etc)

::: js/src/wasm/WasmBinaryConstants.h
@@ +349,5 @@
>      I64TruncSSatF64                      = 0x06,
>      I64TruncUSatF64                      = 0x07,
>  
> +    // Bulk memory operations.  Note, these are unofficial, but in accordance
> +    // with the proposal as of June 2018.

Let's put #ifdef here to protect the feature.

(make sure to compile by simulating a non-Nightly build, to make sure a beta build will still compile despite the guard being disabled)

::: js/src/wasm/WasmOpIter.h
@@ +2373,5 @@
>  }
>  
>  template <typename Policy>
>  inline bool
> +OpIter<Policy>::readMemDrop(uint32_t* segIx)

ditto for renaming

@@ +2379,5 @@
> +    MOZ_ASSERT(Classify(op_) == OpKind::MemDrop);
> +
> +    if (!env_.usesMemory())
> +        return fail("can't touch memory without memory");
> +

I think you also need to check that the segment referred by the segment index is known, by checking the length of the segment array (probably taken from the environment).

Please add a test case for this.

@@ +2422,5 @@
> +
> +    if (!popWithType(ValType::I32, dst))
> +        return false;
> +
> +    return readVarU32(segIx);

ditto

Also, please join this check with the usesMemory check to group them logically.

@@ +2455,5 @@
> +
> +    if (env_.tables.length() == 0)
> +        return fail("can't table.drop without a table");
> +
> +    return readVarU32(segIx);

ditto, check segment index

@@ +2476,5 @@
> +
> +    if (!popWithType(ValType::I32, dst))
> +        return false;
> +
> +    return readVarU32(segIx);

and ditto

::: js/src/wasm/WasmTextToBinary.cpp
@@ +3084,5 @@
>  
> +static AstMemDrop*
> +ParseMemDrop(WasmParseContext& c, bool inParens)
> +{
> +    WasmToken segIxTok;

nit: rename token? (everywhere in this file)

@@ +3110,5 @@
>      return new(c.lifo) AstMemFill(start, val, len);
>  }
> +
> +static AstMemInit*
> +ParseMemInit(WasmParseContext& c, bool inParens)

If you'd merge the two AstExprKinds MemInit and TableInit, then you could reuse the same code with a different AST node constructed at the end. The resulting AstSegmentInit code would need to contain the opcode (either MemInit or TableInit), and it would spare us some code duplication, which is nice.

@@ +3132,5 @@
> +    return new(c.lifo) AstMemInit(segIxTok.index(), dst, src, len);
> +}
> +
> +static AstTableCopy*
> +ParseTableCopy(WasmParseContext& c, bool inParens)

ditto with MemCopy

@@ +3150,5 @@
> +    return new(c.lifo) AstTableCopy(dest, src, len);
> +}
> +
> +static AstTableDrop*
> +ParseTableDrop(WasmParseContext& c, bool inParens)

ditto with MemDrop

@@ +4756,5 @@
> +           ResolveExpr(r, s.len());
> +}
> +
> +static bool
> +ResolveTableInit(Resolver& r, AstTableInit& s)

(Code duplication might go away here too)

@@ +5502,5 @@
> +           e.writeVarU32(s.segIx());
> +}
> +
> +static bool
> +EncodeTableInit(Encoder& e, AstTableInit& s)

(and code duplication would disappear here too)

::: js/src/wasm/WasmTypes.h
@@ +2028,5 @@
>      MemFill,
> +    MemInit,
> +    TableCopy,
> +    TableDrop,
> +    TableInit,

There's many missing things if you add new SymbolicAddresses: for instance, they should be reflected in AddressOf and wasm::NeedsBuiltinThunk and wasm::ThunkedNativeToDescription. (Note that we try to have smaller patches that still compile when taken separately, so as to make the life of fuzzers simpler. If we didn't do this, then a bisection might result in a build failure, which is very unfortunate to help classifying issues)

Comment 25

[Julian Seward [:jseward]]

Thank you for the reviews.

Here's a new patch set.  In summary:

* the "boundaries" between patches have been cleaned up in a couple of places,
  and the sequence modified, so that each patch builds and runs wasm/ tests
  OK.

* a new patch 08-deferred-validation.diff has been added, to deal with
  deferred validation of data segment indices in memory.{init,drop}.

* all requested renamings and style fixes have been done

* some of the AST, compiler, etc level duplication between memory.{copy, drop,
  init} and table.{copy, drop, init} has been removed by having single
  cases/data structores for {memory, table}.copy, etc.

* there's a new test file, passive-segs-boundary.js, that tests
  exception/failure conditions for the new instructions.

* commit messages are updated accordingly

Comment 26

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-01-prelim-fixes.diff

> Was the incorrect ExprType causing any issues at some point in the codebase?

No, no issues.  It's just an inconsistency that I noticed whilst reading
through the code.

Comment 27

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-02-memFillCopy-tidying.diff

::: js/src/wasm/WasmInstance.cpp
@@ +423,5 @@
>          uint8_t* rawBuf = arrBuf.dataPointerEither().unwrap();
>  
>          // Here, we know that |len - 1| cannot underflow.
>          typedef CheckedInt<uint32_t> CheckedU32;
> +        CheckedU32 lenMinus1 = CheckedU32(len - 1);

> Why to use a CheckedU32 if len-1 can't underflow? Could it just be an uint32
> instead?

The point of wrapping it a CheckedU32 is not to see if len-1 will underflow
(as you say, it won't), but rather to make it possible to use len-1 in the
wraparound checks later, in which we do need to check for overflow.

Comment 28

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-03-changes-to-Table.diff

Comment 29

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-04-move-Segs-to-Code.diff

> Looks good, we could have simpler code for cloning I think.
> [..]
> I think the `clone` memory allocation leaks here. Instead, what we could do
> is:
> - first fallibly copy all the vectors
> - then use elemSegmentsClone.emplaceBack to construct in place the new elem
>   segment
> - then set fields we haven't set yet (elemCodeRangeIndices1_ and 2)
> This will imply some changes in the code (notably, the CloneElemSegment
> function would probably be removed), but is simpler and avoids one malloc.

I rewrote CloneElemSegments completely, per your instructions, and I think it
won't leak now.  But I'm not sure.  Can you look again pls?

Comment 30

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-05-add-InitVector.diff

> ::: js/src/wasm/WasmJS.cpp
> @@ +1089,5 @@
> >                             Handle<FunctionVector> funcImports,
> >                             const GlobalDescVector& globals,
> >                             HandleValVector globalImportValues,
> >                             const WasmGlobalObjectVector& globalObjs,
> > +                           const ShareableBytes* bytecode,
> 
> Could it be SharedBytes?

Maybe, but I can't use SharedBytes in WasmJS.h because then I'd have to
include WasmCode.h in WasmJS.h.  Would it make sense to move ShareableBytes,
MutableBytes and SharedBytes from WasmCode.h to WasmTypes.h?

Comment 31

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-06-add-passive-Segs.diff

> @@ +1140,5 @@
> > +                ReportOutOfMemory(cx);
> > +                return nullptr;
> > +            }
> > +            memcpy((char*)li->begin(),
> > +                   (char*)bytecode->begin() + dseg.bytecodeOffset, dseg.length);
> 
> Where are the length checks? If they're done in another patch during
> validation, I think it'd be nice to assert that the offsets are in range.

The offsets/lengths here are created in DecodeDataSection in WasmValidate.cpp.
Note that they are merely offsets/lengths in the the bytecode stream, created
by DecodeDataSection, and so are not under the control of attackers -- at
least in the sense that they could be used to point outside of the instruction
stream, to arbitrary memory.  If that were the case than the existing logic in
DecodeDataSection would be buggy.

Note also, this memcpy doesn't create any more danger than we already have.
For an "active" (MVP-style) data segments, we will be memcpying this data into
the (one-and-only) memory at instantiation time.  With new "passive" segments,
we're still memcpying the same data, except not to the "memory", rather to
temporary storage (a DataSegmentInit) which we may later use in memory.init
instructions.

> @@ +260,5 @@
> > +ComputeWasmCallee(uint32_t funcIndexIndex,
> > +                  Handle<FunctionVector> funcImports,
> > +                  const Metadata& metadata, const Table& table,
> > +                  const CodeRangeVector& codeRanges, const ElemSegment& seg,
> > +                  Tier tier, const Instance* instance, uint8_t* codeBase);
> 
> I think most things passed to this function could be retrieved from the Code,
> which would make the signature much smaller and nicer.

Doesn't look feasible; the two call sites get their arguments from very
different places -- not just pulling stuff out of a Code.

> ::: js/src/wasm/WasmValidate.cpp
> @@ +1931,5 @@
> > +        uint32_t initializerKind;
> > +        if (!d.readVarU32(&initializerKind))
> > +            return d.fail("expected elem initializer-kind field");
> > +
> > +        if (initializerKind != uint32_t(InitializerKind::Active) &&
> 
> I think these (and below in the DecodeDataSection function) should be ifdef'd,
> otherwise it's not Nightly-only.

Per my comments for the next patch in the series
(07-new-insns-pre-codegen.diff), I think it is already ifdef'd enough.

> @@ +1938,4 @@
> >  
> >          MOZ_ASSERT(env->tables.length() <= 1);
> > +        if (env->tables.length() == 0)
> > +            return d.fail("elem segment requires a table section");
> 
> What happens to the table index here? Unless I missed something, there's still
> a future we can have several tables per module.
> 
> @@ -2103,3 @@
> >  
> > -        if (linearMemoryIndex != 0)
> > -            return d.fail("linear memory index must currently be 0");
> 
> (Same question for memory index)

The proposal
https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md
doesn't say anything much about this, only the following:

  Since WebAssembly currently does not allow for multiple memories, the memory
  index must be zero. We can repurpose this field as a flags field.
  [..]
  The table.* instructions behave similary to the memory.* instructions, with
  the difference that they operate on element segments and tables, instead of
  data segments and memories.

So I assume there is nothing we can do for the time being, until such time as
there's a coherent complete proposal for multiple memories and tables.

Comment 32

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-07-new-insns-pre-codegen.diff

> ::: js/src/wasm/WasmBinaryConstants.h
> @@ +349,5 @@
> >      I64TruncSSatF64                      = 0x06,
> >      I64TruncUSatF64                      = 0x07,
> >  
> > +    // Bulk memory operations.  Note, these are unofficial, but in accordance
> > +    // with the proposal as of June 2018.
> 
> Let's put #ifdef here to protect the feature.

I don't think this is necessary.  We can use the
#ifdef ENABLE_WASM_BULKMEM_OPS structure that existed before this bug, and
insert the 5 new instructions inside that.  I checked that they are guarded
properly for both validation (DecodeFunctionBodyExprs), compilation
(BaseCompiler::emitBody and (Ion) EmitBodyExprs), and for text-to-binary
conversion WasmTokenStream::next).  So I don't think there's any danger of
parsing, validating or compiling a module that uses these without
ENABLE_WASM_BULKMEM_OPS being set.

> @@ +2422,5 @@
> > +
> > +    if (!popWithType(ValType::I32, dst))
> > +        return false;
> > +
> > +    return readVarU32(segIx);
> 
> Also, please join this check with the usesMemory check to group them
> logically.

But then it would be inconsistent with the styling in readMemInit just below.

Comment 33

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-08-deferred-validation.diff

This is a new patch in the series, to provide deferred validation support.

Comment 34

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-09-new-insns-cg-and-run.diff

> Any chance we could templatize the implementations of Drop? Or at least share
> the code that does the checks in a static function? (less crazy)

I'm not sure how to do this, since it would require parameterising over the
following differences

  class  ElemSegmentVector  vs  DataSegmentVector
  class  ElemSegmentInitVector  vs  DataSegmentInitVector
  literal  JSMSG_WASM_INVALID_PASSIVE_ELEM_SEG vs
           JSMSG_WASM_INVALID_PASSIVE_DATA_SEG

In addition, these functions are somewhat shorter than they used to be,
following revision after your review.  So I'm not sure it's worth it.

Comment 35

[Julian Seward [:jseward]]

Attachment: bug1471500-r19-10-tests.diff

Comment 36

[Julian Seward [:jseward]]

Comment on attachment 8998782 [details] [diff] [review]
bug1471500-r19-01-prelim-fixes.diff

Carrying over r+ bbouvier from previous iteration.

Comment 37

[Julian Seward [:jseward]]

Comment on attachment 8998784 [details] [diff] [review]
bug1471500-r19-03-changes-to-Table.diff

Carrying over r+ bbouvier from previous iteration.

Comment 38

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8998783 [details] [diff] [review]
bug1471500-r19-02-memFillCopy-tidying.diff

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

LGTM, thanks!

::: js/src/wasm/WasmInstance.cpp
@@ +404,2 @@
>              return 0;
> +        }

nit: no braces for 1-line if bodies

@@ +439,2 @@
>              return 0;
> +        }

nit: ditto here

@@ +443,5 @@
>          uint8_t* rawBuf = arrBuf.dataPointerEither().unwrap();
>  
>          // Here, we know that |len - 1| cannot underflow.
>          typedef CheckedInt<uint32_t> CheckedU32;
> +        CheckedU32 lenMinus1 = CheckedU32(len - 1);

nit: it's used only once, maybe inline it where it's used instead of defining a local variable?

Comment 39

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8998787 [details] [diff] [review]
bug1471500-r19-04-move-Segs-to-Code.diff

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

r+ given the memory leak is fixed. Thanks!

::: js/src/wasm/WasmModule.cpp
@@ +253,5 @@
>      if (!code().setTier2(std::move(tier2Arg), *bytecode_, *linkData2))
>          return false;
>      linkData().setTier2(std::move(linkData2));
> +    for (uint32_t i = 0; i < code().elemSegments().length(); i++)
> +        code().elemSegments()[i].setTier2(std::move(env2->elemSegments[i].elemCodeRangeIndices(Tier::Ion)));

nit: doesn't fit on 99 chars, please split it in 2 lines and add {} for the for body

@@ +820,5 @@
> +// constructor for it, because there's no way to make that fallible.
> +// Unfortunately, manually cloning an ElemSegment also implies manually
> +// cloning the Uint32Vectors that it contains.
> +static bool
> +CloneElemSegment(const ElemSegment& orig, ElemSegment* out)

In my opinion, this function is simple enough that we could inline it at its only call site instead.

@@ +825,5 @@
> +{
> +    out->tableIndex = orig.tableIndex;
> +    out->offset = orig.offset;
> +
> +    out->elemFuncIndices.clear();

nit: here and below, i don't think the clear() calls are necessary. To make sure of this, we can instead MOZ_ASSERT(out->elemFuncIndicies.empty()) etc.

@@ +1258,5 @@
> +            if (!dataSegments.appendAll(code_->dataSegments()))
> +                return false;
> +
> +            ElemSegmentVector elemSegments;
> +            for (auto& eseg : code_->elemSegments()) {

nit: spell out the full type of eseg (const ElemSegment&)

@@ +1264,5 @@
> +                MOZ_ASSERT(eseg.elemCodeRangeIndices2_.empty());
> +
> +                // ElemSegment doesn't have a (fallible) copy constructor,
> +                // so we have to clone it "by hand".
> +                ElemSegment* clone = js_new<ElemSegment>();

I think there's still a leak here, because the memory allocated to clone is never de-allocated: the "move" below just moves the data fields around and the elemSegments vector has its own memory management. Fortunately, it's simple to solve: you can just use a stack allocation here (i.e. ElemSegment clone;) and it should be fine.

Comment 40

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8998790 [details] [diff] [review]
bug1471500-r19-06-add-passive-Segs.diff

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

I'm pretty sure there are still latent bugs here, plus a few architectural comments that would be nice to address, so this would need another look. We're definitely on the right track!

::: js/src/wasm/WasmAST.h
@@ +1154,5 @@
>  };
>  
>  class AstDataSegment : public AstNode
>  {
> +    Maybe<AstExpr*> offsetIfActive_;

a Maybe<X*> is either a non-null pointer, or Nothing. Can it just be an AstExpr*, then, defined to nullptr when it's Nothing(), and defined to something non-null otherwise?

@@ +1171,5 @@
>  typedef AstVector<AstDataSegment*> AstDataSegmentVector;
>  
>  class AstElemSegment : public AstNode
>  {
> +    Maybe<AstExpr*> offsetIfActive_;

same question here

::: js/src/wasm/WasmJS.cpp
@@ +1121,2 @@
>      DataSegmentInitVector dataSegInitVec;
> +    for (const DataSegment& seg : code->dataSegments()) {

Architecture comment: it is actually more usual that we do this kind of things in Module::instantiate. Can you move the two init vector initialization loops there?

@@ +1128,5 @@
> +                ReportOutOfMemory(cx);
> +                return nullptr;
> +            }
> +            memcpy((char*)dsi->begin(),
> +                   (char*)bytecode->begin() + seg.bytecodeOffset,

You *do* need to perform the length check here, otherwise it can totally copy memory that's outside the bytecode, if the offset is very large. DecodeDataSegments does just the static checking and doesn't have a length check, because this check is performed at *runtime* when instantiating the segments and performing the actualy memcpys, see also https://searchfox.org/mozilla-central/source/js/src/wasm/WasmModule.cpp#640-669
Can you add a test case for this in the tests patch, please?

So you need something like this here too. Probably we wouldn't want partial initialization in case of errors, so we'd first perform all the checks and then only do the memcpys. (Can you add a test for this, please?)

@@ +1207,5 @@
>          return nullptr;
>  
> +    // Now fill in any missing segment initialiser instance pointers we
> +    // skipped above.
> +    for (const UniquePtr<ElemSegmentInit>& segInit : instance->elemSegInitVec()){

nit: space before {

@@ +1214,5 @@
> +        for (WasmCallee& callee : *segInit) {
> +            if (!callee.instance)
> +                callee.instance = instance;
> +        }
> +    }

Architecture comment: since this loop is infallible, could it be done in Instance ctor instead?

::: js/src/wasm/WasmModule.cpp
@@ +721,5 @@
> +                        const Metadata& metadata, const Table& table,
> +                        const CodeRangeVector& codeRanges,
> +                        const ElemSegment& seg, Tier tier,
> +                        const Instance* instance, uint8_t* codeBase,
> +                        WasmCallee* out)

This signature can take three fewer arguments by passing a wasm::Code:
- metadata := code.metadata()
- tier := code.bestTier()
- codeRanges := metadata(tier).codeRanges;
- codeBase := code.segment(tier).base();

(this would probably remove the need for a few definitions on the caller side, btw)

@@ +806,5 @@
>      for (const ElemSegment& seg : code_->elemSegments()) {
> +        // Skip passive segments.  Those may get applied later, under
> +        // program control, per comments above.
> +        if (!seg.offsetIfActive)
> +            continue;

nit: \n after continue;

::: js/src/wasm/WasmModule.h
@@ +254,5 @@
> +// not yet known, in which case the caller must patch in the value itself.
> +//
> +// This function exists so as to avoid duplicating logic in
> +// Instance::tableInit (where it processes active element segments) and
> +// WasmInstanceObject::create (where it processes passive element segments).

nit: we can remove the last paragraph (we don't usually embed the reason why a code has been factored out)

::: js/src/wasm/WasmTextToBinary.cpp
@@ +36,5 @@
>  
>  using namespace js;
>  using namespace js::wasm;
>  
> +using std::pair;

nit: this is unused, you can remove it and the blank line that follows

@@ +4038,5 @@
>          if (!fragments.append(text.text()))
>              return nullptr;
>      }
>  
> +    auto* data = new(c.lifo) AstDataSegment(Some(nullptr), std::move(fragments));

This could Nothing() instead of Some(nullptr), right?

@@ +5867,5 @@
> +        if (!e.writeOp(Op::End))
> +            return false;
> +    } else {
> +        if (!e.writeVarU32(uint32_t(InitializerKind::Passive)))
> +            return false;

Since there's the exact same code in EncodeDataSegment, maybe we could have a static function `EncodeTableSegmentIndexOrFlags` that factors this chunk out?

Comment 41

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8998789 [details] [diff] [review]
bug1471500-r19-05-add-InitVector.diff

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

About SharedBytes vs ShareableBytes: even though this change belongs to another patch in the queue, would it work if we declare SharedBytes in WasmCode.h instead of early declaring ShareableBytes, in WasmJS.h?

(I think this patch is made almost obsolete by my previous review on the next patch; sorry for the caused confusion)

::: js/src/wasm/WasmInstance.cpp
@@ +668,5 @@
> +
> +    for (UniquePtr<ElemSegmentInit>& esi : elemSegInitVec_) {
> +        if (esi)
> +            esi = nullptr;
> +    }

Even better: the destructor of UniquePtr does this for you: https://searchfox.org/mozilla-central/source/mfbt/UniquePtr.h#288

So technically we can remove these two loops.

::: js/src/wasm/WasmJS.cpp
@@ +1077,5 @@
>                             Handle<FunctionVector> funcImports,
>                             const GlobalDescVector& globals,
>                             HandleValVector globalImportValues,
>                             const WasmGlobalObjectVector& globalObjs,
> +                           const ShareableBytes* bytecode,

If I'm not mistaken this is unused in this patch?

::: js/src/wasm/WasmJS.h
@@ +223,5 @@
>                                        Handle<FunctionVector> funcImports,
>                                        const wasm::GlobalDescVector& globals,
>                                        wasm::HandleValVector globalImportValues,
>                                        const WasmGlobalObjectVector& globalObjs,
> +                                      const wasm::ShareableBytes* bytecode,

(unused in this patch)

::: js/src/wasm/WasmModule.cpp
@@ +1323,5 @@
>                                              funcImports,
>                                              metadata().globals,
>                                              globalImportValues,
>                                              globalObjs,
> +                                            bytecode_.get(),

nit: this and the comment above should be in another patch, probably?

::: js/src/wasm/WasmTypes.h
@@ +1117,5 @@
> +        entry(entry)
> +    {}
> +    // The instance associated with the code address.
> +    const Instance* instance;
> +    // The code address.

Well, we have several of them :) We also have several entries, so I suspect a more legitimate comment would be "The table entry code address". (It is still true even for asm.js, for which the normal entry is used when calling as a function pointer)

@@ +1139,5 @@
> +// The final structures have type DataSegmentInitVector and
> +// ElemSegmentInitVector respectively.
> +
> +typedef  Vector<uint8_t,    0, SystemAllocPolicy>  DataSegmentInit;
> +

nit: remove the blank line here

@@ +1148,5 @@
> +// {memory,table}.drop instructions.
> +
> +typedef  Vector<UniquePtr<DataSegmentInit>, 0, SystemAllocPolicy>
> +         DataSegmentInitVector;
> +

ditto

Comment 42

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8998796 [details] [diff] [review]
bug1471500-r19-07-new-insns-pre-codegen.diff

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

Nice, I like what you've done with the generalization of MemOrTable here, thanks.

::: js/src/wasm/WasmTextToBinary.cpp
@@ +3082,5 @@
> +    return new(c.lifo) AstMemOrTableCopy(isMem, dest, src, len);
> +}
> +
> +static AstMemOrTableDrop*
> +ParseMemOrTableDrop(WasmParseContext& c, bool inParens, bool isMem)

nit: inParens isn't used, let's remove it

Comment 43

[Lars T Hansen [:lth]]

> About SharedBytes vs ShareableBytes: even though this change belongs to
> another patch in the queue, would it work if we declare SharedBytes in
> WasmCode.h instead of early declaring ShareableBytes, in WasmJS.h?

Whatever works is fine with me, and the cleaner the better obviously.

That said, sometimes we just write out definitions in full to avoid circular includes.  I think WasmJS.h is one of the places where that happens, see eg WasmInstanceObject::create.  Here we write out RefPtr<const Code> even though that is aliased as SharedCode in WasmCode.h.

Comment 44

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8998800 [details] [diff] [review]
bug1471500-r19-09-new-insns-cg-and-run.diff

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

Nice!

::: js/src/wasm/WasmInstance.cpp
@@ +429,5 @@
>  
>  /* static */ int32_t
> +Instance::memDrop(Instance* instance, uint32_t segIndex)
> +{
> +    const DataSegmentVector& dataSegs = instance->code().dataSegments();

Can you move this definition just above the MOZ_ASSERT(dataSegs.length(), ...) (transitively :))?

@@ +433,5 @@
> +    const DataSegmentVector& dataSegs = instance->code().dataSegments();
> +    DataSegmentInitVector& dataSegInitVec = instance->dataSegInitVec();
> +
> +    size_t initVecLen = dataSegInitVec.length();
> +    MOZ_ASSERT(dataSegs.length() == initVecLen);

Can you move this assert just above `const DataSegment& seg = ...`

@@ +450,5 @@
> +
> +    const DataSegment& seg = dataSegs[segIndex];
> +
> +    // If this is an active initializer, something is badly wrong.
> +    MOZ_RELEASE_ASSERT(!seg.offsetIfActive);

Nice checks.

@@ +493,5 @@
> +/* static */ int32_t
> +Instance::memInit(Instance* instance, uint32_t dstOffset, uint32_t srcOffset,
> +                  uint32_t len, uint32_t segIndex)
> +{
> +    const DataSegmentVector& dataSegs = instance->code().dataSegments();

same suggestion to group things relating to dataSegs together below

@@ +532,5 @@
> +    if (len == 0) {
> +        // Even though the length is zero, we must check for valid offsets.
> +        if (dstOffset < memLen && srcOffset < seg.length) {
> +            return 0;
> +        }

nit: no {} for 1-line if bodies

@@ +543,5 @@
> +            highestSrcOffset.isValid() &&
> +            highestDstOffset.value() < memLen &&
> +            highestSrcOffset.value() < segLen)
> +        {
> +            MOZ_RELEASE_ASSERT(srcOffset + (len - 1) < segLen);

We can remove this assertion, since it's tautologically equivalent to the above highestSrcOffset.value() < segLen?

@@ +554,5 @@
> +    }
> +
> +    JSContext* cx = TlsContext.get();
> +    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_WASM_OUT_OF_BOUNDS);
> +

nit: remove \n

@@ +569,5 @@
> +        // Even though the number of items to copy is zero, we must check
> +        // for valid offsets.
> +        if (dstOffset < tableLen && srcOffset < tableLen) {
> +            return 0;
> +        }

nit: no {}

@@ +586,5 @@
> +            if (dstOffset > srcOffset) {
> +                for (uint32_t i = len; i > 0; i--)
> +                    table->copy(dstOffset + (i - 1), srcOffset + (i - 1));
> +            }
> +            else if (dstOffset < srcOffset) {

nit: this goes onto the previous line, right next to the }

@@ +603,5 @@
> +
> +/* static */ int32_t
> +Instance::tableDrop(Instance* instance, uint32_t segIndex)
> +{
> +    const ElemSegmentVector& elemSegs = instance->code().elemSegments();

Symmetrically to memoryDrop, we could group elemSegs uses in this function, to make understanding easier.

@@ +637,5 @@
> +/* static */ int32_t
> +Instance::tableInit(Instance* instance, uint32_t dstOffset, uint32_t srcOffset,
> +                    uint32_t len, uint32_t segIndex)
> +{
> +    const ElemSegmentVector& elemSegs = instance->code().elemSegments();

ditto

@@ +681,5 @@
> +    if (len == 0) {
> +        // Even though the length is zero, we must check for valid offsets.
> +        if (dstOffset < tableLen && srcOffset < segLen) {
> +            return 0;
> +        }

nit: no {}

@@ +695,5 @@
> +        {
> +            MOZ_RELEASE_ASSERT(srcOffset + (len - 1) < segLen);
> +            for (uint32_t i = 0; i < len; i++) {
> +                WasmCallee callee = (*segInit)[srcOffset + i];
> +                MOZ_ASSERT(callee.entry && callee.instance);

nit: we usually prefer to have two asserts in this kind of case, so the diagnostic is easier when the assertion fails

@@ +704,5 @@
> +    }
> +
> +    JSContext* cx = TlsContext.get();
> +    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_WASM_OUT_OF_BOUNDS);
> +

nit: remove \n

::: js/src/wasm/WasmIonCompile.cpp
@@ +2783,5 @@
> +
> +    if (f.inDeadCode())
> +        return false;
> +
> +    MDefinition* segIndexM = f.constant(Int32Value(int32_t(segIndex)), MIRType::Int32);

nit: maybe rename segIndexM to segIndexDef? (or even simply this could be segIndex, if the above segIndex is segIndexLit or something like this)
Also, can you move this further down, just before it's used? (second passArg)

@@ +2855,5 @@
> +static bool
> +EmitMemOrTableInit(FunctionCompiler& f, bool isMem)
> +{
> +    uint32_t segIndex = 0;
> +    MDefinition* dstOffM, *srcOffM, *lenM;

nit: please rename to dstOffsetDef, srcOffsetDef, lenDef (or simply dstOffset, srcOffset and len)

@@ +2856,5 @@
> +EmitMemOrTableInit(FunctionCompiler& f, bool isMem)
> +{
> +    uint32_t segIndex = 0;
> +    MDefinition* dstOffM, *srcOffM, *lenM;
> +

nit: remove \n

@@ +2863,5 @@
> +
> +    if (f.inDeadCode())
> +        return false;
> +
> +    MDefinition* segIndexM = f.constant(Int32Value(int32_t(segIndex)), MIRType::Int32);

nit: same renaming suggestion here (and move it down closer to passArg too)

Comment 45

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8998802 [details] [diff] [review]
bug1471500-r19-10-tests.diff

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

The passive-segs-boundary test is great, the other one is too complicated and really needs breaking down into smaller tests.

::: js/src/jit-test/tests/wasm/passive-segs-boundary.js
@@ +14,5 @@
> +// module is constructed with memory or table initializers.  haveMemOrTable
> +// determines whether there is actually a memory or table to work with.
> +
> +function do_test(insn1, insn2,
> +                 shouldFail, errKind, errText,

shouldFail === !!errKind here I think

@@ +159,5 @@
> +mem_test("",
> +         "(memory.init 1 (i32.const 0xFFFE) (i32.const 1) (i32.const 3))",
> +         WebAssembly.RuntimeError, /index out of bounds/);
> +
> +// init: seg ix is valid passive, zero len, but src offset OOB

nit: please spell out OOB in comments

::: js/src/jit-test/tests/wasm/passive-segs-smoketest.js
@@ +32,5 @@
> +// or 'x' to indicate an exception (null table entry).  It actually calls
> +// functions rather than merely printing the table entries after running the
> +// instance's testfn(), so as to verify that the table.{init,copy}
> +// implementations do whatever is necessary to make the resulting entries
> +// properly callable.

This is overly complicated. Can you break it into a bunch of very small unit test cases, which don't need a driver, and just test the obvious things for each opcode?
It's really fine to have code duplication in tests, the most important thing is that they're simple enough that we understand them if we need to do so from a simple glance. This criteria is not accomplished with this test, so we need something simpler.

Comment 46

[Lars T Hansen [:lth]]

Comment on attachment 8998798 [details] [diff] [review]
bug1471500-r19-08-deferred-validation.diff

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

Apart from one fairly trivial bug this looks good to me, nice work.

::: js/src/wasm/WasmGenerator.h
@@ +127,1 @@
>      LifoAlloc                  lifo;

Nit: I might be inclined to align the other fields here with this new one, or to put this new one last.

::: js/src/wasm/WasmValidate.cpp
@@ +2291,5 @@
> +    // report it later as an error, we can at least report a correct offset.
> +    haveHighestDataSegIndex = true;
> +    if (segIndex > highestDataSegIndex) {
> +        highestDataSegIndex = segIndex;
> +        highestDataSegIndexOffset = offsetInModule;

This is actually incorrect.  Consider the case where we call notifyDataSegmentIndex once during a compilation, with segIndex 0.  In this case, segIndex == highestDataSegIndex, so we will not execute this code, and though highestDataSegIndex will be correct, highestDataSegIndexOffset will remain at its initial value, which is wrong.  The first time through we must set the highestDataSegIndexOffset unconditionally.

@@ +2299,2 @@
>  bool
> +DeferredValidationState::performDeferredValidation(const ModuleEnvironment* env,

Subtlety here: by convention in the wasm code, if a thing is const we pass a reference, so "const ModuleEnvironment& env"; conversely, if it may be updated we pass a pointer, so "ModuleEnvironment* env".  You'll see this reflected various places in the validator.  For example, DecodeCodeSection takes a pointer but dereferences it to generate a const& to pass to DecodeFunctionBody.  (IMO we could discuss whether DecodeCodeSection needs to take a pointer; not obvious to me that it does.)

If you change this, be sure also to update the commit comment, which mentions this specially.

Comment 47

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-01-prelim-fixes.diff

Comment 48

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-02-memFillCopy-tidying.diff

Comment 49

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-03-changes-to-Table.diff

Comment 50

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-04-move-Segs-to-Code.diff

Comment 51

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-05-add-InitVector.diff

> Comment on attachment 8998789 [details] [diff] [review] [diff] [review]
> bug1471500-r19-05-add-InitVector.diff
>
> > ::: js/src/wasm/WasmJS.cpp
> > @@ +1089,5 @@
> > >                             Handle<FunctionVector> funcImports,
> > >                             const GlobalDescVector& globals,
> > >                             HandleValVector globalImportValues,
> > >                             const WasmGlobalObjectVector& globalObjs,
> > > +                           const ShareableBytes* bytecode,
> > 
> > Could it be SharedBytes?
> 
> Maybe, but I can't use SharedBytes in WasmJS.h because then I'd have to
> include WasmCode.h in WasmJS.h.  Would it make sense to move ShareableBytes,
> MutableBytes and SharedBytes from WasmCode.h to WasmTypes.h?

In the end I did just that -- moved  ShareableBytes, MutableBytes and
SharedBytes from WasmCode.h to WasmTypes.h.  Also, I put this change into
the 06-add-passive-Segs patch because it's not actually necessary in this
patch.

Comment 52

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-06-add-passive-Segs.diff

> Comment on attachment 8998790 [details] [diff] [review] [diff] [review]
> bug1471500-r19-06-add-passive-Segs.diff
>
> ::: js/src/wasm/WasmJS.cpp
> @@ +1121,2 @@
> >      DataSegmentInitVector dataSegInitVec;
> > +    for (const DataSegment& seg : code->dataSegments()) {
> 
> Architecture comment: it is actually more usual that we do this kind of
> things in Module::instantiate. Can you move the two init vector
> initialization loops there?

Per email discussion, the two loops got moved into Instance::init.  That also
makes it possible to remove the post-creation fixup loop for the Instance
pointers for table initialisers.

> @@ +1128,5 @@
> > +                ReportOutOfMemory(cx);
> > +                return nullptr;
> > +            }
> > +            memcpy((char*)dsi->begin(),
> > +                   (char*)bytecode->begin() + seg.bytecodeOffset,
> 
> You *do* need to perform the length check here, otherwise it can totally copy
> memory that's outside the bytecode, if the offset is very
> large. DecodeDataSegments does just the static checking and doesn't have a
> length check, because this check is performed at *runtime* when instantiating
> the segments and performing the actualy memcpys, see also
> https://searchfox.org/mozilla-central/source/js/src/wasm/WasmModule.cpp#640-669
> Can you add a test case for this in the tests patch, please?
> 
> So you need something like this here too. Probably we wouldn't want partial
> initialization in case of errors, so we'd first perform all the checks and
> then only do the memcpys. (Can you add a test for this, please?)

Per discussion on irc, seg.bytecodeOffset and seg.length are indeed made
safe in DecodeDataSegments.  So, skipping this.

> @@ +1214,5 @@
> > +        for (WasmCallee& callee : *segInit) {
> > +            if (!callee.instance)
> > +                callee.instance = instance;
> > +        }
> > +    }
> 
> Architecture comment: since this loop is infallible, could it be done in
> Instance ctor instead?

Per comments above, the loop has been removed entirely.

Comment 53

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-07-new-insns-pre-codegen.diff

Comment 54

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-08-deferred-validation.diff

Comment 55

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-09-new-insns-cg-and-run.diff

Comment 56

[Julian Seward [:jseward]]

Attachment: bug1471500-r20-10-tests.diff

> Comment on attachment 8998802 [details] [diff] [review] [diff] [review]
> bug1471500-r19-10-tests.diff
>
> ::: js/src/jit-test/tests/wasm/passive-segs-smoketest.js
> @@ +32,5 @@
> > +// or 'x' to indicate an exception (null table entry).  It actually calls
> > +// functions rather than merely printing the table entries after running the
> > +// instance's testfn(), so as to verify that the table.{init,copy}
> > +// implementations do whatever is necessary to make the resulting entries
> > +// properly callable.
> 
> This is overly complicated. Can you break it into a bunch of very small unit
> test cases, which don't need a driver, and just test the obvious things for
> each opcode?

I split it into a bunch of small tests, each of which tests just one
instruction (except the last test).  But it's pretty much impossible to
do with with zero JS driver framework.  It is significantly simpler though.
I also renamed it to passive-segs-nonboundary.js.

I also improved this test so as to work with a mixture of imported and
local functions (in the table.init bits), which is important but which
the previous patch iterations didn't test.

Finally, a race condition discovered in passive-segs-boundary.js has been
fixed.  In the case where a module does not declare a memory, but does have
data segments and also a function that uses a "memory." instruction, one of
two different exceptions can be reported (has data seg but has no memory, or
uses a "memory." instruction but has no memory), and which of those appears
first depends on a the relative progress of the function-compiling thread vs
the module-checking thread.

Comment 57

[Julian Seward [:jseward]]

Thank you for the second round reviews.

I just attached a new patch set (r20).  All review comments have been
addressed.  There are no major changes.

Comment 58

[Julian Seward [:jseward]]

Comment on attachment 9005800 [details] [diff] [review]
bug1471500-r20-01-prelim-fixes.diff

Carrying forward bbouvier r+

Comment 59

[Julian Seward [:jseward]]

Comment on attachment 9005801 [details] [diff] [review]
bug1471500-r20-02-memFillCopy-tidying.diff

Carrying forward bbouvier r+

Comment 60

[Julian Seward [:jseward]]

Comment on attachment 9005802 [details] [diff] [review]
bug1471500-r20-03-changes-to-Table.diff

Carrying forward bbouvier r+

Comment 61

[Julian Seward [:jseward]]

Comment on attachment 9005803 [details] [diff] [review]
bug1471500-r20-04-move-Segs-to-Code.diff

Carrying forward bbouvier r+

Comment 62

[Julian Seward [:jseward]]

Comment on attachment 9005806 [details] [diff] [review]
bug1471500-r20-07-new-insns-pre-codegen.diff

Carrying forward bbouvier r+

Comment 63

[Julian Seward [:jseward]]

Comment on attachment 9005807 [details] [diff] [review]
bug1471500-r20-08-deferred-validation.diff

Carrying forward lth r+

Comment 64

[Julian Seward [:jseward]]

Comment on attachment 9005808 [details] [diff] [review]
bug1471500-r20-09-new-insns-cg-and-run.diff

Carrying forward bbouvier r+

Comment 65

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9005804 [details] [diff] [review]
bug1471500-r20-05-add-InitVector.diff

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

LGTM, thanks!

::: js/src/wasm/WasmTypes.h
@@ +1132,5 @@
> +// Support for passive data and element segments -- lazy initialisation.
> +//
> +// At instantiation time, we prepare the required initialising data for each
> +// passive segment, copying it into new memory.  This is done separately for
> +// data segments and elem segments.  We also create an vector of pointers to

nit: a* vector of

Comment 66

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9005805 [details] [diff] [review]
bug1471500-r20-06-add-passive-Segs.diff

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

LGTM, thanks!

::: js/src/wasm/WasmInstance.cpp
@@ +633,5 @@
> +                ReportOutOfMemory(cx);
> +                return false;
> +            }
> +            memcpy((char*)dsi->begin(),
> +                   (char*)bytecode->begin() + seg.bytecodeOffset,

Since we proved it can't be out of bounds (sorry for my numerous misreadings), can we have a MOZ_ASSERT that checks this before this memcpy?

@@ +636,5 @@
> +            memcpy((char*)dsi->begin(),
> +                   (char*)bytecode->begin() + seg.bytecodeOffset,
> +                   seg.length);
> +        }
> +        if (!dataSegInitVec_.append(std::move(dsi))) {

Can you, above this loop, do:

dataSegInitVec_.reserve(code().dataSegments().length())

so you can use infallibleAppend here?

@@ +655,5 @@
> +                return false;
> +            }
> +
> +            Tier tier = code().bestTier();
> +            Table& table = *tables()[seg.tableIndex];

Can it be const?

@@ +662,5 @@
> +                       seg.elemFuncIndices.length());
> +
> +            for (uint32_t i = 0; i < seg.elemCodeRangeIndices(tier).length(); i++) {
> +                ComputeWasmCallee(code(), this, funcImports, i, table, seg,
> +                                  &(*esi)[i]);

(I think this should be equivalent to (esi + i), but I'm all for explicitness here; the operator[] could be doing goofy stuff under the hood)

@@ +665,5 @@
> +                ComputeWasmCallee(code(), this, funcImports, i, table, seg,
> +                                  &(*esi)[i]);
> +            }
> +        }
> +        if (!elemSegInitVec_.append(std::move(esi))) {

ditto for reserve() and infallibleAppend here.

::: js/src/wasm/WasmModule.cpp
@@ +616,5 @@
> +                        const ElemSegment& seg, WasmCallee* out)
> +{
> +    const Tier tier = code.bestTier();
> +#ifdef DEBUG
> +    const Metadata& metadata = code.metadata();

If you use DebugOnly<const Metadata&>, you can remove the #ifdef DEBUG block.

::: js/src/wasm/WasmTypes.h
@@ +162,5 @@
> +
> +struct ShareableBytes : ShareableBase<ShareableBytes>
> +{
> +    // Vector is 'final', so instead make Vector a member and add boilerplate.
> +    Bytes bytes;

(pre-existing) nit: can you add a \n after this attribute member?

@@ +165,5 @@
> +    // Vector is 'final', so instead make Vector a member and add boilerplate.
> +    Bytes bytes;
> +    ShareableBytes() = default;
> +    explicit ShareableBytes(Bytes&& bytes) : bytes(std::move(bytes)) {}
> +    size_t sizeOfExcludingThis(MallocSizeOf m) const { return bytes.sizeOfExcludingThis(m); }

nit: rename m to mallocSizeOf

@@ +169,5 @@
> +    size_t sizeOfExcludingThis(MallocSizeOf m) const { return bytes.sizeOfExcludingThis(m); }
> +    const uint8_t* begin() const { return bytes.begin(); }
> +    const uint8_t* end() const { return bytes.end(); }
> +    size_t length() const { return bytes.length(); }
> +    bool append(const uint8_t *p, uint32_t ct) { return bytes.append(p, ct); }

nit: * goes next to uint8_t
+ please give full names to the parameters

::: js/src/wasm/WasmValidate.cpp
@@ +1945,5 @@
> +                                             env->types.length(), &offset))
> +            {
> +                return false;
> +            }
> +            offsetIfActive = Some(offset);

nit: Can you use offsetIfActive.emplace(offset) instead?

@@ +2126,5 @@
> +                                             env->types.length(), &segOffset))
> +            {
> +                return false;
> +            }
> +            seg.offsetIfActive = Some(segOffset);

ditto

Comment 67

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9005809 [details] [diff] [review]
bug1471500-r20-10-tests.diff

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

::: js/src/jit-test/tests/wasm/passive-segs-boundary.js
@@ +299,5 @@
> +    }}}}
> +}
> +
> +
> +//---- table.copy ---------------------------------------------------------

Can you add tests for memory.copy, or group them in the same file if they're existing somewhere else?

::: js/src/jit-test/tests/wasm/passive-segs-nonboundary.js
@@ +76,5 @@
> +    let tab_impmod_t = gen_tab_impmod_t(instruction);
> +    let tab_impmod_b = wasmTextToBinary(tab_impmod_t);
> +
> +    for (let i = 0; i < expected_result_vector.length; i++) {
> +        let inst = new Instance(new Module(tab_impmod_b),

Can the Instance be created above the loop?

@@ +87,5 @@
> +        let expected = expected_result_vector[i];
> +        let actual = undefined;
> +        try {
> +            actual = inst.exports.testfn(i);
> +            assertEq(actual != null, true);

nit: use !==

@@ +97,5 @@
> +        }
> +
> +        if (actual !== expected) {
> +            print("tab_test fail: insn = '" + instruction + "', index = " + i
> +                  + ", expected = " + expected + ", actual = " + actual);

You can also provide a message as a third parameter to the assertEq function.

Comment 68

[Lars T Hansen [:lth]]

In https://github.com/WebAssembly/bulk-memory-operations/issues/27#issuecomment-419153886 there's a comment:

> The init/drop ops are also verified to only use passive segments, so deferring the validation requires keeping track of all expected passive segment indices.

I scanned this patch set looking for evidence that we do fail to validate when mem.init/drop references an active segment, but I didn't see it.  (The patch set is large and I may well have overlooked it.)  Do we have code for that or do we just trap at runtime?

Comment 69

[Lars T Hansen [:lth]]

I should also say that I haven't verified that Andrew's assertion in that comment is actually the proposed semantics.

Comment 70

[Julian Seward [:jseward]]

(In reply to Lars T Hansen [:lth] from comment #68)
> I scanned this patch set looking for evidence that we do fail to validate
> when mem.init/drop references an active segment, but I didn't see it.

It's a good question.  Currently it's a runtime trap.  The *-10-test.diff
patch contains the tests below, and the checks themselves are in the 
*-09-new-insns-cg-and-run.diff patch.  If it is decided that this should
be checked at validation time then it should be easy to push the relevant
checks into the DeferredValidationState mechanism (*-08-deferred-validation.diff)
when the time comes.

+// drop with data seg ix indicating an active segment
+mem_test("memory.drop 2", "",
+         WebAssembly.RuntimeError, /use of invalid passive data segment/);
+
+// init with data seg ix indicating an active segment
+mem_test("(memory.init 2 (i32.const 1234) (i32.const 1) (i32.const 1))", "",
+         WebAssembly.RuntimeError, /use of invalid passive data segment/);

+// drop with elem seg ix indicating an active segment
+tab_test("table.drop 2", "",
+         WebAssembly.RuntimeError, /use of invalid passive element segment/);
+
+// init with elem seg ix indicating an active segment
+tab_test("(table.init 2 (i32.const 12) (i32.const 1) (i32.const 1))", "",
+         WebAssembly.RuntimeError, /use of invalid passive element segment/);

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72060e122785
Complete initial implementation of the bulk-memory proposal.  Part 1 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7311eb19d848
Complete initial implementation of the bulk-memory proposal.  Part 2 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd9684a0ce2b
Complete initial implementation of the bulk-memory proposal.  Part 3 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf8c958385d
Complete initial implementation of the bulk-memory proposal.  Part 4 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac5a9eee5eb
Complete initial implementation of the bulk-memory proposal.  Part 5 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7308df9f62
Complete initial implementation of the bulk-memory proposal.  Part 6 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c6a3767c74
Complete initial implementation of the bulk-memory proposal.  Part 7 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6898b065caf3
Complete initial implementation of the bulk-memory proposal.  Part 8 of 10.  r=lth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e8a31975b4
Complete initial implementation of the bulk-memory proposal.  Part 9 of 10.  r=bbouvier.
https://hg.mozilla.org/integration/mozilla-inbound/rev/db19cd760ebe
Complete initial implementation of the bulk-memory proposal.  Part 10 of 10.  r=bbouvier.

Comment 72

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/72060e122785
https://hg.mozilla.org/mozilla-central/rev/7311eb19d848
https://hg.mozilla.org/mozilla-central/rev/fd9684a0ce2b
https://hg.mozilla.org/mozilla-central/rev/cbf8c958385d
https://hg.mozilla.org/mozilla-central/rev/2ac5a9eee5eb
https://hg.mozilla.org/mozilla-central/rev/9f7308df9f62
https://hg.mozilla.org/mozilla-central/rev/e9c6a3767c74
https://hg.mozilla.org/mozilla-central/rev/6898b065caf3
https://hg.mozilla.org/mozilla-central/rev/95e8a31975b4
https://hg.mozilla.org/mozilla-central/rev/db19cd760ebe