Skip to content

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
Collaborator

bors commented Sep 17, 2024

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

@bors
Copy link
Collaborator

bors commented Sep 17, 2024

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

@workingjubilee
Copy link
Member

hm!

@bors r+

@bors
Copy link
Collaborator

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
Collaborator

bors commented Sep 22, 2024

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

@bors
Copy link
Collaborator

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
bors added a commit that referenced this pull request Sep 19, 2025
cg_llvm: Move target machine command-line quoting from C++ to Rust

When this code was introduced in #130446 and #131805, it was complicated by the need to maintain compatibility with earlier versions of LLVM.

Now that LLVM 20 is the baseline (#145071), we can do all of the quoting in pure Rust code, and pass two flat strings to LLVM to be used as-is.

---

In this PR, my priority has been to preserve the existing behaviour as much as possible, without worrying too much about what the behaviour *should* be. (Though I did avoid a leading space before the first argument.)
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 20, 2025
cg_llvm: Move target machine command-line quoting from C++ to Rust

When this code was introduced in rust-lang/rust#130446 and rust-lang/rust#131805, it was complicated by the need to maintain compatibility with earlier versions of LLVM.

Now that LLVM 20 is the baseline (rust-lang/rust#145071), we can do all of the quoting in pure Rust code, and pass two flat strings to LLVM to be used as-is.

---

In this PR, my priority has been to preserve the existing behaviour as much as possible, without worrying too much about what the behaviour *should* be. (Though I did avoid a leading space before the first argument.)
Muscraft pushed a commit to Muscraft/rust that referenced this pull request Sep 24, 2025
cg_llvm: Move target machine command-line quoting from C++ to Rust

When this code was introduced in rust-lang#130446 and rust-lang#131805, it was complicated by the need to maintain compatibility with earlier versions of LLVM.

Now that LLVM 20 is the baseline (rust-lang#145071), we can do all of the quoting in pure Rust code, and pass two flat strings to LLVM to be used as-is.

---

In this PR, my priority has been to preserve the existing behaviour as much as possible, without worrying too much about what the behaviour *should* be. (Though I did avoid a leading space before the first argument.)
rust-cloud-vms bot pushed a commit to makai410/rustc_public that referenced this pull request Oct 12, 2025
cg_llvm: Move target machine command-line quoting from C++ to Rust

When this code was introduced in rust-lang/rust#130446 and rust-lang/rust#131805, it was complicated by the need to maintain compatibility with earlier versions of LLVM.

Now that LLVM 20 is the baseline (rust-lang/rust#145071), we can do all of the quoting in pure Rust code, and pass two flat strings to LLVM to be used as-is.

---

In this PR, my priority has been to preserve the existing behaviour as much as possible, without worrying too much about what the behaviour *should* be. (Though I did avoid a leading space before the first argument.)
bors added a commit that referenced this pull request Oct 22, 2025
Remove current code for embedding command-line args in PDB

The compiler currently has code that will obtain a list of quoted command-line arguments, and pass it through to TargetMachine creation, so that the command-line args can be embedded in PDB output.

This PR removes that code, due to subtle concerns that might not have been apparent when it was originally added.

---

Those concerns include:
- The entire command-line quoting process is repeated every time a target-machine-factory is created. In incremental builds this typically occurs 500+ times, instead of happening only once. The repeated quoting constitutes a large chunk of instructions executed in the `large-workspace` benchmark.
  - See #146804 (comment) for an example of the perf consequences of skipping all that work.
  - This overhead occurs even when building for targets or configurations that don't emit PDB output.
- Command-line arguments are obtained in a way that completely bypasses the query system, which is a problem for the integrity of incremental compilation.
  - Fixing this alone is likely to inhibit incremental rebuilds for most or all CGUs, even in builds that don't emit PDB output.
- Command-line arguments and the executable path are obtained in a way that completely bypasses the compiler's path-remapping system, which is a reproducibility hazard.
  - #128842

---

Relevant PRs:
- #113492
- #130446
- #131805
- #146700
- #146973

Zulip thread:
- https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Some.20PDB.20info.20bypasses.20the.20query.20system.20and.20path.20remapping/with/541432211

---

According to #96475, one of the big motivations for embedding the command-line arguments was to enable tools like Live++. [It appears that Live++ doesn't actually support Rust yet](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/embeded.20compiler.20args.20and.20--remap-path-prefix/near/523800010), so it's possible that there aren't any existing workflows for this removal to break.

In the future, there could be a case for reintroducing some or all of this functionality, guarded behind an opt-in flag so that it doesn't cause problems for other users. But as it stands, the current implementation puts a disproportionate burden on other users and on compiler maintainers.
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Oct 27, 2025
Remove current code for embedding command-line args in PDB

The compiler currently has code that will obtain a list of quoted command-line arguments, and pass it through to TargetMachine creation, so that the command-line args can be embedded in PDB output.

This PR removes that code, due to subtle concerns that might not have been apparent when it was originally added.

---

Those concerns include:
- The entire command-line quoting process is repeated every time a target-machine-factory is created. In incremental builds this typically occurs 500+ times, instead of happening only once. The repeated quoting constitutes a large chunk of instructions executed in the `large-workspace` benchmark.
  - See rust-lang/rust#146804 (comment) for an example of the perf consequences of skipping all that work.
  - This overhead occurs even when building for targets or configurations that don't emit PDB output.
- Command-line arguments are obtained in a way that completely bypasses the query system, which is a problem for the integrity of incremental compilation.
  - Fixing this alone is likely to inhibit incremental rebuilds for most or all CGUs, even in builds that don't emit PDB output.
- Command-line arguments and the executable path are obtained in a way that completely bypasses the compiler's path-remapping system, which is a reproducibility hazard.
  - rust-lang/rust#128842

---

Relevant PRs:
- rust-lang/rust#113492
- rust-lang/rust#130446
- rust-lang/rust#131805
- rust-lang/rust#146700
- rust-lang/rust#146973

Zulip thread:
- https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Some.20PDB.20info.20bypasses.20the.20query.20system.20and.20path.20remapping/with/541432211

---

According to rust-lang/rust#96475, one of the big motivations for embedding the command-line arguments was to enable tools like Live++. [It appears that Live++ doesn't actually support Rust yet](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/embeded.20compiler.20args.20and.20--remap-path-prefix/near/523800010), so it's possible that there aren't any existing workflows for this removal to break.

In the future, there could be a case for reintroducing some or all of this functionality, guarded behind an opt-in flag so that it doesn't cause problems for other users. But as it stands, the current implementation puts a disproportionate burden on other users and on compiler maintainers.
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