Skip to content

/rust

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

rustc: Group linked libraries where needed #49316

Merged
merged 1 commit into from Mar 30, 2018

Conversation

Reviewers

@michaelwoerister michaelwoerister

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
5 participants
@alexcrichton @rust-highfive @bors @japaric @michaelwoerister
@alexcrichton
Copy link
Member

alexcrichton commented Mar 23, 2018

This commit fixes a longstanding issue with the compiler with circular
dependencies between libcore and libstd. The core crate requires at least one
symbol, the ability to unwind. The std crate is the crate which actually
defines this symbol, but the std crate also depends on the core crate.

This circular dependency is in general disallowed in Rust as crates cannot have
cycles amongst them. A special exception is made just for core/std, but this is
also unfortunately incompatible with how GNU linkers work. GNU linkers will
process undefined symbols in a left-to-right fashion, only actually linking an
rlib like libstd if there are any symbols used from it. This strategy is
incompatible with circular dependencies because if we otherwise don't use
symbols from libstd we don't discover that we needed it until we're later
processing libcore's symbols!

To fix this GNU linkers support the --start-group and --end-group options
which indicate "libraries between these markers may have circular dependencies
amongst them. The linker invocation has been updated to automatically pass these
arguments when we're invoking a GNU linker and automatically calculate where the
arguments need to go (around libstd and libcore)

Closes #18807
Closes #47074

@rust-highfive rust-highfive assigned michaelwoerister Mar 23, 2018

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Mar 23, 2018

r? @michaelwoerister

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

@rust-highfive rust-highfive added the S-waiting-on-review label Mar 23, 2018

@alexcrichton alexcrichton force-pushed the alexcrichton:start-group-end-group branch from 7394e66 to 60e6480 Mar 23, 2018

@alexcrichton alexcrichton referenced this pull request Mar 23, 2018

Closed

Multiple codegen units breaks linking: "undefined reference to rust_begin_unwind" #47074

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 23, 2018

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

@alexcrichton alexcrichton force-pushed the alexcrichton:start-group-end-group branch 3 times, most recently from 3b41480 to eec0d65 Mar 23, 2018

@japaric japaric referenced this pull request Mar 27, 2018

Closed

Fix linking issues when using multiple codegen units #53

1 of 1 task complete
@michaelwoerister

michaelwoerister reviewed Mar 27, 2018
View changes

Copy link
Contributor

michaelwoerister left a comment

Thanks, @alexcrichton! r=me with the nits addressed.
src/librustc_trans/back/link.rs Outdated
// Linkage::NotLinked |
// Linkage::IncludedFromDylib => continue,
// _ => {}
// }

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Mar 27, 2018

Contributor

Did you leave this in on purpose?

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Mar 27, 2018

Author Member

er oops, no!
src/librustc_trans/back/link.rs
if group_end.is_some() && group_start.is_none() {
group_end = None;
}

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Mar 27, 2018

Contributor

Maybe add an assert!(group_start.is_some() == group_end.is_some())?

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Mar 27, 2018

Author Member

Ah unfortunately that assertion tripped when I added it, so I ended up having to implement it this way for libstd specifically

@alexcrichton alexcrichton force-pushed the alexcrichton:start-group-end-group branch from eec0d65 to 8ad2f7f Mar 27, 2018

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Mar 27, 2018

@bors: r=michaelwoerister
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 27, 2018

📌 Commit 8ad2f7f has been approved by michaelwoerister

@bors bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 27, 2018

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 28, 2018

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

@bors bors added S-waiting-on-author and removed S-waiting-on-bors labels Mar 28, 2018

@alexcrichton alexcrichton force-pushed the alexcrichton:start-group-end-group branch from 8ad2f7f to 50ad5f9 Mar 28, 2018

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Mar 28, 2018

@bors: r=michaelwoerister
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 28, 2018

📌 Commit 50ad5f9 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 28, 2018

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 29, 2018

⌛️ Testing commit 50ad5f9 with merge 0579f55...

bors added a commit that referenced this pull request Mar 29, 2018

@bors
Auto merge of #49316 - alexcrichton:start-group-end-group, r=michaelw… 
…oerister

rustc: Group linked libraries where needed

This commit fixes a longstanding issue with the compiler with circular
dependencies between libcore and libstd. The `core` crate requires at least one
symbol, the ability to unwind. The `std` crate is the crate which actually
defines this symbol, but the `std` crate also depends on the `core` crate.

This circular dependency is in general disallowed in Rust as crates cannot have
cycles amongst them. A special exception is made just for core/std, but this is
also unfortunately incompatible with how GNU linkers work. GNU linkers will
process undefined symbols in a left-to-right fashion, only actually linking an
rlib like libstd if there are any symbols used from it. This strategy is
incompatible with circular dependencies because if we otherwise don't use
symbols from libstd we don't discover that we needed it until we're later
processing libcore's symbols!

To fix this GNU linkers support the `--start-group` and `--end-group` options
which indicate "libraries between these markers may have circular dependencies
amongst them. The linker invocation has been updated to automatically pass these
arguments when we're invoking a GNU linker and automatically calculate where the
arguments need to go (around libstd and libcore)

Closes #18807
Closes #47074
Loading status checks…
0579f55
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 29, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review and removed S-waiting-on-bors labels Mar 29, 2018

@japaric japaric referenced this pull request Mar 29, 2018

Merged

Initial commit of the second newsletter. Still some TODOs for @japaric #73

@alexcrichton
rustc: Group linked libraries where needed 
This commit fixes a longstanding issue with the compiler with circular
dependencies between libcore and libstd. The `core` crate requires at least one
symbol, the ability to unwind. The `std` crate is the crate which actually
defines this symbol, but the `std` crate also depends on the `core` crate.

This circular dependency is in general disallowed in Rust as crates cannot have
cycles amongst them. A special exception is made just for core/std, but this is
also unfortunately incompatible with how GNU linkers work. GNU linkers will
process undefined symbols in a left-to-right fashion, only actually linking an
rlib like libstd if there are any symbols used from it. This strategy is
incompatible with circular dependencies because if we otherwise don't use
symbols from libstd we don't discover that we needed it until we're later
processing libcore's symbols!

To fix this GNU linkers support the `--start-group` and `--end-group` options
which indicate "libraries between these markers may have circular dependencies
amongst them. The linker invocation has been updated to automatically pass these
arguments when we're invoking a GNU linker and automatically calculate where the
arguments need to go (around libstd and libcore)

Closes #18807
Closes #47074
Loading status checks…
88114f6

@alexcrichton alexcrichton force-pushed the alexcrichton:start-group-end-group branch from 50ad5f9 to 88114f6 Mar 29, 2018

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Mar 29, 2018

@bors: r=michaelwoerister
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 29, 2018

📌 Commit 88114f6 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 29, 2018

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 29, 2018

⌛️ Testing commit 88114f6 with merge 491f4bf...

bors added a commit that referenced this pull request Mar 29, 2018

@bors
Auto merge of #49316 - alexcrichton:start-group-end-group, r=michaelw… 
…oerister

rustc: Group linked libraries where needed

This commit fixes a longstanding issue with the compiler with circular
dependencies between libcore and libstd. The `core` crate requires at least one
symbol, the ability to unwind. The `std` crate is the crate which actually
defines this symbol, but the `std` crate also depends on the `core` crate.

This circular dependency is in general disallowed in Rust as crates cannot have
cycles amongst them. A special exception is made just for core/std, but this is
also unfortunately incompatible with how GNU linkers work. GNU linkers will
process undefined symbols in a left-to-right fashion, only actually linking an
rlib like libstd if there are any symbols used from it. This strategy is
incompatible with circular dependencies because if we otherwise don't use
symbols from libstd we don't discover that we needed it until we're later
processing libcore's symbols!

To fix this GNU linkers support the `--start-group` and `--end-group` options
which indicate "libraries between these markers may have circular dependencies
amongst them. The linker invocation has been updated to automatically pass these
arguments when we're invoking a GNU linker and automatically calculate where the
arguments need to go (around libstd and libcore)

Closes #18807
Closes #47074
Loading status checks…
491f4bf
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 491f4bf to master...

@bors bors merged commit 88114f6 into rust-lang:master Mar 30, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Bobo1239 Bobo1239 referenced this pull request Mar 30, 2018

Closed

upgrade OpenSSL to 1.0.2p #180

@alexcrichton alexcrichton deleted the alexcrichton:start-group-end-group branch Mar 30, 2018

@glandium glandium referenced this pull request Mar 31, 2018

Closed

It's impossible to write a panic crate for no_std #48661

@japaric

This comment has been minimized.

Sign in to view
Copy link
Member

japaric commented Apr 3, 2018

@alexcrichton it seems that panic_fmt is not in the "missing lang items" list so this logic doesn't add {start,end}-group when only that lang item is in play.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Apr 3, 2018

@japaric hm it's listed here but I may be misremembering what that's used for. Do you have an example I can poke around?
@japaric

This comment has been minimized.

Sign in to view
Copy link
Member

japaric commented Apr 4, 2018

@alexcrichton #48661 (comment) still produces a shared library that contains an undefined reference to rust_begin_unwind. The linker invocation looks like this:

"cc"
"-Wl,--as-needed"
"-Wl,-z,noexecstack"
"-m64"
"-L"
"$SYSROOT/lib/rustlib/x86_64-unknown-linux-gnu/lib"
"$PWD/target/release/deps/bar.bar0.rcgu.o"
"-o"
"$PWD/target/release/deps/libbar.so"
"-Wl,--version-script=/tmp/rustc.NVjXDd5FIjVZ/list"
"-Wl,--gc-sections"
"-Wl,-z,relro,-z,now"
"-Wl,-O1"
"-nodefaultlibs"
"-L"
"/home/japaric/tmp/bar/target/release/deps"
"-L"
"$SYSROOT/lib/rustlib/x86_64-unknown-linux-gnu/lib"
"-Wl,-Bstatic"
# -Wl,--start-group
"$PWD/target/release/deps/libpanic-3f5354702551e825.rlib"
"$SYSROOT/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-4d12ce26aecbbf20.rlib"
# -Wl,--end-group
"-shared"
"-Wl,-Bdynamic"

I would expect the logic added in this PR to add the {start,end}-group flags in the places where I added comments, but today's nightly doesn't add those flags to the linker invocation. If you re-invoke the linker with the {start,end}-group flags you get a shared library with a defined rust_begin_unwind symbol.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Apr 4, 2018

Ok thanks for the info! I think I've narrowed this down and am testing a fix

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 4, 2018

@alexcrichton
Fix another circulare deps link args issue 
It turns out that the support in rust-lang#49316 wasn't enough to handle all cases
notably the example in rust-lang#48661. The underlying bug was connected to panic=abort
where lang items were listed in the `missing_lang_items` sets but didn't
actually exist anywhere.

This caused the linker backend to deduce that start-group/end-group wasn't
needed because not all items were defined. Instead the missing lang items that
don't actually need to have a definition are filtered out and not considered for
the start-group/end-group arguments

Closes rust-lang#48661
Loading status checks…
48ede3f

@alexcrichton alexcrichton referenced this pull request Apr 4, 2018

Merged

Fix another circular deps link args issue #49672

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Apr 4, 2018

I've posted #49672 with a fix for that

bors added a commit that referenced this pull request Apr 7, 2018

@bors
Auto merge of #49672 - alexcrichton:fix-another-std-core-cycle, r=mic… 
…haelwoerister

Fix another circular deps link args issue

It turns out that the support in #49316 wasn't enough to handle all cases
notably the example in #48661. The underlying bug was connected to panic=abort
where lang items were listed in the `missing_lang_items` sets but didn't
actually exist anywhere.

This caused the linker backend to deduce that start-group/end-group wasn't
needed because not all items were defined. Instead the missing lang items that
don't actually need to have a definition are filtered out and not considered for
the start-group/end-group arguments

Closes #48661
Loading status checks…
7807074

toidiu pushed a commit to toidiu/rust that referenced this pull request Apr 10, 2018

@alexcrichton toidiu
Fix another circulare deps link args issue 
It turns out that the support in rust-lang#49316 wasn't enough to handle all cases
notably the example in rust-lang#48661. The underlying bug was connected to panic=abort
where lang items were listed in the `missing_lang_items` sets but didn't
actually exist anywhere.

This caused the linker backend to deduce that start-group/end-group wasn't
needed because not all items were defined. Instead the missing lang items that
don't actually need to have a definition are filtered out and not considered for
the start-group/end-group arguments

Closes rust-lang#48661
ae261d5

This was referenced Jun 20, 2018

Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.