rustc: Lower link args to `@`-files on Windows more by alexcrichton · Pull Request #47507 · rust-lang/rust

alexcrichton · 2018-01-17T01:24:30Z

When spawning a linker rustc has historically been known to blow OS limits for
the command line being too large, notably on Windows. This is especially true of
incremental compilation where there can be dozens of object files per
compilation. The compiler currently has logic for detecting a failure to spawn
and instead passing arguments via a file instead, but this failure detection
only triggers if a process actually fails to spawn.

Unfortunately on Windows we've got something else to worry about which is
cmd.exe. The compiler may be running a linker through cmd.exe where
cmd.exe has a limit of 8192 on the command line vs 32k on CreateProcess.
Moreso rustc actually succeeds in spawning cmd.exe today, it's just that after
it's running cmd.exe fails to spawn its child, which rustc doesn't currently
detect.

Consequently this commit updates the logic for the spawning the linker on
Windows to instead have a heuristic to see if we need to pass arguments via a
file. This heuristic is an overly pessimistic and "inaccurate" calculation which
just calls len on a bunch of OsString instances (where len is not
precisely the length in u16 elements). This number, when exceeding the 6k
threshold, will force rustc to always pass arguments through a file.

This strategy should avoid us trying to parse the output on Windows of the
linker to see if it successfully spawned yet failed to actually sub-spawn the
linker. We may just be passing arguments through files a little more commonly
now...

The motivation for this commit was a recent bug in Gecko 1 when beta testing,
notably when incremental compilation was enabled it blew out the limit on
cmd.exe. This commit will also fix#46999 as well though as emscripten uses a
bat script as well (and we're blowing the limit there).

Closes#46999

rust-highfive · 2018-01-17T01:24:41Z

r? @michaelwoerister

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

alexcrichton · 2018-01-17T01:24:50Z

I'm also tagging this as beta-nominated since incremental is in beta now

Mark-Simulacrum · 2018-01-17T03:51:07Z

This seems to be false? The code below compares with 6*1024, which is 6,144. Though it's already fairly clear what we're trying to say.

Oh that's actually intentional in the sense that we are sort of checking against 8192, I just randomly chose 6k instead to assume some percentage of slop when calculating the length of the command line (I've updated the comment though to hopefully reflect this!)

michaelwoerister · 2018-01-17T10:28:43Z

Thank you, Alex!

r=me with the inconsistency that @Mark-Simulacrum mentioned ironed out.

alexcrichton · 2018-01-17T15:08:27Z

@bors: r=michaelwoerister

bors · 2018-01-17T15:08:28Z

📌 Commit 9e50cc8 has been approved by michaelwoerister

Work around excessive command-line length issues by disabling incremental rust compilation, which is enabled by default outside `cargo --release` starting with Rust 1.24. Incremental rust builds shouldn't help much in automation, where sccache provides the only continuity between build environments. In the meantime, they add a lot of object files to the link line. See rust-lang/rust#47507 about addressing the underlying issue upstream. MozReview-Commit-ID: LRwUj3fhiaO --HG-- extra : rebase_source : 1739a7570b2e7fe40ead3b301ea20c2fe79f0431

alexcrichton · 2018-01-18T20:02:51Z

@bors: p=1

