Created attachment 296820 [details] patch

Comment on attachment 296820 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=296820&action=review > JSTests/wasm/js-api/test_Data.js:58 > +(function DataSectionWithoutMemory() { > + const builder = (new Builder()) > + .Type().End() > + .Data() > + .Segment([0xff]).Offset(0).End() > + .End(); > + const bin = builder.WebAssembly().get(); > + const module = new WebAssembly.Module(bin); > + const memory = new WebAssembly.Memory(memoryDescription); > + assert.throws(() => new WebAssembly.Instance(module, { imp: { memory: memory } }), RangeError, `cannot initialize non-zero data segments when no memory is present`); > +})(); This sure looks like it should be a validation error. > JSTests/wasm/js-api/test_Data.js:70 > +(function EmptyDataSectionWithoutMemory() { > + const builder = (new Builder()) > + .Type().End() > + .Data() > + .Segment([]).Offset(0).End() > + .End(); > + const bin = builder.WebAssembly().get(); > + const module = new WebAssembly.Module(bin); > + const memory = new WebAssembly.Memory(memoryDescription); > + const instance = new WebAssembly.Instance(module, { imp: { memory: memory } }); > +})(); Ditto > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:218 > + } else { > + for (auto& segment : data) { > + if (UNLIKELY(segment->sizeInBytes)) > + return throwException(state, scope, createRangeError(state, makeString(ASCIILiteral("cannot initialize non-zero data segments when no memory is present")))); > + } > } This seems like it should be caught by validation.

Andreas doesn't seem to want to move things to validation time: https://github.com/WebAssembly/design/issues/897#issuecomment-266505611 I proposed some clarifications here: https://github.com/WebAssembly/design/pull/902 For now I'd just go with what I implemented, and revisit when we reach spec consensus.

(In reply to comment #3) > Andreas doesn't seem to want to move things to validation time: > https://github.com/WebAssembly/design/issues/897#issuecomment-266505611 > > I proposed some clarifications here: > https://github.com/WebAssembly/design/pull/902 > > For now I'd just go with what I implemented, and revisit when we reach spec > consensus. Using a Data when no memory is present is definitely a validation error.

Comment on attachment 296820 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=296820&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:202 > + if (memory) { This should turn into an assert.

(In reply to comment #4) > (In reply to comment #3) > > Andreas doesn't seem to want to move things to validation time: > > https://github.com/WebAssembly/design/issues/897#issuecomment-266505611 > > > > I proposed some clarifications here: > > https://github.com/WebAssembly/design/pull/902 > > > > For now I'd just go with what I implemented, and revisit when we reach spec > > consensus. > > Using a Data when no memory is present is definitely a validation error. Is it? I can make it so, but we don't do that at the moment and I'm not sure the spec says so.

Created attachment 297197 [details] patch Address comments, this is now ready for landing (and blocks bug #163919).

Comment on attachment 297197 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297197&action=review r=me with comments. > JSTests/wasm/js-api/element-data.js:8 > +(function ElementBeforeData() { What's the advantage of wrapping this in a function? > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:197 > +template <typename Scope, typename N, typename ...Args> Why is N needed it looks like you only use it with size_t anyway?

Comment on attachment 297197 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297197&action=review >> JSTests/wasm/js-api/element-data.js:8 >> +(function ElementBeforeData() { > > What's the advantage of wrapping this in a function? I've been doing that for all the tests: it appears in the stack traces if it fails, and it's simpler than having a comment IMO. >> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:197 >> +template <typename Scope, typename N, typename ...Args> > > Why is N needed it looks like you only use it with size_t anyway? It just means we don't have to change the type if it changes at the call site (and we don't get inadvertent coercion). It also catch cases where the 3 parameters don't have the same type.

Comment on attachment 297197 [details] patch Clearing flags on attachment: 297197 Committed r209874: <http://trac.webkit.org/changeset/209874>

All reviewed patches have been landed. Closing bug.