Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Partially fix no_std build for cratelift-codegen #1262

Closed
wants to merge 1 commit into from
Closed

Partially fix no_std build for cratelift-codegen #1262

wants to merge 1 commit into from

Conversation

IcyDefiance
Copy link
Contributor

@IcyDefiance IcyDefiance commented Dec 1, 2019

  • Add a feature gate for the std feature in packed_struct
  • Replace a few std references with core and alloc

See #1261 for more info. I'm intentionally avoiding the use of an issue keyword here, because this PR only fixes points 1 and 2 in that issue.

@bnjbvr The placeholder text says to ping you if I don't know who can review this, and it doesn't look like I can assign a reviewer at all, so here you go.

@bnjbvr
Copy link
Member

bnjbvr commented Dec 2, 2019

Thanks. Out of curiosity, does this mean that the cargo build step is done with std even in non-std builds? If not, doesn't this cause compilation errors in the codegen-shared crate?

@IcyDefiance
Copy link
Contributor Author

IcyDefiance commented Dec 3, 2019

That was a fun question to investigate. You can use std during the build step (e.g. inside codegen-meta), but it turns out there's a catch: "Features of dependencies are enabled if they're enabled in build-dependencies." (rust-lang/cargo#5730) For that reason, I had to add another feature gate to codegen-meta just to disable codegen-shared/std.


While doing that, I realized that I also forgot to disable codegen-shared/std inside codegen. Fixing that exposed a few more errors, so I fixed those and then added #![no_std] to codegen-shared for good measure.

cranelift-codegen/shared/src/lib.rs Outdated Show resolved Hide resolved
cranelift-codegen/shared/Cargo.toml Outdated Show resolved Hide resolved
@bnjbvr
Copy link
Member

bnjbvr commented Dec 13, 2019

Hey @IcyDefiance can you please rebase? (we removed the "packed_struct" dependency recently.) Then I'll make sure this is triaged for review. Thanks!

@IcyDefiance
Copy link
Contributor Author

Cool, that simplifies this PR a bit. I don't need to modify any Cargo.toml files anymore. One more reference to std was added to codegen-shared since the last review, but that was easy to replace with core.

@abrown
Copy link
Collaborator

abrown commented Dec 14, 2019

@IcyDefiance, I recently saw bytecodealliance/wasmtime#554 and wonder if the same applies here? (As I understand that PR, @alexcrichton moved back to use std:: in wasmtime).

@IcyDefiance
Copy link
Contributor Author

IcyDefiance commented Dec 14, 2019

Well, I hope not. I would like to make something similar to nebulet (an OS that runs entirely in ring 0, relying on a jitter for sandboxing), so if the same change is applied to cranelift-wasm and its dependencies it would be kind of unfortunate.

That said, it's not up to me, and I don't expect some crazy experimental use case to guide that sort of decision. It'll probably be a while before I switch back to that project anyway. I'm just saying it would be pretty cool.

@alexcrichton
Copy link
Member

I would personally think that this shouldn't be merged to stay consistent with bytecodealliance/wasmtime#554. As mentioned on that PR it doesn't mean we'll never add support for this, just that this probably isn't the way we'll implement it if we do implement it one day. If it's not needed at this time as well then we've got time to figure out how best to organize this.

@bjorn3
Copy link
Contributor

bjorn3 commented Dec 16, 2019

Nebulet already uses Cranelift, while there is no no-std user of Wasmtime.

@ratmice
Copy link

ratmice commented Dec 16, 2019

Hmm, that features are enabled when building seems weird is there another catch for when host != target? I haven't yet wrapped my head around what this means for the proposal in the wasmtime PR mentioned above which recommended moving to the no_std_compat crate 😕

Edit: A comment in the issue noted above makes it clear that features do leak across targets

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants