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

Make the write_core (and optionally write) features no_std compatible #400

Merged
merged 6 commits into from
Nov 25, 2021

Conversation

kevinaboos
Copy link
Contributor

@kevinaboos kevinaboos commented Nov 19, 2021

Summary

This PR relaxes the requirement that write_core and write need and must enable the std feature.

Motivation

To enable the ability to use this crate's awesome write functionality in no_std environments.
For example, this is needed when using various wasmtime subcrates in a no_std environment.

Notes

  • I have tested this with all combinations of features that I could come up with. The cargo tests also pass.
  • The write feature could optionally enable std, if desired. Currently I didn't leave that feature there, but in the past, the write_core feature itself also enabled the std feature. I'm happy to make this change if requested.
    • The few write-related functions that require std have now been gated with a #[cfg(feature = "std")] attribute. Currently this only includes StreamingBuffer, which requires std::io.
  • The hashbrown dependency is already part of the indirect dependency tree, so I used the same version that indexmap depends on.
  • Not a big deal, but the IndexSet typedefs at the top of /src/write/string.rs can be removed once this PR lands: Unify std and no_std APIs for IndexMap and IndexSet indexmap-rs/indexmap#207. I can submit another PR here after that.

…nger requiring the `std` feature.

Note that the `write` feature could optionally enable `std`, if desired.
Also, adjust the features required for example binaries.
Cargo.toml Show resolved Hide resolved
crates/examples/Cargo.toml Outdated Show resolved Hide resolved
@philipc
Copy link
Contributor

philipc commented Nov 20, 2021

What's the status of the wasmtime no_std support? Have you talked with the wasmtime developers to be sure that they are also willing to support this? While this patch is small enough, supporting it requires ongoing effort, and I want to be certain that this effort is justified.

@kevinaboos
Copy link
Contributor Author

kevinaboos commented Nov 22, 2021

What's the status of the wasmtime no_std support? Have you talked with the wasmtime developers to be sure that they are also willing to support this? While this patch is small enough, supporting it requires ongoing effort, and I want to be certain that this effort is justified.

In my view, opening up this crate's object file writing abilities to no_std environments is quite valuable in and of itself. I myself could utilize such functionality independently of wasmtime, e,g., in the core of an OS kernel or any other bare metal environment.

