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

Port libpanic_abort and libpanic_unwind to CloudABI #47190

Merged
merged 2 commits into from
Jan 6, 2018

Conversation

EdSchouten
Copy link
Contributor

@EdSchouten EdSchouten commented Jan 4, 2018

This change ports both the libpanic* libraries to CloudABI.

The most interesting part of this pull request, however, is that it imports the CloudABI system call API into the Rust tree through a Git submodule. These will also be used by my port of libstd to CloudABI extensively, as that library obviously needs to invoke system calls to implement its primitives.

I have taken the same approach as libc: src/libcloudabi + src/rustc/cloudabi_shim. If some other naming scheme is preferred, feel free to let me know! As libcloudabi is pretty small, maybe it makes sense to copy, instead of using a submodule?

CloudABI uses LLVM's libunwind for stack unwinding. There was a small
bug that went by unnoticed, namely that it was not built with -fno-rtti.
This caused it to (indirectly) depend on the entire C++ runtime.

Now that that issue has been resolved, it is also perfectly fine to make
use of this library for programming languages other than C++.
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)


[lib]
name = "cloudabi"
path = "../../libcloudabi/rust/cloudabi.rs"
Copy link
Member

@kennytm kennytm Jan 4, 2018

Choose a reason for hiding this comment

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

Could we make libcloudabi a crates.io package instead of a submodule? (assuming we don't want to copy the files)

@alexcrichton
Copy link
Member

Thanks! It looks like the bindings used here are pretty slim, would it be possible to inline the cloudabi crate definitions instead of pulling in a new submodule?

@EdSchouten
Copy link
Contributor Author

EdSchouten commented Jan 4, 2018

Hi Kenny, Alex,

I have to confess I'm still a pretty much a newbie when it comes to Cargo, so I appreciate your input on this matter a lot.

We already have the CloudABI definitions as a crate on crates.io, called cloudabi. I personally think that it makes a lot of sense to depend on that instead of using a submodule, but I haven't managed to get that working properly. If I change the Cargo.toml files as follows:

--- a/src/libpanic_abort/Cargo.toml
+++ b/src/libpanic_abort/Cargo.toml
@@ -14,4 +14,4 @@ core = { path = "../libcore" }
 libc = { path = "../rustc/libc_shim" } 
 
 [target.'cfg(target_os = "cloudabi")'.dependencies]
-cloudabi = { path = "../rustc/cloudabi_shim" }
+cloudabi = { version = "0.0.2", default-features = false }
diff --git a/src/libstd/Cargo.toml b/src/libstd/Cargo.toml
index 5e4f5204aa..0b4433d879 100644
--- a/src/libstd/Cargo.toml
+++ b/src/libstd/Cargo.toml
@@ -39,7 +39,7 @@ rustc_msan = { path = "../librustc_msan" }
 rustc_tsan = { path = "../librustc_tsan" }
 
 [target.'cfg(target_os = "cloudabi")'.dependencies]
-cloudabi = { path = "../rustc/cloudabi_shim" }
+cloudabi = { version = "0.0.2", default-features = false }
 
 [build-dependencies]
 build_helper = { path = "../build_helper" }

I can no longer build a simple tool that makes use of the standard libraries:

$ xargo run --target x86_64-unknown-cloudabi
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling libc v0.2.35
   Compiling cloudabi v0.0.2
   Compiling cc v1.0.3
   Compiling unwind v0.0.0 (file:///home/ed/projects/rust/src/libunwind)
   Compiling cfg-if v0.1.2
   Compiling core v0.0.0 (file:///home/ed/projects/rust/src/libcore)
error[E0463]: can't find crate for `core`

This is probably because cloudabi depends on the wrong instance of core, right? In cloudabi_shim we explicitly override that to use libcore in the Rust source tree.

If depending on an external crate is going to be hard, I think that Alex's idea of inlining the crate definitions makes sense. Alex: I can copy over the .rs source files from the CloudABI source tree into src/libcloudabi and give that a custom Cargo.toml. Is that what you're proposing?

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2018
@alexcrichton
Copy link
Member

Yes unfortunately anything that std depends on can't come vanilla from crates.io. The standard library depends explicitly (via Cargo.toml) on libcore, but nothing on crates.io does that so Cargo doesn't understand to sequence the compilations correctly. The shim crate is necessary to introduce this explicit dependency.

That being said though it'd be great if we could inline some specific pieces (perhaps unsafe). We already have way too many submodules as it is :(

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2018
@EdSchouten
Copy link
Contributor Author

Thanks for confirming.

Right now there are two crates in my patched Rust source tree that make use of CloudABI's definitions: libpanic_abort and libstd. This is why I put this code in a shared cloudabi crate. That said, I may be able to avoid the dependency in libpanic_abort by using any of the two other code paths. Once removed, libstd remains the only consumer, meaning I can move the generated sources into src/libstd/sys/cloudabi/....

If it turns out there are other places in the source tree where a dependency on the generated sources is needed, we can reconsider creating a separate crate, but for now there are ways around it.

I'll update this pull request later today!

Ideally, we should make use of CloudABI's internal proc_raise(SIGABRT)
system call. POSIX abort() requires things like flushing of stdios,
which may not be what we want under panic conditions. Invoking the raw
CloudABI system call would have prevented that.

Unfortunately, we have to make use of the "cloudabi" crate to invoke raw
CloudABI system calls. This is undesired, as discussed in the pull
request (rust-lang#47190).
@EdSchouten
Copy link
Contributor Author

Done! I think this pull request should now be good to go!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 5, 2018

📌 Commit 91611fc has been approved by alexcrichton

kennytm added a commit to kennytm/rust that referenced this pull request Jan 6, 2018
…xcrichton

Port libpanic_abort and libpanic_unwind to CloudABI

This change ports both the libpanic* libraries to CloudABI.

The most interesting part of this pull request, however, is that it imports the CloudABI system call API into the Rust tree through a Git submodule. These will also be used by my port of libstd to CloudABI extensively, as that library obviously needs to invoke system calls to implement its primitives.

I have taken the same approach as libc: `src/libcloudabi` + `src/rustc/cloudabi_shim`. If some other naming scheme is preferred, feel free to let me know! As `libcloudabi` is pretty small, maybe it makes sense to copy, instead of using a submodule?
bors added a commit that referenced this pull request Jan 6, 2018
Rollup of 7 pull requests

- Successful merges: #46947, #47170, #47190, #47205, #47217, #47220, #47230
- Failed merges: #47233
@bors bors merged commit 91611fc into rust-lang:master Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants