Improve sizeof in Emscripten CMake implementation #18278
Comments
|
Ah yes, I was looking into this the other day myself. The fix I came up with was to write the size to stdout rather than returning it. I did think about trying to do the binary scanning thing, but since we still have two different output files (one JS and one wasm) I think we would still need to maintain our own custom version of this utility. |
|
Could we maybe just emit memory into a separate file during CMake checks? (IIUC, Emscripten has an option for that) Then we could do binary scanning consistently for both output formats. |
|
I believe scanning the wasm would work fine, no need to a separate memory file. The problem is the the upstream cmake test assumes the direct output file is that one to scan.. so I don't see how we could forking the cmake plugin (which I was hoping to do). |
|
Yeah, I think a small change will be still necessary then. stdout check sounds good too, I just thought that doing binary scanning might be more future-proof and make it easier to revert to upstream CMake support if we ever drop JS output. Or maybe we could leave custom CMake overrides for asm.js only at some point. Maybe it would be possible to contribute the stdout check upstream? Unless they want it to work for cross-compiling too when you can't run the output binary... |
|
Agreed, binary scanning of the .wasm is probably the best bet.. we might even be able to get away with simply injecting |
|
Hm, if there are no sizeof differences between the two targets, sure, that sounds great too. |
Update the CheckTypeSize files to v3.20.0 (mostly arbitrary version). Local patch to CheckTypeSize is not much smaller. Instead of executing the program and using the return value (doesn't work for return values greater than 255), we simply inject `-oformat=wasm` onto the link command, and then we can use `strings` on the binary just like the upstream check. This is basically a single line patch against upstream now. Fixes: #18278 #18238 #17268 #18084 #17811
Update the CheckTypeSize files to v3.10.2 (mostly arbitrary version, but this is what we use in CI). Local patch to CheckTypeSize is not much smaller. Instead of executing the program and using the return value (doesn't work for return values greater than 255), we simply inject `-oformat=wasm` onto the link command, and then we can use `strings` on the binary just like the upstream check. This is basically a single line patch against upstream now. Fixes: #18278 #18238 #17268 #18084 #17811
Update the CheckTypeSize files to v3.10.2 (mostly arbitrary version, but this is what we use in CI). Local patch to CheckTypeSize is not much smaller. Instead of executing the program and using the return value (doesn't work for return values greater than 255), we simply inject `-oformat=wasm` onto the link command, and then we can use `strings` on the binary just like the upstream check. This is basically a single line patch against upstream now. Fixes: #18278 #18238 #17268 #18084 #17811
Update the CheckTypeSize files to v3.10.2 (mostly arbitrary version, but this is what we use in CI). Local patch to CheckTypeSize is not much smaller. Instead of executing the program and using the return value (doesn't work for return values greater than 255), we simply inject `-oformat=wasm` onto the link command, and then we can use `strings` on the binary just like the upstream check. This is basically a single line patch against upstream now. This change also avoids a second issue which is that node failures (non-zero return codes) are indistinguishable from non-zero return codes from user code. Fixes: #18278 #18238 #17268 #18084 #17811
Currently the CMake
check_type_sizecheck in Emscripten is implemented by returning the size as an exit code. This is problematic when Node.js itself throws an error and exits with an exit code - most commonly 7 - and that value ends up propagated to actual C code, which can lead to some rather surprising and hard to debug behaviour. It also doesn't work for sizes > 255 bytes.This leads to issues like #18238, #17268, #18084, #17811 and others.
This check was implemented back in #3447 because the upstream implementation - that instead relies on
stringsextraction from the binary - can't work on asm.js files. Now that we have Wasm as a default, perhaps the easy fix would be to at least use the upstream implementation for Wasm files, and something custom for asm.js to at least cover most common use-cases.The text was updated successfully, but these errors were encountered: