-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
cranelift-codegen no longer builds with no_std #1158
Comments
In addition to that PR, I started addressing point 3 on the no_std-thiserror branch on my fork. (diff) Building that branch will give you the same errors that I pasted in the above post. |
I got cranelift-wasm and its dependencies (including cranelift-codegen) to compile with no_std in my fork. To do so, I had to fork thiserror and v0.48.2 of wasmparser (there have been breaking changes since then). I also copied the Considering the current lack of interest in supporting no_std, as expressed in my PR, I won't bother making any new PRs. However, they're good enough for the project I'm playing with, so they may also be useful to someone else, and they may be informative if there is any future interest in supporting no_std. |
I happen to know of at least one project where building cranelift-wasm as no_std is really helpful, so I just want to say I appreciate this work. |
Sorry we didn't get back to you earlier, things have been a bit hectic lately, and no-std support is not checked in automation, so it's definitely something we may break a lot. The main issue here was finding a proper core reviewer for your work; if there's someone who could chime in and review such changes, it would be definitely helpful. And then, we could reopen your PR here and in other places and take your changes, if you were so inclined. |
(Of course, that also depends on the |
By "lack of interest" I was referring to Alex's comment in my PR, but if that's not the case then I can make some new PRs later today. I'll start with thiserror and wasmparser and see how it goes. |
It's nice to be able to experiment on a smaller crate like |
Opened https://github.com/bytecodealliance/wasmparser/issues/198 . I'm proposing not depend on structures that require memory allocations (this means no hashbrown )
These ideas might not be applied here |
Is this still being worked on? What are the current issues blocking no_std support? |
I'm at least not personally aware of this being worked on, but no blocker other than "just needs some elbow grease". If you'd be up for it PRs would be most welcome! |
I made a pretty half-assed attempt at this but managed to dig up some useful info: a major blocker to getting this to work is getting several dependencies involved in
All of those issues come from the The really obnoxious part about removing Aside from
I haven't tried to get I would really appreciate some guidance for what to work towards next. This is clearly going to take a LOT of work to get |
|
My recommendation would be to first start off by gating "easy" things behind on-by-default Cargo features. For example After that next bit that I would recommend is to do this incrementally. The main crate in question I believe is At that point I think it might be worth reevaluating taking stock of what's remaining. I think it would be useful to classify dependencies as you've done, but with a refreshed list at that point. Some of these are going to be somewhat hard like |
I'm having a hard time with removing I should also point out that the |
For now, I just put no-op stubs in the Right now, my development approach is to hack on all of these intersecting API changes and changes to the dependency trees on a fork, which I have set up to be running the full CI PR test suite: marceline-cramer#1 Once that branch on my fork is to a place I'm happy with and all the pieces fit together practically as a rough sketch, I can start decomposing it into finer-grained, more polished things to implement as upstream-facing PRs. I did implement the "ratcheting" approach to get down to I added a field to The reason I had gotten as far with The main kicker with That's about where I am now. The CI mostly passes, although Cranelift's unit tests fail with slightly different code output compared to the expected code. I can chalk that up to either an impatient use of The main issue now is that, by default, |
It is used by cg_clif to include performance timings collected by Cranelift in rustc self profile profiles. Having thr timing module be a nop for no_std builds is fine. |
Unconditionally using core instead of std should work fine. In std builds, core is still available. |
Sounds good! I'd recommend gating
I'd probably recommend adding a
This sounds generally fine and possible for us to land. When making PRs if you wouldn't mind leaving
FWIW I think it's definitely ok to not get the entire crate and all its features compiling with no_std. Only getting a subset working (albeit we still need a useful subset, but the one you have is useful) is more than ok. Can always work to improve the subset supported later on.
I'm assuming that you're probably going to add a
In the spirit of ratcheting and progress over time I'll reiterate it's completely ok to get only some features working and not others. In this workspace most dependencies disable default features and then individual crates will enable features in addition to specifying One example here is that the If your target is |
It would work fine if not for the |
Sounds great, I'll remember that when I open the
Not including the
Yeah, I suppose that is definitely the easiest way of going about it. I would definitely be glad to have just stripped-down
I was not aware that the dependency on |
I've tracked down a few things that are preventing this from happening.
packed_struct
requires std by defaultcranelift-codegen
andcranelift-codegen-shared
that can be replaced with core or alloc.thiserror
requires std, because the Error trait doesn't exist in core or alloc.The first two are trivial to fix, and I'll create a PR with those changes in a second.
However, there are 11 uses of the
Display
andInto<result::CodegenError>
traits that prevent me from placingthiserror
behind a feature gate, and I'm not sure what to do about them.Here are the 11 things that rely on those traits, as reported by rustc (click to expand)
Additional info:
thiserror
doesn't want to add a feature-gate for just the Error trait, but it's possible to provide another implementation of Display (and presumably Into). Option toimpl Error
under feature flag dtolnay/thiserror#43The text was updated successfully, but these errors were encountered: