Simplify and fix SAFE_HEAP

[RReverser]

This streamlines the SAFE_HEAP transform in acorn-optimizer. I was going to send this as a separate cleanup PR as part of my other work on acorn-optimizer, but it happens to also be the easy fix for a regression mentioned in #24289 and caused by #24283 which added support for 64-bit integers to acorn-optimizer passes.

I could fix that issue separately by tweaking unSign, but this way we don't need to bother - the values with correct sign are now automatically retrieved by accessing the correct HEAP* array.

I've added a regression test for the linked issue as well, which fails before this PR and passes after.

[sbc100]

Nice!

[sbc100]

This seems like a clear improvement so I'm tempted to land it once that tests pass, but maybe we should wait for an approval from @kripken since he knows this code better then me.

[RReverser]

@kripken Can you take a look please - this fixes a CI regression in emscripten-releases.

[sbc100]

Is this the first in your series of patches to land? And ready to go?

[RReverser]

Either order is fine, they're pretty independent from each other. It's ready to go, I'm just waiting for @kripken's review since you suggested we should wait for his approval above.

[RReverser]

I'm merging since this is a blocker. I'm happy to address follow-up comments if any in a separate PR.

[kripken]

What happened to the alignment checks?

[RReverser]

No longer necessary because we pass in an index in the corresponding array instead of pre-multiplied pointer.

Those were already guaranteed not to be hit since we always did transform like HEAP32[i] -> SAFE_HEAP_LOAD(i * 4, 4, ...) and then function would just check that i * 4 is divisible by 4, which is always true, but now it's just more obvious since we skip the premultiplication.

[kripken]

Good point. I was also concerned about directly-emitted SAFE_HEAP_* calls from parseTools (not from the acorn transform), but I see we removed those a while ago, which I forgot...

[RReverser]

Oh yeah, I did also notice that

emscripten/site/source/docs/api_reference/preamble.js.rst

Lines 381 to 391 in 976b43d

	function SAFE_HEAP_CLEAR(dest)
	function SAFE_HEAP_ACCESS(dest, type, store, ignore, storeValue)
	function SAFE_HEAP_STORE(dest, value, type, ignore)
	function SAFE_HEAP_LOAD(dest, type, unsigned, ignore)
	function SAFE_HEAP_COPY_HISTORY(dest, src)
	function SAFE_HEAP_FILL_HISTORY(from, to, type)
	function getSafeHeapType(bytes, isFloat)
	function SAFE_HEAP_STORE(dest, value, bytes, isFloat)
	function SAFE_HEAP_LOAD(dest, bytes, isFloat, unsigned)
	function SAFE_FT_MASK(value, mask)
	function CHECK_OVERFLOW(value, bits, ignore, sig)

lists quite a few methods that don't (no longer?) exist, and probably misses bunch of new ones instead.

Maybe that section should be just removed at this point instead of trying in sync, since it's pretty outdated and not rendered in docs anyway?

[kripken]

I agree, yeah, best to just remove that bit, good find.

[RReverser]

But, I think we saw that that JS-side bug is long-standing and never caused any issues, so it doesn't feel urgent to fix.

Fair enough. I was mainly curious in context of those no-op checks we are commenting on, as I assume they used to do something useful in the long past, so technically a regression happened at some point... but idk when. As you say, it's probably not urgent.

If the input code contains HEAP32[x] then by construction x must be 4-byte aligned, no?

Problem I'm thinking about, a lot of code has form like HEAP32[somePtr >> 2] and such, whether handcrafted or generated via makeGetValue, and in those cases there is no guarantee somePtr is already correctly aligned.

JS transform could detect at least this common form of access and check the alignment. Or maybe at least makeGetValue/getHeapOffset itself could insert those checks when SAFE_HEAP is enabled.

[sbc100]

Yes, but what about this?

// a JS function that is called from wasm, and given a (maybe unaligned) pointer
function jsHelper(ptr) {
  return HEAP32[ptr >> 2];
}

But I don't think the old code handled this did it? It just checked the value of ptr >> 2 at runtime, right? So there is no regression here is there?

[sbc100]

JS transform could detect at least this common form of access and check the alignment. Or maybe at least makeGetValue/getHeapOffset itself could insert those checks when SAFE_HEAP is enabled.

To be clear neither the old new the version the JS transform do this though do they?

[RReverser]

To be clear neither the old new the version the JS transform do this though do they?

If you mean before/after this PR, then yeah.

But judging by the removed code, it used to in the long past. And, well, even if it didn't, just saying it would be a useful addition.

[kripken]

Yes, this isn't a recent regression. But it may have regressed a long time ago, or never worked... Anyhow, alignment checks from JS are pretty minor, not worth worrying about.