New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change size_t and friends to use long types.
#5916
Conversation
|
I'm neutral on this myself. If we do it carefully I don't object. Main risk is people with existing compiled bitcode, that suddenly wouldn't link with other bitcode, I suspect. |
|
I think this is the right thing to do in general. As for how to help people more specifically than that, I don't really have any ideas I would consider "good" :). There are probably several ways you could wind up using mangled names; CLI export lists and JS code that just does |
|
Yeah, I'm really just worried about people I know use bitcode libraries for third party code, like some game engine projects. They don't rebuild those constantly, so this might need a manual update step for them. But if we talk to them that should be fine. @juj knows the details there. |
|
This is a great change, since it will unify behavior with native Linux builds as well. In order to do this, we do want to bump version from |
|
We don't embed the emscripten version in bitcode - although maybe we could use metadata for that somehow? Not sure it's worth the overhead. I agree this warrants a bump to |
6fba603
to
e3a3e65
Compare
|
The LLVM patch, the emscripten-fastcomp-clang patch, and the emscripten patch are now all updated and in sync. |
|
Do we have a plan for landing this? From the discussion above, it sounds like we should merge to master first. Also we should probably wait on @juj's impending LLVM merge. |
The LLVM merge happened and was tagged. I think we should work to merge to master next, and then to merge this and bump to 1.38.0. |
015b27b
to
6d9e138
Compare
|
I've now rebased this against incoming. The CI tests are failing because this PR depends on the accompanying emscripten-fastcomp and emscripten-fastcomp-clang PRs, but the tests pass for me locally when I put all of them together. For reference, the corresponding LLVM patch for the WebAssembly target is here. Is there anything else that needs doing here? |
|
Just to be sure, which tests did you run? If you ran With their landing, please also push a |
|
Ah, it seems I was only running asm.js tests. When I run a binaryen test, I get lots of errors like this: The key error seems to be |
|
It looks like that stack trace, inside the JS compiler, is coming from SpiderMonkey. That's probably not good - the only VM we test the compiler on is node.js. And indeed However, this makes sense as a theory up to the point where you say asm.js tests pass but wasm ones do not - that global-using line of code should be hit in both cases, and the same VM used for the compiler as well...? |
|
When I switched to wasm, I got errors suggesting the node.js on my system doesn't support wasm. I have a spidermonkey shell build handy, so I thought that'd be the easiest thing to use instead. But I can get a newer node.js. |
|
I'm using a newer version of node now, which helps, but I'm still seeing some failures. It's not clear that they're due to my patch though; I just did a build from the incoming branches, without my patch, and I get the same failures. Here's an example: Is this expected? |
|
It's probably ok to ignore those noderawfs tests for purposes of these PRs. But please file a separate bug, with your node version etc. - likely you are hitting a noderawfs issue that is OS or node version dependent. |
This, along with an accompanying emscripten-fastcomp change, change size_t from
unsigned inttounsigned long, and related changes. The size of size_t is unchanged, in all configurations. As discussed in the proposed LLVM patch, the motivation for this change is to synchronize the asm.js and wasm ABIs, and to useunsigned longforsize_ton wasm to make symbol names more consistent between wasm32 and wasm64.The most visible effect of this is a change to C++ mangled names using size_t. size_t is now mangled to 'm' rather than 'j'. While mangled names aren't visible from within C++, this may affect tools which depend on mangled names.
Another visible effect is that code passing size_t values to printf with "%u", "%x", or similar now get a warning. The warning may be safely ignored, as there is no change in behavior. It can also be fixed by adding the 'z' modifier, such as "%zu", "%zx", and so on.
I looked into the possibility of adding warnings to help users who might be affected by this, but I didn't find any good place to do this. I'm open to suggestions here.