Bug 163903

Created attachment 292688 [details] patch Implement WebAssembly.Module

Looks like you made the bots angry.

(In reply to comment #2) > Looks like you made the bots angry. That's super weird... I built and ran testWASM locally just fine, and did a regex replace that should have caught this. I'll fix when I'm in later, should be easy, I think the rest of the patch is fine.

Comment on attachment 292688 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=292688&action=review r=me with some comments. > JSTests/ChangeLog:21 > + - Beef up the testing infrastructure under JSTests/wasm so that more complex > + modules can be created and tested (instead of writing the bits by hand). +1! > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:50 > + m_errorMessage = "Modules doens't start with '\\0asm'"; typo: doesn't > Source/JavaScriptCore/wasm/WasmModuleParser.h:81 > + bool m_failed = true; > + String m_errorMessage; Why have both m_failed and m_errorMessage? It seems like failed() could just return !!m_errorMessage. Also, I think the new hotness in WebKit is to do: bool m_failed { true }; > Source/JavaScriptCore/wasm/WasmPlan.h:71 > + bool m_failed = true; > + String m_errorMessage; ditto. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:56 > + JSValue val = state->argumentCount() ? state->argument(0) : jsUndefined(); You don't need to check the argument count here. if you try to get an argument past the real argument count arguement(i) returns jsUndefined(). > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:59 > + JSArrayBuffer* arrayBuffer = obj ? jsDynamicCast<JSArrayBuffer*>(obj) : nullptr; > + JSArrayBufferView* arrayBufferView = obj ? jsDynamicCast<JSArrayBufferView*>(obj) : nullptr; You can just use val and delete obj. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:64 > + bool isNeutered = arrayBufferView ? arrayBufferView->isNeutered() : arrayBuffer->impl()->isNeutered(); > + if (isNeutered) Why store this in a local? You don't reuse it the fact you are checking if the buffer is neutered is pretty clear.

(In reply to comment #3) > (In reply to comment #2) > > Looks like you made the bots angry. > > That's super weird... I built and ran testWASM locally just fine, and did a > regex replace that should have caught this. I'll fix when I'm in later, > should be easy, I think the rest of the patch is fine. It's probably an auto-merge with my floating points patch.

Created attachment 292761 [details] patch Comments addressed. Let me know if you want me to do a follow-up (see below). > > Source/JavaScriptCore/wasm/WasmModuleParser.h:81 > > + bool m_failed = true; > > + String m_errorMessage; > > Why have both m_failed and m_errorMessage? It seems like failed() could just > return !!m_errorMessage. Ideally we'd have something like LLVM's ErrorOr (contains an error+message or a value), but I figure the way I've set it up makes it ~impossible to mess up: by default things fail until the end of the parse function is reached. If you forget to set the message it asserts out, if you try to use the content while there's an error it asserts out. I didn't find something like ErrorOr in WTF, maybe I can add one? It's almost like std::variant but more explicit. > > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:56 > > + JSValue val = state->argumentCount() ? state->argument(0) : jsUndefined(); > > You don't need to check the argument count here. if you try to get an > argument past the real argument count arguement(i) returns jsUndefined(). Ah OK, I'd cargo-culted from another part of the code, and then re-cargo'd from CompileError. I fixed CompileError, I can fix the source separately. > > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:59 > > + JSArrayBuffer* arrayBuffer = obj ? jsDynamicCast<JSArrayBuffer*>(obj) : nullptr; > > + JSArrayBufferView* arrayBufferView = obj ? jsDynamicCast<JSArrayBufferView*>(obj) : nullptr; > > You can just use val and delete obj. Done, I wasn't sure if everything gets CSE'd but it looks simple enough that it should. > > > Looks like you made the bots angry. > > > > That's super weird... I built and ran testWASM locally just fine, and did a > > regex replace that should have caught this. I'll fix when I'm in later, > > should be easy, I think the rest of the patch is fine. > > It's probably an auto-merge with my floating points patch. Oh yeah you're right, I rebuilt other tests but not that one after my merge.

Comment on attachment 292761 [details] patch Rejecting attachment 292761 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 292761, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in JSTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/2368387

Created attachment 292762 [details] patch Oops, I always forget about that OOPS in the ChangeLog.

Comment on attachment 292762 [details] patch Clearing flags on attachment: 292762 Committed r207825: <http://trac.webkit.org/changeset/207825>

All reviewed patches have been landed. Closing bug.

(In reply to comment #6) > Created attachment 292761 [details] > patch > > Comments addressed. Let me know if you want me to do a follow-up (see below). > > > > > Source/JavaScriptCore/wasm/WasmModuleParser.h:81 > > > + bool m_failed = true; > > > + String m_errorMessage; > > > > Why have both m_failed and m_errorMessage? It seems like failed() could just > > return !!m_errorMessage. > > Ideally we'd have something like LLVM's ErrorOr (contains an error+message > or a value), but I figure the way I've set it up makes it ~impossible to > mess up: by default things fail until the end of the parse function is > reached. If you forget to set the message it asserts out, if you try to use > the content while there's an error it asserts out. > > I didn't find something like ErrorOr in WTF, maybe I can add one? It's > almost like std::variant but more explicit. > The current paradigm for this is to return a WARN_UNUSED_RETURN boolean and take the result by reference. With the current system you basically have to intentionally write code incorrectly. It also eliminates the need for an m_failed bit.

(In reply to comment #11) > (In reply to comment #6) > > Created attachment 292761 [details] > > patch > > > > Comments addressed. Let me know if you want me to do a follow-up (see below). > > > > > > > > Source/JavaScriptCore/wasm/WasmModuleParser.h:81 > > > > + bool m_failed = true; > > > > + String m_errorMessage; > > > > > > Why have both m_failed and m_errorMessage? It seems like failed() could just > > > return !!m_errorMessage. > > > > Ideally we'd have something like LLVM's ErrorOr (contains an error+message > > or a value), but I figure the way I've set it up makes it ~impossible to > > mess up: by default things fail until the end of the parse function is > > reached. If you forget to set the message it asserts out, if you try to use > > the content while there's an error it asserts out. > > > > I didn't find something like ErrorOr in WTF, maybe I can add one? It's > > almost like std::variant but more explicit. > > > > The current paradigm for this is to return a WARN_UNUSED_RETURN boolean and > take the result by reference. With the current system you basically have to > intentionally write code incorrectly. It also eliminates the need for an > m_failed bit. IIUC that's not sufficient for what I want to achieve: 1. The parse result either contains an error or parsed content. 2. The parser *must* set the error message on all error paths. 3. The consumer *cannot* obtain content if an error was generated. Starting with `m_failed = true`, setting it only at the very end of the function, and JS error construction asserting that messages are non-empty ensure 2. Private content + failed/message enforce asserts which guarantee 3. The problem with 1. is that with just m_errorMessage the default is "no error". You can't brainlessly make sure that you've actually set the error state on all paths. We could initialize it to a placeholder "there's an error but I don't know what", but that's pretty ugly! IMO something like ErrorOr is even better than what I've cobbled together. I'll implement something like that for WTF when we're further with WebAssembly.

Comment on attachment 292762 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=292762&action=review Patch LGTM too > JSTests/wasm/assert.js:59 > +const _instanceof = (obj, type) => obj instanceof type; Do we really need a helper function for this? I think the actual language expression is easier to read than a function call. > JSTests/wasm/assert.js:61 > +// Use underscore names to avoid clashing with builtin names. Why would this be needed for anything but instanceof? > Source/JavaScriptCore/wasm/WasmFunctionParser.h:75 > + dataLogLn("Parsing function starting at: ", (uintptr_t)functionStart, " of length: ", functionLength); Style: should be static_cast > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:46 > + m_errorMessage = "Module is " + String::number(length()) + " bytes, expected at least " + String::number(minSize) + " bytes"; I think the general rule is to use StringBuilder for code like this. At least others have recommended this to me. Also, the constant strings could be ASCIILiterals > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:58 > + m_errorMessage = "couldn't parse version number"; Should be ASCIILiteral > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:64 > + m_errorMessage = "unexpected version number"; Ditto > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:80 > + m_errorMessage = "couldn't get section byte"; Ditto (and to others below) > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:194 > // TODO Should be FIXME > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:67 > + const auto* base = arrayBufferView ? static_cast<uint8_t*>(arrayBufferView->vector()) : static_cast<uint8_t*>(arrayBuffer->impl()->data()); Style: I'm not a big fan of auto here