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

Port C code to Rust #7

Merged
merged 39 commits into from Jun 3, 2017

Conversation

Projects
None yet
2 participants
@ids1024
Member

ids1024 commented May 8, 2017

There are a couple advantages to this:

  • There is somewhat less danger of making an incorrect system call, since redox_syscall can be used. Thus an issue like the one fixed in #4 would have been caught.
  • Non-trivial code implemented in Rust does not need to be duplicated. For instance, gethostbyname() could use the host name resolution code in Redox's libstd. This eliminates the need to rewrite the code, and it is generally better to have only one implementation for consistency.

Disadvantages:

  • Adds some bloat to libc.
  • For the simple functions that are just a system call, the Rust implementation is more complicated and uglier, since it has to convert C types to rust types.

I think if this is done, either all the C code in sys/redox should be ported to Rust, or only things like gethostbyname(), that substantially benefit, should.

Thoughts?

ids1024 added some commits May 8, 2017

@jackpot51

This comment has been minimized.

Member

jackpot51 commented May 8, 2017

Pretty cool. Try to avoid allocation of cstr by using strlen to make a u8 slice. Paths do not need to be UTF-8

@ids1024

This comment has been minimized.

Member

ids1024 commented May 8, 2017

CStr is like str and doesn't actually store the bytes, and also doesn't guarantee UTF8. So I think the overhead there is small. I'll have to check the assembly to know exactly what it ends up being.

As for the conversion to &str (which assumes the string is UTF-8), redox_syscall takes &str for paths. If paths do not have to be UTF-8, they should take slices instead. (Although, it seems like a good idea to me to require UTF-8 paths).

@ids1024

This comment has been minimized.

Member

ids1024 commented May 8, 2017

I think the solution here is to replace &str with AsRef<[u8]> in the syscall crate.

ids1024 added some commits May 11, 2017

@ids1024

This comment has been minimized.

Member

ids1024 commented May 11, 2017

@jackpot51 So do you think it would be good to replace everything in sys/redox with Rust code (which shouldn't be too hard; there isn't much code there now.)?

This does seem to increase binary size, but #[no_std] would address that.

@jackpot51

This comment has been minimized.

Member

jackpot51 commented May 11, 2017

Yeah, I think it would be if it is easy

ids1024 added some commits May 12, 2017

@jackpot51

This comment has been minimized.

Member

jackpot51 commented May 13, 2017

Glad to see this coming along! Looks good!

ids1024 added some commits May 15, 2017

@ids1024

This comment has been minimized.

Member

ids1024 commented May 21, 2017

This is largely done, pending the merge of the PR linked above and changes here to match (trivial). And perhaps some testing.

ids1024 added some commits May 21, 2017

@ids1024 ids1024 changed the title from [WIP] Proof of concept for using Rust code in newlib to [WIP] Port C code to Rust May 22, 2017

ids1024 added some commits May 23, 2017

@ids1024 ids1024 changed the title from [WIP] Port C code to Rust to Port C code to Rust May 30, 2017

@ids1024

This comment has been minimized.

Member

ids1024 commented May 30, 2017

This should be ready to merge. If you get build errors related to libtest, install the git version of xargo or comment out that part of Xargo.toml.

The PR in the libc repo also needs to be merged (and the submodule updated, of course). A new version of redox_syscall should be pushed to crates.io; or the Cargo.toml here could be updated to pull from git instead, but I can't see any reason to do that.

@ids1024

This comment has been minimized.

Member

ids1024 commented May 31, 2017

I've pushed a commit to call C++ global constructors/destructors, which depends on redox-os/gcc#2. Ideally that would be a separate PR, but since this one rewrites all the C code, that change depends on/conflicts with this one.

ids1024 added some commits Jun 3, 2017

Disable sysconf()/_times() to fix binutils/gcc
This probably won't break anything that wasn't already broken.

@ids1024 ids1024 force-pushed the ids1024:newlib-rust branch from 718b13e to 079655d Jun 3, 2017

@jackpot51 jackpot51 merged commit 12ff74b into redox-os:redox Jun 3, 2017

@ids1024 ids1024 deleted the ids1024:newlib-rust branch Aug 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment