[Debug Info] Generate typedef nodes for ptr/ref types (and msvc arrays)

[Walnut356]

View all comments

This kills like 17 birds with 1 stone. It allows displaying the proper &/&mut/*const/*mut type name for *-gnu targets, and fixes a bunch of issues with visualizing *-msvc targets.

In short, none of the debuggers (in their current states) respect the "name" field that is passed to LLVMRustDIBuilderCreatePointerType. That field does appear in the DWARF data, but GDB and LLDB don't care.

This patch wraps the pointer nodes (and msvc array type) in a typedef before finalizing them. Worth noting, this typedef trick is already being used for *-msvc primitive types.

*-gnu

Mainly fixes type-name output. Screenshots should be self-explanatory.

GDB

GDB by default hard-codes pointer types to *mut. This "fixes" that without requiring a code change in GDB, but that's mostly just a happy side effect.

LLDB

TypeSystemClang ignores the name field of the pointer in the DWARF info. Using a typedef sidesteps that deficiency. We could maybe modify TypeSystemClang so this isn't necessary, but since it relies on clang (read: c/c++) compiler type representations under the hood, I'm not sure if pointers can have names and it's not really reasonable to change clang itself to accommodate rust.

*-msvc

As opposed to DWARF, the name field does not exist anywhere in the PDB data. There are 2 reasons for this

1. Pointer nodes do not contain a name field

2. Primitive types are unique, special nodes that have an additional unique, special representation for pointer-to-primitive

The issue with this is with container types, for example Vec. Vec<T>'s heap pointer is not a *mut T, it's a *mut u8 that is cast to a T when needed using (more or less) PhantomData<T>. From the type's perspective, T only "exists" in the generic parameters. That means the debugger, working from the type's perspective, must look it up by name, (e.g. Vec<ref$<u8> > must look up the string "ref$<u8>").

Since those type names aren't in the PDB data, the lookup fails, the debugger cannot cast the heap pointer, and thus cannot visualize the elements of the container.

In LLDB, the sole arbiter of "what types exist" when doing a type lookup is the PDB data itself. I'm sure the msdia works the same way, but LLDB's native PDB parser checks the type stream, and any pointer-node-to-T is formatted C-style as T *. This problem also affects Microsoft's debugger.

array$<T,N> also needs a typedef, as arrays have a bespoke node whose "name" field is also ignored in favor of the C-style format (e.g. T[N]). If you use Visual Sudio's natvis diagnostics logging, you can see errors such as this:

Natvis: C:\Users\ant_b\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\etc\liballoc.natvis(10,23): Error: identifier "array$<u32,7>" is undefined
    Error while evaluating '(array$<u32,7>*)buf.inner.ptr.pointer.pointer' in the context of type 'sample.exe!alloc::vec::Vec<array$<u32,7>,alloc::alloc::Global>'.

LLDB (via CodeLLDB)

CDB via Visual Studio 2022

CDB via C/C++ extension in Visual Studio Code

The output is identical to Visual Studio, but I want to make a special note because i had to jump through a few hoops to get it to work correctly. Built with stable, it worked the same as the "Before" image from Visual Studio, but built with the patched compiler the Vec visualizer wasn't working.

Clearly based on the Visual Studio "After" screenshot, the natvis files still work. If you binary-patch the extension so that it outputs verbose logging info it appears it never even tried to load liballoc.natvis for some reason?

I manually placed the natvis files in C:\Users\<USER>\.vscode\extensions\ms-vscode.cpptools-1.26.3-win32-x64\debugAdapters\vsdbg\bin\Visualizers\ and it worked fine so iunno. Probably worth someone else testing too. Might also be because I'm only using a stage1 build instead of a full toolchain install? I'm not sure.

Alternatives

I tried some fiddling with using the reference debug info node (which does have a valid counterpart in both DWARF and PDB). The issue is that LLDB uses TypeSystemClang, which is very C/C++-oriented. In Rust, references are borrowing pointers. In C++ references are not objects, they are not guaranteed to have a representation at run-time. That means no array-of-refs, no ref-to-ref, no pointer-to-ref. LLDB seems to interpret ref-to-ref incorrectly. That can be worked around but the hack necessary for that is heinous and infects the visualizers too. It also means without the visualizers, the type-name output is sorta worse than it is now.

[rustbot]

r? @oli-obk

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

[Walnut356]

oh yeah, i'm not sure if we even run the debuginfo test suite anymore? But if we do, this is gonna fail a lot of those tests (in a good way). I can get that cleaned up if/when those failures happen.

[Walnut356]

Ah, so we do still run the debuginfo tests.

One notable problem is that gdb method calls via the T.func() syntax breaks. GDB will attempt to call the function via the process and the process (in the case of function-call.rs) segfaults. This can probably be fixed pretty easily on their end though, and using the more verbose function_call::RegularStruct::get_x(&r) works just fine for the time being.

All the other failures were just minor considerations for "unwrapping" the typedef into its actual type.

[oli-obk]

r? @wesleywiser or reassign to someone else who knows debuginfo well

[rustbot]

wesleywiser is not on the review rotation at the moment.
They may take a while to respond.

[bors]

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

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Walnut356]

@rustbot ready

[Walnut356]

Also @Mark-Simulacrum maybe? Otherwise i'm not sure who else would be able to review this

[Enselic]

After #155352 lands I plan on nominating myself for review rotation on ./tests/debuginfo/*. As practice, I'll pretend that I was assigned this PR.

r? Enselic

My first question is: Can we split this PR up into smaller PRs? For example, it seems as if this PR uses different techniques for different debuggers? Can we split out the parts relevant only for gdb into their own PR? Minimal PRs makes review much easier.

[Walnut356]

Unfortunately, not really. The changes in the debug info generation directly affects how GDB visualizes the data. Without the GDB changes, the visualizers break and like half the tests fail.

It's been a while since i submitted this so i dont remember exactly what the problem was, but iirc none of the visualizer API propogates through typedef chains, and that can cause problems both for type discovery and when accessing things like template args. The GDB changes just looking through the typedef to operate on the underlying type like we were doing before. Now that i think on it, we might need the same for LLDB's visualizers. I'll check that in the next day or two

[Walnut356]

also i suppose @tromey and @VorpalBlade since y'all know more about GDB than I do, how bad is this regression to the typical user:

One notable problem is that gdb method calls via the T.func() syntax breaks. GDB will attempt to call the function via the process and the process (in the case of function-call.rs) segfaults. This can probably be fixed pretty easily on their end though, and using the more verbose function_call::RegularStruct::get_x(&r) works just fine for the time being.

[VorpalBlade]

also i suppose @tromey and @VorpalBlade since y'all know more about GDB than I do, how bad is this regression to the typical user:

One notable problem is that gdb method calls via the T.func() syntax breaks. GDB will attempt to call the function via the process and the process (in the case of function-call.rs) segfaults. This can probably be fixed pretty easily on their end though, and using the more verbose function_call::RegularStruct::get_x(&r) works just fine for the time being.

It seems unfortunate, but apart from very simple cases calling functions in the debugee for rust code has been hit and miss before for me, probably due to due generics being more prevalent in Rust than in templates in C++.

I'm not on a rust dev team (so take what I say with a grain of salt), but I would perhaps send a bug report or even patch to the gdb devs about this as soon as possible, given the slow release cycles of some distros. That said I don't think old LTS distros should block this being merged. I also think that whatever the fix is needs to work with both old and new rust debugees.

[Enselic]

In the PR description you have e.g. mut_mut_mut_mut, and vec_val examples. But I don't find that test in the Rust repo. If that is what you have been using to test and verify your changes, it seems very useful to add that kind of comprehensive testing to the ./tests/debuginfo test suite. It will also make code review easier.

Would you mind adding that test to this PR, please? Ideally as the first commit without any other fixes, so that the diff from your fixes are easy to see. Thanks! (Meanwhile, I will continue to review your PR.)

[Walnut356]

Oh, it's a very stupid file i use to eyeball changes, check for regressions with CodeLLDB (likely the most common debugger adapter i think, but we'll see when they release the debug info survey results), and create visuals for PR's. The TL;DR is it's a single giant function that has approximately 1 of everything we care about. Most of it overlaps with the existing tests, and some of it covers future plans (e.g. proper fat pointer support).

The nested references and pointers are an artifact of the prior attempt at this change that used fiddly recursive logic that took a few tries to get right. That type of logic isn't necessary with this implementation, so there's no need to test it.

[Enselic]

As I've dug deeper, I've discovered that this program:

fn foo(f: &usize) {
    println!("Value: {}", f);
}

fn main() {
    let x = 5;
    foo(&x);
}

with plain gdb (not rust-gdb) says that the type of f is *mut usize:

$ rustc -g /home/martin/src/main.rs && gdb -ex "b main::foo" -ex "run" -ex "ptype f" --args main
Breakpoint 1, main::foo (f=0x7fffffffe2b0) at /home/martin/src/main.rs:2
2           println!("Value: {}", f);
type = *mut usize

I've debugged gdb with gdb, and the reason is that gdb doesn't think the type have a name in this part of gdb code:

    case TYPE_CODE_PTR:
      {
        if (type->name () != nullptr)
          gdb_puts (type->name (), stream);
        else
          {
            /* We currently can't distinguish between pointers and
               references.  */
            gdb_puts ("*mut ", stream);
            type_print (type->target_type (), "", stream, 0);
          }
      }

(For reference, here is the top of the gdb stacktrace to reach that code)

#0  rust_internal_print_type (type=0x555556522700, varstring=0x555555dae803 "", stream=0x55555640c390, show=1, level=0, flags=0x7fffffffdfc0, for_rust_enum=false, podata=0x7fffffffdf3c)
    at rust-lang.c:1114
#1  0x0000555555abd054 in rust_language::print_type (this=this@entry=0x555556148730 <rust_language_defn>, type=type@entry=0x555556522780, varstring=varstring@entry=0x555555dae803 "",
    stream=0x55555640c390, show=show@entry=1, level=level@entry=0, flags=0x7fffffffdfc0) at rust-lang.c:1834
#2  0x0000555555babc66 in whatis_exp (exp=<optimized out>, show=1) at typeprint.c:497
#3  0x0000555555796ee5 in cmd_func (cmd=<optimized out>, args=<optimized out>, from_tty=<optimized out>) at cli/cli-decode.c:2810
#4  0x0000555555b75065 in execute_command (p=<optimized out>, p@entry=0x55555616c030 "ptype f", from_tty=<optimized out>) at top.c:632

But with dwarfdump main, the type info looks correct:

< 3><0x00000872>        DW_TAG_formal_parameter
                          DW_AT_location              len 0x0002: 0x9108: 
                              DW_OP_fbreg 8
                          DW_AT_name                  f
                          DW_AT_decl_file             0x00000009 /home/martin/src/main.rs
                          DW_AT_decl_line             0x00000001
                          DW_AT_type                  <0x00000723>
< 1><0x00000723>    DW_TAG_typedef
                      DW_AT_type                  <0x0000072c>
                      DW_AT_name                  &usize
< 1><0x0000072c>    DW_TAG_pointer_type
                      DW_AT_type                  <0x000000a8>
                      DW_AT_name                  &usize
                      DW_AT_address_class         0x00000000
< 1><0x000000a8>    DW_TAG_base_type
                      DW_AT_name                  usize
                      DW_AT_encoding              DW_ATE_unsigned
                      DW_AT_byte_size             0x00000008

So it looks like gdb wants to work well with Rust without any Python scripts.

To me, it seems better to focus on getting the basics right, than to stack workarounds on top of each other.

So, before we go ahead with something like this, I'd like to understand why gdb is not already working well for this case. Maybe it makes more sense to fix gdb.

Disclaimer: There might very well be decisions or context I am missing. But this is where I stand right now.

View changes since the review

[Walnut356]

with plain gdb (not rust-gdb) says that the type of f is *mut usize

I'm not 100% clear on the implications of the wording, but ptype's help text reads "Contrary to "whatis", "ptype" always unrolls any typedefs.". I assume "unroll" essentially means "look through", but i'm not sure. If that's the case, whatis should print the correct type name (or maybe ptype /r?).

To me, it seems better to focus on getting the basics right, than to stack workarounds on top of each other.

