[webkit-reviews] review granted: [Bug 179106] WebAssembly: Unexpected "RangeError: Maximum call stack size exceeded." : [Attachment 328043] patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 179106: WebAssembly: Unexpected "RangeError: Maximum call stack size
exceeded."
https://bugs.webkit.org/show_bug.cgi?id=179106

Attachment 328043: patch

https://bugs.webkit.org/attachment.cgi?id=328043&action=review




--- Comment #12 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 328043
  --> https://bugs.webkit.org/attachment.cgi?id=328043
patch

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

r=me

> JSTests/wasm/function-tests/double-instance.js:16
> +// our implementation used to set it to UINT_MAX causing an erroneous stack

nit: UINTPTR_MAX

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:462
> +	   Value* actualStackLimitPointer =
m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(),
instanceValue(),
safeCast<int32_t>(Instance::offsetOfActualStackLimitPointer()));
> +	   Value* actualStackLimit =
m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(),
actualStackLimitPointer);
> +	   m_currentBlock->appendNew<MemoryValue>(m_proc, Store, origin(),
actualStackLimit, instanceValue(),
safeCast<int32_t>(Instance::offsetOfCachedStackLimit()));

nice

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1319
> +    restoreWebAssemblyGlobalState(RestoreCachedStackLimit::No,
m_info.memory, instanceValue(), m_proc, m_currentBlock);

Why No here? The above comment makes me think it should be Yes

> Source/JavaScriptCore/wasm/WasmInstance.h:138
> +    void** m_actualStackLimitPointer { nullptr };

Name suggestion: for me, a name like, "pointerToStackLimit" would be more
intuitive. WDYT? Up to you if you want to change it or not, but if you do, I'd
recommend changing other various methods with this name as well.