rustc_llvm: Fix flattened CLI args by aeubanks · Pull Request #131805 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes string manipulation errors introduced in #130446.
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Failed to set assignee to augie: invalid assignee
Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.
This comment has been minimized.
Thanks.
I'm... actually not sure how rustc finished bootstrap on the LLVM 20 integration build with this error? It has built since then, right?
Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling)
label
This only affects the command line portion in debug info on windows AFAIK. The only reason we found this was because we had tests for deterministic builds on windows, and the bad strings were sometimes garbage.
huuuh.
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xbb2be6)[0x7fb88cd3fbe6]
/lib/x86_64-linux-gnu/libc.so.6(+0x3c050)[0x7fb88bfe2050]
/lib/x86_64-linux-gnu/libc.so.6(+0x15b33c)[0x7fb88c10133c]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(LLVMRustCreateTargetMachine+0x494)[0x7fb88d224a54]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0x101decb)[0x7fb88d1aaecb]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0x101ca7e)[0x7fb88d1a9a7e]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xf52efd)[0x7fb88d0dfefd]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0x1024ded)[0x7fb88d1b1ded]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0x102428f)[0x7fb88d1b128f]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xf618d1)[0x7fb88d0ee8d1]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(_RNvXs5_Cs1wnS3Bmr5sj_18rustc_codegen_llvmNtB5_18LlvmCodegenBackendNtNtNtCsly6bReQAbem_17rustc_codegen_ssa6traits7backend14CodegenBackend13codegen_crate+0xca)[0x7fb88d0ce5ba]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xee55d7)[0x7fb88d0725d7]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xe5c2e0)[0x7fb88cfe92e0]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(_RNvMs3_NtCslONYkxYabyj_15rustc_interface7queriesNtB5_6Linker24codegen_and_build_linker+0x22)[0x7fb88d094de2]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xbbb2b5)[0x7fb88cd482b5]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xbb37a5)[0x7fb88cd407a5]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xb81293)[0x7fb88cd0e293]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xbc4a7b)[0x7fb88cd51a7b]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xbbcc82)[0x7fb88cd49c82]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0xb7cfd7)[0x7fb88cd09fd7]
/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-ad72a12415e858c4.so(+0x6c6f7db)[0x7fb892dfc7db]
/lib/x86_64-linux-gnu/libc.so.6(+0x89134)[0x7fb88c02f134]
/lib/x86_64-linux-gnu/libc.so.6(__clone+0x40)[0x7fb88c0aea40]
note: we would appreciate a report at https://github.com/rust-lang/rust
help: you can increase rustc's stack size by setting RUST_MIN_STACK=16777216
Comment on lines +493 to +499
| auto ArgsCppStr = std::string(ArgsCstrBuff + buffer_offset, |
|---|
| ArgsCstrBuffLen - buffer_offset); |
| auto i = 0; |
| while (i != std:🧵:npos) { |
| i = ArgsCppStr.find('\0', i + 1); |
| if (i != std:🧵:npos) |
| ArgsCppStr.replace(i, i + 1, " "); |
| ArgsCppStr.replace(i, 1, " "); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, re-reading the reference about std:🧵:npos, I think I may have misunderstood something about it.
Why is it correct at all as part of a conditional expression like a while or if? It only takes on its magic "until the end of the string" meaning when used as an argument to std::string's functions, doesn't it? Which the control-flow expressions are not invocations of.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://cplusplus.com/reference/string/string/find/
Return Value
The position of the first character of the first match.
If no matches were found, the function returns string::npos.
npos isn't magic, it's in practice just a way to say -1. find() returns npos if something isn't found, and at that point we don't take this if / exit the loop.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay!
📌 Commit 18bbf5f has been approved by durin42
It is now in the queue for this repository.
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
bors added a commit that referenced this pull request
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
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
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
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
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-workspacebenchmark.- 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.
Relevant PRs:
Zulip thread:
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, 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
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-workspacebenchmark.- 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.
Relevant PRs:
- rust-lang/rust#113492
- rust-lang/rust#130446
- rust-lang/rust#131805
- rust-lang/rust#146700
- rust-lang/rust#146973
Zulip thread:
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, 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.
Kobzol pushed a commit to Kobzol/stdarch that referenced this pull request
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.)
makai410 pushed a commit to makai410/rust that referenced this pull request
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.)
makai410 pushed a commit to makai410/rust that referenced this pull request
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.)
makai410 pushed a commit to makai410/rustc_public that referenced this pull request
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.)
Labels
Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling)
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.