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

rustc_llvm: adapt to flattened CLI args in LLVM #130446

Merged
merged 2 commits into from
Sep 22, 2024

Conversation

durin42
Copy link
Contributor

@durin42 durin42 commented Sep 16, 2024

This changed in
llvm/llvm-project@e190d07. I decided to stick with more duplication between the ifdef blocks to make the code easier to read for the next two years before we can plausibly drop LLVM 19.

@rustbot label: +llvm-main

try-job: x86_64-msvc

@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2024

r? @cuviper

rustbot has assigned @cuviper.
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. llvm-main Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling) labels Sep 16, 2024
@rust-log-analyzer

This comment has been minimized.

@durin42 durin42 force-pushed the llvm-20-fix-CommandLineArgs branch from b73d755 to 29a16e8 Compare September 16, 2024 21:04
@nebulark
Copy link
Contributor

Was already going to make a PR for this, as this is due to my change in LLVM, but you were faster.
I think it might make sense to expose the Argv0 and the flattened commandline as args for LLVMRustCreateTargetMachine. That way we can format the string on the rust. What do you think?

Still wip but something like this: master...nebulark:rust_contributing:fix_cl

This changed in
llvm/llvm-project@e190d07. I decided to
stick with more duplication between the ifdef blocks to make the code
easier to read for the next two years before we can plausibly drop LLVM
19.

@rustbot label: +llvm-main
@durin42 durin42 force-pushed the llvm-20-fix-CommandLineArgs branch from 29a16e8 to ad0eceb Compare September 16, 2024 23:58
@durin42
Copy link
Contributor Author

durin42 commented Sep 17, 2024

Was already going to make a PR for this, as this is due to my change in LLVM, but you were faster. I think it might make sense to expose the Argv0 and the flattened commandline as args for LLVMRustCreateTargetMachine. That way we can format the string on the rust. What do you think?

Still wip but something like this: master...nebulark:rust_contributing:fix_cl

I don't really feel strongly - I was mostly trying to not change the interface between the Rust and C++ code until we could really clean house on the old codepath, at which point I'd think harder about it.

We've got a continuous build that builds Rust HEAD against LLVM HEAD as that's what we use internally, so we noticed pretty quickly.

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

heh, clang-format

@nebulark
Copy link
Contributor

Had another look. There is now a difference in output (besides the now missing erronous "cc1" part). I'd expect this windows-only test to fail:
https://github.com/rust-lang/rust/blob/master/tests/run-make/pdb-buildinfo-cl-cmd/rmake.rs

Previously the arguments were quoted via llvm::sys::printArg:
https://github.com/llvm/llvm-project/blob/41f1b467a29d2ca4e35df37c3aa79a0a8c04bc4f/clang/lib/CodeGen/BackendUtil.cpp#L325-L357
https://github.com/llvm/llvm-project/blob/d703b922961e0d02a5effdd4bfbb23ad50a3cc9f/llvm/lib/Support/Program.cpp#L83-L99

For me it's fine though to leave it as it is, so everything compiles again. I'll make a follow up pr in this case.

@workingjubilee
Copy link
Member

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
… r=<try>

rustc_llvm: adapt to flattened CLI args in LLVM

This changed in
llvm/llvm-project@e190d07. I decided to stick with more duplication between the ifdef blocks to make the code easier to read for the next two years before we can plausibly drop LLVM 19.

`@rustbot` label: +llvm-main

try-job: x86_64-msvc
@bors
Copy link
Contributor

bors commented Sep 17, 2024

⌛ Trying commit 86d67b7 with merge 784ce4a...

@bors
Copy link
Contributor

bors commented Sep 17, 2024

☀️ Try build successful - checks-actions
Build commit: 784ce4a (784ce4ac9ba31931d7396b68609803b37d0025cf)

@workingjubilee
Copy link
Member

hm!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2024

📌 Commit 86d67b7 has been approved by workingjubilee

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 22, 2024
…s, r=workingjubilee

rustc_llvm: adapt to flattened CLI args in LLVM

This changed in
llvm/llvm-project@e190d07. I decided to stick with more duplication between the ifdef blocks to make the code easier to read for the next two years before we can plausibly drop LLVM 19.

`@rustbot` label: +llvm-main

try-job: x86_64-msvc
@bors
Copy link
Contributor

bors commented Sep 22, 2024

⌛ Testing commit 86d67b7 with merge 1f9a018...

@bors
Copy link
Contributor

bors commented Sep 22, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 1f9a018 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 22, 2024
@bors bors merged commit 1f9a018 into rust-lang:master Sep 22, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 22, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1f9a018): 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 (primary -3.2%, secondary 2.5%)

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.5% [2.0%, 3.0%] 5
Improvements ✅
(primary)
-3.2% [-3.2%, -3.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.2% [-3.2%, -3.2%] 1

Cycles

Results (primary 0.6%)

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.6% [0.6%, 0.7%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.6% [0.6%, 0.7%] 2

Binary size

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

Bootstrap: 769.289s -> 770.432s (0.15%)
Artifact size: 341.54 MiB -> 341.54 MiB (-0.00%)

matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Oct 17, 2024
Fixes string manipulation errors introduced in rust-lang#130446.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 17, 2024
rustc_llvm: Fix flattened CLI args

Fixes string manipulation errors introduced in rust-lang#130446.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2024
Rollup merge of rust-lang#131805 - aeubanks:flat, r=durin42

rustc_llvm: Fix flattened CLI args

Fixes string manipulation errors introduced in rust-lang#130446.
@durin42 durin42 deleted the llvm-20-fix-CommandLineArgs branch December 11, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm-main Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling) 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.

8 participants