Bug 161707

It begins.

Created attachment 293479 [details] Patch

Attachment 293479 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py:37: Undefined variable 'Wasm' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py:125: Undefined variable 'isUnary' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py:126: Undefined variable 'isBinary' [pylint/E0602] [5] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 293479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293479&action=review > Source/JavaScriptCore/wasm/WasmFormat.cpp:45 > + return "f64"; Autogen? > Source/JavaScriptCore/wasm/WasmValidate.cpp:122 > +{ reserveCapacity here? Actually I've been moving a bunch of the parser to tryAllocate / tryReserve so that we can gracefully fail when OOM because of busted inputs, that's probably better here. > Source/JavaScriptCore/wasm/WasmValidate.cpp:131 > + m_locals.append(type); Can be unchecked. > Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py:1 > +#! /usr/bin/python Isn't this better: #!/usr/bin/env python > Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py:63 > + case UnaryOpType::""" + toCpp(name) + """: { Can you use string interpolation instead? It's easier to read.

Comment on attachment 293479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293479&action=review r=me > Source/JavaScriptCore/ChangeLog:17 > + 1) We need to handle result types from basic blocks. > + 2) We need to handle popping things from stacks when they don't exist. Do you have bugs open for these? If so, it's worth linking to them. > Source/JavaScriptCore/testWasm.cpp:475 > + if (plan.failed()) { > + dataLogLn("Module failed to compile with error: ", plan.errorMessage()); > + CRASH(); > + } > + > + if (plan.resultSize() != 2 || !plan.result(0) || !plan.result(1)) { Can you add a helper function for this type of pattern? Maybe like validatePlan(size_t expectedSize) or something? > Source/JavaScriptCore/wasm/WasmValidate.cpp:245 > + m_errorMessage = makeString("Arity mismatch in call"); Suggestion: Maybe it's worth providing the two arity values in the error message? > Source/JavaScriptCore/wasm/WasmValidate.cpp:264 > + return true; Do we care about what the last value's type here is? Or do we just ignore it? > Source/JavaScriptCore/wasm/WasmValidate.cpp:288 > + // FIXME: add better location information here. Can you link to a bugzilla for this?

Comment on attachment 293479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293479&action=review >> Source/JavaScriptCore/ChangeLog:17 >> + 2) We need to handle popping things from stacks when they don't exist. > > Do you have bugs open for these? If so, it's worth linking to them. Yep, changed. >> Source/JavaScriptCore/testWasm.cpp:475 >> + if (plan.resultSize() != 2 || !plan.result(0) || !plan.result(1)) { > > Can you add a helper function for this type of pattern? Maybe like validatePlan(size_t expectedSize) or something? Done. >> Source/JavaScriptCore/wasm/WasmFormat.cpp:45 >> + return "f64"; > > Autogen? I'm not sure how that would help. Maybe if we had more types. The code to autogen this would be roughly as long as this function. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:122 >> +{ > > reserveCapacity here? Actually I've been moving a bunch of the parser to tryAllocate / tryReserve so that we can gracefully fail when OOM because of busted inputs, that's probably better here. makes sense. I changed this function and addLocal to return a bool and use tryReserveCapacity(). >> Source/JavaScriptCore/wasm/WasmValidate.cpp:131 >> + m_locals.append(type); > > Can be unchecked. Fixed. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:245 >> + m_errorMessage = makeString("Arity mismatch in call"); > > Suggestion: Maybe it's worth providing the two arity values in the error message? Done. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:264 >> + return true; > > Do we care about what the last value's type here is? Or do we just ignore it? Yes but I plan on fixing that when I fix how the stack layout works with control flow in https://bugs.webkit.org/show_bug.cgi?id=164100. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:288 >> + // FIXME: add better location information here. > > Can you link to a bugzilla for this? Done. >> Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py:1 >> +#! /usr/bin/python > > Isn't this better: #!/usr/bin/env python Fixed. >> Source/JavaScriptCore/wasm/generateWasmValidateInlinesHeader.py:63 >> + case UnaryOpType::""" + toCpp(name) + """: { > > Can you use string interpolation instead? It's easier to read. Alas, no. We don't have python 3.6 only 2.6 :/

Created attachment 293576 [details] Patch for landing

Created attachment 293579 [details] Patch for landing

Comment on attachment 293579 [details] Patch for landing Clearing flags on attachment: 293579 Committed r208238: <http://trac.webkit.org/changeset/208238>

All reviewed patches have been landed. Closing bug.