compiler: Use LLVM's Comdat support by workingjubilee · Pull Request #131876 · rust-lang/rust

workingjubilee · 2024-10-18T08:15:29Z

Acting on these long-ago issues:

r? @Zalathar

rustbot · 2024-10-18T08:15:39Z

Some changes occurred in coverage instrumentation.

cc @Zalathar

rustbot · 2024-10-18T15:33:02Z

These commits modify compiler targets.
(See the Target Tier Policy.)

workingjubilee · 2024-10-18T15:44:14Z

These commits modify compiler targets.

They really don't they just touch the public API...

Zalathar · 2024-10-19T00:40:12Z

Thanks. r=me after squishing the fixups.

Zalathar · 2024-10-19T01:29:33Z

@bors r+

bors · 2024-10-19T01:29:36Z

📌 Commit 49bb96c has been approved by Zalathar

It is now in the queue for this repository.

workingjubilee · 2024-10-19T05:06:35Z

-extern "C" void LLVMRustSetComdat(LLVMModuleRef M, LLVMValueRef V,
- const char *Name, size_t NameLen) {
- Triple TargetTriple = Triple(unwrap(M)->getTargetTriple());
- GlobalObject *GV = unwrap<GlobalObject>(V);
- if (TargetTriple.supportsCOMDAT()) {
- StringRef NameRef(Name, NameLen);
- GV->setComdat(unwrap(M)->getOrInsertComdat(NameRef));
- }
-}


afaict the LLVM-C API doesn't have an exact match of the support check we do here, which is why I implemented it via Rust. I'll go fix that.

…r=Zalathar compiler: Use LLVM's Comdat support Acting on these long-ago issues: - rust-lang#46437 - rust-lang#68955

…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#127462 (std: uefi: Add basic Env variables) - rust-lang#131537 (Fix range misleading field access) - rust-lang#131838 (bootstrap: allow setting `--jobs` in config.toml) - rust-lang#131871 (x86-32 float return for 'Rust' ABI: treat all float types consistently) - rust-lang#131876 (compiler: Use LLVM's Comdat support) - rust-lang#131890 (Update `use` keyword docs to describe precise capturing) - rust-lang#131899 (Mark unexpected variant res suggestion as having placeholders) - rust-lang#131908 (rustdoc: Switch from FxHash to sha256 for static file hashing.) r? `@ghost` `@rustbot` modify labels: rollup

…r=Zalathar compiler: Use LLVM's Comdat support Acting on these long-ago issues: - rust-lang#46437 - rust-lang#68955

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#127462 (std: uefi: Add basic Env variables) - rust-lang#131537 (Fix range misleading field access) - rust-lang#131838 (bootstrap: allow setting `--jobs` in config.toml) - rust-lang#131871 (x86-32 float return for 'Rust' ABI: treat all float types consistently) - rust-lang#131876 (compiler: Use LLVM's Comdat support) - rust-lang#131890 (Update `use` keyword docs to describe precise capturing) - rust-lang#131899 (Mark unexpected variant res suggestion as having placeholders) - rust-lang#131908 (rustdoc: Switch from FxHash to sha256 for static file hashing.) - rust-lang#131916 (small interpreter error cleanup) - rust-lang#131919 (zero-sized accesses are fine on null pointers) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-10-19T11:01:07Z

@bors r-
#131922 (comment)

Zalathar · 2024-10-19T12:50:40Z

+let name_buf = get_value_name(val).to_vec();
+ let name =
+CString::from_vec_with_nul(name_buf).or_else(|buf| CString::new(buf.into_bytes())).unwrap();
+set_comdat(llmod, val, &name);


Oh, looks like we need the supports-comdat check here too.
Or maybe this is a sign that the check should be inside set_comdat instead?

~~Isn't this because !self.is_like_aix && !self.is_like_osx should be !self.is_like_aix || !self.is_like_osx?~~
No.

I mean it's really a sign that "setting a comdat" needs to be a function on the CodegenCx, imo, where we already have acquired all the info, so that it isn't called when we don't have access to that.

Yeah, that makes sense. Happy to leave that for future work.

Migrate `llvm::set_comdat` and `llvm::SetUniqueComdat` to LLVM-C FFI. Note, now we can call `llvm::set_comdat` only when the target actually supports adding comdat. As this has no convenient LLVM-C API, we implement this as `TargetOptions::supports_comdat`. Co-authored-by: Stuart Cook <Zalathar@users.noreply.github.com>

Zalathar · 2024-10-20T01:10:18Z

This succeeded in aarch64-apple, so let's try it again.

@bors r+

bors · 2024-10-20T01:10:20Z

📌 Commit 4927600 has been approved by Zalathar

It is now in the queue for this repository.

