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

Use MSVC-style escaping when passing a response/@ file to lld on windows #122596

Merged
merged 2 commits into from Mar 21, 2024

Conversation

rcxdude
Copy link
Contributor

@rcxdude rcxdude commented Mar 16, 2024

LLD parses @ files like the command arguments on the platform it's on, so on windows it needs to follow the MSVC style to work correctly. Otherwise builds can fail if the linker command gets too long and the build path contains spaces.

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2024
@petrochenkov
Copy link
Contributor

It looks like ld64.lld always uses GNU-style parsing, regardless of host.

@petrochenkov
Copy link
Contributor

Can this special case be removed after changes from this PR?
https://github.com/rust-lang/rust/blob/master/compiler/rustc_codegen_ssa/src/back/command.rs#L103-L108

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2024
@rcxdude
Copy link
Contributor Author

rcxdude commented Mar 19, 2024

Can this special case be removed after changes from this PR? https://github.com/rust-lang/rust/blob/master/compiler/rustc_codegen_ssa/src/back/command.rs#L103-L108

Yes, I suspect that will need to be removed for wasm to work correctly on windows with this change. I'll try to get a setup to test it. Looking at that, it does suggest another option would be to use that option for lld by default on windows, instead of using a different escaping approach. Do you have any opinion on which may be better? just specifying the quoting feels like it may be more consistent across different versions of lld, but I'm not sure.

For ld64.lld I'm not sure if there's anything that should be done there. Is cross-compiling from windows to macOS something that can currently be made to work? I don't see much info on it online (only linux->macOS).

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 19, 2024

just specifying the quoting feels like it may be more consistent across different versions of lld

The problem is that the support for --rsp-quoting is as much inconsistent between lld versions as the default quoting behavior.
lld-link and wasm-ld support it, ld.lld supports it unless it's for windows-gnu, ld64.ld doesn't support it.
The current approach relying on the defaults seems fine to me.

Is cross-compiling from windows to macOS something that can currently be made to work?

I don't know ¯_(ツ)_/¯

@rcxdude
Copy link
Contributor Author

rcxdude commented Mar 19, 2024

OK, I'll test that WASM still works with the change (trying to remove that special case), and update the PR from that (probably this evening).

LLD parses @ files like the command arguments on the platform it's on,
so on windows it needs to follow the MSVC style to work correctly.
Otherwise builds can fail if the linker command gets too long and the
build path contains spaces.
@rcxdude
Copy link
Contributor Author

rcxdude commented Mar 20, 2024

OK, finally managed to get a WASM toolchain build working, as I suspected that special case needed removing, does everything look good to go?

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2024

📌 Commit 6b0a706 has been approved by petrochenkov

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2024
@bors
Copy link
Contributor

bors commented Mar 21, 2024

⌛ Testing commit 6b0a706 with merge 7d01878...

@bors
Copy link
Contributor

bors commented Mar 21, 2024

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 7d01878 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 21, 2024
@bors bors merged commit 7d01878 into rust-lang:master Mar 21, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 21, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7d01878): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.7% [1.8%, 4.4%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.97s -> 672.198s (0.03%)
Artifact size: 314.87 MiB -> 314.86 MiB (-0.00%)

@mati865
Copy link
Contributor

mati865 commented Mar 22, 2024

ld.lld supports it unless it's for windows-gnu

Maybe it's just missing a command-line option. If so, fixing that would be trivial for LLD 19.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants