Simplify and fix SAFE_HEAP (#24291)

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.

Author: RReverser

src/lib/libgetvalue.js

@@ -59,19 +59,6 @@ var LibraryMemOps = {
    * @param {string} type
    */`,
   $getValue: getValueImpl,
-
-#if SAFE_HEAP
-  // The same as the above two functions, but known to the safeHeap pass
-  // in tools/acorn-optimizer.mjs.  The heap accesses within these two
-  // functions will *not* get re-written.
-  // Note that we do not use the alias mechanism here since we need separate
-  // instances of above setValueImpl/getValueImpl functions.
-  $setValue_safe__internal: true,
-  $setValue_safe: setValueImpl,
-
-  $getValue_safe__internal: true,
-  $getValue_safe: getValueImpl,
-#endif
 };
 
 addToLibrary(LibraryMemOps);

src/runtime_safe_heap.js

@@ -8,91 +8,39 @@
 #error "should only be inclded in SAFE_HEAP mode"
 #endif
 
-/** @param {number|boolean=} isFloat */
-function getSafeHeapType(bytes, isFloat) {
-  switch (bytes) {
-    case 1: return 'i8';
-    case 2: return 'i16';
-    case 4: return isFloat ? 'float' : 'i32';
-    case 8: return isFloat ? 'double' : 'i64';
-    default: abort(`getSafeHeapType() invalid bytes=${bytes}`);
-  }
-}
-
 #if SAFE_HEAP_LOG
 var SAFE_HEAP_COUNTER = 0;
 #endif
 
-/** @param {number|boolean=} isFloat */
-function SAFE_HEAP_STORE(dest, value, bytes, isFloat) {
+function SAFE_HEAP_INDEX(arr, idx, action) {
 #if CAN_ADDRESS_2GB
-  dest >>>= 0;
+  idx >>>= 0;
 #endif
+  const bytes = arr.BYTES_PER_ELEMENT;
+  const dest = idx * bytes;
 #if SAFE_HEAP_LOG
-  dbg('SAFE_HEAP store: ' + [dest, value, bytes, isFloat, SAFE_HEAP_COUNTER++]);
-#endif
-  if (dest <= 0) abort(`segmentation fault storing ${bytes} bytes to address ${dest}`);
-#if SAFE_HEAP == 1
-  if (dest % bytes !== 0) abort(`alignment error storing to address ${dest}, which was expected to be aligned to a multiple of ${bytes}`);
-#else
-  if (dest % bytes !== 0) warnOnce(`alignment error in a memory store operation, alignment was a multiple of ${(((dest ^ (dest-1)) >> 1) + 1)}, but was was expected to be aligned to a multiple of ${bytes}`);
+  dbg(`SAFE_HEAP ${action}: ${[arr.constructor.name, idx, SAFE_HEAP_COUNTER++]}`);
 #endif
+  if (idx <= 0) abort(`segmentation fault ${action} ${bytes} bytes at address ${dest}`);
 #if EXIT_RUNTIME
   if (runtimeInitialized && !runtimeExited) {
 #else
   if (runtimeInitialized) {
 #endif
     var brk = _sbrk(0);
-    if (dest + bytes > brk) abort(`segmentation fault, exceeded the top of the available dynamic heap when storing ${bytes} bytes to address ${dest}. DYNAMICTOP=${brk}`);
+    if (dest + bytes > brk) abort(`segmentation fault, exceeded the top of the available dynamic heap when ${action} ${bytes} bytes at address ${dest}. DYNAMICTOP=${brk}`);
     if (brk < _emscripten_stack_get_base()) abort(`brk >= _emscripten_stack_get_base() (brk=${brk}, _emscripten_stack_get_base()=${_emscripten_stack_get_base()})`); // sbrk-managed memory must be above the stack
     if (brk > wasmMemory.buffer.byteLength) abort(`brk <= wasmMemory.buffer.byteLength (brk=${brk}, wasmMemory.buffer.byteLength=${wasmMemory.buffer.byteLength})`);
   }
-  setValue_safe(dest, value, getSafeHeapType(bytes, isFloat));
-  return value;
-}
-function SAFE_HEAP_STORE_D(dest, value, bytes) {
-  return SAFE_HEAP_STORE(dest, value, bytes, true);
+  return idx;
 }
 
-/** @param {number|boolean=} isFloat */
-function SAFE_HEAP_LOAD(dest, bytes, unsigned, isFloat) {
-#if CAN_ADDRESS_2GB
-  dest >>>= 0;
-#endif
-  if (dest <= 0) abort(`segmentation fault loading ${bytes} bytes from address ${dest}`);
-#if SAFE_HEAP == 1
-  if (dest % bytes !== 0) abort(`alignment error loading from address ${dest}, which was expected to be aligned to a multiple of ${bytes}`);
-#else
-  if (dest % bytes !== 0) warnOnce(`alignment error in a memory load operation, alignment was a multiple of ${(((dest ^ (dest-1)) >> 1) + 1)}, but was was expected to be aligned to a multiple of ${bytes}`);
-#endif
-#if EXIT_RUNTIME
-  if (runtimeInitialized && !runtimeExited) {
-#else
-  if (runtimeInitialized) {
-#endif
-    var brk = _sbrk(0);
-    if (dest + bytes > brk) abort(`segmentation fault, exceeded the top of the available dynamic heap when loading ${bytes} bytes from address ${dest}. DYNAMICTOP=${brk}`);
-    if (brk < _emscripten_stack_get_base()) abort(`brk >= _emscripten_stack_get_base() (brk=${brk}, _emscripten_stack_get_base()=${_emscripten_stack_get_base()})`); // sbrk-managed memory must be above the stack
-    if (brk > wasmMemory.buffer.byteLength) abort(`brk <= wasmMemory.buffer.byteLength (brk=${brk}, wasmMemory.buffer.byteLength=${wasmMemory.buffer.byteLength})`);
-  }
-  var type = getSafeHeapType(bytes, isFloat);
-  var ret = getValue_safe(dest, type);
-  if (unsigned) ret = unSign(ret, parseInt(type.slice(1), 10));
-#if SAFE_HEAP_LOG
-  dbg('SAFE_HEAP load: ' + [dest, ret, bytes, isFloat, unsigned, SAFE_HEAP_COUNTER++]);
-#endif
-  return ret;
-}
-function SAFE_HEAP_LOAD_D(dest, bytes, unsigned) {
-  return SAFE_HEAP_LOAD(dest, bytes, unsigned, true);
+function SAFE_HEAP_LOAD(arr, idx) {
+  return arr[SAFE_HEAP_INDEX(arr, idx, 'loading')];
 }
 
-function SAFE_FT_MASK(value, mask) {
-  var ret = value & mask;
-  if (ret !== value) {
-    abort(`Function table mask error: function pointer is ${value} which is masked by ${mask}, the likely cause of this is that the function pointer is being called by the wrong type.`);
-  }
-  return ret;
+function SAFE_HEAP_STORE(arr, idx, value) {
+  return arr[SAFE_HEAP_INDEX(arr, idx, 'storing')] = value;
 }
 
 function segfault() {

test/js_optimizer/safeHeap-output.js

@@ -1,65 +1,57 @@
-SAFE_HEAP_STORE(x, 1, 1);
+SAFE_HEAP_STORE(HEAP8, x, 1);
 
-SAFE_HEAP_STORE(x * 2, 2, 2);
+SAFE_HEAP_STORE(HEAP16, x, 2);
 
-SAFE_HEAP_STORE(x * 4, 3, 4);
+SAFE_HEAP_STORE(HEAP32, x, 3);
 
-SAFE_HEAP_STORE(x, 4, 1);
+SAFE_HEAP_STORE(HEAPU8, x, 4);
 
-SAFE_HEAP_STORE(x * 2, 5, 2);
+SAFE_HEAP_STORE(HEAPU16, x, 5);
 
-SAFE_HEAP_STORE(x * 4, 6, 4);
+SAFE_HEAP_STORE(HEAPU32, x, 6);
 
-SAFE_HEAP_STORE_D(x * 4, 7, 4);
+SAFE_HEAP_STORE(HEAPF32, x, 7);
 
-SAFE_HEAP_STORE_D(x * 8, 8, 8);
+SAFE_HEAP_STORE(HEAPF64, x, 8);
 
-SAFE_HEAP_STORE(x * 8, 9n, 8);
+SAFE_HEAP_STORE(HEAP64, x, 9n);
 
-SAFE_HEAP_STORE(x * 8, 10n, 8);
+SAFE_HEAP_STORE(HEAPU64, x, 10n);
 
-a1 = SAFE_HEAP_LOAD(x, 1, 0);
+a1 = SAFE_HEAP_LOAD(HEAP8, x);
 
-a2 = SAFE_HEAP_LOAD(x * 2, 2, 0);
+a2 = SAFE_HEAP_LOAD(HEAP16, x);
 
-a3 = SAFE_HEAP_LOAD(x * 4, 4, 0);
+a3 = SAFE_HEAP_LOAD(HEAP32, x);
 
-a4 = SAFE_HEAP_LOAD(x, 1, 1);
+a4 = SAFE_HEAP_LOAD(HEAPU8, x);
 
-a5 = SAFE_HEAP_LOAD(x * 2, 2, 1);
+a5 = SAFE_HEAP_LOAD(HEAPU16, x);
 
-a6 = SAFE_HEAP_LOAD(x * 4, 4, 1);
+a6 = SAFE_HEAP_LOAD(HEAPU32, x);
 
-a7 = SAFE_HEAP_LOAD_D(x * 4, 4, 0);
+a7 = SAFE_HEAP_LOAD(HEAPF32, x);
 
-a8 = SAFE_HEAP_LOAD_D(x * 8, 8, 0);
+a8 = SAFE_HEAP_LOAD(HEAPF64, x);
 
-a9 = SAFE_HEAP_LOAD(x * 8, 8, 0);
+a9 = SAFE_HEAP_LOAD(HEAP64, x);
 
-a10 = SAFE_HEAP_LOAD(x * 8, 8, 1);
+a10 = SAFE_HEAP_LOAD(HEAPU64, x);
 
-foo = SAFE_HEAP_STORE(1337, 42, 1);
+foo = SAFE_HEAP_STORE(HEAPU8, 1337, 42);
 
-SAFE_HEAP_LOAD(bar(SAFE_HEAP_LOAD_D(5 * 8, 8, 0)) * 2, 2, 0);
+SAFE_HEAP_LOAD(HEAP16, bar(SAFE_HEAP_LOAD(HEAPF64, 5)));
 
-SAFE_HEAP_STORE_D(x * 4, SAFE_HEAP_LOAD(y * 4, 4, 0), 4);
+SAFE_HEAP_STORE(HEAPF32, x, SAFE_HEAP_LOAD(HEAP32, y));
 
 function SAFE_HEAP_FOO(ptr) {
   return HEAP8[ptr];
 }
 
-function setValue_safe(ptr) {
-  return HEAP8[ptr];
-}
-
-function getValue_safe(ptr) {
-  return HEAP8[ptr];
-}
-
 function somethingElse() {
-  return SAFE_HEAP_LOAD(ptr, 1, 0);
+  return SAFE_HEAP_LOAD(HEAP8, ptr);
 }
 
 HEAP8.length;
 
-SAFE_HEAP_LOAD(length, 1, 0);
+SAFE_HEAP_LOAD(HEAP8, length);

test/js_optimizer/safeHeap.js

@@ -40,12 +40,6 @@ HEAPF32[x] = HEAP32[y];
 function SAFE_HEAP_FOO(ptr) {
   return HEAP8[ptr];
 }
-function setValue_safe(ptr) {
-  return HEAP8[ptr];
-}
-function getValue_safe(ptr) {
-  return HEAP8[ptr];
-}
 
 // but do handle everything else
 function somethingElse() {

test/test_core.py

@@ -7495,8 +7495,15 @@ def test_embind_dynamic_initialization(self):
     self.do_run_in_out_file_test('embind/test_dynamic_initialization.cpp')
 
   @no_wasm2js('wasm_bigint')
-  def test_embind_i64_val(self):
+  @parameterized({
+    '': (False,),
+    'safe_heap': (True,),
+  })
+  def test_embind_i64_val(self, safe_heap):
     self.set_setting('WASM_BIGINT')
+    if safe_heap and '-fsanitize=address' in self.emcc_args:
+      self.skipTest('asan does not work with SAFE_HEAP')
+    self.set_setting('SAFE_HEAP', safe_heap)
     self.emcc_args += ['-lembind']
     self.node_args += shared.node_bigint_flags(self.get_nodejs())
     self.do_run_in_out_file_test('embind/test_i64_val.cpp', assert_identical=True)
@@ -8967,7 +8974,7 @@ def test_asan_modularized_with_closure(self):
   def test_safe_heap_user_js(self):
     self.set_setting('SAFE_HEAP')
     self.do_runf('core/test_safe_heap_user_js.c',
-                 expected_output=['Aborted(segmentation fault storing 1 bytes to address 0)'], assert_returncode=NON_ZERO)
+                 expected_output=['Aborted(segmentation fault storing 1 bytes at address 0)'], assert_returncode=NON_ZERO)
 
   def test_safe_stack(self):
     self.set_setting('STACK_OVERFLOW_CHECK', 2)

test/test_other.py

@@ -12050,7 +12050,7 @@ def test_safe_heap_2(self):
   def test_safe_heap_log(self):
     self.set_setting('SAFE_HEAP')
     self.set_setting('SAFE_HEAP_LOG')
-    self.do_runf('hello_world.c', 'SAFE_HEAP load: ')
+    self.do_runf('hello_world.c', 'SAFE_HEAP loading: ')
 
   def test_mini_printfs(self):
     def test(code):