Github Deduplicate native libs before they are passed to the linker by ChrisDent...

Copy link

Contributor

rust-highfive commented 18 days ago

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

joshtriplett commented 18 days ago

This seems reasonable, but also, I think it's a good motivation to do #76992 as well. With that change, it'd be reasonable to deduplicate libraries even if they're not adjacent.

petrochenkov commented 17 days ago

@joshtriplett
This is a not a trivial problem, there are always issues like #84254 or https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/spec/windows_gnu_base.rs#L33 that require preserving library order or disabling deduplication even in presence of --(start,end)-group (which also isn't necessarily supported on all platforms).

That's why I want to land the linking modifiers infra (#83507) first, and not change or fix any deduplication behavior (including #73319) until a modifier for disabling/enabling deduplication is added.
I plan to work on this in May/June (I just got busy with some unexpected life stuff last week, so I may not start this work immediately).

(FWIW, I don't think MSxDOS/ntapi#2 is purely a rustc's issue, with macro expansion you can do a lot of things slowing down you compilation deliberately, not necessarily related to linking.)

ChrisDenton commented 17 days ago

Consider the following linker library order:

libA mylib libA libA libA libB

As far as I'm aware (please correct me if I'm wrong), this is exactly the same as:

libA mylib libA libB

Which is all this PR currently does. However the following is not the same, even if the linker is allowed to circle back to the start:

libA mylib libB

In the first cases, if a needed symbol is unresolved in mylib it will first be looked for in libA. In the last case it will look for the unresolved symbols in libB. In essence, from the view of mylib, it's swapped the order of libA and libB:

libA mylib libB libA ...

Therefore I think this PR is the most that can be done without potentially breaking some linking strategies.

petrochenkov commented 17 days ago

Yeah, this PR should be pretty safe (from the description, I didn't check the code), modulo the "mingw linker + mixed import-static library" case linked above.

jackh726 commented 17 days ago

I'm definitely not a good reviewer for this. So let's r? @petrochenkov

ChrisDenton commented 17 days ago

modulo the "mingw linker + mixed import-static library" case linked above.

In the specific libmsvcrt.a case this PR doesn't pose a problem due to the way it's linked. In the more general case, I would (naïvely?) hope that Mingw does not otherwise produce libraries that it's own linker can't properly consume. On the other hand, if it does then it would be good if there's a test case I can use.

Either way I guess one workaround would be to allow up to one repetition. Perhaps only if the target linker is gcc/windows. Though I am slightly reluctant to introduce too many temporary hacks.

petrochenkov commented 16 days ago

I would (naïvely?) hope that Mingw does not otherwise produce libraries that it's own linker can't properly consume

This is true, libmsvcrt.a is a very special case that can be solved by a linking modifier once linking modifiers (#83507) are implemented.

@bors r+ , this is the most conservative deduplication strategy, and once the modifiers are implemented we'll probably end up with aggressive deduplication (and opt-out when necessary) which is compatible with what this PR does.

bors commented 16 days ago

Commit e40faef has been approved by petrochenkov

Dylan-DPC commented 15 days ago

likely failure of rollup ?

@bors r-

petrochenkov commented 15 days ago

Seems unlikely.
@bors r+ rollup=iffy

bors commented 15 days ago

Commit e40faef has been approved by petrochenkov

bors commented 15 days ago

Testing commit e40faef with merge b79abea265ac16320d751e3075208554fae99591...

rust-log-analyzer commented 15 days ago

A job failed! Check out the build log: (web) (plain)

  0  424M    0  926k    0     0   2261      0 54:38:56  0:06:59 54:31:57     0
  0  424M    0  926k    0     0   2256      0 54:46:12  0:07:00 54:39:12     0
  0  424M    0  926k    0     0   2250      0 54:54:58  0:07:01 54:47:57     0
  0  424M    0  926k    0     0   2248      0 54:57:54  0:07:02 54:50:52     0
curl: (56) OpenSSL SSL_read: Connection reset by peer, errno 54
clang+llvm-10.0.0-x86_64-apple-darwin/lib/libLLVMAnalysis.a: Lzma library error:  No progress is possible
tar: Error exit delayed from previous errors.
##[error]Process completed with exit code 1.
[command]/usr/local/bin/git version
git version 2.31.1
[command]/usr/local/bin/git config --local --name-only --get-regexp core\.sshCommand
[command]/usr/local/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :

bors commented 15 days ago

Test failed - checks-actions

@bors retry network error

bors commented 14 days ago

Testing commit e40faef with merge 2d11e25...

bors commented 14 days ago

Test successful - checks-actions
Approved by: petrochenkov
Pushing 2d11e25 to master...