Skip to content
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

emscripten: Use the latest emsdk 3.1.68 #131533

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

workingjubilee
Copy link
Member

This should fix our precompiled std being unsound in std::fs code.

Should resolve #131467 I hope.

This should fix our precompiled std being unsound in `std::fs` code.
@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 11, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Oct 11, 2024

I have to say that I'm often clueless when reviewing these kinds of PRs :) Do we have some documentation about what does it mean to update this? Do we even document somewhere which version of emscripten we use for given targets, or is it just an "internal implementation detail".

@workingjubilee
Copy link
Member Author

workingjubilee commented Oct 11, 2024

I have to say that I'm often clueless when reviewing these kinds of PRs :) Do we have some documentation about what does it mean to update this? Do we even document somewhere which version of emscripten we use for given targets, or is it just an "internal implementation detail".

We do not, but we should, probably. I can add the relevant details for our platform support pages?

I think since we build the SDK, it shouldn't have a significant consequence? (aside from the bugfix, of course)

I am not entirely sure what the consequences of this are, tbh. I'm also not entirely sure this won't break Pyodide for @juntyr, either, given 3.1.68 is not 3.1.46, but I'm not sure what to make of emsdk's versioning. It could be SemVer, in which case this should be fine, but then why did 3.1.42 break ABI?

Perhaps @alexcrichton or @Manishearth might know?

@juntyr
Copy link
Contributor

juntyr commented Oct 11, 2024

Updating the Emscripten version to something newer is definitely a good idea. But it should go hand-in-hand with some documentation (I'll submit a PR later) on this coupling between Emscripten and Rust that won't matter in most cases until you get a weird bug, and how to work around it with -Zbuild-std.

I think I was experiencing this issue for a long while but simply never noticed. Pyodide can use ~2GB RAM before anything breaks, and up until a few days ago even the senseless overallocation during file reading never hit the boundary. So it was by pure chance and additional up-front memory pressure that allocating an extra 1.8GB was no longer fine and I noticed the ABI mismatch.

kleisauke added a commit to kleisauke/libc that referenced this pull request Oct 11, 2024
And make the changes in commit 63b0d67 unconditional.

Supersedes: rust-lang#3569.
Cherry-picks a couple of changes from: rust-lang#3307.
Depends on: rust-lang/rust#131533.
@hoodmane
Copy link
Contributor

Thanks for helping to support the Emscripten target @workingjubilee! I really appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emscripten metadata file size is wrong
5 participants