Rollup of 4 pull requests Successful merges: - rust-lang#131876 (compiler: Use LLVM's Comdat support) - rust-lang#131941 (compiletest: disambiguate html-tidy from rust tidy tool) - rust-lang#131942 (compiler: Adopt rust-analyzer impls for `LayoutCalculatorError`) - rust-lang#131945 (rustdoc: Clean up footnote handling) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131876 - workingjubilee:llvm-c-c-c-comdat, r=Zalathar compiler: Use LLVM's Comdat support Acting on these long-ago issues: - rust-lang#46437 - rust-lang#68955

bors · 2024-10-20T05:56:09Z

⌛ Testing commit 4927600 with merge d68c327...

…nem,workingjubilee Make `llvm::set_section` take a `&CStr` There's no reason to convert the section name to an intermediate `String`, when the LLVM-C API wants a C string anyway. Follow-up to rust-lang#131876.

Rollup merge of rust-lang#131962 - Zalathar:llvm-set-section, r=Swatinem,workingjubilee Make `llvm::set_section` take a `&CStr` There's no reason to convert the section name to an intermediate `String`, when the LLVM-C API wants a C string anyway. Follow-up to rust-lang#131876.

rustbot assigned Zalathar Oct 18, 2024

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. labels Oct 18, 2024

workingjubilee mentioned this pull request Feb 8, 2020
Audit src/rustllvm/RustWrapper.cpp for C APIs that are now in upstream LLVM. #68955
Open
3 tasks

Zalathar reviewed Oct 18, 2024
View reviewed changes

Comment threadcompiler/rustc_codegen_llvm/src/coverageinfo/mod.rs Outdated

Zalathar reviewed Oct 18, 2024
View reviewed changes

Comment threadcompiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

Zalathar added A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Oct 18, 2024

Zalathar reviewed Oct 19, 2024
View reviewed changes

Comment threadcompiler/rustc_target/src/spec/mod.rs

cg_llvm: Switch llvm::add_global to &CStr
888efe7

workingjubilee force-pushed the llvm-c-c-c-comdat branch 2 times, most recently from 268a153 to 49bb96cCompare October 19, 2024 01:08

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 Oct 19, 2024

workingjubilee commented Oct 19, 2024
View reviewed changes

workingjubilee mentioned this pull request Oct 19, 2024
Rollup of 8 pull requests #131914
Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 19, 2024
Rollup merge ofrust-lang#131876- workingjubilee:llvm-c-c-c-comdat, …
ccfe9dc
…r=Zalathar compiler: Use LLVM's Comdat support Acting on these long-ago issues: - rust-lang#46437 - rust-lang#68955

matthiaskrgr mentioned this pull request Oct 19, 2024
Rollup of 10 pull requests #131922
Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 19, 2024

Zalathar reviewed Oct 19, 2024
View reviewed changes

workingjubileeand others added 2 commits October 19, 2024 10:46

llvm: Delete LLVMRustSetComdat
4927600

workingjubilee force-pushed the llvm-c-c-c-comdat branch from 49bb96c to 4927600Compare October 19, 2024 17:47

Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 19, 2024
(DO NOT MERGE) checkrust-lang#131876on aarch64-apple
d9f4d42

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 20, 2024

Zalathar mentioned this pull request Oct 20, 2024
Rollup of 4 pull requests #131958
Merged

bors merged commit 0bfc49b into rust-lang:masterOct 20, 2024

rustbot added this to the 1.84.0 milestone Oct 20, 2024

Zalathar mentioned this pull request Oct 20, 2024
Make llvm::set_section take a &CStr#131962
Merged

workingjubilee deleted the llvm-c-c-c-comdat branch October 20, 2024 07:05

Uh oh!

Uh oh!

Conversation

workingjubilee commented Oct 18, 2024

Uh oh!

rustbot commented Oct 18, 2024

Uh oh!

Uh oh!

Uh oh!

rustbot commented Oct 18, 2024

Uh oh!

workingjubilee commented Oct 18, 2024

Uh oh!

Uh oh!

Zalathar commented Oct 19, 2024

Uh oh!

Zalathar commented Oct 19, 2024

Uh oh!

bors commented Oct 19, 2024

Uh oh!

workingjubileeOct 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthiaskrgr commented Oct 19, 2024

Uh oh!

ZalatharOct 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klensyOct 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubileeOct 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZalatharOct 19, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Oct 20, 2024

Uh oh!

bors commented Oct 20, 2024

Uh oh!

bors commented Oct 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

workingjubileeOct 19, 2024•
edited
Loading

ZalatharOct 19, 2024•
edited
Loading

klensyOct 19, 2024•
edited
Loading

workingjubileeOct 19, 2024•
edited
Loading