Regarding wasmtime: I'm not sure if wasmtime is actually relevant to this PR, I only mentioned it as one of many possible examples. However, since you asked, I don't mind answering --- no, it is unlikely that wasmtime will support no_std any time soon, as many of its crates rely on libstd/libc functionality. However, I am working with a variety of Rust team members (as part of Futurewei's push to support open-source projects within the Rust ecosystem) to port wasmtime to bare metal environments, such as our OS Theseus. This involves modification of its existing crates, redesigning parts of wasmtime to use safer, more modern interfaces to platform-specific functionality, and addressing any shortcomings in crates on which it depends.

Nevertheless, I believe this PR shouldn't be affected by wasmtime in any way and can stand on its own. I'm happy to make any changes you feel necessary to ease the maintenance of this crate. If perhaps you would like another sign of goodwill, I have made some additions to the gimli crate in the past (for LSDA parsing in Theseus's custom unwinder) and am happy to contribute those back as part of proving my good intentions herein.

@philipc
Copy link
Contributor

philipc commented Nov 22, 2021

In my view, opening up this crate's object file writing abilities to no_std environments is quite valuable in and of itself. I myself could utilize such functionality independently of wasmtime, e,g., in the core of an OS kernel or any other bare metal environment.

Can you give an example of what you would achieve with that? In my experience, it would be quite unusual for an OS kernel or bare metal environment to be writing object or executable file formats. Generally, these file formats are written by users for the purpose of allowing an OS kernel to load them. An OS kernel doesn't typically generate executable files itself, it only loads existing files, and a bare metal environment doesn't deal with these executable file formats at all. I'm not even convinced that you would want to use the parts of wasmtime that depend on write support in these environments.

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 22, 2021

One example where a kernel generates an elf file is creating a core dump when a process crashed.

@philipc
Copy link
Contributor

philipc commented Nov 22, 2021

Ok, so can we restrict this to just the executable file formats, not object file formats then?

@kevinaboos
Copy link
Contributor Author

Thanks for your replies! I have done my best to respond below.

Can you give an example of what you would achieve with that? In my experience, it would be quite unusual for an OS kernel or bare metal environment to be writing object or executable file formats. Generally, these file formats are written by users for the purpose of allowing an OS kernel to load them. An OS kernel doesn't typically generate executable files itself, it only loads existing files, and a bare metal environment doesn't deal with these executable file formats at all.

Sure, it may indeed be "unusual", though I don't believe that should be grounds to prohibit it. In addition to core dumps, here are a few other examples off the top of my head, some of which I have already experimented with:

  • Any JIT scenario
  • Rewriting object files in an arbitrary platform-specific way, e.g., for optimization opportunities with unforeseen hardware, etc.
  • Emitting debug info based on some kind of runtime behavior, e.g., the addresses where an object/executable file was loaded (including which sections where loaded) for usage with a remote debugging session
  • Supporting loading & linking of raw object files (Theseus does this at runtime)
    • Potentially extendable by a memoization-like approach in which relocations in the object files are written out for future usage as an accelerated checkpoint/restore split linking process

I'm not even convinced that you would want to use the parts of wasmtime that depend on write support in these environments.

I do want to, and in fact I currently am. I agree with your umbrella statements about classic, traditional OSes; however, not every OS has a traditional division between userspace and kernelspace, so the line may be blurred between what is the kernel and what is not. Most importantly, the assumption that a standard library exists for every platform and is always available isn't true; even if a standard library exists, there's no real reason to force certain layers of the systems software stack to depend upon it if they don't need to.

Ok, so can we restrict this to just the executable file formats, not object file formats then?

We could, but there's no real reason to impose an artificial restriction on users of your crate. For example, I would benefit from that feature without having a std library, and others may in the future. Lots of exciting stuff going on in the space of research OSes & bare metal environments using Rust.

Cargo.toml Outdated Show resolved Hide resolved
src/write/string.rs Show resolved Hide resolved
…ite_std`.

* Remove optional features from the `std` feature that were enabled unconditionally even if `write_core` wasn't enabled.

* Use the `hashbrown` `HashMap` implementation only if `std` is disabled, as before.
@kevinaboos
Copy link
Contributor Author

kevinaboos commented Nov 23, 2021

I've pushed a new commit that avoids the problem of unnecessarily bringing in the indexmap/std and crc32fast/std features when std is enabled but write_core isn't. This works by introducing another orthogonal feature called write_std (name open to changes) that enables only the std-specific features related to write support.

The advantages are that features of dependencies are never unnecessarily enabled; the "disadvantage" (if one considers it as a such) is that an additional feature exists.

Cargo.toml Show resolved Hide resolved
…s the behavior of `write` unchanged from the perspective of dependent crates.

* Add the `write_std` feature to the `doc` generation feature. This could optionally just be `write`, too.
@kevinaboos
Copy link
Contributor Author

I've pushed a commit with the requested fixes. The only (very minor) issue is that there isn't a feature that's truly symmetric with read, i.e., write_core plus all file format features without std, but that's likely okay because it wasn't there before this PR. I can add it if preferred.

@philipc
Copy link
Contributor

philipc commented Nov 24, 2021

I think that's okay. If we were to change anything, I think it would be to make the read features match the write features: rename std to read_std and change read to enable read_std too.

@kevinaboos
Copy link
Contributor Author

Fixed the rustfmt issues. All checks should pass now.

Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

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

Thanks!

@philipc philipc merged commit 42326eb into gimli-rs:master Nov 25, 2021
@kevinaboos
Copy link
Contributor Author

Thanks for merging this in and for your great code reviews!

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

Successfully merging this pull request may close these issues.

3 participants