Fix Python exception when debugging.

[davidtwco]

During debugging, I would get the below exception. Looking into this a little, I found that changing the is_null_variant function to keep checking for fat pointers until finding the true discriminant fixed the issue. I'm not familiar with what this code should really be doing - so it's certainly possible that this fix makes no sense but it did stop the exception from happening, so there's that.

Not sure who to assign for this, so I'll let the bot handle it.

Traceback (most recent call last):
  File "/home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/etc/gdb_rust_pretty_printing.py", line 166, in rust_pretty_printer_lookup_function
    if encoded_enum_info.is_null_variant():
  File "/home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/etc/debugger_pretty_printers_common.py", line 295, in is_null_variant
    return discriminant_val.as_integer() == 0
  File "/home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/etc/gdb_rust_pretty_printing.py", line 83, in as_integer
    return int(self.gdb_val)
gdb.error: Cannot convert value to long.

[Mark-Simulacrum]

r? @tromey or @michaelwoerister

[kennytm]

This PR is identical to #47617, so please address the comment in #47617 (comment).

[davidtwco]

Oh, I hadn't noticed there was already a PR for this. Reading through that PR, I'm not sure I understand what changes are required, I'm more than happy to make any changes if clarified.

(Also, while the other PR was closed, unless this change could break the pretty printer for some cases, it might be worthwhile considering a fix such as this as it's quite confusing when you run into the issue debugging, even if it is an accidental fix.)

[TimNN]

Ping from triage, @michaelwoerister, this PR needs your review.

[michaelwoerister]

I'm still skeptical about merging an accidental fix. I'll try to set aside some time to find the root cause.

[pietroalbini]

Ping from triage @michaelwoerister! Did you find the root cause of this?

[davidtwco]

I'm happy to help in debugging the issue (may need some pointers for what to look for and where to look) or in fixing it (probably with some mentoring instructions as I've not touched that part of the compiler).

[tromey]

I compiled the test program from the other PR with rustc 1.27.0. Looking at the DWARF for vec_u8, the type says:

 <3><1ce8>: Abbrev Number: 6 (DW_TAG_union_type)
    <1ce9>   DW_AT_name        : (indirect string, offset: 0xfed): Option<alloc::vec::Vec<u8>>
    <1ced>   DW_AT_byte_size   : 24
    <1cee>   DW_AT_alignment   : 8
 <4><1cef>: Abbrev Number: 7 (DW_TAG_member)
    <1cf0>   DW_AT_name        : (indirect string, offset: 0x601): RUST$ENCODED$ENUM$0$None
    <1cf4>   DW_AT_type        : <0x1cfb>
    <1cf8>   DW_AT_alignment   : 8
    <1cf9>   DW_AT_data_member_location: 0

Following the type of the encoded member shows:

 <3><1cfb>: Abbrev Number: 8 (DW_TAG_structure_type)
    <1cfc>   DW_AT_name        : (indirect string, offset: 0x5d5): Some
    <1d00>   DW_AT_byte_size   : 24
    <1d01>   DW_AT_alignment   : 8
 <4><1d02>: Abbrev Number: 7 (DW_TAG_member)
    <1d03>   DW_AT_name        : (indirect string, offset: 0x273): __0
    <1d07>   DW_AT_type        : <0x1d8a>
    <1d0b>   DW_AT_alignment   : 8
    <1d0c>   DW_AT_data_member_location: 0

Following that type gives:

 <3><1d8a>: Abbrev Number: 8 (DW_TAG_structure_type)
    <1d8b>   DW_AT_name        : (indirect string, offset: 0x434): Vec<u8>
    <1d8f>   DW_AT_byte_size   : 24
    <1d90>   DW_AT_alignment   : 8
 <4><1d91>: Abbrev Number: 7 (DW_TAG_member)
    <1d92>   DW_AT_name        : (indirect string, offset: 0x3cf): buf
    <1d96>   DW_AT_type        : <0x1dae>
    <1d9a>   DW_AT_alignment   : 8
    <1d9b>   DW_AT_data_member_location: 0
 <4><1d9c>: Abbrev Number: 7 (DW_TAG_member)
    <1d9d>   DW_AT_name        : (indirect string, offset: 0x430): len
    <1da1>   DW_AT_type        : <0x1e0f>
    <1da5>   DW_AT_alignment   : 8
    <1da6>   DW_AT_data_member_location: 16

