When testing std, don't create two variants of threading internals #58399

jethrogb · 2019-02-12T12:24:14Z

When compiling the unit tests of std, there are basically two copies of std: one normal one, and one with cfg(test). This can lead to problems if both copies think they uniquely define a global resource.

One such global resource is the static variables used for the Thread Local Storage implementation on some platforms, including SGX and possibly Redox (cc @jackpot51). Another is the static variables used for the Task Queue implementation on some platforms, including SGX. This PR does two things:

Instead of having a cfg(test)-copy of mod thread, just re-export realstd::thread. Move all the unit tests that were in that module to a separate module. This all leads to some dead code fallout, so add allow directives in various places.
Use conditional compilation on SGX to detect misuse of the cfg(test)-version of the implementation early.

I didn't audit std for other uses of static that might be problematic.

rust-highfive · 2019-02-12T12:24:25Z

r? @dtolnay

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

jethrogb · 2019-02-12T12:24:33Z

src/libstd/io/stdio.rs

@@ -7,6 +7,9 @@ use io::{self, Initializer, BufReader, LineWriter};
 use sync::{Arc, Mutex, MutexGuard};
 use sys::stdio;
 use sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard};
+#[cfg(test)]
+use realstd::thread::LocalKey;
+#[cfg(not(test))]


This is... ugly. But I don't really see another way.

jethrogb · 2019-02-12T14:17:31Z

Note for people following along via email: I updated the description to include info about the second commit.

Centril · 2019-02-12T14:39:56Z

r? @alexcrichton

bors · 2019-02-12T15:22:56Z

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

rust-highfive · 2019-02-12T15:43:21Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:0c41e893:start=1549981032816462754,finish=1549981033780339075,duration=963876321
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:09:19] 
[01:09:19] running 119 tests
[01:09:44] .iiiii...i.....i..i...i..i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i.i. 100/119
[01:09:48] i......iii.i.....ii
[01:09:48] 
[01:09:48]  finished in 29.660
[01:09:48] travis_fold:end:test_debuginfo

---
[01:25:38] travis_fold:start:test_stage1-std
travis_time:start:test_stage1-std
Testing std stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:25:38]    Compiling std v0.0.0 (/checkout/src/libstd)
[01:25:50] error[E0599]: no method named `as_inner` found for type `&realstd::thread::JoinHandle<T>` in the current scope
[01:25:50]   --> src/libstd/sys/unix/ext/thread.rs:35:14
[01:25:50]    |
[01:25:50] 35 |         self.as_inner().id() as RawPthread
[01:25:50]    |
[01:25:50]    = help: items from traits can only be used if the trait is in scope
[01:25:50]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:25:50]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:25:50]            `use realstd::sys_common::AsInner;`
[01:25:50] 
[01:25:50] error[E0599]: no method named `into_inner` found for type `realstd::thread::JoinHandle<T>` in the current scope
[01:25:50]   --> src/libstd/sys/unix/ext/thread.rs:39:14
[01:25:50]    |
[01:25:50] 39 |         self.into_inner().into_id() as RawPthread
[01:25:50]    |
[01:25:50]    = help: items from traits can only be used if the trait is in scope
[01:25:50]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:25:50]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:25:50]            `use realstd::sys_common::IntoInner;`
[01:25:50] error: aborting due to 2 previous errors
[01:25:50] 
[01:25:50] For more information about this error, try `rustc --explain E0599`.
[01:25:50] error: Could not compile `std`.
[01:25:50] error: Could not compile `std`.
[01:25:50] 
[01:25:50] To learn more, run the command again with --verbose.
[01:25:50] 
[01:25:50] 
[01:25:50] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:25:50] 
[01:25:50] 
[01:25:50] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:25:50] Build completed unsuccessfully in 0:28:13
[01:25:50] Build completed unsuccessfully in 0:28:13
[01:25:50] make: *** [check] Error 1
[01:25:50] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1f331595
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Feb 12 15:43:15 UTC 2019
---
travis_time:end:013c56ae:start=1549986197309356229,finish=1549986197314499681,duration=5143452
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00868346
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2a293754
travis_time:start:2a293754
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0bab5b66
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2019-02-12T17:52:15Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:00810a80:start=1549988685461705896,finish=1549988686423445690,duration=961739794
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:10:20] 
[01:10:20] running 119 tests
[01:10:45] .iiiii...i.....i..i...i..i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i.i. 100/119
[01:10:49] i......iii.i.....ii
[01:10:49] 
[01:10:49]  finished in 28.907
[01:10:49] travis_fold:end:test_debuginfo

---
[01:27:00]    |
[01:27:00] 82 | #[allow_internal_unstable]
[01:27:00]    | ^^^^^^^^^^^^^^^^^^^^^^^^^^
[01:27:00] 
[01:27:12] error[E0599]: no method named `as_inner` found for type `&realstd::thread::JoinHandle<T>` in the current scope
[01:27:12]   --> src/libstd/sys/unix/ext/thread.rs:35:14
[01:27:12]    |
[01:27:12] 35 |         self.as_inner().id() as RawPthread
[01:27:12]    |
[01:27:12]    = help: items from traits can only be used if the trait is in scope
[01:27:12]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:27:12]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:27:12]            `use realstd::sys_common::AsInner;`
[01:27:12] 
[01:27:12] error[E0599]: no method named `into_inner` found for type `realstd::thread::JoinHandle<T>` in the current scope
[01:27:12]   --> src/libstd/sys/unix/ext/thread.rs:39:14
[01:27:12]    |
[01:27:12] 39 |         self.into_inner().into_id() as RawPthread
[01:27:12]    |
[01:27:12]    = help: items from traits can only be used if the trait is in scope
[01:27:12]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:27:12]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:27:12]            `use realstd::sys_common::IntoInner;`
[01:27:12] error: aborting due to 2 previous errors
[01:27:12] 
[01:27:12] For more information about this error, try `rustc --explain E0599`.
[01:27:12] error: Could not compile `std`.
[01:27:12] error: Could not compile `std`.
[01:27:12] 
[01:27:12] To learn more, run the command again with --verbose.
[01:27:12] 
[01:27:12] 
[01:27:12] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:27:12] 
[01:27:12] 
[01:27:12] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:27:12] Build completed unsuccessfully in 0:28:41
[01:27:12] Build completed unsuccessfully in 0:28:41
[01:27:12] make: *** [check] Error 1
[01:27:12] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:00170f4e
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Feb 12 17:52:09 UTC 2019
---
travis_time:end:073fce48:start=1549993931372101060,finish=1549993931435820440,duration=63719380
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:10271d7e
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0271a0e2
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

jethrogb · 2019-02-13T07:12:10Z

Ok, this should be ready for review now. I originally tried to surgically replace use of the affected globals (see e.g. a982943da5fb3f288cfa7e99fc218b34159ac695), but they're hidden behind various layers of abstraction that are not accessible through the realstd public API.

I've come up with some alternatives that could be considered instead:

Make the globals available behind a perma-unstable feature gate, and then enable that feature in cfg(test). This will “poison” various types involved and will require making them pub.
Move the globals to a separate crate that std depends on. However, some of the globals use types defined in std (Once, Mutex), so that doesn't seem possible.
Weak linkage?

