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.

Sign up for GitHub

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

Jump to bottom

Force posix-style quoting on lld, independent of host platform #77543

Merged
merged 1 commit into from
Oct 5, 2020
Merged

Force posix-style quoting on lld, independent of host platform #77543

merged 1 commit into from
Oct 5, 2020

Conversation

Mark-Simulacrum
Copy link
Member

This just blindly applies the logic from @eddyb's comment here: #76466 (comment)

Hopefully, this fixed #76466 -- I cannot test this though.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 Oct 4, 2020
@Mark-Simulacrum Mark-Simulacrum force-pushed the rsp-quoting branch 3 times, most recently from 9b62c0b to ec7a77a Compare October 4, 2020 20:20
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 4, 2020
Closed
@Mark-Simulacrum
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Trying commit ec7a77a4631bb005d78b755a9584cbdbf2c756d6 with merge c4ca2fa230dcaef515acf148ec70c1850bbe5dc1...

@bors
Copy link
Contributor

bors commented Oct 4, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: c4ca2fa230dcaef515acf148ec70c1850bbe5dc1 (c4ca2fa230dcaef515acf148ec70c1850bbe5dc1)

@Mark-Simulacrum
Copy link
Member Author

It looks like we have a confirmation that this is a fix for the LLD bug, so I am unilaterally beta accepting - this fixes a critical regression, and seems like a straightforward and reasonable patch.

@Mark-Simulacrum Mark-Simulacrum added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Oct 5, 2020
@Mark-Simulacrum
Instruct lld that our @ files are posix-style, not Windows
e8325b0 
An upstream LLVM change changed behavior here to respect the host system quoting
rules; previously the posix-style format was always used for @files.
@eddyb
Copy link
Member

eddyb commented Oct 5, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 5, 2020

📌 Commit e8325b0 has been approved by eddyb

@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 Oct 5, 2020
@Mark-Simulacrum
Copy link
Member Author

@bors p=1

@bors
Copy link
Contributor

bors commented Oct 5, 2020

⌛ Testing commit e8325b0 with merge f317a93...

@bors
Copy link
Contributor

bors commented Oct 5, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing f317a93 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 5, 2020
@bors bors merged commit f317a93 into rust-lang:master Oct 5, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 5, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 5, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2020
@bors
Auto merge of rust-lang#77577 - Mark-Simulacrum:stable-next, r=pietro…
90ed0a8 
…albini

[stable] 1.47 release

This PR includes backports of:

* Fix miscompile in SimplifyBranchSame rust-lang#77549
* Force posix-style quoting on lld, independent of host platform rust-lang#77543

Note that both are still beta-nominated/beta-accepted, as they need to be backported to 1.48 as well (future beta branch).
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 8, 2020
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.49.0, 1.48.0 Oct 8, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 8, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2020
@bors
Auto merge of rust-lang#77705 - Mark-Simulacrum:beta-next, r=Mark-Sim…
212e76c 
…ulacrum

[beta] backports

This backports the following:

* Improve build-manifest to work with the improved promote-release rust-lang#77407
* Force posix-style quoting on lld, independent of host platform rust-lang#77543
* Fix miscompile in SimplifyBranchSame rust-lang#77549
* Update RLS and Rustfmt rust-lang#77590
* Move `EarlyOtherwiseBranch` to mir-opt-level 2 rust-lang#77582
@cuviper
Copy link
Member

cuviper commented Oct 23, 2020

@Mark-Simulacrum is there a reason this was only for LldFlavor::Wasm? There's now evidence of a problem elsewhere:
https://users.rust-lang.org/t/linking-with-lld-fails-but-only-in-32-bit-due-to-space-in-path/50521/

@Mark-Simulacrum Mark-Simulacrum deleted the rsp-quoting branch October 23, 2020 01:35
@Mark-Simulacrum
Copy link
Member Author

@eddyb indicated it would break things if it wasn't (https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/last.20minute.20beta.20backport/near/212289115); I did not follow up beyond that. Given the insta-stable backport I was hesitant to be too general, especially as I personally cannot test on windows at all.

@mati865
Copy link
Contributor

mati865 commented Oct 23, 2020

Mentioned llvm/llvm-project@928e9e1 did change only ELF backed. So there must have been another change for COFF that was missed.

@Boscop
Copy link

Boscop commented Oct 30, 2020

I'd really appreciate if this can be fixed :)

(Currently it's preventing me from building my VST plugin for 32-bit with lld.)

@Mark-Simulacrum
Copy link
Member Author

I highly recommend filing an issue if this is still a problem. I suspect digging through lld commit history to figure out if perhaps the quoting default changed on other platforms as well would be worthwhile to try and track down the problem.

@Boscop
Copy link

Boscop commented Oct 31, 2020

@Mark-Simulacrum Do you mean filing an issue on this repo? Or where?

@Mark-Simulacrum
Copy link
Member Author

Yes, on this repository, ideally with steps for reproduction.

@Boscop Boscop mentioned this pull request Nov 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

Linker error with wasm target with spaces in install path
8 participants
@Mark-Simulacrum @rust-highfive @bors @eddyb @cuviper @mati865 @Boscop @rustbot