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

rustbuild: Remove one LLD workaround #76127

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Conversation

petrochenkov
Copy link
Contributor

The version of LLD shipped with Rust no longer have this issue.

Closes #68647

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2020
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Aug 30, 2020

UPD: Caused by #72145, I had -Ctarget-cpu=native set.

Full ./x.py test --stage 2 still doesn't pass with LLD on MSVC, compiletest crashes on some accesses to thread-local storage when running UI tests.

./x.py test src/test/ui
Unhandled exception at 0x00007FF692F336F3 in compiletest.exe: 0xC0000005: Access violation reading location 0xFFFFFFFFFFFFFFFF. occurred
[Inline Frame] compiletest.exe!core::intrinsics::copy_nonoverlapping() Line 1858
	at C:\msys64\home\we\rust\library\core\src\intrinsics.rs(1858)
[Inline Frame] compiletest.exe!core::ptr::swap_nonoverlapping_bytes() Line 503
	at C:\msys64\home\we\rust\library\core\src\ptr\mod.rs(503)
[Inline Frame] compiletest.exe!core::ptr::swap_nonoverlapping() Line 445
	at C:\msys64\home\we\rust\library\core\src\ptr\mod.rs(445)
[Inline Frame] compiletest.exe!core::ptr::swap_nonoverlapping_one() Line 462
	at C:\msys64\home\we\rust\library\core\src\ptr\mod.rs(462)
[Inline Frame] compiletest.exe!core::mem::swap() Line 698
	at C:\msys64\home\we\rust\library\core\src\mem\mod.rs(698)
[Inline Frame] compiletest.exe!core::mem::replace() Line 833
	at C:\msys64\home\we\rust\library\core\src\mem\mod.rs(833)
[Inline Frame] compiletest.exe!std::thread::local::lazy::LazyKeyInner::initialize() Line 306
	at C:\msys64\home\we\rust\library\std\src\thread\local.rs(306)
[Inline Frame] compiletest.exe!std::thread::local::fast::Key::try_initialize() Line 427
	at C:\msys64\home\we\rust\library\std\src\thread\local.rs(427)
[Inline Frame] compiletest.exe!std::thread::local::fast::Key::get() Line 412
	at C:\msys64\home\we\rust\library\std\src\thread\local.rs(412)
compiletest.exe!std::io::stdio::LOCAL_STDERR::__getit() Line 177
	at C:\msys64\home\we\rust\library\std\src\thread\local.rs(177)
[Inline Frame] compiletest.exe!std::thread::local::LocalKey::try_with() Line 264
	at C:\msys64\home\we\rust\library\std\src\thread\local.rs(264)
compiletest.exe!std::thread::local::LocalKey::with<core::cell::RefCell<core::option::Option<alloc::boxed::Box<Write>>>,closure-0,core::option::Option<alloc::boxed::Box<Write>>>() Line 241
	at C:\msys64\home\we\rust\library\std\src\thread\local.rs(241)
compiletest.exe!std::io::stdio::set_panic() Line 832
	at C:\msys64\home\we\rust\library\std\src\io\stdio.rs(832)
compiletest.exe!test::run_test_in_process() Line 535
	at C:\msys64\home\we\rust\library\test\src\lib.rs(535)
[Inline Frame] compiletest.exe!test::run_test::run_test_inner::{{closure}}() Line 450
	at C:\msys64\home\we\rust\library\test\src\lib.rs(450)
compiletest.exe!std::sys_common::backtrace::__rust_begin_short_backtrace<closure-0,tuple<>>() Line 140
	at C:\msys64\home\we\rust\library\std\src\sys_common\backtrace.rs(140)
[Inline Frame] compiletest.exe!std::thread::{{impl}}::spawn_unchecked::{{closure}}::{{closure}}() Line 458
	at C:\msys64\home\we\rust\library\std\src\thread\mod.rs(458)
[Inline Frame] compiletest.exe!std::panic::{{impl}}::call_once() Line 308
	at C:\msys64\home\we\rust\library\std\src\panic.rs(308)
[Inline Frame] compiletest.exe!std::panicking::try::do_call() Line 381
	at C:\msys64\home\we\rust\library\std\src\panicking.rs(381)
[Inline Frame] compiletest.exe!std::panicking::try() Line 345
	at C:\msys64\home\we\rust\library\std\src\panicking.rs(345)
[Inline Frame] compiletest.exe!std::panic::catch_unwind() Line 382
	at C:\msys64\home\we\rust\library\std\src\panic.rs(382)
[Inline Frame] compiletest.exe!std::thread::{{impl}}::spawn_unchecked::{{closure}}() Line 457
	at C:\msys64\home\we\rust\library\std\src\thread\mod.rs(457)
compiletest.exe!core::ops::function::FnOnce::call_once<closure-0,tuple<>>() Line 227
	at C:\msys64\home\we\rust\library\core\src\ops\function.rs(227)
[Inline Frame] compiletest.exe!alloc::boxed::{{impl}}::call_once() Line 1042
	at C:\msys64\home\we\rust\library\alloc\src\boxed.rs(1042)
[Inline Frame] compiletest.exe!alloc::boxed::{{impl}}::call_once() Line 1042
	at C:\msys64\home\we\rust\library\alloc\src\boxed.rs(1042)
compiletest.exe!std::sys::windows::thread::{{impl}}::new::thread_start() Line 56
	at C:\msys64\home\we\rust\library\std\src\sys\windows\thread.rs(56)
kernel32.dll!BaseThreadInitThunk�()
ntdll.dll!RtlUserThreadStart�()

@petrochenkov
Copy link
Contributor Author

With x86_64-pc-windows-gnu things are worse, with LLD enabled build hangs when trying to build src/tools/tidy.
cc @mati865

@mati865
Copy link
Contributor

mati865 commented Aug 30, 2020

@petrochenkov can you obtain stack trace from hanged LLD on windows-gnu? I suppose it will look like LLD -> libstdc++ -> winpthreads -> Windows DLLs.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

r=me with nit resolved (whether I'm right about it or not)

let can_use_lld = mode != Mode::Std;

if let Some(host_linker) = self.linker(compiler.host, can_use_lld) {
if let Some(host_linker) = self.linker(compiler.host, true) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we always pass true now, could we drop that arg as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test suite is still run without explicit linker because there are tests that check linker search etc.
Perhaps we could switch to LLD there as well, but it may require adjusting some tests.
I'll try to do it in a separate PR.

@petrochenkov
Copy link
Contributor Author

@mati865
I'll try to provide the backtrace a bit later.

@bors r=Mark-Simulacrum rollup

@bors
Copy link
Contributor

bors commented Aug 31, 2020

📌 Commit 21c624a has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2020
@mati865
Copy link
Contributor

mati865 commented Aug 31, 2020

@petrochenkov I won't mind process dump or steps to reproduce it either, whatever is easiest for you.

@petrochenkov
Copy link
Contributor Author

@mati865
The step to reproduce is to run x.py test tidy basically.
(With

[rust]
use-lld = true

in config.toml)

@mati865
Copy link
Contributor

mati865 commented Aug 31, 2020

I cannot reproduce the hang:

$ pacman -Qs 64-lld
local/mingw-w64-x86_64-lld 10.0.1-1
    Linker tools for LLVM (mingw-w64)

If you encounter it again stack trace or a dump would be useful.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#75938 (Added some `min_const_generics` revisions into `const_generics` tests)
 - rust-lang#76050 (Remove unused function)
 - rust-lang#76075 (datastructures: replace `once_cell` crate with an impl from std)
 - rust-lang#76115 (Restore public visibility on some parsing functions for rustfmt)
 - rust-lang#76127 (rustbuild: Remove one LLD workaround)

Failed merges:

r? @ghost
@bors bors merged commit 9caf08f into rust-lang:master Aug 31, 2020
@mati865
Copy link
Contributor

mati865 commented Sep 1, 2020

@petrochenkov I can reproduce the hang with rust-lld shipped by Rust:

    frame #0: 0x00007ffba90ebe44 ntdll.dll`NtWaitForSingleObject + 20
    frame #1: 0x00007ffba6b826ee KernelBase.dll`WaitForSingleObjectEx + 142
    frame #2: 0x000000006494209c libwinpthread-1.dll`pthread_cond_init + 556
    frame #3: 0x00000000649421de libwinpthread-1.dll`pthread_cond_init + 878
    frame #4: 0x000000006494246f libwinpthread-1.dll`pthread_cond_signal + 207
    frame #5: 0x00000000029a5119 rust-lld.exe`std::condition_variable::notify_one() + 9
    frame #6: 0x000000000218854c rust-lld.exe`llvm::parallel::detail::TaskGroup::spawn(std::function<void ()>) + 412
    frame #7: 0x0000000000ffd62b rust-lld.exe`(anonymous namespace)::Writer::run() + 5499
    frame #8: 0x000000000119e465 rust-lld.exe`lld::coff::writeResult() + 837
    frame #9: 0x00000000011aa4b4 rust-lld.exe`lld::coff::LinkerDriver::link(llvm::ArrayRef<char const*>) + 30628
    frame #10: 0x00000000011c99b3 rust-lld.exe`lld::coff::link(llvm::ArrayRef<char const*>, bool, llvm::raw_ostream&, ll
vm::raw_ostream&) + 243
    frame #11: 0x000000000122ba1e rust-lld.exe`lld::mingw::link(llvm::ArrayRef<char const*>, bool, llvm::raw_ostream&, l
lvm::raw_ostream&) + 12894
    frame #12: 0x0000000002c4143b rust-lld.exe`main + 1243
    frame #13: 0x00000000004013f8 rust-lld.exe`__tmainCRTStartup + 584
    frame #14: 0x000000000040151b rust-lld.exe
    frame #15: 0x00007ffba82f6fd4 kernel32.dll`BaseThreadInitThunk + 20
    frame #16: 0x00007ffba909cec1 ntdll.dll`RtlUserThreadStart + 33

Short explanation: ancient winpthreads library used by CI is quite buggy, this hang is caused by one of them (libstdc++ uses winpthreads for C++ multithreading). I'll try to raise topic about upgrading mingw-w64 once again...

@petrochenkov
Copy link
Contributor Author

@mati865
Thanks for getting to this sooner than me.
Yes, x.py with use-lld = true uses rust-lld shipped with Rust, I didn't try the one from pacman.

@mati865
Copy link
Contributor

mati865 commented Sep 2, 2020

Yes, x.py with use-lld = true uses rust-lld shipped with Rust, I didn't try the one from pacman.

Maybe for msvc (I haven't really investigated it) but for other targets this passes -fuse-ld=lld to the linker: #75111

@petrochenkov
Copy link
Contributor Author

Ok, I'll test again and look why rust-lld was used in my case.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 5, 2020
…imulacrum

Fix rust.use-lld when linker is not set

Fixes rust-lang#76127 (comment)

Previously when `[<target>].linker` was not configured `rust.use-lld` would set it to `rust-lld` on platforms where it should not.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 5, 2020
…imulacrum

Fix rust.use-lld when linker is not set

Fixes rust-lang#76127 (comment)

Previously when `[<target>].linker` was not configured `rust.use-lld` would set it to `rust-lld` on platforms where it should not.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2020
…ulacrum

Fix rust.use-lld when linker is not set

Fixes rust-lang#76127 (comment)

Previously when `[<target>].linker` was not configured `rust.use-lld` would set it to `rust-lld` on platforms where it should not.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2020
rustbuild: Build tests with LLD if `use-lld = true` was passed

Addresses rust-lang#76127 (comment).

Our test suite is generally ready to run with an explicitly specified linker (rust-lang#45191),
 so LLD specified with `use-lld = true` works as well.

Only 4 tests fail (on `x86_64-pc-windows-msvc`):
```
ui/panic-runtime/lto-unwind.rs
run-make-fulldeps/debug-assertions
run-make-fulldeps/foreign-exceptions
run-make-fulldeps/test-harness
```
All of them are legitimate issues with LLD (or at least with combination Rust+LLD) and manifest in segfaults on access to TLS (rust-lang#76127 (comment)). UPD: These issues are caused by rust-lang#72145 and appear because I had `-Ctarget-cpu=native` set.

UPD: Further commits build tests with LLD for non-MSVC targets and propagate LLD to more places when `use-lld` is enabled.
@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linking libtest with lld-link results in undefined symbols
6 participants