Bug 163919 – WebAssembly: improve compilation error messages

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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.

[JF Bastien]

I implemented std::expected in WTF, and plan to use it to improve error messages soon.

[JF Bastien]

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).

[JF Bastien]

Oh and before it gets asked, same answer as Yusuke on auto -> Result syntax:
  https://bugs.webkit.org/show_bug.cgi?id=164757#c27
:-)

[Saam Barati]

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

[JF Bastien]

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.

[JF Bastien]

Created attachment 297149 [details]
WIP

This is almost good to go, I need to update the tests.

[JF Bastien]

Created attachment 297159 [details]
WIP

A few tests, more to come. I'm half-online, will add others later.

[JF Bastien]

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.

[Saam Barati]

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.

[JF Bastien]

Created attachment 297228 [details]
patch

Merge, address comments, ChangeLog.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 297228 [details]
patch

Clearing flags on attachment: 297228

Committed r209880: <http://trac.webkit.org/changeset/209880>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.