[webkit-reviews] review granted: [Bug 171537] WebAssembly: running out of executable memory should throw OoM : [Attachment 313960] 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 171537: WebAssembly: running out of executable memory should throw OoM
https://bugs.webkit.org/show_bug.cgi?id=171537

Attachment 313960: patch

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




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

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

r=me

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:320
> +    return exec->vm().canUseJIT() && Options::useBaselineJIT();

nice

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:179
> +	   if (UNLIKELY(!binding)) {

Do your tests trigger this?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:310
> +	   LinkBuffer linkBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail);
> +	   if (UNLIKELY(linkBuffer.didFailToAllocate()))
> +	       return makeUnexpected(BindingFailure::OutOfMemory);

Do your tests stress this?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:606
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail);
> +    if (UNLIKELY(patchBuffer.didFailToAllocate()))

ditto

> Source/JavaScriptCore/wasm/WasmBinding.cpp:664
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail);
> +    if (UNLIKELY(patchBuffer.didFailToAllocate()))
> +	   return makeUnexpected(BindingFailure::OutOfMemory);
> +

ditto

> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:95
> +    LinkBuffer linkBuffer(*context.wasmEntrypointJIT, nullptr,
JITCompilationCanFail);
> +    if (UNLIKELY(linkBuffer.didFailToAllocate())) {
> +	   Base::fail(holdLock(m_lock), makeString("Out of executable memory
while tiering up function at index ", String::number(m_functionIndex)));
> +	   return;
> +    }

Do any of your tests trigger this?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:62
> +	   if (UNLIKELY(!binding)) {

Do your tests trigger this?

> Tools/Scripts/run-jsc-stress-tests:1261
> +    run("default-wasm", "--useConcurrentGC=0" , "--useConcurrentJIT=0",
"--jitMemoryReservationSize=15000", "--useBaselineJIT=0", "--useDFGJIT=0",
"--useFTLJIT=0", "-m")

I see a potential bug where we don't honor this option. It won't matter in this
case since your region is smaller than what's used, but since you're relying on
this option, you might as well fix it. There is a direct use of
fixedExecutableMemoryPoolSize instead of asking the allocator its reservation
size. Look in JITStubRoutine.h. It should be trivial to fix just by loading it
from the allocator instead.