Skip to content
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

[Debuginfo] Use Reference DINode for references, delineate between mut and const ref/ptr #134597

Closed
wants to merge 3,962 commits into from

Conversation

Walnut356
Copy link
Contributor

@Walnut356 Walnut356 commented Dec 21, 2024

Adds LLVMRustDIBuilderCreateReferenceType to create proper reference type elements for ty::Ref, rather than blanket-outputting pointer types for all pointers and references

Also uses LLVMRustDIBuilderCreateQualifiedType in a similar way to this pr to make immutable references and pointers into const <type> * .

Tested with *-msvc with cdb and LLDB, as well as *-windows-gnu with LLDB and GDB

sample code
    let val1 = 1u8;
    let mut val2 = 0u8;
    let ref_val = &val1;
    let mut_val = &mut val2;
    let ref_ref = &&val1;
    let mut_mut = &mut &mut val2;
    let ref_mut = & &mut val2;
    let mut_ref = &mut &val2;

    let ref_ref_ref = &&&val1;
    let ref_ref_ref_ref = &&&&val1;
    let mut_mut_mut = &mut &mut &mut val2;
    let mut_mut_mut_mut = &mut &mut &mut &mut val2;

    let ref_mut_mut = &&mut &mut val2;
    let mut_ref_ref = &mut &&val2;
    let ref_mut_ref = & &mut &val2;
    let mut_ref_mut = &mut &&mut val2;

    let box_ptr = Box::new(0u8);
    let const_ptr = &val1 as *const _;
    let mut_ptr = &mut val2 as *mut _;
    let ptr_ptr = (&const_ptr) as *const _;
    let ptr_ref = &&val1 as *const &u8;
    let ref_ptr = &const_ptr;

Unformatted output Before:

image

Unformatted output After:

image

With formatter script:

image

Hacks

In LLDB, nested references (&&T) flat out do not work. Nested pointers (*const *const T) and ptr-to-ref (*&T)/ref-to-pointer (&*T) work, but presumably ref-to-ref doesn't work because it is not a legal construct in C/C++. As we're piggybacking on TypeSystemClang, that probably isn't something we can change on the LLDB end.

To work around this, you can convert the inner ref into a pointer (&&T -> &*T) to prevent && from ever occurring. This has the downside of losing debug information though, as ref-to-ref that looks like ref-to-ptr is not differentiable from an actual ref-to-ptr.

To work around that, I've hijacked dwarf's rvalue_reference tag (which does not have a real equivalent in rust anyway, and is incredibly conveniently denoted by &&) to indicate ref-to-ref. Debugger scripts can then match on this to "unpack" the type name to accurately convert it back to its Rust equivalent. I've left a solid essay explaining this in detail in the rust debuginfo code to help ease the maintenance burden of this.

AFAIK there is not an alternative that maintains the ref vs pointer information besides wrapping the values in an even more obtrusive struct.

