-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add the libstd-modifications needed for the L4Re target #43972
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
src/libstd/sys_common/util.rs
Outdated
|
||
// L4Re is not able to handle more than 1MB. | ||
#[cfg(target_os = "l4re")] | ||
let amt = amt.unwrap_or(1 * 1024 * 1024); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately tidy
disallows you from writing this using #[cfg]
or cfg!()
. See https://github.com/rust-lang/rust/blob/master/src/tools/tidy/src/pal.rs for detail.
[00:03:46] tidy error: /checkout/src/libstd/sys_common/util.rs:26: platform-specific cfg: cfg(not(target_os = "l4re"))
[00:03:46] tidy error: /checkout/src/libstd/sys_common/util.rs:30: platform-specific cfg: cfg(target_os = "l4re")
a2f2f7b
to
1224c5f
Compare
Thanks for the explanation! |
1224c5f
to
5089909
Compare
I think l4re is classified as "target family is unix", but maybe with these |
In the strict sense, L4(Re) is a microkernel with almost all of the components |
Id like to add that we needed less #[cfg] annotations than any other unix family member. In many cases (for example with the set_name function in thread.rs) you need one annotation to choose one of the implementations. That leads to a number of #cfg annotations that is always needed when implementing a new target for unix. |
src/libstd/sys/unix/thread.rs
Outdated
|
||
// L4Re only supports a maximum of 1Mb per default. | ||
#[cfg(target_os = "l4re")] | ||
let stack_size = cmp::min(stack_size, 1024 * 1024); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we rely on pthread_create
to return an error for larger stack sizes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will run in an error when pthread_attr_setstacksize is called. (sys/unix/thread.rs line: 55)
Most systems do not have a fixed maximum. We tried to put it to to the point where also the minimum size is checked. Putting it to the pthread_attr_setstacksize check would in my opinion put to much complexity there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard library tends to prefer to expose the underlying platform semantics as much as possible, avoiding trying to make shims and such where possible. It sounds like the default stack size on l4re should be less, but I believe we should still let the error propagate from pthread_attr_setstacksize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@humenda would it be possible to change the min_stack_size
function instead of adding this clause here?
src/libstd/lib.rs
Outdated
@@ -469,6 +469,7 @@ pub mod error; | |||
pub mod ffi; | |||
pub mod fs; | |||
pub mod io; | |||
#[cfg(not(target_os = "l4re"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my main hesitation for this PR, just excluding the net
module. This, AFAIK, doesn't happen on any other platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point and I know that it is unusual. As Sebastian already mentioned "L4Re looks like a normal (uClibc) Unix, just without networking". Implementing a completely new platform would lead to large amounts of redundant code while we are able to (even if it is unusual) can completely disable the net part with three statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to not have networking implemented, Emscripten for example doesn't implement threads. To do this, however, the shims in the sys
module should all return errors on l4re instead of being compiled away.
I see your point about completely removing networking, but on the other hand,
how many platforms does Rust current target which don't have networking?
Using the uClibc functions for networking on L4Re is undefined behaviour,
there's no notion of networking. IMHO, the cleanest solution is the removal. If
you don't agree, please help us to find a different solution.
Thanks
|
@humenda oh so right now the conventional design of libstd is that it doesn't tailor the top-level API available per-platform, it's always there unconditionally 100% of the time. This may change in the future, but for now it's the status quo that needs to be maintained. It's fine to not call libc functions and instead just return errors in Rust. |
Hi @TobiasSchaffner, are you able to make the changes as @alexcrichton suggests, i.e. probably just making the net module a skeleton that always errors? |
Hi @@aidhans
I'll take a look at the weekend or at Monday.
Cheers
|
As suggested in the discussion of PR rust-lang#43972, std should provide a uniform API to all platforms. Since there's no networking on L4Re, this now is a module in `sys::net` providing types and functions/methods returning an error for each action.
@alexcrichton Could you please have a look at the changes? |
Thanks! Want to update the libc submodule now as well? |
@alexcrichton I moved the line to the `min_stack_size` function in `util.rs`,
although I don't like the change. `min_stack_size` suggests that it returns the
minimum. It doesn't do so for other platforms. 2M is certainly not the minimum
for Linux and returning 1M for L4Re is the maximum for this platform. However,
it'll work.
|
Looks good to me! Appears to be a rebase conflict though? |
* Match definition of c_char in os/raw.rs with the libc definition Due to historic reasons, os/raw.rs redefines types for c_char from libc, but these didn't match. Now they do :). * Enable signal reset on exec for L4Re L4Re has full signal emulation and hence it needs to reset the signal set of the child with sigemptyset. However, gid and uid should *not* be set.
This commit adds the needed modifications to compile the std crate for the L4 Runtime environment (L4Re). A target for the L4Re was introduced in commit: c151220 In many aspects implementations for linux also apply for the L4Re microkernel. Two uncommon characteristics had to be resolved: * L4Re has no network funktionality * L4Re has a maximum stacksize of 1Mb for threads Co-authored-by: Sebastian Humenda <[email protected]>
As suggested in the discussion of PR rust-lang#43972, std should provide a uniform API to all platforms. Since there's no networking on L4Re, this now is a module in `sys::net` providing types and functions/methods returning an error for each action.
242e4c4
to
5d1a9d7
Compare
@bors: r+ |
📌 Commit 5d1a9d7 has been approved by |
src/libstd/sys/redox/thread.rs
Outdated
@@ -89,3 +91,18 @@ pub mod guard { | |||
pub unsafe fn current() -> Option<usize> { None } | |||
pub unsafe fn init() -> Option<usize> { None } | |||
} | |||
|
|||
pub fn min_stack() -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of duplicating all this logic in all the various platforms, could just the default min stack size get implemented on various platforms?
fea627c
to
0f8c9d7
Compare
src/libstd/sys/unix/thread.rs
Outdated
#[cfg(not(target_os = "l4re"))] | ||
pub const default_min_stack_size: usize = 2 * 1024 * 1024; | ||
#[cfg(target_os = "l4re")] | ||
pub const default_min_stack_size: usize = 1024 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conventionally in rust constants have full uppercase names like DEFAULT_MIN_STACK_SIZE
The default min stack size value is smaller on l4re and therefore this value has to be different depending on the platform.
@bors: r+ |
📌 Commit b2b5063 has been approved by |
⌛ Testing commit b2b5063 with merge 991066932ac6e7cfb76ec0c920867134d219811b... |
💔 Test failed - status-travis |
@bors: retry
|
Add the libstd-modifications needed for the L4Re target This commit adds the needed modifications to compile the std crate for the L4 Runtime environment (L4Re). A target for the L4Re was introduced in commit: c151220 In many aspects implementations for linux also apply for the L4Re microkernel. Some uncommon characteristics had to be resolved: * L4Re has no network funktionality * L4Re has a maximum stacksize of 1Mb for threads * L4Re has no uid or gid Co-authored-by: Sebastian Humenda <[email protected]>
☀️ Test successful - status-appveyor, status-travis |
This commit adds the needed modifications to compile the std crate for the L4 Runtime environment (L4Re).
A target for the L4Re was introduced in commit: c151220
In many aspects implementations for linux also apply for the L4Re microkernel.
Some uncommon characteristics had to be resolved:
Co-authored-by: Sebastian Humenda [email protected]