(this'll get backported to beta once merged)

bors · 2018-01-19T13:13:13Z

⌛ Testing commit 9e50cc8b73d8df9fae1645299a8bf1251e9493d4 with merge c75f68ca414269a3de08c9963657fc2b37a078b1...

bors · 2018-01-19T16:14:15Z

💔 Test failed - status-appveyor

alexcrichton · 2018-01-19T16:15:45Z

@bors: retry

Spurious appveyor 32-bit test timeouts #46903

bors · 2018-01-19T18:23:13Z

⌛ Testing commit 9e50cc8b73d8df9fae1645299a8bf1251e9493d4 with merge 0b574b519c158f160bcc9078c3fc693a7bee22ec...

bors · 2018-01-19T21:24:01Z

💔 Test failed - status-appveyor

alexcrichton · 2018-01-19T22:01:39Z

[01:56:46] test [run-make] run-make\long-linker-command-lines-cmd-exe has been running for over 60 seconds

on second thought, not spurious

alexcrichton · 2018-01-20T02:10:31Z

@bors: r=michaelwoerister

bors · 2018-01-20T21:36:33Z

💔 Test failed - status-appveyor

alexcrichton · 2018-01-20T22:20:39Z

@bors: r=michaelwoerister

bors · 2018-01-20T22:20:40Z

📌 Commit 3ca320c has been approved by michaelwoerister

bors · 2018-01-21T00:37:51Z

⌛ Testing commit 3ca320c with merge 3d56b33...

…ster rustc: Lower link args to `@`-files on Windows more When spawning a linker rustc has historically been known to blow OS limits for the command line being too large, notably on Windows. This is especially true of incremental compilation where there can be dozens of object files per compilation. The compiler currently has logic for detecting a failure to spawn and instead passing arguments via a file instead, but this failure detection only triggers if a process actually fails to spawn. Unfortunately on Windows we've got something else to worry about which is `cmd.exe`. The compiler may be running a linker through `cmd.exe` where `cmd.exe` has a limit of 8192 on the command line vs 32k on `CreateProcess`. Moreso rustc actually succeeds in spawning `cmd.exe` today, it's just that after it's running `cmd.exe` fails to spawn its child, which rustc doesn't currently detect. Consequently this commit updates the logic for the spawning the linker on Windows to instead have a heuristic to see if we need to pass arguments via a file. This heuristic is an overly pessimistic and "inaccurate" calculation which just calls `len` on a bunch of `OsString` instances (where `len` is not precisely the length in u16 elements). This number, when exceeding the 6k threshold, will force rustc to always pass arguments through a file. This strategy should avoid us trying to parse the output on Windows of the linker to see if it successfully spawned yet failed to actually sub-spawn the linker. We may just be passing arguments through files a little more commonly now... The motivation for this commit was a recent bug in Gecko [1] when beta testing, notably when incremental compilation was enabled it blew out the limit on `cmd.exe`. This commit will also fix#46999 as well though as emscripten uses a bat script as well (and we're blowing the limit there). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1430886Closes#46999

bors · 2018-01-21T03:38:30Z

💔 Test failed - status-appveyor

bors · 2018-01-21T12:09:08Z

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

When spawning a linker rustc has historically been known to blow OS limits for the command line being too large, notably on Windows. This is especially true of incremental compilation where there can be dozens of object files per compilation. The compiler currently has logic for detecting a failure to spawn and instead passing arguments via a file instead, but this failure detection only triggers if a process actually fails to spawn. Unfortunately on Windows we've got something else to worry about which is `cmd.exe`. The compiler may be running a linker through `cmd.exe` where `cmd.exe` has a limit of 8192 on the command line vs 32k on `CreateProcess`. Moreso rustc actually succeeds in spawning `cmd.exe` today, it's just that after it's running `cmd.exe` fails to spawn its child, which rustc doesn't currently detect. Consequently this commit updates the logic for the spawning the linker on Windows to instead have a heuristic to see if we need to pass arguments via a file. This heuristic is an overly pessimistic and "inaccurate" calculation which just calls `len` on a bunch of `OsString` instances (where `len` is not precisely the length in u16 elements). This number, when exceeding the 6k threshold, will force rustc to always pass arguments through a file. This strategy should avoid us trying to parse the output on Windows of the linker to see if it successfully spawned yet failed to actually sub-spawn the linker. We may just be passing arguments through files a little more commonly now... The motivation for this commit was a recent bug in Gecko [1] when beta testing, notably when incremental compilation was enabled it blew out the limit on `cmd.exe`. This commit will also fixrust-lang#46999 as well though as emscripten uses a bat script as well (and we're blowing the limit there). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1430886Closesrust-lang#46999

alexcrichton · 2018-01-22T04:52:04Z

I've rebased but this isn't ready for merging, I'm investigating that last failure.

alexcrichton · 2018-01-22T15:42:14Z

@bors: r=michaelwoerister

bors · 2018-01-22T15:42:15Z

📌 Commit 66366f9 has been approved by michaelwoerister

bors · 2018-01-22T20:30:21Z

⌛ Testing commit 66366f9 with merge ae920dc...

…ster rustc: Lower link args to `@`-files on Windows more When spawning a linker rustc has historically been known to blow OS limits for the command line being too large, notably on Windows. This is especially true of incremental compilation where there can be dozens of object files per compilation. The compiler currently has logic for detecting a failure to spawn and instead passing arguments via a file instead, but this failure detection only triggers if a process actually fails to spawn. Unfortunately on Windows we've got something else to worry about which is `cmd.exe`. The compiler may be running a linker through `cmd.exe` where `cmd.exe` has a limit of 8192 on the command line vs 32k on `CreateProcess`. Moreso rustc actually succeeds in spawning `cmd.exe` today, it's just that after it's running `cmd.exe` fails to spawn its child, which rustc doesn't currently detect. Consequently this commit updates the logic for the spawning the linker on Windows to instead have a heuristic to see if we need to pass arguments via a file. This heuristic is an overly pessimistic and "inaccurate" calculation which just calls `len` on a bunch of `OsString` instances (where `len` is not precisely the length in u16 elements). This number, when exceeding the 6k threshold, will force rustc to always pass arguments through a file. This strategy should avoid us trying to parse the output on Windows of the linker to see if it successfully spawned yet failed to actually sub-spawn the linker. We may just be passing arguments through files a little more commonly now... The motivation for this commit was a recent bug in Gecko [1] when beta testing, notably when incremental compilation was enabled it blew out the limit on `cmd.exe`. This commit will also fix#46999 as well though as emscripten uses a bat script as well (and we're blowing the limit there). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1430886Closes#46999

bors · 2018-01-22T23:32:12Z

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

[beta] Backports Cherry-picked into beta: - #47401 - #47494 - #47503 - #47507

Work around excessive command-line length issues by disabling incremental rust compilation, which is enabled by default outside `cargo --release` starting with Rust 1.24. Incremental rust builds shouldn't help much in automation, where sccache provides the only continuity between build environments. In the meantime, they add a lot of object files to the link line. See rust-lang/rust#47507 about addressing the underlying issue upstream. MozReview-Commit-ID: LRwUj3fhiaO UltraBlame original commit: 261725e65af9b5d98ff593db3db08632bf019454

Work around excessive command-line length issues by disabling incremental rust compilation, which is enabled by default outside `cargo --release` starting with Rust 1.24. Incremental rust builds shouldn't help much in automation, where sccache provides the only continuity between build environments. In the meantime, they add a lot of object files to the link line. See rust-lang/rust#47507 about addressing the underlying issue upstream. MozReview-Commit-ID: LRwUj3fhiaO

rust-highfive assigned michaelwoerister Jan 17, 2018

alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 17, 2018

alexcrichton mentioned this pull request Jan 17, 2018
emscripten linking with many dependencies broken on Windows #46999
Closed

Mark-Simulacrum reviewed Jan 17, 2018
View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2018

alexcrichton force-pushed the rerun-bat-scripts branch from d09be7a to 9e50cc8Compare January 17, 2018 15:07

alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 17, 2018

Nemikolh mentioned this pull request Jan 18, 2018
Unresolved external symbols when linking with incremental compilation #45929
Closed

nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 18, 2018

kennytm 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 Jan 19, 2018

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 19, 2018

alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 19, 2018

alexcrichton force-pushed the rerun-bat-scripts branch from 9e50cc8 to db2c548Compare January 20, 2018 02:10

alexcrichton force-pushed the rerun-bat-scripts branch from b3b3768 to 3ca320cCompare January 20, 2018 22:20

alexcrichton force-pushed the rerun-bat-scripts branch from 3ca320c to 66366f9Compare January 22, 2018 04:50

bors merged commit 66366f9 into rust-lang:masterJan 22, 2018

MaloJaffre mentioned this pull request Jan 23, 2018
[beta] Backports #47682
Merged

bors added a commit that referenced this pull request Jan 23, 2018
Auto merge of#47682- MaloJaffre:beta-backport, r=alexcrichton
ed9751a
[beta] Backports Cherry-picked into beta: - #47401 - #47494 - #47503 - #47507

alexcrichton deleted the rerun-bat-scripts branch January 26, 2018 21:55

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 2, 2018

ollie27 mentioned this pull request Feb 19, 2018
Non-ANSI paths cause rustc to fail to link on windows #48318
Closed

BatmanAoD mentioned this pull request Mar 1, 2018
1.24.1 announcement rust-lang/blog.rust-lang.org#230
Merged

SimonSapin mentioned this pull request Mar 3, 2018
"Argument list too long" with many files on Windows rust-lang/cc-rs#292
Closed

alexcrichton mentioned this pull request Jan 7, 2020
Use response files for rustc rust-lang/cargo#7762
Closed

Uh oh!

Uh oh!

Conversation

alexcrichton commented Jan 17, 2018

Uh oh!

rust-highfive commented Jan 17, 2018

Uh oh!

alexcrichton commented Jan 17, 2018

Uh oh!

Mark-SimulacrumJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

alexcrichtonJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Jan 17, 2018

Uh oh!

alexcrichton commented Jan 17, 2018

Uh oh!

bors commented Jan 17, 2018

Uh oh!

alexcrichton commented Jan 18, 2018

Uh oh!

bors commented Jan 19, 2018

Uh oh!

bors commented Jan 19, 2018

Uh oh!

alexcrichton commented Jan 19, 2018

Uh oh!

bors commented Jan 19, 2018

Uh oh!

bors commented Jan 19, 2018

Uh oh!

alexcrichton commented Jan 19, 2018

Uh oh!

alexcrichton commented Jan 20, 2018

Uh oh!

bors commented Jan 20, 2018

Uh oh!

alexcrichton commented Jan 20, 2018

Uh oh!

bors commented Jan 20, 2018

Uh oh!

bors commented Jan 21, 2018

Uh oh!

bors commented Jan 21, 2018

Uh oh!

bors commented Jan 21, 2018

Uh oh!

alexcrichton commented Jan 22, 2018

Uh oh!

alexcrichton commented Jan 22, 2018

Uh oh!

bors commented Jan 22, 2018

Uh oh!

bors commented Jan 22, 2018

Uh oh!

bors commented Jan 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants