Bug 163919 - WebAssembly: improve compilation error messages
Summary: WebAssembly: improve compilation error messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on: 147142 165733 165812 165893
Blocks: 159775 165813 165862 165924 165833
  Show dependency treegraph
 
Reported: 2016-10-24 16:04 PDT by JF Bastien
Modified: 2016-12-15 15:43 PST (History)
6 users (show)

See Also:


Attachments
WIP (98.23 KB, patch)
2016-12-13 17:23 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
WIP (106.17 KB, patch)
2016-12-14 11:06 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
WIP (157.72 KB, patch)
2016-12-14 17:38 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
WIP (165.74 KB, patch)
2016-12-14 19:37 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
patch for review (182.04 KB, patch)
2016-12-14 22:13 PST, JF Bastien
sbarati: review+
jfbastien: commit-queue-
Details | Formatted Diff | Diff
patch (189.05 KB, patch)
2016-12-15 15:05 PST, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-10-24 16:04:58 PDT
The error message in WebAssembly.CompileError are currently pretty limited. Improve them. I'm adding FIXME with this bug ID where that's the case.
Comment 1 JF Bastien 2016-11-14 17:33:43 PST
I implemented std::expected in WTF, and plan to use it to improve error messages soon.
Comment 2 JF Bastien 2016-12-13 17:23:39 PST
Created attachment 297052 [details]
WIP

WIP patch, all is ready for comments except:
 - WasmB3IRGenerator part which I still have to do.
 - Tests which need to be updated.

I'll also wait for Saam and Keith's patches for Element + Global to land before landing this change.

I'm thinking about punting good validator error messages for now. It would be pretty much the same thing as I've already done (see fail() helper in WasmParser.h).
Comment 3 JF Bastien 2016-12-13 17:25:51 PST
Oh and before it gets asked, same answer as Yusuke on auto -> Result syntax:
  https://bugs.webkit.org/show_bug.cgi?id=164757#c27
:-)
Comment 4 Saam Barati 2016-12-13 19:16:32 PST
Comment on attachment 297052 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297052&action=review

This approach seems reasonable to me.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:81
> +    if (UNLIKELY(!addResult))                        \
> +    return addResult.getUnexpected();                \
> +    } while (0)

Indentation looks off here.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:323
> +    return PartialResult();

Why not "return { }"
for this and all the other "return PartialResult()"

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:384
> +        WASM_PARSER_FAIL_IF(!parseUInt8(opcode), "can't get ", segmentNumber, "th Data segment's opcode");

JSC's parser also adds a macro that would be named like so in your naming scheme
WASM_PARSER_FAIL_IF_FALSE()
and then this line of code could be
WASM_PARSER_FAIL_IF_FALS(parseUInt8 ...)

I'm not sure if it's better or not, but it's worth mentioning.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:197
> +    if (UNLIKELY(condition != I32))
> +        return UnexpectedType<Result::ErrorType>("validation failed: select condition must be i32");

Seems like you should have a macro for a condition failing here to return a string wrapped in UnexpectedType<Result::ErrorType>

Also these can be ASCIILiteral
Comment 5 JF Bastien 2016-12-14 11:06:17 PST
Created attachment 297102 [details]
WIP

This WIP addresses the other comments, and doesn't yet deal with B3 generation, improving the validation messages, nor the tests.


> > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:384
> > +        WASM_PARSER_FAIL_IF(!parseUInt8(opcode), "can't get ", segmentNumber, "th Data segment's opcode");
> 
> JSC's parser also adds a macro that would be named like so in your naming
> scheme
> WASM_PARSER_FAIL_IF_FALSE()
> and then this line of code could be
> WASM_PARSER_FAIL_IF_FALS(parseUInt8 ...)
> 
> I'm not sure if it's better or not, but it's worth mentioning.

I think the positive one reads more easily: "fail if this happens" instead of "fail if this doesn't happen". I read this as "fail if you can't parse uint8". It's nicer in other places too: "fail if this is empty" and "fail if the type isn't `void`".


> > Source/JavaScriptCore/wasm/WasmValidate.cpp:197
> > +    if (UNLIKELY(condition != I32))
> > +        return UnexpectedType<Result::ErrorType>("validation failed: select condition must be i32");
> 
> Seems like you should have a macro for a condition failing here to return a
> string wrapped in UnexpectedType<Result::ErrorType>
> 
> Also these can be ASCIILiteral

Yeah I'll move it to the same thing as the rest of the patch, so it'll be much nicer.
Comment 6 JF Bastien 2016-12-14 17:38:26 PST
Created attachment 297149 [details]
WIP

This is almost good to go, I need to update the tests.
Comment 7 JF Bastien 2016-12-14 19:37:02 PST
Created attachment 297159 [details]
WIP

A few tests, more to come. I'm half-online, will add others later.
Comment 8 JF Bastien 2016-12-14 22:13:34 PST
Created attachment 297168 [details]
patch for review

The patch is ready for review, but is still blocked on two patches which I'll get to tomorrow.

I'll also add a changelog.
Comment 9 Saam Barati 2016-12-15 14:13:14 PST
Comment on attachment 297168 [details]
patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=297168&action=review

r=me

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:152
> +        return fail(__VA_ARGS__);                 \

Style: Indentation is wrong here.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:268
> +        WASM_PARSER_FAIL_IF(!parseVarUInt32(alignment), "can't get store alignment");
> +        WASM_PARSER_FAIL_IF(!parseVarUInt32(offset), "can't get store offset");
> +        WASM_TRY_POP_EXPRESSION_STACK_INTO(value, "store value");
> +        WASM_TRY_POP_EXPRESSION_STACK_INTO(pointer, "store pointer");
> +        WASM_TRY_ADD_TO_CONTEXT(store(static_cast<StoreOpType>(op), pointer, value, offset));

Style: It seems weird that one macro error requires "can't <blah" and the other doesn't

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:402
> +            WASM_PARSER_FAIL_IF(exp.kindIndex, "can't export Table ", exp.kindIndex, " only one Table is currently supported");

Nit: I think this is easier to understand if you say only a zero index table is allowed. Saying only one doesn't say what index is expected.
Comment 10 JF Bastien 2016-12-15 15:05:56 PST
Created attachment 297228 [details]
patch

Merge, address comments, ChangeLog.
Comment 11 WebKit Commit Bot 2016-12-15 15:06:58 PST
Attachment 297228 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:76:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:77:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:78:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:79:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:86:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmParser.h:89:  Do not use 'using namespace FailureHelper;'.  [build/using_namespace] [4]
Total errors found: 6 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Commit Bot 2016-12-15 15:42:56 PST
Comment on attachment 297228 [details]
patch

Clearing flags on attachment: 297228

Committed r209880: <http://trac.webkit.org/changeset/209880>
Comment 13 WebKit Commit Bot 2016-12-15 15:43:01 PST
All reviewed patches have been landed.  Closing bug.