Add basic `thread` #110

tbu- · 2017-02-18T01:31:19Z

It supports spawn and join so far.

Only works on x86_64 so far.

tbu- · 2017-02-18T01:32:35Z

Will add the other architectures soon (unfortunately, the syscall_clone (__steed_clone) function needs to be written for each target separately). Does that look good so far? It should run and work on x86_64.

anatol · 2017-03-23T07:10:04Z

It seems it does not include implementation for a number of Thread functions like yield_now current sleep_ms etc..

tbu- · 2017-03-23T11:02:52Z

Yes. I tried to get the bare-minimum of thread support running, namely: spawning a thread. I didn't manage to on all platforms. I currently do not work on it, so feel free to pick it up if you want.

(If you do, I'd suggest you to also only try thread spawning first, do the rest later. It's hard enough. ^^ Inline assembly for architectures you have never heard before.)

anatol · 2017-03-23T15:52:06Z

@japaric does this PR look good? Could you please review/merge it before starting work on other thread pieces?

tbu- · 2017-03-24T00:05:36Z

This still needs support for the rest of the architectures before it can go forward. From looking at my commit list, at least sparc and PowerPC64 are missing.

homunkulus · 2017-04-12T03:40:47Z

☔ The latest upstream changes (presumably f1624e5) made this pull request unmergeable. Please resolve the merge conflicts.

tbu- · 2017-04-14T22:56:37Z

@homunkulus try

homunkulus · 2017-04-14T22:56:40Z

⌛ Trying commit b943119 with merge b943119...

japaric

First, sorry for taking so long to review this.

Second, thanks @tbu- for tackling this. This is tricky stuff!

The implementation looks reasonable to me. It's a good starting point. I mainly have nits about creating issues about what still needs to be implemented.

japaric · 2017-04-14T23:29:43Z

Cargo.toml

@@ -6,7 +6,7 @@ name = "std"
 version = "0.1.0"

 [dependencies]
-sc = "0.1.5"
+sc = { git = "git://github.com/japaric/syscall.rs" }


can this be switched back to the latest release?

japaric · 2017-04-14T23:37:26Z

src/libc/internal/aarch64.rs

+        // Save `fn_` and `arg` on the child stack.
+        stp x0,x3,[x1,#-16]!
+
+        mov x8,#220 // CLONE


I don't recall if it's possible or not but could NR_CLONE from the sc crate be used here (and on all the other instances) instead hardcoding the value? (I know the value is very unlikely to change; this is just for clarity and to avoid duplication)

I don't know how to do that. (This value won't change, it would break a lot of programs and Linux is commited to backward-compatiblity on the syscall layer.)

OK, that's fine.

japaric · 2017-04-14T23:39:22Z

src/libc/internal/aarch64.rs

