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

Mustang nostd support #156

Merged
merged 3 commits into from
Feb 6, 2023
Merged

Mustang nostd support #156

merged 3 commits into from
Feb 6, 2023

Conversation

jordanisaacs
Copy link
Contributor

@jordanisaacs jordanisaacs commented Jan 30, 2023

c-scape had an accidental dependance on std::f64 which prevented compiling on no_std which is fixed by using libm.

Added feature support for nostd which exports c_scape rather than c_gull which still includes enough for a working dlmalloc/unwinding. I was able to compile and run a no_std executable with the following program. It also works if you use the #[start] feature. Build command is

cargo build --target=x86_64-mustang-linux-gnu -Zbuild-std=core,alloc 

In terms of documentation, not entirely sure what the #[start] is adding vs. directly implementing main. Also because mustang is controlling all of program startup we can probably determine the safety of the start function pretty easily. related tracking issue.

What I am looking to next is pulling out c-scape's thread implementation, switching dlmalloc to rustix/rust threads, and implementing the rustix backend for unwinding to remove the need for any libc hackery.

@sunfishcode
Copy link
Owner

Interesting!

Would it work for c-gull to instead have a std feature, and to disable all of c-gull's own contents when not(feature = "std"), so that it serves as a kind of pass-through for c-scape in no-std mode? Then mustang could just forward its own std feature to c-gull and not have to separately depend on c-scape. It seems like that would simplify some of the cargo features here.

@jordanisaacs
Copy link
Contributor Author

Ya that seems like a much better solution. I am not the biggest fan with how I wrote those cargo features anyway but it was the best I could get when needing to explictly mention c-scape.

@sunfishcode
Copy link
Owner

Also, would you be able to add something like that no-std main as a test crate in the test-crates directory, so that we can avoid accidentally regressing it?

@jordanisaacs
Copy link
Contributor Author

Yep, I can do that. Also found this from the unstable book.

Controlling the entry point is possible in two ways: the #[start] attribute,
or overriding the default shim for the C main function with your own.

So I'll stick with the shim.

@jordanisaacs
Copy link
Contributor Author

jordanisaacs commented Jan 30, 2023

All updated, but not a fan of the mess of features in c-gull's Cargo.toml and cfg(feature = "std") in lib.rs. Unfortunately can't use [target.'cfg(feature = "std")'.dependencies] as you can't specify features then.

c-gull/Cargo.toml Outdated Show resolved Hide resolved
c-gull/src/lib.rs Outdated Show resolved Hide resolved
tz-rs = "0.6.11"
printf-compat = "0.1.1"
log = { version = "0.4.14", default-features = false }
c-scape = { path = "../c-scape", version = "0.6.1", default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

Why does c-scape need default-features = false?

If that were removed, could we also remove the c-scape/default below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c-scape by default builds with std which propagates down to its dependencies such as rustix. I was trying to avoid breaking any of the current default features.

Copy link
Contributor Author

@jordanisaacs jordanisaacs Feb 3, 2023

Choose a reason for hiding this comment

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

Looked into it a little more, c-scape also by default builds with resolve which relies on resolve-sync which is a library that uses std.

Copy link
Owner

Choose a reason for hiding this comment

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

Should we move resolve out into c-gull then? I think that makes sense, but don't know offhand if there's some subtle dependency there. We could also do that in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, moving resolve it into c-gull seems like the right call. I can do that in a separate PR. I went through the git history and resolve was around before c-gull existed. I am not sure if there was a reason it wasn't moved to c-gull, but simplest guess is it just got missed.

@jordanisaacs
Copy link
Contributor Author

I also updated the test/readme to work with check compilations. And added a comment on the usage of #[start] vs. extern "C" main.

@sunfishcode
Copy link
Owner

Looks good! Could you also add an entry to .github/workflows/main.yml at the bottom to test the new mustang-nostd test-crate?

@jordanisaacs
Copy link
Contributor Author

Added two entries, a cargo run & test.

@jordanisaacs
Copy link
Contributor Author

Fixed the CI declaration I think

@sunfishcode
Copy link
Owner

Looks like it still gets

$ cargo +nightly-2022-11-17 run -Zbuild-std=core,alloc --target=../../mustang/target-specs/x86_64-unknown-linux-gnu.json
error: target path "../../mustang/target-specs/x86_64-unknown-linux-gnu.json" is not a valid file

@jordanisaacs
Copy link
Contributor Author

Found it, was using host_target not mustang_target.

@jordanisaacs
Copy link
Contributor Author

jordanisaacs commented Feb 6, 2023

Looked into why it failed, because default handle_alloc_error to panic isn't stabilized on the specified nightly toolchain. I can either implement a handle_alloc_error, use the default feature, or bump the toolchain version used by the tests. I prefer updating the toolchain.

edit: Just added the default feature with a TODO comment to remove when the toolchain is updated (in another PR) in case other things need to be changed.

@sunfishcode
Copy link
Owner

Updating the toolchain works! It's just pinned so that we can deal with the churn at our own pace :-).

@sunfishcode sunfishcode merged commit ec2fd54 into sunfishcode:main Feb 6, 2023
@sunfishcode
Copy link
Owner

A TODO also works for now. We can update the toolchain whenever it's convenient.

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.

2 participants