[x86_64-fortanix-unknown-sgx] Failing stdlib threading test

[sardok]

After PR-144465 got merged, the test named test_named_thread (in library/src/std/thread/tests.rs) started failing for sgx target. I managed to find that, the breaking change is ThreadInit refactoring. For some reason, the call to set_current in ThreadInit::init looks ineffective. When i move set_current & set_name calls from ThreadInit::init to the rust_start closure (just like pre-PR), the test passes.

Here the changes that fixes this failure:

diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs
index 983d189b070..0565c4ba98f 100644
--- a/library/std/src/thread/mod.rs
+++ b/library/std/src/thread/mod.rs
@@ -229,14 +229,15 @@ pub fn init(self: Box<Self>) -> Box<dyn FnOnce() + Send> {
         // so that it may call std::thread::current() in its implementation. This is also
         // why we take Box<Self>, to ensure the Box is not destroyed until after this point.
         // Cloning the handle does not invoke the global allocator, it is an Arc.
-        if let Err(_thread) = set_current(self.handle.clone()) {
-            // The current thread should not have set yet. Use an abort to save binary size (see #123356).
-            rtabort!("current thread handle already set during thread spawn");
-        }
+        // if let Err(_thread) = set_current(self.handle.clone()) {
+        //     // The current thread should not have set yet. Use an abort to save binary size (see #123356).
+        //     rtabort!("current thread handle already set during thread spawn");
+        // }

-        if let Some(name) = self.handle.cname() {
-            imp::set_name(name);
-        }
+        // if let Some(name) = self.handle.cname() {
+        //     imp::set_name(name);
+        // }
+        let _ = self.handle;

         self.rust_start
     }
@@ -572,10 +573,21 @@ fn drop(&mut self) {
         }

         let f = MaybeDangling::new(f);
+        let their_thread = thread.clone();

         // The entrypoint of the Rust thread, after platform-specific thread
         // initialization is done.
         let rust_start = move || {
+
+            if let Err(_thread) = set_current(their_thread.clone()) {
+                // The current thread should not have set yet. Use an abort to save binary size (see #123356).
+                rtabort!("current thread handle already set during thread spawn");
+            }
+
+            if let Some(name) = their_thread.cname() {
+                imp::set_name(name);
+            }
+
             let f = f.into_inner();
             let try_result = panic::catch_unwind(panic::AssertUnwindSafe(|| {
                 crate::sys::backtrace::__rust_begin_short_backtrace(|| hooks.run());

The PR-144465 is in beta branch now.

Thank you.

[jethrogb]

The thread code is full of globals that get duplicated between cfg(test) and cfg(not(test)) code:

$ objdump -t build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/std-b60fcc1ca1d183d8|c++filt|awk '$3 != "F"'| grep ::thread::|rev|sort|rev
00000000002610e0 l     O .bss	0000000000000010 .hidden std[3451035a9c168709]::thread::current::id::ID
00000000002611c0 l     O .bss	0000000000000010 .hidden std[36fff146dfcc5e9e]::thread::current::id::ID
0000000000260e38 g     O .data	0000000000000028 __rust_internals::std::sys::pal::sgx::thread::TASK_QUEUE
0000000000260cc8 l     O .data	0000000000000010 std[3451035a9c168709]::thread::spawnhook::SPAWN_HOOKS::{closure#0}::__RUST_STD_INTERNAL_VAL
0000000000260e18 g     O .data	0000000000000010 std[36fff146dfcc5e9e]::thread::spawnhook::SPAWN_HOOKS::{closure#0}::__RUST_STD_INTERNAL_VAL
0000000000261608 l     O .bss	0000000000000008 .hidden std[36fff146dfcc5e9e]::thread::main_thread::MAIN
0000000000261060 l     O .bss	0000000000000008 <std[3451035a9c168709]::thread::Builder>::spawn_unchecked_::{closure#0}::MIN
0000000000261160 g     O .bss	0000000000000008 <std[36fff146dfcc5e9e]::thread::Builder>::spawn_unchecked_::{closure#0}::MIN
0000000000261068 l     O .bss	0000000000000008 .hidden <std[3451035a9c168709]::thread::ThreadId>::new::COUNTER
0000000000261168 l     O .bss	0000000000000008 .hidden <std[36fff146dfcc5e9e]::thread::ThreadId>::new::COUNTER
0000000000261078 l     O .bss	0000000000000010 .hidden std[3451035a9c168709]::thread::current::CURRENT
00000000002611d8 g     O .bss	0000000000000010 std[36fff146dfcc5e9e]::thread::current::CURRENT

So cfg(test) and cfg(not(test)) code have different views of the world.

https://github.com/rust-lang/rust/pull/144465/files#diff-7b5285795b588a41d58f849c1cf98979c8725dd34dc7834dd7897d1eadc7fc0eR578
The closure is created by a function called from cfg(test) code. So the closure calls cfg(test) functions.

https://github.com/rust-lang/rust/blob/main/library/std/src/sys/pal/sgx/abi/mod.rs#L63-L73
The enclave entry code calls crate::sys::thread::Thread::entry() if it's a thread entry, but that's cfg(not(test)) code.

So when the thread starts, it's running cfg(not(test)) code. Only if it branches via function pointer (not via static call), will it enter cfg(test) code (because cfg(test) code created the thread). It used to be the case that set_current was called from the closure, so after the branch into cfg(test) code. After #144465 it's called before branching into cfg(test) code.

Here's the output of the test where I've added instrumentation to all calls that access std::thread::current::CURRENT to print the function name and whether it's the cfg(test) or cfg(not(test)) version:

[Test named thread] begin
[lib] set_current
[lib] current_or_unnamed
[lib] init_current
[Test named thread] joinning target thread id: 1
[lib] set_current
[test] current
[test] init_current
[Spawned threadid=2 / 2 name=(unnamed)] asserting ...
[lib] try_with_current
[test] drop_current
[lib] drop_current
[lib] try_with_current
[test] drop_current
[lib] drop_current

[jethrogb]

@bjorn3 you wrote in #139795 (comment):

A lot of places in libstd use #[cfg(not(test))] and instead import from "realstd" (which is the libstd that libtest links against rather than the libstd that is being tested) when testing to avoid duplicate lang items. Is there any reason why SGX can't do the same?

How do you suggest dealing with item visibility issues with this approach?

[jethrogb]

I guess the other uses just make a pub item that's perma-unstable.