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

Fix GDB pretty-printers for compressed enums #47617

Closed
wants to merge 1 commit into from

Conversation

gentoo90
Copy link
Contributor

Pretty-printing of Option failed with exception:

Traceback (most recent call last):
  File "/opt/rust-bin-9999/lib/rustlib/etc/gdb_rust_pretty_printing.py", line 166, in rust_pretty_printer_lookup_function
    if encoded_enum_info.is_null_variant():
  File "/opt/rust-bin-9999/lib/rustlib/etc/debugger_pretty_printers_common.py", line 295, in is_null_variant
    return discriminant_val.as_integer() == 0
  File "/opt/rust-bin-9999/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.
Before After
gdb_before gdb_after

main.rs

fn main() {
    let vec_u8: Option<Vec<u8>> = Some(vec![1,2,3]);
    let empty_vec_u8: Option<Vec<u8>> = None;
    let string: Option<String> = Some("I'm an optional string!".to_owned());
    let empty_string: Option<String> = None;

     println!("breakpoint here");
}

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@gentoo90
Copy link
Contributor Author

r? @michaelwoerister

@michaelwoerister
Copy link
Member

Thanks, @gentoo90! Hm, this seem rather like an accidental fix of the issue. A fat pointer should not have any struct fields that we can further recurse into.

@eddyb, do optimized enums works with byte offsets for the discriminant now?

@eddyb
Copy link
Member

eddyb commented Jan 22, 2018

A fat pointer should not have any struct fields that we can further recurse into.

Yes it does (at least internally). However, we should be encoding a byte offset and some other information (not currently done). @tromey knows more about the ideal solution here.

@michaelwoerister
Copy link
Member

What I meant was: A fat pointer should not have any fields that are themselves structs.

@eddyb
Copy link
Member

eddyb commented Jan 22, 2018

@michaelwoerister Oh huh, this implementation doesn't even read the encoded field path?
Anyway, the issue I was thinking of is #46173, with the latest updates at #32920 (comment).

@michaelwoerister
Copy link
Member

The field path should be in __disr_field_indices if I read this correctly.
The python-based pretty printers should also be able to just read bytes from memory and convert them. It would have to know where to read and how much to read though -- which could be encoded in the field name instead of the field path. The proper implementation that @tromey is working on will probably still take some time.

@eddyb
Copy link
Member

eddyb commented Jan 22, 2018

@michaelwoerister I assumed that the decoder would be in gdb / lldb themselves, not some in-tree python scripts, otherwise I would've changed it back in #45225.

@michaelwoerister
Copy link
Member

For now, we still rely on the in-tree pretty printers mostly, I think.

@michaelwoerister
Copy link
Member

Or at least, they are still used often, so we should keep them working.

@carols10cents carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 22, 2018
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2018
@kennytm
Copy link
Member

kennytm commented Jan 31, 2018

Hi @gentoo90! Just to check if you are still on this.

@michaelwoerister, I've change the tag back to S-waiting-on-review. Could you clarify what OP should do to the PR? If I understand #47617 (comment) correctly, what you mean is the fix is not correct (String and Vec<u8> are not fat pointers for instance), and the original code comment about fat pointer is invalid either?

@pietroalbini
Copy link
Member

@michaelwoerister ping from triage!

@shepmaster
Copy link
Member

/cc @rust-lang/compiler — we haven't heard from @michaelwoerister in a week or two; is anyone else able to review this?

@michaelwoerister
Copy link
Member

Back from vacation now...

I'm going to close this since it only fixes the problem accidentally and would probably fail for more complicated cases than Option.

@gentoo90 gentoo90 deleted the gdb-pretty-printers branch February 12, 2018 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants