-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[RFC] Support .comment
section like GCC/Clang (!llvm.ident
)
#97550
Conversation
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
I'm surprised this wasn't already added.
I am pretty sure it is already possible to reliably identify the exact rustc version used soly based on the standard library functions linked into the executable. Without stripping you can trivially lookup the stable crate id from a libcore symbol name (the stable crate id depends among other things on the rustc version) and even without symbols I'm pretty sure there are enough differences between versions to identify which rustc is used. In any case it will be possible to at least identify that rustc is used if you don't restrict yourself to |
Yeah, I am just trying to look at it from the shoes of users working on proprietary software where it is common to strip it and clean/obfuscate/tweak binaries in further ways. Some of them may suddenly realize their executable is now explicitly marked and come to complain about it (even if they should be checking for extraneous sections anyway).
Definitely, I am sure it is possible to find fancier ways. I wouldn't be surprised if NNs are being applied to the problem already. :) |
r? @bjorn3 Considering that this is annotated as [RFC], maybe you should open a PR to https://github.com/rust-lang/rfcs/ instead? Don't know how complex of a decision this is, so maybe a compiler team MCP is enough? |
(@bjorn3 To be clear, I wouldn't mind doing it by default and getting closer to GCC and Clang; I am just playing devil's advocate) |
Definitely, if it is deemed necessary, I can write one. I wasn't sure, so I tagged it here for the moment. Thanks! |
FWIW, rustc is already setting the producer string and adding it to debuginfo: rust/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Lines 813 to 816 in 0f06824
This is what you get when you run strings on rustc output that has debuginfo:
|
Yeah, that is |
Maybe I missed the motivation for doing this:
|
It is quite useful for easily figuring out which rustc version an executable was built with. This could for example be used to determine that it needs to be rebuilt if the rustc version it was built with has a CVE, but there are many more reasons to need which compiler version an executable was built with. |
That does indeed sound useful. |
To clarify a bit on those two you mention:
Another point of view (not exactly a motivation, but related) is that one could argue it should be left up to the backend, e.g. if LLVM usually does it (when given the information), then it sounds reasonable to let it do it. |
Sorry. I believe consistency is it should be named .comment and not idk .version. The real question is: is it helpful for rust users? I believe so. |
Given that this is a public-facing change, a FCP is probably more appropriate? |
I don't have permission to start an fcp. Also should this be a compiler or lang team fcp? |
☔ The latest upstream changes (presumably #96285) made this pull request unmergeable. Please resolve the merge conflicts. |
Discussed in T-compiler meeting. @rfcbot fcp merge |
Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
[RFC] Support `.comment` section like GCC/Clang (`!llvm.ident`) Both GCC and Clang write by default a `.comment` section with compiler information: ```txt $ gcc -c -xc /dev/null && readelf -p '.comment' null.o String dump of section '.comment': [ 1] GCC: (GNU) 11.2.0 $ clang -c -xc /dev/null && readelf -p '.comment' null.o String dump of section '.comment': [ 1] clang version 14.0.1 (https://github.com/llvm/llvm-project.git c62053979489ccb002efe411c3af059addcb5d7d) ``` They also implement the `-Qn` flag to avoid doing so: ```txt $ gcc -Qn -c -xc /dev/null && readelf -p '.comment' null.o readelf: Warning: Section '.comment' was not dumped because it does not exist! $ clang -Qn -c -xc /dev/null && readelf -p '.comment' null.o readelf: Warning: Section '.comment' was not dumped because it does not exist! ``` So far, `rustc` only does it for WebAssembly targets and only when debug info is enabled: ```txt $ echo 'fn main(){}' | rustc --target=wasm32-unknown-unknown --emit=llvm-ir -Cdebuginfo=2 - && grep llvm.ident rust_out.ll !llvm.ident = !{!27} ``` The RFC part of this PR is about which behavior should `rustc` follow: - Always add it. - Add it by default, i.e. have an opt-out flag (GCC, Clang). - Have an opt-in flag. - Never add it (current). There is also the question of whether debug info being enabled matters for that decision, given the current behavior of WebAssembly targets. For instance, adding it by default gets us closer to other popular compilers, but that may surprise some users with an information leak. The most conservative option is to only do so opt-in, even if debug info is enabled (some users may be stripping debug info and not expecting something else to be leaked elsewhere). Implementation-wise, this covers both `ModuleLlvm::new()` and `ModuleLlvm::new_metadata()` cases by moving the addition to `context::create_module` and adds a few test cases. ThinLTO also sees the `llvm.ident` named metadata duplicated (in temporary outputs), so this deduplicates it like it is done for `wasm.custom_sections`. The tests also check this duplication does not take place.
@bors r- failed in rollup: #107134 (comment) |
@ojeda any updates on the failure? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the target should fix those test failures.
Both GCC and Clang write by default a `.comment` section with compiler information: ```txt $ gcc -c -xc /dev/null && readelf -p '.comment' null.o String dump of section '.comment': [ 1] GCC: (GNU) 11.2.0 $ clang -c -xc /dev/null && readelf -p '.comment' null.o String dump of section '.comment': [ 1] clang version 14.0.1 (https://github.com/llvm/llvm-project.git c62053979489ccb002efe411c3af059addcb5d7d) ``` They also implement the `-Qn` flag to avoid doing so: ```txt $ gcc -Qn -c -xc /dev/null && readelf -p '.comment' null.o readelf: Warning: Section '.comment' was not dumped because it does not exist! $ clang -Qn -c -xc /dev/null && readelf -p '.comment' null.o readelf: Warning: Section '.comment' was not dumped because it does not exist! ``` So far, `rustc` only does it for WebAssembly targets and only when debug info is enabled: ```txt $ echo 'fn main(){}' | rustc --target=wasm32-unknown-unknown --emit=llvm-ir -Cdebuginfo=2 - && grep llvm.ident rust_out.ll !llvm.ident = !{!27} ``` In the RFC part of this PR it was decided to always add the information, which gets us closer to other popular compilers. An opt-out flag like GCC and Clang may be added later on if deemed necessary. Implementation-wise, this covers both `ModuleLlvm::new()` and `ModuleLlvm::new_metadata()` cases by moving the addition to `context::create_module` and adds a few test cases. ThinLTO also sees the `llvm.ident` named metadata duplicated (in temporary outputs), so this deduplicates it like it is done for `wasm.custom_sections`. The tests also check this duplication does not take place. Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Thanks @cuviper! I had this in the long backlog... The missing target makes sense. Nowadays it seems almost all tests use Thus I rebased and updated the
|
Ah, yes, that was moved in #109770. |
I'll approve the relatively minor fixes, and pass the rest through: @bors r=bjorn3 |
☀️ Test successful - checks-actions |
Worked -- thanks for the quick delta-review @cuviper! By the way, this could probably use a relnotes tag, to avoid surprises for users as noted in OP. |
Finished benchmarking commit (0308df2): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis 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.
Bootstrap: 658.594s -> 659.657s (0.16%) |
Language -------- - [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.] (rust-lang/rust#111717) - [Make `noop_method_call` warn by default.] (rust-lang/rust#111916) - [Support interpolated block for `try` and `async` in macros.] (rust-lang/rust#112953) - [Make `unconditional_recursion` lint detect recursive drops.] (rust-lang/rust#113902) - [Future compatibility warning for some impls being incorrectly considered not overlapping.] (rust-lang/rust#114023) - [The `invalid_reference_casting` lint is now **deny-by-default** (instead of allow-by-default)] (rust-lang/rust#112431) Compiler -------- - [Write version information in a `.comment` section like GCC/Clang.] (rust-lang/rust#97550) - [Add documentation on v0 symbol mangling.] (rust-lang/rust#97571) - [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.] (rust-lang/rust#114562) - [Only check outlives goals on impl compared to trait.] (rust-lang/rust#109356) - [Infer type in irrefutable slice patterns with fixed length as array.] (rust-lang/rust#113199) - [Discard default auto trait impls if explicit ones exist.] (rust-lang/rust#113312) - Add several new tier 3 targets: - [`aarch64-unknown-teeos`] (rust-lang/rust#113480) - [`csky-unknown-linux-gnuabiv2`] (rust-lang/rust#113658) - [`riscv64-linux-android`] (rust-lang/rust#112858) - [`riscv64gc-unknown-hermit`] (rust-lang/rust#114004) - [`x86_64-unikraft-linux-musl`] (rust-lang/rust#113411) - [`x86_64-unknown-linux-ohos`] (rust-lang/rust#113061) - [Add `wasm32-wasi-preview1-threads` as a tier 2 target.] (rust-lang/rust#112922) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.] (rust-lang/rust#94748) - [Merge functionality of `io::Sink` into `io::Empty`.] (rust-lang/rust#98154) - [Implement `RefUnwindSafe` for `Backtrace`] (rust-lang/rust#100455) - [Make `ExitStatus` implement `Default`] (rust-lang/rust#106425) - [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`] (rust-lang/rust#111081) - [Change default panic handler message format.] (rust-lang/rust#112849) - [Cleaner `assert_eq!` & `assert_ne!` panic messages.] (rust-lang/rust#111071) - [Correct the (deprecated) Android `stat` struct definitions.] (rust-lang/rust#113130) Stabilized APIs --------------- - [Unsigned `{integer}::div_ceil`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.div_ceil) - [Unsigned `{integer}::next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of) - [Unsigned `{integer}::checked_next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of) - [`std::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html) - [`std::os::unix::fs::chown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html) - [`std::os::unix::fs::fchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html) - [`std::os::unix::fs::lfchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html) - [`LocalKey::<Cell<T>>::get`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get) - [`LocalKey::<Cell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set) - [`LocalKey::<Cell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take) - [`LocalKey::<Cell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace) - [`LocalKey::<RefCell<T>>::with_borrow`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow) - [`LocalKey::<RefCell<T>>::with_borrow_mut`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut) - [`LocalKey::<RefCell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1) - [`LocalKey::<RefCell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1) - [`LocalKey::<RefCell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1) These APIs are now stable in const contexts: - [`rc::Weak::new`] (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new) - [`sync::Weak::new`] (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new) - [`NonNull::as_ref`] (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref) Cargo ----- - [Encode URL params correctly for `SourceId` in `Cargo.lock`.] (rust-lang/cargo#12280) - [Bail out an error when using `cargo::` in custom build script.] (rust-lang/cargo#12332) Compatibility Notes ------------------- - [Update the minimum external LLVM to 15.] (rust-lang/rust#114148) - [Check for non-defining uses of return position `impl Trait`.] (rust-lang/rust#112842) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Remove LLVM pointee types, supporting only opaque pointers.] (rust-lang/rust#105545) - [Port PGO/LTO/BOLT optimized build pipeline to Rust.] (rust-lang/rust#112235) - [Replace in-tree `rustc_apfloat` with the new version of the crate.] (rust-lang/rust#113843) - [Update to LLVM 17.] (rust-lang/rust#114048) - [Add `internal_features` lint for internal unstable features.] (rust-lang/rust#108955) - [Mention style for new syntax in tracking issue template.] (rust-lang/rust#113586)
Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * For an external LLVM, set dependency of llvm >= 15, in accordance with the upstream changes. * Add a patch with a backport from LLVM 17.0.3 fixing codegen for PPC, ref. rust-lang/rust#116845 Upstream changes: Version 1.73.0 (2023-10-05) ========================== Language -------- - [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.] (rust-lang/rust#111717) - [Make `noop_method_call` warn by default.] (rust-lang/rust#111916) - [Support interpolated block for `try` and `async` in macros.] (rust-lang/rust#112953) - [Make `unconditional_recursion` lint detect recursive drops.] (rust-lang/rust#113902) - [Future compatibility warning for some impls being incorrectly considered not overlapping.] (rust-lang/rust#114023) - [The `invalid_reference_casting` lint is now **deny-by-default** (instead of allow-by-default)] (rust-lang/rust#112431 Compiler -------- - [Write version information in a `.comment` section like GCC/Clang.] (rust-lang/rust#97550) - [Add documentation on v0 symbol mangling.] (rust-lang/rust#97571) - [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.] (rust-lang/rust#114562) - [Only check outlives goals on impl compared to trait.] (rust-lang/rust#109356) - [Infer type in irrefutable slice patterns with fixed length as array.] (rust-lang/rust#113199) - [Discard default auto trait impls if explicit ones exist.] (rust-lang/rust#113312) - Add several new tier 3 targets: - [`aarch64-unknown-teeos`] (rust-lang/rust#113480) - [`csky-unknown-linux-gnuabiv2`] (rust-lang/rust#113658) - [`riscv64-linux-android`] (rust-lang/rust#112858) - [`riscv64gc-unknown-hermit`] (rust-lang/rust#114004) - [`x86_64-unikraft-linux-musl`] (rust-lang/rust#113411) - [`x86_64-unknown-linux-ohos`] (rust-lang/rust#113061) - [Add `wasm32-wasi-preview1-threads` as a tier 2 target.] (rust-lang/rust#112922) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.] (rust-lang/rust#94748) - [Merge functionality of `io::Sink` into `io::Empty`.] (rust-lang/rust#98154) - [Implement `RefUnwindSafe` for `Backtrace`] (rust-lang/rust#100455) - [Make `ExitStatus` implement `Default`] (rust-lang/rust#106425) - [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`] (rust-lang/rust#111081) - [Change default panic handler message format.] (rust-lang/rust#112849) - [Cleaner `assert_eq!` & `assert_ne!` panic messages.] (rust-lang/rust#111071) - [Correct the (deprecated) Android `stat` struct definitions.] (rust-lang/rust#113130) Stabilized APIs --------------- - [Unsigned `{integer}::div_ceil`] (https://doc.rust-lang.org/stable/std/primitiv e.u32.html#method.div_ceil) - [Unsigned `{integer}::next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of) - [Unsigned `{integer}::checked_next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of) - [`std::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html) - [`std::os::unix::fs::chown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html) - [`std::os::unix::fs::fchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html) - [`std::os::unix::fs::lfchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html) - [`LocalKey::<Cell<T>>::get`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get) - [`LocalKey::<Cell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set) - [`LocalKey::<Cell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take) - [`LocalKey::<Cell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace) - [`LocalKey::<RefCell<T>>::with_borrow`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow) - [`LocalKey::<RefCell<T>>::with_borrow_mut`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut) - [`LocalKey::<RefCell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1) - [`LocalKey::<RefCell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1) - [`LocalKey::<RefCell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1) These APIs are now stable in const contexts: - [`rc::Weak::new`] (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new) - [`sync::Weak::new`] (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new) - [`NonNull::as_ref`] (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref) Cargo ----- - [Encode URL params correctly for `SourceId` in `Cargo.lock`.] (rust-lang/cargo#12280) - [Bail out an error when using `cargo::` in custom build script.] (rust-lang/cargo#12332) Misc ---- Compatibility Notes ------------------- - [Update the minimum external LLVM to 15.] (rust-lang/rust#114148) - [Check for non-defining uses of return position `impl Trait`.] (rust-lang/rust#112842) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Remove LLVM pointee types, supporting only opaque pointers.] (rust-lang/rust#105545) - [Port PGO/LTO/BOLT optimized build pipeline to Rust.] (rust-lang/rust#112235) - [Replace in-tree `rustc_apfloat` with the new version of the crate.] (rust-lang/rust#113843) - [Update to LLVM 17.] (rust-lang/rust#114048) - [Add `internal_features` lint for internal unstable features.] (rust-lang/rust#108955) - [Mention style for new syntax in tracking issue template.] (rust-lang/rust#113586)
Both GCC and Clang write by default a
.comment
section with compiler information:They also implement the
-Qn
flag to avoid doing so:So far,
rustc
only does it for WebAssembly targets and only when debug info is enabled:The RFC part of this PR is about which behavior should
rustc
follow:There is also the question of whether debug info being enabled matters for that decision, given the current behavior of WebAssembly targets.
For instance, adding it by default gets us closer to other popular compilers, but that may surprise some users with an information leak. The most conservative option is to only do so opt-in, even if debug info is enabled (some users may be stripping debug info and not expecting something else to be leaked elsewhere).
Implementation-wise, this covers both
ModuleLlvm::new()
andModuleLlvm::new_metadata()
cases by moving the addition tocontext::create_module
and adds a few test cases.ThinLTO also sees the
llvm.ident
named metadata duplicated (in temporary outputs), so this deduplicates it like it is done forwasm.custom_sections
. The tests also check this duplication does not take place.