Fix emmalloc_trim() to work again (#25126)

Found problems:

1. The convention for sbrk() changed at some point to require 16b
alignment. (`__alignof__(max_align_t)`) This can create gaps in the
alignment of emmalloc heap end sentinel region and the previous free
region. This must require changing the assumption in emmalloc that heap
end sentinel region would always be fixed 16b -> allow it to be
arbitrary sized (in practice 16-31b).
2. sbrk() implementation had round-up to nearest max_align_t, but didn't
handle negative sbrk() values.
3. in 4GB mode, a 32-bit sbrk() is not enough to accommodate support for
both sbrk() increases and decreases (unless we take a convention that
sbrk() can only be increased by < 2GB amount.. which probably won't hold
in use cases that want to claim the whole heap immediately). Add a
function sbrk64() that can be used in 4GB builds to be able to express
both memory increases and decresases.
4. test case expectations have changed at some point. test_emmalloc_trim
was written with an assumption that emmalloc would acquire the whole
heap with sbrk() at startup.. but no longer does so. Adjusted test to
not assume this.
5. printing bare memory values in test doesn't work well in e.g. ubsan
test mode, so remove the direct value prints in the test, and just
assert() the expectations.

Fixes: #23343

Author: juj

system/lib/emmalloc.c

@@ -61,6 +61,8 @@
 #include <emscripten/heap.h>
 #include <emscripten/threading.h>
 
+void *_sbrk64(int64_t numBytes);
+
 #ifdef __EMSCRIPTEN_TRACING__
 #include <emscripten/trace.h>
 #endif
@@ -137,6 +139,7 @@ static volatile uint8_t multithreadingLock = 0;
 
 #define IS_POWER_OF_2(val) (((val) & ((val)-1)) == 0)
 #define ALIGN_UP(ptr, alignment) ((uint8_t*)((((uintptr_t)(ptr)) + ((alignment)-1)) & ~((alignment)-1)))
+#define ALIGN_DOWN(ptr, alignment) ((uint8_t*)(((uintptr_t)(ptr)) & ~((alignment)-1)))
 #define HAS_ALIGNMENT(ptr, alignment) ((((uintptr_t)(ptr)) & ((alignment)-1)) == 0)
 
 static_assert(IS_POWER_OF_2(MALLOC_ALIGNMENT), "MALLOC_ALIGNMENT must be a power of two value!");
@@ -467,7 +470,8 @@ static bool claim_more_memory(size_t numBytes) {
   numBytes = (size_t)ALIGN_UP(numBytes, MALLOC_ALIGNMENT);
 
   // Claim memory via sbrk
-  uint8_t *startPtr = (uint8_t*)sbrk(numBytes);
+  assert((int64_t)numBytes >= 0);
+  uint8_t *startPtr = (uint8_t*)_sbrk64((int64_t)numBytes);
   if ((intptr_t)startPtr == -1) {
 #ifdef EMMALLOC_VERBOSE
     MAIN_THREAD_ASYNC_EM_ASM(err('claim_more_memory: sbrk failed!'));
@@ -483,6 +487,9 @@ static bool claim_more_memory(size_t numBytes) {
   // Create a sentinel region at the end of the new heap block
   Region *endSentinelRegion = (Region*)(endPtr - sizeof(Region));
   create_used_region(endSentinelRegion, sizeof(Region));
+#ifdef EMMALLOC_VERBOSE
+  MAIN_THREAD_ASYNC_EM_ASM(out('claim_more_memory: created a sentinel memory region at address ' + ptrToString($0)), endSentinelRegion);
+#endif
 
   // If we are the sole user of sbrk(), it will feed us continuous/consecutive memory addresses - take advantage
   // of that if so: instead of creating two disjoint memory regions blocks, expand the previous one to a larger size.
@@ -1165,7 +1172,7 @@ struct mallinfo emmalloc_mallinfo() {
   struct mallinfo info;
   // Non-mmapped space allocated (bytes): For emmalloc,
   // let's define this as the difference between heap size and dynamic top end.
-  info.arena = emscripten_get_heap_size() - (size_t)sbrk(0);
+  info.arena = emscripten_get_heap_size() - (size_t)_sbrk64(0);
   // Number of "ordinary" blocks. Let's define this as the number of highest
   // size blocks. (subtract one from each, since there is a sentinel node in each list)
   info.ordblks = count_linked_list_size(&freeRegionBuckets[NUM_FREE_BUCKETS-1])-1;
@@ -1227,73 +1234,118 @@ struct mallinfo emmalloc_mallinfo() {
 }
 EMMALLOC_ALIAS(mallinfo, emmalloc_mallinfo);
 
-#if 0
 // Note! This function is not fully multithreading safe: while this function is running, other threads should not be
 // allowed to call sbrk()!
 static int trim_dynamic_heap_reservation(size_t pad) {
   ASSERT_MALLOC_IS_ACQUIRED();
 
   if (!listOfAllRegions) {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): cannot trim memory, emmalloc is currently not initialized to manage any dynamic memory at all.'));
+#endif
     return 0; // emmalloc is not controlling any dynamic memory at all - cannot release memory.
   }
   uint8_t *previousSbrkEndAddress = listOfAllRegions->endPtr;
-  assert(sbrk(0) == previousSbrkEndAddress);
+  void *sbrkAddr = _sbrk64(0);
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): _sbrk64(0) = ' + ptrToString($0) + ', previousSbrkEndAddress = ' + ptrToString($1)), sbrkAddr, previousSbrkEndAddress);
+#endif
+  assert(sbrkAddr == previousSbrkEndAddress);
   size_t lastMemoryRegionSize = ((size_t*)previousSbrkEndAddress)[-1];
-  assert(lastMemoryRegionSize == 16); // // The last memory region should be a sentinel node of exactly 16 bytes in size.
-  Region *endSentinelRegion = (Region*)(previousSbrkEndAddress - sizeof(Region));
+  Region *endSentinelRegion = (Region*)(previousSbrkEndAddress - lastMemoryRegionSize);
   Region *lastActualRegion = prev_region(endSentinelRegion);
 
-  // Round padding up to multiple of 4 bytes to keep sbrk() and memory region alignment intact.
-  // Also have at least 8 bytes of payload so that we can form a full free region.
-  size_t newRegionSize = (size_t)ALIGN_UP(pad, 4);
-  if (pad > 0) {
-    newRegionSize += sizeof(Region) - (newRegionSize - pad);
+  if (!region_is_free(lastActualRegion)) {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): Last actual region ' + ptrToString($0) + ' is in use, there is nothing to trim from.'), lastActualRegion);
+#endif
+    return 0;
   }
 
-  if (!region_is_free(lastActualRegion) || lastActualRegion->size <= newRegionSize) {
-    return 0; // Last actual region is in use, or caller desired to leave more free memory intact than there is.
+  // Sanitize odd alignments for padding values - this is the minimum alignment
+  // that emmalloc could handle. Align up to be conservative towards caller.
+  pad = (size_t)ALIGN_UP(pad, 4);
+
+  // Calculate how many bytes we can shrink the sbrk() reservation by.
+  // Is the last free region smaller than what was requested to be left behind?
+  // If so, then there is nothing we can trim.
+  if (pad >= lastActualRegion->size) {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): Last actual region does not have enough space to leave ' + Number($0) + ' bytes of free memory in it.'), pad);
+#endif
+    return 0;
   }
 
-  // This many bytes will be shrunk away.
-  size_t shrinkAmount = lastActualRegion->size - newRegionSize;
-  assert(HAS_ALIGNMENT(shrinkAmount, 4));
+  // Subtract region size members off to calculate the excess bytes in payload.
+  size_t shrinkAmount = lastActualRegion->size - pad - 2*sizeof(size_t);
+  // sbrk() alignment is multiple of __alignof__(max_align_t), so round the
+  // trimming down to ensure that alignment is preserved.
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): shrinkAmount ' + Number($0) + '.'), shrinkAmount);
+#endif
+  shrinkAmount = (size_t)ALIGN_DOWN(shrinkAmount, __alignof__(max_align_t));
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): shrinkAmount2 ' + Number($0) + '.'), shrinkAmount);
+#endif
+  // Nothing left to trim?
+  if (!shrinkAmount) {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): Aligning for sbrk() requirements removed opportunity to trim.'));
+#endif
+    return 0;
+  }
 
   unlink_from_free_list(lastActualRegion);
-  // If pad == 0, we should delete the last free region altogether. If pad > 0,
-  // shrink the last free region to the desired size.
-  if (newRegionSize > 0) {
+
+  size_t newRegionSize = lastActualRegion->size - shrinkAmount;
+
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): Shrinking ' + Number($0) + ' bytes off the free heap end region. New free heap end region size: ' + Number($1) + ' bytes.'), shrinkAmount, newRegionSize);
+#endif
+  // If we can't fit a free Region in the shrunk space, we should delete the 
+  // the last free region altogether.
+  if (newRegionSize >= sizeof(Region)) {
     create_free_region(lastActualRegion, newRegionSize);
     link_to_free_list(lastActualRegion);
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): Created new free heap end region at ' + ptrToString($0) + '. Size: ' + Number($1) + ' bytes.'), lastActualRegion, newRegionSize);
+#endif
+  } else {
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): Not enough room to fit a free heap end region. Discarding it altogether.'));
+#endif
+    newRegionSize = 0;
   }
 