+#[naked]
+#[no_mangle]
+unsafe extern "C" fn __steed_clone() {
+    // Syscall number is passed in x8, syscall arguments in x0, x1, x2, x3, x4.


I love these comments ❤️ ❤️ ❤️ . Great work, @tbu-. 👍

Some of the MUSL implementations say very little about what's going on.

japaric · 2017-04-14T23:42:27Z

src/libc/internal/x86.rs

+}
+
+pub unsafe fn set_thread_pointer(thread_data: *mut ()) {
+    let mut user_desc = linux::user_desc {


Could you add a note mentioning that the meaning of flags is explained in the asm block above?

japaric · 2017-04-14T23:52:38Z

src/thread/mod.rs

+impl ThreadId {
+    // Generate a new unique thread ID.
+    fn new() -> ThreadId {
+        /*


Could you create an issue and add a TODO about this?

japaric · 2017-04-15T00:15:12Z

src/libc/mod.rs

+}
+*/
+
+pub unsafe fn pthread_join(pthread: pthread_t, retval: *mut *mut c_void)


This implementation looks different from the MUSL one, where did it come from? (I checked src/thread/pthread_join.c). Regardless, it looks reasonable to me.

It's a simplified copy of __timedwait_cp from src/thread/__timedwait.c, which is called in pthread_join by MUSL.

Could you add a comment mentioning that file and that this is a simplified version?

japaric · 2017-04-15T00:18:17Z

src/sys/linux/stack_overflow.rs

+pub struct Handler(());
+
+impl Handler {
+    pub unsafe fn new() -> Handler {


Could you create an issue and add a TODO about this?

japaric · 2017-04-15T00:19:37Z

src/sys/linux/os.rs

@@ -15,6 +15,10 @@ pub fn exit(code: i32) -> ! {
    unsafe { linux::exit_group(code) }
 }

+pub fn page_size() -> usize {


Could you create an issue and add a TODO about this?

japaric · 2017-04-15T00:26:49Z

src/libc/mod.rs

+}
+*/
+
+pub unsafe fn pthread_create(pthread: *mut pthread_t,


This implementation looks different from the MUSL one, where did it come from? (I checked src/thread/pthread_create.c) Perhaps it is a simplified / minimal version of it. In any case, it looks reasonable to me.

It's a simplified version of the MUSL one, pthread_create in src/thread/pthread_create.c.

Could you add a comment mentioning that file and that this is a simplified version, and the rationale behind omitting some parts (like the copy_tls stuff)?

japaric · 2017-04-15T00:44:18Z

src/sys/linux/thread.rs

+}
+
+/*
+impl Drop for Thread {


Could you create an issue and add a TODO about this?

tbu- · 2017-04-15T08:03:24Z

@homunkulus try

homunkulus · 2017-04-15T08:03:27Z

⌛ Trying commit 3663209 with merge 3663209...

homunkulus · 2017-04-15T08:54:35Z

💔 Test failed - status-travis

tbu- · 2017-04-15T11:22:19Z

It only failed on tier-2 platforms.

homunkulus · 2017-04-15T12:15:37Z

☔ The latest upstream changes (presumably #126) made this pull request unmergeable. Please resolve the merge conflicts.

japaric · 2017-04-15T12:36:52Z

It only failed on tier-2 platforms.

Please open an issue about the targets that are missing thread support.

Ideally, we should cfg the thread code on the platforms where it's unimplemented to keep steed compiling and hello would working but panic when the thread API is used. But, if that tedious, feel free to just leave things as they are and put these targets in the allow_failures list

tbu- · 2017-04-15T12:52:17Z

Please open an issue about the targets that are missing thread support.

#13 (comment)

tbu- · 2017-04-15T14:46:00Z

@homunkulus try

homunkulus · 2017-04-15T14:46:05Z

⌛ Trying commit 52f2697 with merge 52f2697...

homunkulus · 2017-04-15T14:59:18Z

💔 Test failed - status-travis

tbu- · 2017-04-15T15:38:02Z

@japaric It seems that the parameters to __steed_clone are optimized out when compiling in release mode. Do you have an idea on how to change that? It might be that LLVM looks at the actual symbol and sees that it doesn't declare any parameters.

tbu- · 2017-04-15T15:46:00Z

If I put it into a different crate and disable LTO, then it works, but if either LTO is enabled or __steed_clone is in the same crate, the optimization happens and the code doesn't work in release mode...

Not sure what to do about this.

tbu- · 2017-04-15T15:46:18Z

It sounds like I'm invoking UB somewhere.

japaric · 2017-04-15T15:51:37Z

Hmmm, the problem could the extern "C" / symbol approach since in LTO there's only one object file. Perhaps you could try something like this:

fn syscall_clone(
    fn_: extern "C" fn(*mut c_void) -> *mut c_void,
    child_stack: *mut c_void,
    flags: c_ulong,
    arg: *mut c_void,
    ptid: *mut pid_t,
    newtls: *mut c_void,
    ctid: *mut pid_t,
) -> pid_t {
    unsafe {
        __steed_clone();
    }
}

the return type is going to be a problem there. Perhaps change the signature of __steed_clone to fn() -> pid_t and put an intrinsics::unreachable() at the end of __steed_clone implementation.

tbu- · 2017-04-15T16:34:28Z

Changing the signature of __steed_clone didn't help, the parameters still seem to be optimized out. Maybe I need to annotate the asm! macro somehow.

japaric · 2017-04-15T16:39:27Z

the parameters still seem to be optimized out.

Maybe try something like test::black_box?

It supports `spawn` and `join` so far. Only works on x86_64 so far. CC japaric#13.

tbu- · 2017-04-21T13:55:40Z

I've read "This Week in Rust" and found that they just added the thing I need: global_asm! lets me write functions in pure assembly, without any added stuff by the compiler. This should fix the issues in release build.

@homunkulus try

homunkulus · 2017-04-21T13:55:43Z

⌛ Trying commit de3632e with merge de3632e...

homunkulus · 2017-04-21T14:09:26Z

💔 Test failed - status-travis

japaric · 2017-04-21T14:14:12Z

Oh, nice. global_asm! landed 👍.

ld.lld: error: std.cgu-0.rs:(.text+0x0): duplicate symbol '__steed_clone'

My hunch tells me that adding a [cfg(not(test)] to the symbol may fix this.

This allows us to write the whole function in assembly without hacks.

tbu- · 2017-04-21T14:17:58Z

Yep, also thought so.

@homunkulus try

homunkulus · 2017-04-21T14:18:01Z

⌛ Trying commit 835f2c2 with merge 835f2c2...

homunkulus · 2017-04-21T14:33:29Z

☀️ Test successful - status-travis
State: approved= try=True

tbu- · 2017-04-21T14:33:47Z

\o/

mrhota · 2017-04-22T01:55:43Z

src/libc/internal/aarch64.rs

@@ -34,7 +32,10 @@ unsafe extern "C" fn __steed_clone() {
    //
    // We save `fn_` and `arg` on the child stack.

-    asm!("
+    global_asm!("


you could even commit actual .s files, if you're interested, and do global_asm!(include_str!("./asm_file.s"));

mrhota · 2017-04-22T02:14:08Z

src/libc/mod.rs

+    // where CLONE and EXIT are the respective syscalls. Because it mangles
+    // with the stack (in two processes at once), it needs to be written in the
+    // target's assembly.
+    #[link_name = "__steed_clone"]


I wonder if you can achieve what @japaric suggested above with NR_CLONE by doing this:

#[no_mangle] extern "C" fn __steed_clone(nr: usize // <-- or whatever correct type is for syscall numbers fn_: extern "C" fn(*mut c_void) -> *mut c_void, child_stack: *mut c_void, flags: c_ulong, arg: *mut c_void, ptid: *mut pid_t, newtls: *mut c_void, ctid: *mut pid_t) -> pid_t; #[inline] // or whatever fn syscall_clone(fn_: extern "C" fn(*mut c_void) -> *mut c_void, child_stack: *mut c_void, flags: c_ulong, arg: *mut c_void, ptid: *mut pid_t, newtls: *mut c_void, ctid: *mut pid_t) -> pid_t { // assuming the first arg below is correct... __steed_clone(sc::nr::CLONE, fn_, child_stack, flags, arg, ptid, newtls, ctid) }

Now, in the assembly, can't you retrieve the syscall number from rax or whatever the correct register is?

Yes, that would be possible, but not zero-cost. :) It would actually pass that parameter at runtime in this case.

japaric · 2017-04-22T14:13:27Z

Excellent work, @tbu-! 💯

anatol · 2017-05-05T18:43:03Z

thread::current is needed for reentrant mutexes. @tbu- do you plan to implement thread::current?

tbu- · 2017-05-05T20:20:47Z

Yes, I do. However, I'm also implementing mutexes, I hope we're not duplicating work.

anatol · 2017-05-05T20:24:54Z

anatol@2e05d1a

tbu- · 2017-05-05T20:33:42Z

tbu-@b1a8b91 :/

How did you do it? I copied the stuff from parking_lot.

anatol · 2017-05-05T20:57:04Z

I implemented rwlock/mutex using atomics, somewhat similar to musl implementation. https://github.com/anatol/steed/blob/2e05d1ac3ef94fe91d0bfb1e2b6eccf281c6f419/src/sys/linux/mutex.rs . Reentrant mutexes fail to compile as thread::current is missing. Also I need to review memory ordering - currently I use Ordering::Relaxing and it works on x86_64 but my gut feeling says it might not be enough for architectures with weak memory models.

I never used parking_lot but their implementation looks too complicated to me.

japaric · 2017-05-06T13:41:07Z

I'd personally prefer to use parking-lot's implementation. It's been out there (and been tested) for a while and have quite a few advantages over libc's, like a const-fn constructors and smaller memory usage.

anatol · 2017-05-06T14:42:20Z

I guess it it matter of priorities but it makes sense to avoid complexity and start with a simple textbook implementation for synchronization. The simple atomics based solution is widely known and used in many places, including musl.

tbu- mentioned this pull request Apr 7, 2017

[RFC] split platform support in two tiers #121

Merged

tbu- force-pushed the pr_thread_basic branch from 43fc651 to b943119 Compare April 14, 2017 22:52

japaric reviewed Apr 15, 2017

View reviewed changes

tbu- force-pushed the pr_thread_basic branch from 3663209 to 3cfc3cc Compare April 15, 2017 14:40

tbu- added 2 commits April 19, 2017 20:55

Add basic thread

9177e93

It supports `spawn` and `join` so far. Only works on x86_64 so far. CC japaric#13.

Add x86 support for threads

38181bd

tbu- added 5 commits April 19, 2017 20:55

Add PowerPC support for threads

46743c6

Fix typo

360456a

Address pull request comments, add TODOs and issues

9b9e452

Mark thread stuff as unimplemented!() on unsupported platforms

ee4292d

Add thread test to CI, on supported platforms

3e66df6

tbu- force-pushed the pr_thread_basic branch from 52f2697 to de3632e Compare April 21, 2017 13:47

Use global_asm! instead of #[naked] functions

835f2c2

This allows us to write the whole function in assembly without hacks.

tbu- force-pushed the pr_thread_basic branch from de3632e to 835f2c2 Compare April 21, 2017 14:17

tbu- mentioned this pull request Apr 21, 2017

Update the remaining parts to Rust 1.16.0 #140

Merged

mrhota reviewed Apr 22, 2017

View reviewed changes

japaric merged commit 38c9086 into japaric:master Apr 22, 2017

homunkulus mentioned this pull request Apr 22, 2017

Almost finish the env module #141

Merged

japaric mentioned this pull request Apr 23, 2017

Implement thread_local storage #111

Closed

Add basic thread #110

Add basic thread #110

Conversation

tbu- commented Feb 18, 2017

tbu- commented Feb 18, 2017

anatol commented Mar 23, 2017

tbu- commented Mar 23, 2017

anatol commented Mar 23, 2017

tbu- commented Mar 24, 2017

homunkulus commented Apr 12, 2017

tbu- commented Apr 14, 2017

homunkulus commented Apr 14, 2017

japaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbu- commented Apr 15, 2017

homunkulus commented Apr 15, 2017

homunkulus commented Apr 15, 2017

tbu- commented Apr 15, 2017

homunkulus commented Apr 15, 2017

japaric commented Apr 15, 2017

tbu- commented Apr 15, 2017

tbu- commented Apr 15, 2017

homunkulus commented Apr 15, 2017

homunkulus commented Apr 15, 2017

tbu- commented Apr 15, 2017

tbu- commented Apr 15, 2017

tbu- commented Apr 15, 2017

japaric commented Apr 15, 2017

tbu- commented Apr 15, 2017

japaric commented Apr 15, 2017 • edited Loading

tbu- commented Apr 21, 2017

homunkulus commented Apr 21, 2017

homunkulus commented Apr 21, 2017

japaric commented Apr 21, 2017

tbu- commented Apr 21, 2017

homunkulus commented Apr 21, 2017

homunkulus commented Apr 21, 2017

tbu- commented Apr 21, 2017

Choose a reason for hiding this comment

mrhota Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japaric commented Apr 22, 2017

anatol commented May 5, 2017

tbu- commented May 5, 2017

anatol commented May 5, 2017

tbu- commented May 5, 2017

anatol commented May 5, 2017

japaric commented May 6, 2017

anatol commented May 6, 2017

Add basic `thread` #110

Add basic `thread` #110

japaric commented Apr 15, 2017 •

edited

Loading

mrhota Apr 22, 2017 •

edited

Loading