Bug 161471 – WASM functions should be able to use arguments

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

WASM functions should be able to use arguments

[Keith Miller]

Created attachment 287591 [details]
Patch

[Keith Miller]

Created attachment 287593 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/WASMFormat.h:63:  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/testWASM.cpp:335:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 21 files


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

[Keith Miller]

> ERROR: Source/JavaScriptCore/wasm/WASMFormat.h:63:  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/testWASM.cpp:335:  Tests for true/false,
> null/non-null, and zero/non-zero should all be done without equality
> comparisons.  [readability/comparison_to_zero] [5]
> Total errors found: 2 in 21 files

While these are technically violations of WebKit style I think they are warranted in those spots. Each case is surrounded by other checks that the result of similar expressions are equal to non-zero values. Using ! instead just makes the code harder to read.

[Keith Miller]

Created attachment 287594 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/WASMFormat.h:63:  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/testWASM.cpp:335:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 22 files


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

[Keith Miller]

Comment on attachment 287594 [details]
Patch

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

> Source/JavaScriptCore/B3CallingConventions.cpp:29
> +

There should be a `#if ENABLE(B3_JIT)` here. That fixes the 32-bit build. I'll make sure to change that before landing.

[Keith Miller]

Created attachment 287681 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/testWASM.cpp:335:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 14 files


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

[Mark Lam]

Comment on attachment 287681 [details]
Patch

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

> Source/JavaScriptCore/testWASM.cpp:196
> +// FIXME: This is pure hacks and should be changed... please...

If this is still true, you need a bug url to go with this.

[Keith Miller]

Comment on attachment 287681 [details]
Patch

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

>> Source/JavaScriptCore/testWASM.cpp:196
>> +// FIXME: This is pure hacks and should be changed... please...
> 
> If this is still true, you need a bug url to go with this.

I don't think that's an issue. All this code is just here until the actual testing infrastructure is in place. I'll change the comment to exclude a FIXME.

[Benjamin Poulain]

Comment on attachment 287681 [details]
Patch

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

> Source/JavaScriptCore/B3CallingConventions.cpp:35
> +static JSCCallingConvention* staticJSCCallingConvention;

This should be inside jscCallingConvention.

Personally I find it nicer to explicitly initialize it.

> Source/JavaScriptCore/B3CallingConventions.cpp:41
> +    staticJSCCallingConvention = new CallingConvention<jscHeaderSize, nextJSCOffset>(Vector<GPRReg>(), RegisterSet::calleeSaveRegisters());

You could use NeverDestroyed to avoid locking a heap page.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:107
> +    m_locals.grow(count);

I don't think that's what you want.
This change the size of m_locals TO count, and default initialize the value.

If count > m_locals.size(), m_locals.append() will go after that.

What you want is reserveCapacity(size() + count)

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:127
> +    ASSERT(index < m_locals.size());

This should not be needed. Vector does that unless you tell it not to.

What could be valuable is 
    ASSERT(m_locals[index])
to make sure the value is not null.

[Mark Lam]

Keith landed this patch in https://trac.webkit.org/changeset/205330.  

FYI, there was a ChangeLog mixup.  So, don't just believe what you read as the change on trac.  Check the ChangeLog diff itself.