-  // Recreate the sentinel region at the end of the last free region
-  endSentinelRegion = (Region*)((uint8_t*)lastActualRegion + newRegionSize);
-  create_used_region(endSentinelRegion, sizeof(Region));
+  // Call sbrk() to shrink the memory area.
+  void *oldSbrk = _sbrk64(-(int64_t)shrinkAmount);
+  assert((intptr_t)oldSbrk != -1); // Shrinking with sbrk() should never fail.
 
-  // And update the size field of the whole region block.
-  listOfAllRegions->endPtr = (uint8_t*)endSentinelRegion + sizeof(Region);
+  // Ask where sbrk got us at.
+  void *sbrkNow = _sbrk64(0);
 
-  // Finally call sbrk() to shrink the memory area.
-  void *oldSbrk = sbrk(-(intptr_t)shrinkAmount);
-  assert((intptr_t)oldSbrk != -1); // Shrinking with sbrk() should never fail.
-  assert(oldSbrk == previousSbrkEndAddress); // Another thread should not have raced to increase sbrk() on us!
+  // Recreate the sentinel region at the end of the last free region.
+  Region *newEndSentinelRegion = (Region*)((uint8_t*)lastActualRegion + newRegionSize);
+  size_t newEndSentinelRegionSize = (uintptr_t)sbrkNow - (uintptr_t)newEndSentinelRegion;
+
+#ifdef EMMALLOC_VERBOSE
+    MAIN_THREAD_ASYNC_EM_ASM(out('emmalloc_trim(): Created new sentinel end region at ' + ptrToString($0) + '. Size: ' + Number($1) + ' bytes.'), newEndSentinelRegion, newEndSentinelRegionSize);
+#endif
+
+  create_used_region(newEndSentinelRegion, newEndSentinelRegionSize);
+
+  // And update the size field of the whole region block.
+  listOfAllRegions->endPtr = (uint8_t*)newEndSentinelRegion + newEndSentinelRegionSize;
 
   // All successful, and we actually trimmed memory!
   return 1;
 }