rust-highfive · 2019-02-13T08:02:49Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:09150242:start=1550042739175615429,finish=1550042741680382993,duration=2504767564
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
tidy check
[00:03:35] * 565 error codes
[00:03:35] * highest error code: E0722
[00:03:36] * 250 features
[00:03:36] tidy error: /checkout/src/libstd/thread/tests.rs:1: platform-specific cfg: cfg(not(target_os = "emscripten"))
[00:03:36] tidy error: /checkout/src/libstd/thread/tests.rs:251: platform-specific cfg: cfg(not(target_os = "emscripten"))
[00:03:37] some tidy checks failed
[00:03:37] 
[00:03:37] 
[00:03:37] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:37] 
[00:03:37] 
[00:03:37] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:37] Build completed unsuccessfully in 0:00:46
[00:03:37] Build completed unsuccessfully in 0:00:46
[00:03:37] Makefile:68: recipe for target 'tidy' failed
[00:03:37] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0aff0754
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Feb 13 07:29:29 UTC 2019
---
travis_time:end:086cb752:start=1550042970347779856,finish=1550042970352792158,duration=5012302
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:118c4038
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:078ec5f4
travis_time:start:078ec5f4
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:00e50a28
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

jethrogb · 2019-02-13T10:35:43Z

There are some remaining unit tests in src/libstd/sys_common/thread_local.rs that I don't know what to do with. I could remove them, assuming we have sufficient coverage in the tests that use thread_local!?

jethrogb · 2019-02-13T12:01:34Z

Closing this in favor of using #[export_name] and #[linkage] attributes. @jackpot51 you might want to do the same for Redox.

rust-highfive assigned dtolnay Feb 12, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2019

jethrogb commented Feb 12, 2019

View reviewed changes

jethrogb changed the title ~~When testing std, don't create two variants of thread_local!~~ When testing std, don't create two variants of threading internals Feb 12, 2019

rust-highfive assigned alexcrichton and unassigned dtolnay Feb 12, 2019

jethrogb force-pushed the jb/std-test-thread_local branch from fa3df5c to 4612722 Compare February 12, 2019 15:59

jethrogb changed the title ~~When testing std, don't create two variants of threading internals~~ [WIP] When testing std, don't create two variants of threading internals Feb 12, 2019

jethrogb force-pushed the jb/std-test-thread_local branch from 4612722 to 1beafad Compare February 13, 2019 07:00

jethrogb changed the title ~~[WIP] When testing std, don't create two variants of threading internals~~ When testing std, don't create two variants of threading internals Feb 13, 2019

jethrogb force-pushed the jb/std-test-thread_local branch 2 times, most recently from 8e0b3c6 to 5a8d3a5 Compare February 13, 2019 07:24

When testing std, don't create two variants of threading internals

4ba369c

jethrogb force-pushed the jb/std-test-thread_local branch from 5a8d3a5 to 4ba369c Compare February 13, 2019 08:53

jethrogb closed this Feb 13, 2019

jethrogb mentioned this pull request Apr 5, 2019

Use the parking_lot locking primitives #56410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When testing std, don't create two variants of threading internals #58399

When testing std, don't create two variants of threading internals #58399

jethrogb commented Feb 12, 2019 •

edited

Loading

rust-highfive commented Feb 12, 2019

jethrogb Feb 12, 2019

jethrogb commented Feb 12, 2019

Centril commented Feb 12, 2019

bors commented Feb 12, 2019

rust-highfive commented Feb 12, 2019

rust-highfive commented Feb 12, 2019

jethrogb commented Feb 13, 2019 •

edited

Loading

rust-highfive commented Feb 13, 2019

jethrogb commented Feb 13, 2019

jethrogb commented Feb 13, 2019

When testing std, don't create two variants of threading internals #58399

When testing std, don't create two variants of threading internals #58399

Conversation

jethrogb commented Feb 12, 2019 • edited Loading

rust-highfive commented Feb 12, 2019

jethrogb Feb 12, 2019

Choose a reason for hiding this comment

jethrogb commented Feb 12, 2019

Centril commented Feb 12, 2019

bors commented Feb 12, 2019

rust-highfive commented Feb 12, 2019

rust-highfive commented Feb 12, 2019

jethrogb commented Feb 13, 2019 • edited Loading

rust-highfive commented Feb 13, 2019

jethrogb commented Feb 13, 2019

jethrogb commented Feb 13, 2019

jethrogb commented Feb 12, 2019 •

edited

Loading

jethrogb commented Feb 13, 2019 •

edited

Loading