Split up some files into modules/impl FS functions

sunfishcode / mustang

Rust programs written entirely in Rust

Other

853 stars 24 forks source link

Split up some files into modules/impl FS functions #104

Closed carbotaniuman closed 2 years ago

carbotaniuman commented 2 years ago

The main point of this change is to make sure all LFS functions are implemented in terms of the the 64 variant, as well as move all of the fs located code outside of the single massive file. Where functions ending in at were missing, they have been added and all calls updated to reduce code duplication.

Closes (because that's probably better than putting a comment in each issue): #88, partial #87, #71.

sunfishcode commented 2 years ago

Cool! I'll take a closer look in a few days, but I wanted to quickly check in here and say this looks like a great place to start.

Also, there's a possibility that splitting lib.rs into multiple files will lead to undefined symbols at link time; if that happens, one option is to use include! to link the individual files into lib.rs instead of mod.

sunfishcode commented 2 years ago

At a first readthrough, this looks like a good direction. I'll be interested to see what happens when the current CI failures are fixed (rustfmt, and there are likely some conversions needed when dealing with libc::stat fields), and we can see what happens at link time.

carbotaniuman commented 2 years ago

I think you need to punch the button, as the CI hasn't been running for several commits. This seems like a good point to see how far I get in the CI anyways.

carbotaniuman commented 2 years ago

Ok I've finally gotten to the linking errors. How is c-scape built (aka what crate-type), and how/where is it linked in?

sunfishcode commented 2 years ago

Currently, the way it works is:

mustang links in stub versions of "libc.a" and other libraries, to prevent the system libraries from being linked in: https://github.com/sunfishcode/mustang/blob/main/mustang/build.rs#L11
c-scape is just a regular crate that just happens to declare a bunch of extern-C symbols that just happen to resolve everything needed from libc by basic Rust programs.

The tricky part of this setup seems to be that some of the extern-C symbols get dropped before we reach the final link, when the linker realizes they're actually needed.

Putting all the code in a single lib.rs file seems to avoid this problem, though I don't yet know why it works. include!, as opposed to mod would be a way to achieve that, while still having the code in separate files.

I'm also open to other approaches here. "Put everything in lib.rs" is the only thing I've found that works, but if there's a better way to do it, I'm certainly open to it.

carbotaniuman commented 2 years ago

Ok I've solved it with:

[profile.dev.package.c-scape]
codegen-units = 1

[profile.test.package.c-scape]
codegen-units = 1

I'm working on some rustc fixes in this space, but for now that's probably the best workaround for us. I'll take a look at the linking in more depth later.

sunfishcode commented 2 years ago

This is currently marked as a draft; is this ready for review?

carbotaniuman commented 2 years ago

I think this is ready for review now, but I do need to figure out how to add tests for every function that I added though.

carbotaniuman commented 2 years ago

Build won't be green for a while until rustix gets updated, as I'm knocking out termios right now. a39ec76 should be reviewable however, so these changes can simply just be split into a seperate PR.

sunfishcode commented 2 years ago

Would you mind opening a new PR for the termios and any other changes? I'd like to land the main changes here, so that we can build on top of the new code organization.

carbotaniuman commented 2 years ago

Yeah I just did, I realized after a short nap that I could just... use another branch.

sunfishcode commented 2 years ago

This new organization looks great!