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

crux-mir: Support cross-compiling libs to wasm32 target #1147

Merged
merged 11 commits into from
Dec 7, 2023
Merged

Conversation

qsctr
Copy link
Contributor

@qsctr qsctr commented Dec 5, 2023

- Refactor translate_libs.sh a bit to get all the components of the
  target triple
- Only pass x86_64 simd flags to memchr when on x86_64
- Add dlmalloc library required for non-emscripten wasm32 std
@qsctr qsctr added crux MIR Issues relating to Rust/MIR support labels Dec 5, 2023
@qsctr qsctr self-assigned this Dec 5, 2023
@qsctr qsctr marked this pull request as draft December 5, 2023 19:42
@qsctr qsctr marked this pull request as ready for review December 5, 2023 19:43
@qsctr qsctr marked this pull request as draft December 5, 2023 19:45
@qsctr
Copy link
Contributor Author

qsctr commented Dec 5, 2023

Hmm it seems like there is a reference to an apparently now-nonexistent flag --no-model-internal-atomics for translate_libs.sh here. I should do something to prevent people from using it and it getting treated as a target triple by the script, but I'm not sure what is the right thing to do (add the flag back to translate_libs.sh? just delete that part of the docs? something else?)

@RyanGlScott
Copy link
Contributor

Bummer, I completely missed that --no-model-internal-atomics flag when I worked on #1096. I doubt that it would be a small amount of work to add back to crux-mir, so I have opened #1148 as a reminder to do this at some point in the future. As such, I don't think we need to block this PR on that front, although it would be worth setting translate_libs.sh up in such a way that we can pass multiple configuration-related arguments to the script. I'm not strongly opinionated on the best way to do this—perhaps via environment variables? getopt? Something else?

Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @qsctr! This mostly looks reasonable, although I would request that you add some additional documentation so that when we next update the rustc toolchain that crux-mir supports, we have some reminders about why dlmalloc needs to be included. (It's easy to overlook this sort of thing otherwise.)

crux-mir/translate_libs.sh Outdated Show resolved Hide resolved
crux-mir/translate_libs.sh Show resolved Hide resolved
crux-mir/translate_libs.sh Show resolved Hide resolved
crux-mir/translate_libs.sh Show resolved Hide resolved
@@ -0,0 +1,43 @@
[package]
name = "dlmalloc"
version = "0.2.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this exactly the same as the upstream version of dlmalloc-0.2.3, or did you have to apply any patches to make it work with mir-json/crux-mir? If you did have to patch it, it would be worth putting a high-level description of the changes in crux-mir/lib/Patches.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is exactly the same, no patches are required.

@qsctr qsctr marked this pull request as ready for review December 7, 2023 02:56
@qsctr
Copy link
Contributor Author

qsctr commented Dec 7, 2023

although I would request that you add some additional documentation so that when we next update the rustc toolchain that crux-mir supports, we have some reminders about why dlmalloc needs to be included.

What's the best place to put this? Is the comment in translate_libs.sh enough? I'm not sure I should put it in lib/Patches.md since there are no patches.

@qsctr
Copy link
Contributor Author

qsctr commented Dec 7, 2023

Also, should we test a wasm32-unknown-unknown build in CI?

@RyanGlScott
Copy link
Contributor

What's the best place to put this? Is the comment in translate_libs.sh enough?

Short answer: Yes, I think that is likely good enough for now.

Long answer: The translate_libs.sh script was originally created by using a Python script that @m10f wrote to scrape the dependencies from the standard libraries' Cargo.toml files and generate the necessary calls to mir-json. After creating the initial version of the script, however, we quickly discovered many manual tweaks that were necessary to address various corner cases, make certain test cases work, etc. It got to the point that the final version of translate_libs.sh was quite different from what the script produced.

Ultimately, we never checked in the generator script, but I kind of wish that we did, since that script would be a good place to document things such as the inclusion of dlmalloc. After all, when we upgrade crux-mir to support a new rustc toolchain, it is quite possible that the dependencies will change slightly, at which point we will have to change (or rewrite) translate_libs.sh to accommodate it. Of course, we should make sure to look at the old version of translate_libs.sh to preserve what it did before, but it is easy to overlook things (as evidenced by #1148). I'd hate to accidentally drop dlmalloc from translate_libs.sh because of such an oversight.

If we had a proper generator script that documented all of these quirks, I think the likelihood of dropping dlmalloc in the future would be greatly reduced. We could also build a cross-compiled version of crux-mir in the CI, which brings me to your other question...

@RyanGlScott
Copy link
Contributor

Also, should we test a wasm32-unknown-unknown build in CI?

Perhaps so, although I wonder how much we should actually do. For instance, have you run the crux-mir test suite using a Wasm version of the Rust standard libraries? Do all the tests pass? If they don't pass, then unless you want to spend the time fixing up all of the test cases, then we wouldn't be able to run everything in CI with a Wasm target triple. In that scenario, we could conceivably do something like making sure that the Rust standard libraries build, but refrain from doing anything else.

@qsctr
Copy link
Contributor Author

qsctr commented Dec 7, 2023

I don't think it's easy to run the test suite with wasm32, since it seems to use the binary output of rustc as an oracle, which would mean we need to run it in a wasm VM. In any case I don't think there is much runtime (rather than compile time) differences in behavior between targets that might be caused by some bug in crux-mir, so I was more thinking of just making sure TARGET=wasm32-unknown-unknown ./translate_libs.sh runs successfully.

@RyanGlScott
Copy link
Contributor

OK. In that case, I agree that a simple CI action that simply runs TARGET=wasm32-unknown-unknown ./translate_libs.sh would be appropriate. Would you be willing to open an issue as a a reminder to do this?

@qsctr qsctr merged commit 1c9979d into master Dec 7, 2023
31 checks passed
@qsctr qsctr deleted the soroban branch December 7, 2023 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crux MIR Issues relating to Rust/MIR support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants