WASM functions should be able to use arguments
Created attachment 287591 [details] Patch
Created attachment 287593 [details] Patch
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.
> 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.
Created attachment 287594 [details] Patch
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.
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.
Created attachment 287681 [details] Patch
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.
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.
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.
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.
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.