Signed shift on memory access in library_webgl.js when using 4GB support

[landersonfigma]

When using 4GB support, my understanding is that memory accesses are rewritten to use unsigned shifts. In library_webgl.js, glUniform4fv is rewritten from:

value >>= 2;
for (var i = 0; i < 4 * count; i += 4) {
  var dst = value + i;
  view[i] = heap[dst];
  view[i + 1] = heap[dst + 1];
  view[i + 2] = heap[dst + 2];
  view[i + 3] = heap[dst + 3];
}

to:

value >>= 2;
for (var i = 0; i < 4 * count; i += 4) {
  var dst = value + i;
  view[i] = heap[dst >>> 0];
  view[i + 1] = heap[dst + 1 >>> 0];
  view[i + 2] = heap[dst + 2 >>> 0];
  view[i + 3] = heap[dst + 3 >>> 0];
}

The memory accesses inside the loop look correct but should value >>= 2 be value >>>= 2? With value >>= 2, I get memory accesses that read out of bounds. Possibly the same issue here. Let me know if I'm misunderstanding the expected output.

[sbc100]

My guess is that the auto-rewriter is that not that smart and can only reason about expression inside of the HEAPXX[...] accessors.

The tools is here by the way:

emscripten/tools/acorn-optimizer.js

Lines 1235 to 1238 in 0a6f54b

	// Make all JS pointers unsigned. We do this by modifying things like
	// HEAP32[X >> 2] to HEAP32[X >>> 2]. We also need to handle the case of
	// HEAP32[X] and make that HEAP32[X >>> 0], things like subarray(), etc.
	function unsignPointers(ast) {

@kripken is that right? I don't suppose there is any way to know that value >>= 2 happens to be a pointer value?

In general I think the solution here is to use value = {{{ getHeapOffset('offset', 'i32') }}}.

[kripken]

Yeah, the pass has a little magic but that's it:

emscripten/tools/acorn-optimizer.js

Lines 1239 to 1244 in 0a6f54b

	// Aside from the standard emscripten HEAP*s, also identify just "HEAP"/"heap"
	// as representing a heap. This can be used in JS library code in order
	// to get this pass to fix it up.
	function isHeap(name) {
	return isEmscriptenHEAP(name) || name === 'heap' || name === 'HEAP';
	}

Aside from the standard heap names + those two there, it doesn't do any guessing.

Renaming view to heap would enable the magic, so that is an option to fix this. And more generally there is getHeapOffset as mentioned.