Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates emmalloc to support 64-bit architectures (wasm64) by transitioning size fields and constants to use size_t, fixing potential overflow issues in allocation size checks, and adding comprehensive tests for large allocations. The feedback suggests addressing a potential signed/unsigned comparison warning in compute_free_list_bucket by explicitly using an unsigned literal for the bitwise shift.
1d47865 to
a73a2d2
Compare
|
@juj, WDTY? The solution here was mostly vibe coded but looks reasonable to me. |
187b09c to
b36b43c
Compare
| ? 110 - (clz<<2) + ((allocSize >> (29-clz)) ^ 4) | ||
| : MIN( 71 - (clz<<1) + ((allocSize >> (30-clz)) ^ 2), NUM_FREE_BUCKETS-1); | ||
| ? 110 - (clz<<2) + ((allocSize >> (29-clz)) ^ 4) | ||
| : 71 - (clz<<1) + ((allocSize >> (30-clz)) ^ 2); |
There was a problem hiding this comment.
This looks like code size neutral change, lgtm. (There might be a way to squeeze out the if() with a bit trick, but might not make much difference)
b36b43c to
1fdc655
Compare
|
ping @juj, I think all your comments are addressed now. |
In `system/lib/emmalloc.c`: - Define `FREE_REGION_FLAG` as `((size_t)1)` instead of `0x1u` so that `~FREE_REGION_FLAG` does not clear the upper 32 bits of 64-bit region sizes when decoding ceiling sizes or merging free regions. - Increase `MAX_ALLOC_SIZE` on `wasm64` to `0xFFFFFFFFFFull` (~1 TiB) rather than a 32-bit constant, allowing allocations >= 4 GiB. - Cast `alignment` to `uintptr_t` in `ALIGN_UP`, `ALIGN_DOWN`, and `HAS_ALIGNMENT` so `~((uintptr_t)(alignment)-1)` is 64-bit clean. - Return `NUM_FREE_BUCKETS - 1` directly for sizes >= 128 MiB in `compute_free_list_bucket` (and clamp sizes >= 2 GiB in `emmalloc_compute_free_dynamic_memory_fragmentation_map`) to avoid 32-bit truncation in `__builtin_clz`. Fixes: emscripten-core#28028
1fdc655 to
c1ae2ea
Compare
In
system/lib/emmalloc.c:FREE_REGION_FLAGas((size_t)1)instead of0x1uso that~FREE_REGION_FLAGdoes not clear the upper 32 bits of 64-bit regionsizes when decoding ceiling sizes or merging free regions.
MAX_ALLOC_SIZEonwasm64to0xFFFFFFFFFFull(~1 TiB)rather than a 32-bit constant, allowing allocations >= 4 GiB.
alignmenttouintptr_tinALIGN_UP,ALIGN_DOWN, andHAS_ALIGNMENTso~((uintptr_t)(alignment)-1)is 64-bit clean.NUM_FREE_BUCKETS - 1directly for sizes >= 128 MiB incompute_free_list_bucket(and clamp sizes >= 2 GiB inemmalloc_compute_free_dynamic_memory_fragmentation_map) to avoid32-bit truncation in
__builtin_clz.Fixes: #28028