So here I think that the problem is that rustc isn't even following its own approach to representing optimized-out enums. In particular, I think that RUST$ENCODED$ENUM$0$None should probably read RUST$ENCODED$ENUM$0$0$None here.

However, over in bug #32920, this whole area is being redone to be more correct -- instead of writing out special names, rustc will start using the correct DWARF structure to represent enums. However, because the patch for that bug will break all enum debugging for un-updated debuggers, I have been focusing my efforts on the lldb Rust plugin rather than on landing the patch. (It is still unclear to me if this is the best approach...)

As to the patch in this PR -- I lean somewhat toward accepting it. It will only peel off extra layers of structs after the end of the list of encoded field numbers has been reached. In this sense it seems safe to me -- I don't think this situation could normally occur (and in fact it shouldn't occur, that's the underlying rustc bug). That said it does seem mildly risky since who knows what else might happen in some other situation.

[michaelwoerister]

I'm still against just merging this. It clearly does the wrong thing in the general case. As @tromey pointed out, the RUST$ENCODED$ENUM$0$None special field name is wrong. The discriminant in a Option<Vec<u8>> is buried rather deeply:

0: |------------------------------ Option<Vec<u8> -----------------------------------|
1: |--------------------------------- Vec<u8> ---------------------------------------|
2: |---------------------- buf: RawVec<u8> ------------------------|--- len:usize ---|
3: |----- ptr: Unique<T> ------|--- cap: usize ---|--- a: Alloc ---|
4: |--- pointer: NonZero<T> ---|
5: |------------ *T -----------|  <-- discriminant

So the name should be RUST$ENCODED$ENUM$0$0$0$0$0$None. The "fix" only gives the correct answer in this case because for this particular type we always pick the field with index 0 when descending.

To really solve this problem this function would have to be fixed:

rust/src/librustc_codegen_llvm/debuginfo/metadata.rs

Lines 1218 to 1239 in c0955a3

	// HACK(eddyb) the debuggers should just handle offset+size
	// of discriminant instead of us having to recover its path.
	// Right now it's not even going to work for `niche_start > 0`,
	// and for multiple niche variants it only supports the first.
	fn compute_field_path<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>,
	name: &mut String,
	layout: TyLayout<'tcx>,
	offset: Size,
	size: Size) {
	for i in 0..layout.fields.count() {
	let field_offset = layout.fields.offset(i);
	if field_offset > offset {
	continue;
	}
	let inner_offset = offset - field_offset;
	let field = layout.field(cx, i);
	if inner_offset + size <= field.size {
	write!(name, "{}$", i).unwrap();
	compute_field_path(cx, name, field, inner_offset, size);
	}
	}
	}

It seems that @eddyb even left a comment that this is broken :/

@tromey, do you know if it is safe to access the raw bytes of a local variable from the debugger/pretty-printer? If so, we could use RUST$ENCODED$ENUM$<offset>$<size>$None as a stop-gap solution until we get proper Rust support in debuggers.

[tromey]

do you know if it is safe to access the raw bytes of a local variable from the debugger/pretty-printer

Yes, it is fine.

I'm getting close to getting lldb into rustup. And then I can land the real fix for the enum debuginfo bug, see https://github.com/tromey/rust/tree/enum-debuginfo-rebased

[eddyb]

That comment I left should mention #32920 but I'm not sure when that issue was opened.

[stokhos]

Ping from triage! @davidtwco it has been a while since the last time we heard from you, any update on this PR?

[davidtwco]

I'll close this since it seems like @tromey and @michaelwoerister are nearly there with a fix for the underlying issue.