Bug 161707 - Add a WASM function validator.
Summary: Add a WASM function validator.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks: 159775
  Show dependency treegraph
 
Reported: 2016-09-07 13:16 PDT by Keith Miller
Modified: 2016-11-01 13:43 PDT (History)
3 users (show)

See Also:


Attachments
Patch (60.56 KB, patch)
2016-10-31 15:47 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (104.93 KB, patch)
2016-11-01 13:06 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (104.93 KB, patch)
2016-11-01 13:08 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-09-07 13:16:49 PDT
We should add a validator that uses the WASM::FunctionParser.
Comment 1 Keith Miller 2016-10-25 11:18:49 PDT
It begins.
Comment 2 Keith Miller 2016-10-31 15:47:00 PDT
Created attachment 293479 [details]
Patch
Comment 3 WebKit Commit Bot 2016-10-31 15:49:45 PDT
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 4 JF Bastien 2016-10-31 16:02:38 PDT
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 5 Saam Barati 2016-10-31 16:25:15 PDT
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 6 Keith Miller 2016-11-01 09:52:45 PDT
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 :/
Comment 7 Keith Miller 2016-11-01 13:06:06 PDT
Created attachment 293576 [details]
Patch for landing
Comment 8 Keith Miller 2016-11-01 13:08:20 PDT
Created attachment 293579 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2016-11-01 13:43:37 PDT
Comment on attachment 293579 [details]
Patch for landing

Clearing flags on attachment: 293579

Committed r208238: <http://trac.webkit.org/changeset/208238>
Comment 10 WebKit Commit Bot 2016-11-01 13:43:42 PDT
All reviewed patches have been landed.  Closing bug.