glTex(Sub)ImageX functions cannot upload float data from memory beyond 2GB address space

[PhM81]

Hi,
when using the "-s MAXIMUM_MEMORY=4GB" flag i have run into issues with the glTex(Sub)ImageX functions. As soon as an address beyond the 2GB address space is passed as the pixels parameter and the type parameter is GL_FLOAT the following error is reported: "WebGL: INVALID_OPERATION: texImage2D: ArrayBufferView not big enough for request".
The reason for this issue is the usage of the arithmetic right shift operator (>>) in the generated .js file. When passing an address into e.g. glTexImage2D it is being shifted by 2 bit by the javascript code if float data is supposed to be uploaded. Note that the address is stored as a signed 32 bit signed integer on the javascript side, so any address beyound 2GB will be a negative value. Right shifting negative values via >> will add 1 to the left and thus mess up the intention of the shift.
A potential fix to this problem is modifying the library_webgl.js and library_webgl2.js files in the emscripten repository by replacing several ">>" operators with ">>>" operators. These will be able to handle negative integer values as intended. So for example the line

GLctx.texImage2D(target, level, internalFormat, width, height, border, format, type, heap, pixels >> heapAccessShiftForWebGLHeap(heap));

would have to be changed to

GLctx.texImage2D(target, level, internalFormat, width, height, border, format, type, heap, pixels >>> heapAccessShiftForWebGLHeap(heap));

There are probably quite a few other places where this change would be necessary to properly support the "-s MAXIMUM_MEMORY=4GB" option. Basically all the texture data upload functions which accept data as something else than a simple byte array will be affected.

