Clamp maximum memory if set during run_embind_gen#26357
Conversation
|
First time contributing here. Let me know if I need to change anything. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #26311 where compilation with --emit-tsd, MEMORY64=1, USE_PTHREADS=1, and MAXIMUM_MEMORY over 4GB fails during TypeScript definition generation. The root cause is that when generating TypeScript bindings, the code needs to run the compiled WebAssembly in Node.js, which doesn't fully support memory64 yet. The build system lowers memory64 to 32-bit addressing for this step, but when MAXIMUM_MEMORY is greater than 4GB, it exceeds the 32-bit limit (65536 pages × 65536 bytes/page = 4GB), causing a RangeError.
Changes:
- Clamps MAXIMUM_MEMORY to 4GB (2^32 bytes) during TypeScript binding generation when MEMORY64 is enabled
- Adds a test case that reproduces the exact scenario from the bug report
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/link.py | Adds logic to clamp MAXIMUM_MEMORY to 4GB during embind_gen when MEMORY64 is enabled and lowered to 32-bit |
| test/test_other.py | Adds test case for wasm64 with MAXIMUM_MEMORY over 4GB to verify the fix works |
Comments suppressed due to low confidence (6)
test/test_other.py:3739
- There is trailing whitespace at the end of this line. This should be removed to maintain code cleanliness.
'-lembind', '--emit-tsd', 'embind_tsgen_wasm64.d.ts',
test/test_other.py:3740
- There is trailing whitespace at the end of this line. This should be removed to maintain code cleanliness.
'-Wno-pthreads-mem-growth',
test/test_other.py:3742
- There is trailing whitespace at the end of this line. This should be removed to maintain code cleanliness.
'-sALLOW_MEMORY_GROWTH=1',
test/test_other.py:3743
- There is trailing whitespace at the end of this line. This should be removed to maintain code cleanliness.
'-sMAXIMUM_MEMORY=16GB',
test/test_other.py:3744
- There is trailing whitespace at the end of this line. This should be removed to maintain code cleanliness.
'-sMEMORY64'] +
tools/link.py:2028
- Consider adding a brief comment explaining why MAXIMUM_MEMORY is clamped to 4GB. While the comment above explains why MEMORY64 is lowered, it would be clearer to explicitly state that the memory maximum must also be limited to the 32-bit address space (4GB) when lowering is applied. Something like: "When lowering memory64 to 32-bit, clamp MAXIMUM_MEMORY to 4GB limit."
if settings.MAXIMUM_MEMORY and settings.MAXIMUM_MEMORY > 2 ** 32:
settings.MAXIMUM_MEMORY = 2 ** 32
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Maybe for the exact clamp value we should use something like |
Should fix #26311 in the manner suggested by @sbc100.