So, before we go ahead with something like this, I'd like to understand why gdb is not already working well for this case.

As I understand it, at the time GDB's rust support was written, the debug info output by rustc did not differentiate between the 5 types of indirection (&, &mut, *const, *mut, Box<T>). I know the nodes have the correct names now, but i think GDB itself doesn't parse name information for whatever reason? I don't know, and digging through their DWARF parsing code (or contributing a fix) are a bit out of scope for me at the moment. The effect on GDB is mostly incidental, and happens to align with what we want anyway.

The major beneficiaries of this change are LLDB and microsoft's debugger(s). There are literally no alternatives on the PDB side of things because pointer nodes cannot store names at all, and because C/C++ reference and const semantics disagree with rust's (and we piggyback on C/C++ handling).

For DWARF debug info with LLDB, the issue is LLDB uses the clang compiler's structs for type information. I'm not sure if clang's pointer struct even has a name field. I'm relatively certain it doesn't though, because the dwarf parser reads the name tag and afaik, TypeSystemClang doesn't make any attempt to enforce a naming scheme for pointers, it just defers to clang. Asking them to change the in-memory type representation of C objects to account for rust type information is a tough sell (assuming i could even figure out how to do it in a reasonable amount of time), so this is likely the most realistic solution for LLDB too aside from creating a TypeSystemRust so that we can use our own type representation (which is, bare minimum, hundreds of hours of up-front effort and a huge ongoing maintenance burden).

[tromey]

So it looks like gdb wants to work well with Rust without any Python scripts.

Yes, that's correct, at least for compiler-generated entities. gdb tries not to know about library-defined types.

So, before we go ahead with something like this, I'd like to understand why gdb is not already working well for this case. Maybe it makes more sense to fix gdb.

gdb started as a C debugger and sometimes there are still holes. In particular the DWARF reader seems to ignore a DW_AT_name on a DW_TAG_pointer_type, I suppose because this just never came up (it's impossible in C and C++) (though TBH I'm mildly surprised that this wasn't implemented for Ada). Anyway this is fixable.

[Enselic]

The major beneficiaries of this change are LLDB and microsoft's debugger(s).

Ok, then I would like to ask you to demonstrate that with diffs in ./tests/debuginfo, please. Because with the current diffs, only the debugging experience with gdb seems to change.

I would like to again recommend to split this PR up into two. This is a quite complicated PR (in terms of impact), and anything we can do to minimize scope of discussion and impact of change is worthwhile, IMHO.

Also, if you need to add new tests to demonstrate how the change affects the debugging experience, please add the test in a separate commit (maybe even separate PR), so that the diff of the impact of the change can easily be seen in a separate commit. Thanks!

[Enselic]

My apologies, but I still don't understand why we need to change both fn build_pointer_or_reference_di_node() and fn build_fixed_size_array_di_node() in the same PR. They seem completely orthogonal to me. So far I've only looked at the change in the former.

View changes since the review

[Walnut356]

I mean i guess it could be. It's all sortof under the umbrella of "we try to give this information to the debugger, the debugger doesn't care, so we need a typedef to make it care". The context and reasoning are all the same and the implementations are almost identical.

[tromey]

It seems unfortunate, but apart from very simple cases calling functions in the debugee for rust code has been hit and miss before for me, probably due to due generics being more prevalent in Rust than in templates in C++.

The issues I know of in this area are (1) sometimes Rust doesn't use the C ABI, but since there's no way to communicate this via DWARF, gdb has no way to correctly perform the call; and (2) rustc doesn't emit complete debug info and in particular nothing involving traits is emitted. There's open bugs here for both of these things.

If you know of other cases that should work in gdb, please file gdb bugs.