-
Couldn't load subscription status.
- Fork 13.9k
rustc_llvm: adapt to flattened CLI args in LLVM #130446
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
Conversation
This comment has been minimized.
This comment has been minimized.
b73d755 to
29a16e8
Compare
|
Was already going to make a PR for this, as this is due to my change in LLVM, but you were faster. 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
29a16e8 to
ad0eceb
Compare
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. |
This comment has been minimized.
This comment has been minimized.
|
heh, clang-format |
|
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: Previously the arguments were quoted via 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. |
|
@bors try |
… 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
|
☀️ Try build successful - checks-actions |
|
hm! @bors r+ |
…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
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (1f9a018): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis 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.
CyclesResults (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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 769.289s -> 770.432s (0.15%) |
Fixes string manipulation errors introduced in rust-lang#130446.
rustc_llvm: Fix flattened CLI args Fixes string manipulation errors introduced in rust-lang#130446.
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.
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.)
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.)
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.)
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.)
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.
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.
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