I have attached a simple demo program which illustrates this problem
demo.zip
It has been built by:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.33 (0e68b98e85a1644a7224c360fa6cb21bea5dc33d)
clang version 14.0.0 (https://github.com/llvm/llvm-project 1c05c52de2177a328b7d2d07b697af67eb9f8122)
Target: wasm32-unknown-emscripten
Thread model: posix

To build it yourself use something like
emcc -s MAX_WEBGL_VERSION=2 -s MIN_WEBGL_VERSION=2 -s FULL_ES3=1 -s ALLOW_MEMORY_GROWTH=1 -s MAXIMUM_MEMORY=4GB --memoryprofiler -o test.html test.cpp

The program initializes opengl via egl, allocates 3GB of memory and then tries to upload float data into a texture which fails with the aforementioned error. If the float array used for upload is being allocated before allocating the 3GB of memory everything works as expected.

[kripken]

Thanks @PhM81 , that does sound like a bug and your investigation looks correct to me.

I think to solve this we should change the >> operations in all of library_webgl.js, library_webgl2.js, when those >> are done on pointers, to be >>>. But we should only do that when CAN_ADDRESS_2GB is true, that is, when location 2GB+ can be addressed.

This is related to the issue of wasm64, since for both cases we need to know which parameters are pointers. So perhaps it would make sense to look into a shared mechanism for this. Atm I think we have separate ones.

cc @aardappel @juj for thoughts.

[juj]

When making my WebGPU library 4GB (and tentatively Wasm64) -aware, I ended up with the following kind of syntax to be "tight" and to avoid paying any excess code size in different build modes: juj/wasm_webgpu@47abc6a .

The bindings library already uses a lot of direct index accesses to avoid lots of repetitive >> shifts, since marshalling WebGPU descriptor data between Wasm and JS is very heap struct access heavy. So converting the pointers to indices is possible to do relatively cleanly there.

For Emscripten test suite, it would be good to migrate to always run all tests with GLOBAL_BASE = 2GB and later with (GLOBAL_BASE=4GB for Wasm64) if the test shell/browser supports that, that would uncover a lot more issues like this in the JS libs. (basically none of them are 4GB aware atm)

[aardappel]

The obvious answer is that this code should not be in JS. If it was in C, linked into your Wasm code, it would work on all platforms. But appreciate making that happen may be a lot of work.

As for Memory64, it looks like bigints are the only way to avoid 32-bit semantics? That sounds expensive if just done locally, and clumsy (a lot of the current Memory64 Emscripten code is actually.. 53-bit, especially to avoid bigint related type errors absolutely everywhere).

[juj]

The obvious answer is that this code should not be in JS.

Sorry, what code are you referring to?

[aardappel]

The code performing a shift on a pointer/index.

[kripken]

@juj Oh nice, I like that ptrToIndex and shiftPtr approach. What do you think about adding that to upstream? I think that would be a good way to fix this issue.

@aardappel Definitely as much code should be in wasm as possible, but I think for these GL bindings we have no choice. Something needs to do HEAP*.subarray in JS in order to get something to pass to glTexSubImage etc.

[juj]

The code performing a shift on a pointer/index.

Calling back from JS to Wasm in order to implement a simple shift would not be particularly performant. Especially if BigInts are used for 64-bit numbers.

Also in 4GB mode if the function needs to perform byte accesses, the signed->unsigned conversion issue still remains, and the JS code will still need to do a ptr >>> 0 computation.

For JS functions that only need to deal with accessing memory with a fixed shift size, it would be possible to pre-shift in wasm side before calling out from Wasm to JS, but that is a bit opportunistic and not a general solution.

(a lot of the current Memory64 Emscripten code is actually.. 53-bit, especially to avoid bigint related type errors absolutely everywhere)

It would be possible to have separate MEMORY64=1 and MEMORY53=1 build modes that would marshal pointers either as BigInts or doubles.

The general idea here is to come up with a common JS side convention/pattern for handling pointers in a way that avoids these type errors. It is not particularly hard to do, but coming up with a convenient syntax requires some planning.

Btw, is there an ETA or a plan on when 64-bit Wasm memory support would come to browsers? That would enable starting to experimenting with this in conjunction with WebGPU and WebGL and large files the filesystem APIs.

@juj Oh nice, I like that ptrToIndex and shiftPtr approach. What do you think about adding that to upstream? I think that would be a good way to fix this issue.

Yeah, that sounds good. I'll work on a PR for this.

[aardappel]

Calling back from JS to Wasm in order to implement a simple shift would not be particularly performant.

I wasn't implying it would be just for a single shift.. I'd personally would want to move over as much code as possible. But I am not familiar with the code in question so I have no idea of the scope.

It would be possible to have separate MEMORY64=1 and MEMORY53=1

I don't think so, given how painful this code already is. As I replied to you elsewhere, a) this is a first iteration of Memory64 support, b) using Numbers vastly reduces the amount of code that needs to be adapted/conditional (we still want Number for wasm32), and c) it will be a LONG while before we'll see the first Wasm pointer/index that needs more than 53 bits (8 Petabytes??).

is there an ETA or a plan on when 64-bit Wasm memory support would come to browsers?

Information here is still accurate: WebAssembly/memory64#20

[PhM81]

Thanks for the discussion wrt. wasm64. I am looking forward to this feature since it will remove quite a few limitations my current project is dealing with.
As for solving the issue raised here. I have experimented a bit with replacing most of the >> shifts by >>> shifts in library_webgl.js, library_webgl2.js and this looks like it is doing the job.

But we should only do that when CAN_ADDRESS_2GB is true, that is, when location 2GB+ can be addressed.

Why should this change be limited to CAN_ADDRESS_2GB being true? For positive numbers >> and >>> are equivalent so i assume that >>> can always be used?

[juj]

Why should this change be limited to CAN_ADDRESS_2GB being true? For positive numbers >> and >>> are equivalent so i assume that >>> can always be used?

That is true.

These system libraries are optimized down to a single generated code byte. There are people who develop WebGL code for smallest possible output and for e.g. 64k demo competitions.

That may sound strict, though this is possible to do because there is generally only a fixed small size of this code (WebGL API will not be growing unboundedly, it is "done").

Also since we will need an explicit machinery to fix up for pointer accesses for the different memory modes anyways, we can comfortably bake this kind of 2GB vs 4GB management into the same machinery as well.

[PhM81]

Thanks for that clarification. This use case was not really on my radar :)

[mmarczell-graphisoft]

I'm getting this error on the Chrome console:

Uncaught RangeError: Failed to execute 'bufferData' on 'WebGL2RenderingContext': 
    The ArrayBufferView size exceeds the supported range
at _glBufferData (Connector.js:9:354991)

Firefox is a bit more explanatory as usual:

Uncaught TypeError: WebGL2RenderingContext.bufferData: 
    Argument 2 can't be an ArrayBuffer or an ArrayBufferView larger than 2 GB
_glBufferData Connector.js:9

(Other times it crashes at other GL calls, such as _glCompressedTexSubImage2D).

I believe this is the same bug?

[PhM81]

Yes, this looks very similar. Easiest check you can do is to look at the address that is being passed to glBufferData on the C++ side of your code. If it is above the 2GB limit, you are running into the same problem as me. You can solve this by making the changes proposed in the first post.

[mmarczell-graphisoft]

@juj

Yeah, that sounds good. I'll work on a PR for this.

Did you find the time for this? Alternatively do you have some starting advice or general concept on how to go about fixing this?