cg_llvm: Replace most of our DIBuilder wrappers with LLVM-C API bindings #134009

Zalathar · 2024-12-07T13:42:49Z

Many of our LLVMRust wrapper functions for building debug info can be replaced with their equivalents in the LLVM-C API.

This mostly implements #134001, though it punts on some of the trickier cases noted at #134001 (comment).

Zalathar · 2024-12-07T14:44:15Z

Looks like PR CI found some functions that were new in LLVM 19, fair enough.

Zalathar · 2024-12-08T01:43:35Z

Partly rolled back the use of LLVMDIBuilderInsertDeclareRecordAtEnd (which requires LLVM 19), and instead adjusted our own wrapper function to resemble it more closely (diff).

EDIT: Also it turns out that my binding had the wrong return type anyway.

Zalathar · 2024-12-08T03:08:36Z

I have also replaced the DWARF opcode functions with constants, and explained why the remaining LLVMRustDIBuilder* wrapper functions exist (diff).

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

Zalathar · 2024-12-12T03:39:35Z

It occurred to me that this might have perf implications for debug builds. I don't expect to see any major change, but:

@bors try @rust-timer queue

cg_llvm: Replace most of our DIBuilder wrappers with LLVM-C API bindings Many of our `LLVMRust` wrapper functions for building debug info can be replaced with their equivalents in the LLVM-C API. This mostly implements rust-lang#134001, though it punts on some of the trickier cases noted at rust-lang#134001 (comment). --- Marking as draft for now, because I want to give myself more time to review my own changes. r? workingjubilee

bors · 2024-12-12T03:40:48Z

⌛ Trying commit 23c29e8 with merge 3e30f53...

Zalathar · 2024-12-12T03:40:57Z

I made some small style tweaks based on my own review pass, and I also altered the DIFlags conversion code to be a bit more compiler-friendly, based on some experimentation with Compiler Explorer (diff).

bors · 2024-12-12T05:26:23Z

☀️ Try build successful - checks-actions
Build commit: 3e30f53 (3e30f539e40042cd18b54158603339cb93c19dfd)

rust-timer · 2024-12-12T07:51:00Z

Finished benchmarking commit (3e30f53): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.8%, secondary 1.1%)

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)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	2.7%	[1.0%, 4.4%]	2
Improvements ✅ (primary)	-3.8%	[-3.8%, -3.8%]	1
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-0.8%	[-3.8%, 2.1%]	2

Cycles

Results (secondary -3.3%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.9%, -2.8%]	5
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.999s -> 769.284s (-0.09%)
Artifact size: 331.03 MiB -> 331.06 MiB (0.01%)

Zalathar · 2024-12-12T09:44:21Z

Added an explanation of why *const c_uchar is OK for pointer/length strings (diff).

EDIT: Also subsequently did a rebase onto master; no changes.

Zalathar · 2024-12-12T09:45:10Z

I also noticed that our llvm::Bool typedef is slightly incorrect (we use c_uint but the C-side typedef is int), but I'll pursue that separately from this PR.

EDIT: #134204

workingjubilee

Thanks! This is a significant improvement, just a few notes.

compiler/rustc_codegen_llvm/src/debuginfo/mod.rs

workingjubilee · 2025-01-04T18:09:52Z

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

+            /* LowerBound */ 0i64,
+            /* Count */ upper_bound as i64,


These have to be positive numbers, right?

LLVM declares them as int64_t, in both the C and C++ APIs. 🤷‍♂️

Or are you suggesting that we assert that they're non-negative?

I am indeed suggesting that.

workingjubilee · 2025-01-04T18:15:17Z

r=me with nit addressed and the invariant (if any) doc'd on our side

klensy · 2025-01-04T19:15:14Z

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

+  if (Flags & LLVMDIFlagBigEndian)           Result |= DIFlags::FlagLittleEndian;
+  if (Flags & LLVMDIFlagLittleEndian)        Result |= DIFlags::FlagLittleEndian;


Best kind of errors? Copypaste.

klensy · 2025-01-04T19:48:30Z

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

+    /// Mostly equivalent to `LLVMDIBuilderInsertDeclareRecordAtEnd` in LLVM 19,
+    /// except that this works on LLVM 18 and also doesn't return a value.
+    pub(crate) fn LLVMRustDIBuilderInsertDeclareRecordAtEnd<'ll>(


Need fixme with note to change this when minimum llvm will be 19.

Zalathar · 2025-01-04T23:43:41Z

I think I'll try to split off DIFlags and the DWARF constants into their own smaller PRs to land before this one.

bors · 2025-01-05T04:23:14Z

