Bug 171527 – How we build polymorphic cases is wrong when making a call from Wasm

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

[Saam Barati]

We do a totally borked thing and assume the case does not have a code block when it totally does.

[Saam Barati]

*** Bug 171518 has been marked as a duplicate of this bug. ***

[Saam Barati]

*** Bug 171507 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 308798 [details]
patch

[Saam Barati]

Comment on attachment 308798 [details]
patch

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

> Source/JavaScriptCore/jit/Repatch.cpp:765
> +        if (variant.executable()->isHostFunction())

This is the fix for the bug.

[Build Bot]

Attachment 308798 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:192:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 308798 [details]
patch

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

A few comments, but overall looks good.

> Source/JavaScriptCore/wasm/WasmBinding.cpp:128
> +    if (!Options::useCallICsForWebAssemblyToJSCalls()) {

This versus the code below are kinda hard to tell apart. Aren't the mostly similar? Could you pull the common bits out, or is it too weird?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:136
> +        jit.subPtr(MacroAssembler::TrustedImm32(16), MacroAssembler::stackPointerRegister);

Why 16 instead of 8?

[Saam Barati]

Comment on attachment 308798 [details]
patch

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

thanks for the review

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:128
>> +    if (!Options::useCallICsForWebAssemblyToJSCalls()) {
> 
> This versus the code below are kinda hard to tell apart. Aren't the mostly similar? Could you pull the common bits out, or is it too weird?

I think it's different enough (note we only loop over signature once, not twice) that pulling out the common bits will just make everything less readable.

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:136
>> +        jit.subPtr(MacroAssembler::TrustedImm32(16), MacroAssembler::stackPointerRegister);
> 
> Why 16 instead of 8?

This is probably an anti pattern to use 16 directly, but the reason is it's rounded to stack alignment bytes.
I'll do this instead (which is the same as 16, but at least not hard coded):
uint32_t bytes = WTF::roundToMultipleOf(stackAlignmentBytes(), sizeof(Register));

[Saam Barati]

Created attachment 309003 [details]
patch for landing

[Build Bot]

Attachment 309003 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:192:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 309005 [details]
patch for landing

fix style.

[WebKit Commit Bot]

Comment on attachment 309005 [details]
patch for landing

Clearing flags on attachment: 309005

Committed r216173: <http://trac.webkit.org/changeset/216173>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.