emscripten icon indicating copy to clipboard operation
emscripten copied to clipboard

WEBGPU: unexpected right shift when use ALLOW_MEMORY_GROWTH and MAXIMUM_MEMORY

Open xdestiny110 opened this issue 2 years ago • 4 comments

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.47 (431685f05c67f0424c11473cc16798b9587bb536)

The following command

em++ -sALLOW_MEMORY_GROWTH=1 -sUSE_WEBGPU=1 -sMAXIMUM_MEMORY=4gb test.cpp

generates a.out.js as normal. But if we look into the generated a.out.js

function _wgpuBufferGetMappedRange(bufferId, offset, size) {
 bufferId >>>= 0;
 offset >>>= 0;
 size >>>= 0;
 var bufferWrapper = WebGPU.mgrBuffer.objects[bufferId];
 assert(typeof bufferWrapper != "undefined");
 if (size === 0) warnOnce("getMappedRange size=0 no longer means WGPU_WHOLE_MAP_SIZE");
 if (size == -1) size = undefined;
....

it performs a right shift operation on the size parameter at the beginning, which results in a non-negative number. However, the subsequent code logic contains a comparison where size == -1 is checked. This means that size can never become undefined, consequently causing the underlying WebGPU to not perform as expected. When I remove ALLOW_MEMORY_GROWTH or MAXIMUM_MEMORY, the right shift operator disappears. This is test.cpp content

#include <webgpu/webgpu_cpp.h>

int main() {
    wgpu::Buffer buffer;
    buffer.GetMappedRange();
    return 0;
}

xdestiny110 avatar Oct 26 '23 02:10 xdestiny110

Yup, it looks like the if (size == -1) size = undefined; is probably wrong since the type of size is size_t which is an unsigned type.

sbc100 avatar Apr 11 '24 16:04 sbc100

Hm, we didn't test this with the right build config. This almost certainly explains why for so long I thought we had all our sentinel checks correct (checking for 0xFFFFFFFF (didn't work in 64bit)), then it somehow turned out they were all wrong (needed to check for -1).

When I remove ALLOW_MEMORY_GROWTH or MAXIMUM_MEMORY, the right shift operator disappears.

@sbc100 how should we deal with this so it works in both configs? Should we just cast it like if ((size | 0) == -1)?

kainino0x avatar Apr 12 '24 05:04 kainino0x

This could be tricky.. because we convert size_t and pointer values to to JS numbers when they arrive in JS. We have {{{ POINTER_MAX }}}, but that is defined as MEMORY64 ? 'Number.MAX_SAFE_INTEGER' : '0xFFFFFFFF';. I'm not sure our current setup allows for a pointer value to be larger than MAX_SAFE_INTEGER in wasm64.

What is the sentinal value defined as in C?

sbc100 avatar Apr 12 '24 05:04 sbc100

What is the sentinal value defined as in C?

SIZE_MAX, for this one which is size_t. (There are some others where it's UINT32_MAX or UINT64_MAX.)

kainino0x avatar Apr 12 '24 05:04 kainino0x