☔ The latest upstream changes (presumably #133990) made this pull request unmergeable. Please resolve the merge conflicts.

Zalathar · 2025-01-05T07:26:59Z

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

+  if (Flags & LLVMDIFlagSingleInheritance)   Result |= DIFlags::FlagSingleInheritance;
+  if (Flags & LLVMDIFlagMultipleInheritance) Result |= DIFlags::FlagMultipleInheritance;
+  if (Flags & LLVMDIFlagVirtualInheritance)  Result |= DIFlags::FlagVirtualInheritance;


Actually, if I'm bothering to have special treatment for the LLVMDIFlagAccessibility flags below, it makes no sense to not also do the same thing for these LLVMDIFlagPtrToMemberRep flags as well.

(Other than the fact that Rust might never actually use these flags? But following that line of reasoning suggests perhaps not supporting conversion of them at all.)

…ilee cg_llvm: Use constants for DWARF opcodes, instead of FFI calls Split off from rust-lang#134009 to incorporate feedback from rust-lang#134009 (comment). Most of the constant values now come from gimli, which is already a compiler dependency. I noticed that `DW_OP_LLVM_fragment` is an LLVM detail that is not defined by DWARF and could hypothetically change, so I added a static assertion on the C++ side to detect that if it ever happens. r? workingjubilee

Rollup merge of rust-lang#135115 - Zalathar:dwarf-const, r=workingjubilee cg_llvm: Use constants for DWARF opcodes, instead of FFI calls Split off from rust-lang#134009 to incorporate feedback from rust-lang#134009 (comment). Most of the constant values now come from gimli, which is already a compiler dependency. I noticed that `DW_OP_LLVM_fragment` is an LLVM detail that is not defined by DWARF and could hypothetically change, so I added a static assertion on the C++ side to detect that if it ever happens. r? workingjubilee

Make our `DIFlags` match `LLVMDIFlags` in the LLVM-C API In order to be able to use a mixture of LLVM-C and C++ bindings for debuginfo, our Rust-side `DIFlags` needs to have the same layout as LLVM-C's `LLVMDIFlags`, and we also need to be able to convert it to the `DIFlags` accepted by LLVM's C++ API. Internally, LLVM converts between the two types with a simple cast. We can't necessarily rely on that always being true, and LLVM doesn't expose a conversion function, so we have two potential options: - Convert each bit/subvalue individually - Statically assert that doing a cast is actually fine As long as both types do remain the same under the hood (which seems likely), the static-assert-and-cast approach is easier and faster. If the static assertions ever start failing against some future version of LLVM, we'll have to switch over to the convert-each-subvalue approach, which is a bit more error-prone. --- Extracted from rust-lang#134009, though this PR ended up choosing the static-assert-and-cast approach over the convert-each-subvalue approach.

Zalathar · 2025-01-31T02:20:15Z

One of the limitations of this PR is that it has to update all of the bindings at once, to avoid a theoretical UB hazard that would occur if we equivocated LLVM-C's LLVMDIBuilderRef with the C++ DIBuilder *.

To sidestep that, I've filed #136326, which switches over to LLVMDIBuilderRef everywhere (with suitable wrap/unwrap calls), without trying to touch any of the individual bindings.

That should make it possible for later PRs to convert bindings incrementally.

Replace our `LLVMRustDIBuilderRef` with LLVM-C's `LLVMDIBuilderRef` Inspired by trying to split rust-lang#134009 into smaller steps that are easier to review individually. This makes it possible to start incrementally replacing our debuginfo bindings with the ones in the LLVM-C API, all of which operate on `LLVMDIBuilderRef`. There should be no change to compiler behaviour.

Rollup merge of rust-lang#136326 - Zalathar:llvm-di-builder-ref, r=nikic Replace our `LLVMRustDIBuilderRef` with LLVM-C's `LLVMDIBuilderRef` Inspired by trying to split rust-lang#134009 into smaller steps that are easier to review individually. This makes it possible to start incrementally replacing our debuginfo bindings with the ones in the LLVM-C API, all of which operate on `LLVMDIBuilderRef`. There should be no change to compiler behaviour.

Zalathar · 2025-02-01T03:20:34Z

I extracted the first few binding migrations into #136375.

…gjubilee cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 1) Part of rust-lang#134001, follow-up to rust-lang#136326, extracted from rust-lang#134009. This PR performs an arbitrary subset of the LLVM-C binding migrations from rust-lang#134009, which should make it less tedious to review. The remaining migrations can occur in one or more subsequent PRs.

rustbot assigned workingjubilee Dec 7, 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 Dec 7, 2024

Zalathar added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Dec 7, 2024

This comment has been minimized.

Sign in to view

Zalathar force-pushed the llvm-di branch from 1958e03 to c9b0de3 Compare December 8, 2024 01:42

Zalathar force-pushed the llvm-di branch from c9b0de3 to a3a978d Compare December 8, 2024 03:07

Zalathar commented Dec 8, 2024

View reviewed changes

compiler/rustc_codegen_llvm/src/llvm/ffi.rs Show resolved Hide resolved

Zalathar force-pushed the llvm-di branch 2 times, most recently from 07d42da to 23c29e8 Compare December 12, 2024 03:37

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 12, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 12, 2024

Zalathar added 5 commits December 12, 2024 21:04

Make our DIFlags match LLVMDIFlags in the LLVM-C API

1d8fe72

Introduce DIBuilderBox, an owning pointer to DIBuilder

d759865

Use LLVMDIBuilderFinalize

362b4aa

Use LLVMDIBuilderCreateNameSpace

aa9417e

Use LLVMDIBuilderCreateLexicalBlock

8f346a4

Zalathar added 6 commits December 12, 2024 21:04

Use constants for DWARF opcodes, instead of FFI calls

a740567

Properly unwrap LLVMDIBuilderRef in the remaining wrapper functions

b3fc61f

Remove LLVMRustDIFlags

057f238

Add FIXME for auditing optional parameters passed to DIBuilder

505665a

Explain why the remaining LLVMRustDIBuilder* wrappers exist

1837dbe

Explain why (some) pointer/length strings are *const c_uchar

d6297f8

Zalathar force-pushed the llvm-di branch from 4d2d65f to d6297f8 Compare December 12, 2024 10:08

Zalathar marked this pull request as ready for review December 12, 2024 10:09

workingjubilee approved these changes Jan 4, 2025

View reviewed changes

workingjubilee 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2025

klensy reviewed Jan 4, 2025

View reviewed changes

Zalathar mentioned this pull request Jan 5, 2025

cg_llvm: Use constants for DWARF opcodes, instead of FFI calls #135115

Merged

Zalathar commented Jan 5, 2025

View reviewed changes

Zalathar mentioned this pull request Jan 6, 2025

Make our DIFlags match LLVMDIFlags in the LLVM-C API #135156

Merged

klensy mentioned this pull request Jan 14, 2025

Clean the LLVM wrapper #135502

Open

Zalathar mentioned this pull request Jan 31, 2025

Replace our LLVMRustDIBuilderRef with LLVM-C's LLVMDIBuilderRef #136326

Merged

Zalathar mentioned this pull request Feb 1, 2025

cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 1) #136375

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cg_llvm: Replace most of our DIBuilder wrappers with LLVM-C API bindings #134009

cg_llvm: Replace most of our DIBuilder wrappers with LLVM-C API bindings #134009

Zalathar commented Dec 7, 2024 •

edited

Loading

This comment has been minimized.

Zalathar commented Dec 7, 2024

Zalathar commented Dec 8, 2024 •

edited

Loading

Zalathar commented Dec 8, 2024 •

edited

Loading

Zalathar commented Dec 12, 2024

This comment has been minimized.

bors commented Dec 12, 2024

Zalathar commented Dec 12, 2024

bors commented Dec 12, 2024

This comment has been minimized.

rust-timer commented Dec 12, 2024

Zalathar commented Dec 12, 2024 •

edited

Loading

Zalathar commented Dec 12, 2024 •

edited

Loading

workingjubilee left a comment

workingjubilee Jan 4, 2025

Zalathar Jan 4, 2025

workingjubilee Jan 5, 2025

workingjubilee commented Jan 4, 2025

klensy Jan 4, 2025

workingjubilee Jan 4, 2025

klensy Jan 4, 2025

Zalathar commented Jan 4, 2025

bors commented Jan 5, 2025

Zalathar Jan 5, 2025

Zalathar commented Jan 31, 2025

Zalathar commented Feb 1, 2025

		if (Flags & LLVMDIFlagBigEndian) Result \|= DIFlags::FlagLittleEndian;
		if (Flags & LLVMDIFlagLittleEndian) Result \|= DIFlags::FlagLittleEndian;

cg_llvm: Replace most of our DIBuilder wrappers with LLVM-C API bindings #134009

Are you sure you want to change the base?

cg_llvm: Replace most of our DIBuilder wrappers with LLVM-C API bindings #134009

Conversation

Zalathar commented Dec 7, 2024 • edited Loading

This comment has been minimized.

Zalathar commented Dec 7, 2024

Zalathar commented Dec 8, 2024 • edited Loading

Zalathar commented Dec 8, 2024 • edited Loading

Zalathar commented Dec 12, 2024

This comment has been minimized.

bors commented Dec 12, 2024

Zalathar commented Dec 12, 2024

bors commented Dec 12, 2024

This comment has been minimized.

rust-timer commented Dec 12, 2024

Overall result: no relevant changes - no action needed

Zalathar commented Dec 12, 2024 • edited Loading

Zalathar commented Dec 12, 2024 • edited Loading

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Jan 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zalathar commented Jan 4, 2025

bors commented Jan 5, 2025

Choose a reason for hiding this comment

Zalathar commented Jan 31, 2025

Zalathar commented Feb 1, 2025

Zalathar commented Dec 7, 2024 •

edited

Loading

Zalathar commented Dec 8, 2024 •

edited

Loading

Zalathar commented Dec 8, 2024 •

edited

Loading

Zalathar commented Dec 12, 2024 •

edited

Loading

Zalathar commented Dec 12, 2024 •

edited

Loading