Bug 179639 – We need to set topCallFrame when calling Wasm::Memory::grow from the JIT

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

[Saam Barati]

...

[Saam Barati]

<rdar://problem/35513018>

[Saam Barati]

Created attachment 326822 [details]
patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/wasm/WasmInstance.h:47:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmInstance.h:47:  The parameter name "module" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmInstance.h:47:  The parameter name "storeTopCallFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmInstance.h:115:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 11 files


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

[JF Bastien]

Comment on attachment 326822 [details]
patch

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

r=me

> Source/JavaScriptCore/wasm/WasmInstance.cpp:56
> +Ref<Instance> Instance::create(Context* context, Ref<Module>&& module, EntryFrame** topEntryFramePointer, StoreTopCallFrameCallback storeTopCallFrame)

Make it && here too, and WTFMove below.

> Source/JavaScriptCore/wasm/WasmInstance.h:47
> +    static Ref<Instance> create(Context* context, Ref<Module>&& module, EntryFrame** topEntryFramePointer, StoreTopCallFrameCallback storeTopCallFrame = nullptr);

&& here too. Why nullptr? Seems nicer to force passing a no-op function when appropriate, otherwise it's not obvious that you're opting out.

> Source/JavaScriptCore/wasm/WasmInstance.h:110
> +        if (m_storeTopCallFrame)

I'd make this never nullptr.

> Source/JavaScriptCore/wasm/WasmInstance.h:131
> +    WTF::Function<void(void*)> m_storeTopCallFrame;

StoreTopCallFrameCallback from above.

[Saam Barati]

Created attachment 326825 [details]
patch for landing

With comments addressed.

[Saam Barati]

Created attachment 326826 [details]
patch for landing

Fix reviewed by OOPS.

[Build Bot]

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


ERROR: Source/JavaScriptCore/wasm/WasmInstance.h:114:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


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

[Saam Barati]

Looks like my test is taking too long on slow memory mode. Weird.

[Build Bot]

Comment on attachment 326826 [details]
patch for landing

Attachment 326826 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/5219743

New failing tests:
wasm.yaml/wasm/function-tests/grow-memory-cause-gc.js.wasm-slow-memory

[Saam Barati]

Created attachment 326848 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 326848 [details]
patch for landing

Clearing flags on attachment: 326848

Committed r224810: <https://trac.webkit.org/changeset/224810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.