Deficiencies

  • Box is treated like a regular pointer and has no mutability field, so it's not immediately clear how to format the type name. The comments of build_pointer_or_reference_di_node mention eventually maybe handling Box as a smart pointer, so that might be an option to fix things.
  • GDB still formats all raw pointers as *mut (even without any formatting script at all). So no regression there, but it's still strange. It might be fixable in the future? Some cursory testing showed that the pointers didn't even get thrown through the type recognizer, so iunno.
  • GDB custom type formatting is ignored in the VSCode debug pane (but isn't ignored via the manual commands whatis, ptype, etc.). Displaying types is an optional toggle (disabled by default iirc), and it doesn't display any information that's actually incorrect, it just exposes the "internal" debug representation (e.g. u8 *&&) so it should be okay.

@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

@rustbot 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. has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Walnut356
Copy link
Contributor Author

Walnut356 commented Dec 22, 2024

lol that's about what I expected.

Looks like gdb auto-derefs any references, so it errors on any attempts to dereference them (which seems like kinda silly behavior?). The tests currently expect:

$1 = true

Whereas refs are formatted with both the type information and the address:

$1 = (const bool &) @0x5ffb5f: true

In order to make changing these less painful, I'll probably make a separate test to whatis a bunch of ref types and then replace the print commands in all the rest of the tests as such: print *the_a_ref -> print *&the_a_ref. This coerces the value to a pointer and dereferences it, giving identical output to the current tests.

@Walnut356
Copy link
Contributor Author

I've also done a bit of digging and GDB's pointer problem can be solved on their end. Right now they unconditionally format all pointer-tagged types as *mut. It should be pretty reasonable to get that sorted out. AFAICT with how the type printer script is currently written, we won't need any further changes (aside from the tests) if/when that gets fixed.

@jieyouxu jieyouxu added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Dec 23, 2024
@rust-log-analyzer

This comment has been minimized.

@Walnut356
Copy link
Contributor Author

Huh, in captured-fields-1.rs GDB prints check 4 as

  $4 = captured_fields_1::main::{closure_env#3} {my_ref: @0 x7fffffffdf78: captured_fields_1::MyStruct {my_field1: 11, my_field2: 22}}

Note the extra space inbetween @0 and x. Weird. I wasn't able to replicate this locally either. It occurs 57 characters after the start of the line, which doesn't seem like it would be a line-cutoff issue. Either way, it's GDB's output of an address, so i don't think there's really anything i can do to properly fix it on our end. I fixed the test with a slightly awkward [...]

@rust-log-analyzer

This comment has been minimized.

@Walnut356
Copy link
Contributor Author

and now it prints without the extra space

$4 = captured_fields_1::main::{closure_env#3} {my_ref: @0x7fffffffdf78: captured_fields_1::MyStruct {my_field1: 11, my_field2: 22}}

???

@rust-log-analyzer

This comment has been minimized.

@Walnut356
Copy link
Contributor Author

@rustbot review

@bors
Copy link
Contributor

bors commented Jan 5, 2025

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

tshepang and others added 10 commits January 5, 2025 17:51
…orn3

Subtree sync for rustc_codegen_cranelift

Aside from a Cranelift update, nothing major this time.

r? `@ghost`

`@rustbot` label +A-codegen +A-cranelift +T-compiler
rustc-dev-guide subtree update

This PR performs the first update of rustc-dev-guide code from its repository.

r? `@BoxyUwU`
…s, r=workingjubilee

Add a notion of "some ABIs require certain target features"

I think I finally found the right shape for the data and checks that I recently added in rust-lang#133099, rust-lang#133417, rust-lang#134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both `-Ctarget-feature` and `#[target_feature]` are updated to ensure we follow the rules of the ABI.  This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features.

We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this *before* applying `-Ctarget-feature` to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested.

For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;)

As a side-effect this also (unstably) allows *enabling* `x87` when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see rust-lang#133611, but no such change is happening in this PR.

r? `@workingjubilee`
bors and others added 10 commits January 9, 2025 16:18
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#134898 (Make it easier to run CI jobs locally)
 - rust-lang#135195 (Make `lit_to_mir_constant` and `lit_to_const` infallible)
 - rust-lang#135261 (Account for identity substituted items in symbol mangling)

r? `@ghost`
`@rustbot` modify labels: rollup
There is a difference between the `image` (the Dockerfile), the `name` of the job (which determines also its properties) and the `full_name`, which includes the `auto/try/pr` prefix.
CI: fix name of jobs

There is a difference between the `image` (the Dockerfile), the `name` of the job (which determines also its properties) and the `full_name`, which includes the `auto/try/pr` prefix.

Missed this in rust-lang#134898.
@rustbot rustbot added A-compiletest Area: The compiletest test runner A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-run-make Area: port run-make Makefiles to rmake.rs A-rustc-dev-guide Area: rustc-dev-guide A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) O-unix Operating system: Unix-like O-windows Operating system: Windows PG-exploit-mitigations Project group: Exploit mitigations T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue. T-style Relevant to the style team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jan 10, 2025
@Walnut356
Copy link
Contributor Author

Closing in favor of an alternative solution based on the pieces here

@Walnut356 Walnut356 closed this Jan 10, 2025
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#21 exporting to docker image format
#21 sending tarball 29.7s done
#21 DONE 42.6s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
   Compiling rustc_codegen_llvm v0.0.0 (/checkout/compiler/rustc_codegen_llvm)
error: constant `DW_LANG_RUST` is never used
  --> compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs:66:7
   |
66 | const DW_LANG_RUST: c_uint = 0x1c;
   |
   = note: `-D dead-code` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(dead_code)]`


error: constant `DW_ATE_boolean` is never used
   |
   |
68 | const DW_ATE_boolean: c_uint = 0x02;

error: constant `DW_ATE_float` is never used
  --> compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs:70:7
   |
   |
70 | const DW_ATE_float: c_uint = 0x04;

error: constant `DW_ATE_signed` is never used
  --> compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs:72:7
   |
---
   |
74 | const DW_ATE_unsigned: c_uint = 0x07;
   |       ^^^^^^^^^^^^^^^

error: constant `DW_ATE_UTF` is never used
   |
   |
76 | const DW_ATE_UTF: c_uint = 0x10;

error: could not compile `rustc_codegen_llvm` (lib) due to 6 previous errors
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:02:46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-run-make Area: port run-make Makefiles to rmake.rs A-rustc-dev-guide Area: rustc-dev-guide A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic has-merge-commits PR has merge commits, merge with caution. O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) O-unix Operating system: Unix-like O-windows Operating system: Windows PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue. T-style Relevant to the style team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.