-#endif
 
 int emmalloc_trim(size_t pad) {
-  // Reducing the size of the sbrk region is currently broken.
-  // See https://github.com/emscripten-core/emscripten/issues/23343
-  // And https://github.com/emscripten-core/emscripten/pull/13442
-  return 0;
-  /*
   MALLOC_ACQUIRE();
   int success = trim_dynamic_heap_reservation(pad);
   MALLOC_RELEASE();
   return success;
-  */
 }
 EMMALLOC_ALIAS(malloc_trim, emmalloc_trim)
 
@@ -1376,5 +1428,5 @@ void emmalloc_dump_free_dynamic_memory_fragmentation_map() {
 }
 
 size_t emmalloc_unclaimed_heap_memory(void) {
-  return emscripten_get_heap_max() - (size_t)sbrk(0);
+  return emscripten_get_heap_max() - (size_t)_sbrk64(0);
 }

system/lib/libc/sbrk.c

@@ -56,20 +56,25 @@ uintptr_t* emscripten_get_sbrk_ptr() {
 #define READ_SBRK_PTR(sbrk_ptr) (*(sbrk_ptr))
 #endif
 
-void *sbrk(intptr_t increment_) {
-  uintptr_t increment = (uintptr_t)increment_;
-  increment = (increment + (SBRK_ALIGNMENT-1)) & ~(SBRK_ALIGNMENT-1);
+void *_sbrk64(int64_t increment) {
+  if (increment >= 0) {
+    increment = (increment + (SBRK_ALIGNMENT-1)) & ~((int64_t)SBRK_ALIGNMENT-1);
+  } else {
+    increment = -(-increment & ~((int64_t)SBRK_ALIGNMENT-1));
+  }
+
   uintptr_t *sbrk_ptr = (uintptr_t*)emscripten_get_sbrk_ptr();
 
   // To make sbrk thread-safe, implement a CAS loop to update the
   // value of sbrk_ptr.
   while (1) {
     uintptr_t old_brk = READ_SBRK_PTR(sbrk_ptr);
-    uintptr_t new_brk = old_brk + increment;
-    // Check for a) an overflow, which would indicate that we are trying to
-    // allocate over maximum addressable memory. and b) if necessary,
+    int64_t new_brk64 = (int64_t)old_brk + increment;
+    uintptr_t new_brk = (uintptr_t)new_brk64;
+    // Check for a) an over/underflow, which would indicate that we are
+    // allocating over maximum addressable memory. and b) if necessary,
     // increase the WebAssembly Memory size, and abort if that fails.
-    if ((increment > 0 && new_brk <= old_brk)
+    if (new_brk < 0 || new_brk64 != (int64_t)new_brk
      || (new_brk > emscripten_get_heap_size() && !emscripten_resize_heap(new_brk))) {
       errno = ENOMEM;
       return (void*)-1;
@@ -93,6 +98,26 @@ void *sbrk(intptr_t increment_) {
   }
 }
 
+void *sbrk(intptr_t increment_) {
+#if defined(__wasm64__) // TODO || !defined(wasm2gb)
+  // In the correct https://linux.die.net/man/2/sbrk spec, sbrk() parameter is
+  // intended to be treated as signed, meaning that it is not possible in a
+  // 32-bit program to sbrk alloc (or dealloc) more than 2GB of memory at once.
+
+  // Treat sbrk() parameter as signed.
+  return _sbrk64((int64_t)increment_);
+#else
+  // BUG: Currently the Emscripten test suite codifies expectations that sbrk()
+  // values passed to this function are to be treated as unsigned, which means
+  // that in 2GB and 4GB build modes, it is not possible to shrink memory.
+  // To satisfy that mode, treat sbrk() parameters in 32-bit builds as unsigned.
+  // https://github.com/emscripten-core/emscripten/issues/25138
+
+  // Treat sbrk() parameter as unsigned.
+  return _sbrk64((int64_t)(uintptr_t)increment_);
+#endif
+}
+
 int brk(void* ptr) {
 #ifdef __EMSCRIPTEN_SHARED_MEMORY__
   // FIXME

test/code_size/test_codesize_cxx_ctors1.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 19754,
   "a.out.js.gz": 8162,
-  "a.out.nodebug.wasm": 129504,
-  "a.out.nodebug.wasm.gz": 49232,
-  "total": 149258,
-  "total_gz": 57394,
+  "a.out.nodebug.wasm": 129517,
+  "a.out.nodebug.wasm.gz": 49253,
+  "total": 149271,
+  "total_gz": 57415,
   "sent": [
     "__cxa_throw",
     "_abort_js",

test/code_size/test_codesize_cxx_ctors2.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 19732,
   "a.out.js.gz": 8148,
-  "a.out.nodebug.wasm": 128931,
-  "a.out.nodebug.wasm.gz": 48876,
-  "total": 148663,
-  "total_gz": 57024,
+  "a.out.nodebug.wasm": 128944,
+  "a.out.nodebug.wasm.gz": 48891,
+  "total": 148676,
+  "total_gz": 57039,
   "sent": [
     "__cxa_throw",
     "_abort_js",

test/code_size/test_codesize_cxx_except.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 23415,
   "a.out.js.gz": 9145,
-  "a.out.nodebug.wasm": 171266,
-  "a.out.nodebug.wasm.gz": 57323,
-  "total": 194681,
-  "total_gz": 66468,
+  "a.out.nodebug.wasm": 171279,
+  "a.out.nodebug.wasm.gz": 57348,
+  "total": 194694,
+  "total_gz": 66493,
   "sent": [
     "__cxa_begin_catch",
     "__cxa_end_catch",

test/code_size/test_codesize_cxx_except_wasm.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 19643,
   "a.out.js.gz": 8112,
-  "a.out.nodebug.wasm": 144625,
-  "a.out.nodebug.wasm.gz": 54883,
-  "total": 164268,
-  "total_gz": 62995,
+  "a.out.nodebug.wasm": 144638,
+  "a.out.nodebug.wasm.gz": 54898,
+  "total": 164281,
+  "total_gz": 63010,
   "sent": [
     "_abort_js",
     "_tzset_js",

test/code_size/test_codesize_cxx_except_wasm_legacy.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 19643,
   "a.out.js.gz": 8112,
-  "a.out.nodebug.wasm": 142214,
-  "a.out.nodebug.wasm.gz": 54349,
-  "total": 161857,
-  "total_gz": 62461,
+  "a.out.nodebug.wasm": 142227,
+  "a.out.nodebug.wasm.gz": 54362,
+  "total": 161870,
+  "total_gz": 62474,
   "sent": [
     "_abort_js",
     "_tzset_js",

test/code_size/test_codesize_cxx_mangle.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 23465,
   "a.out.js.gz": 9164,
-  "a.out.nodebug.wasm": 235307,
-  "a.out.nodebug.wasm.gz": 78924,
-  "total": 258772,
-  "total_gz": 88088,
+  "a.out.nodebug.wasm": 235320,
+  "a.out.nodebug.wasm.gz": 78938,
+  "total": 258785,
+  "total_gz": 88102,
   "sent": [
     "__cxa_begin_catch",
     "__cxa_end_catch",

test/code_size/test_codesize_cxx_noexcept.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 19754,
   "a.out.js.gz": 8162,
-  "a.out.nodebug.wasm": 131921,
-  "a.out.nodebug.wasm.gz": 50229,
-  "total": 151675,
-  "total_gz": 58391,
+  "a.out.nodebug.wasm": 131934,
+  "a.out.nodebug.wasm.gz": 50245,
+  "total": 151688,
+  "total_gz": 58407,
   "sent": [
     "__cxa_throw",
     "_abort_js",

test/code_size/test_codesize_cxx_wasmfs.json

@@ -1,10 +1,10 @@
 {
   "a.out.js": 7143,
   "a.out.js.gz": 3338,
-  "a.out.nodebug.wasm": 169792,
-  "a.out.nodebug.wasm.gz": 63078,
-  "total": 176935,
-  "total_gz": 66416,
+  "a.out.nodebug.wasm": 169805,
+  "a.out.nodebug.wasm.gz": 63096,
+  "total": 176948,
+  "total_gz": 66434,
   "sent": [
     "__cxa_throw",
     "_abort_js",