DWARF for enums could be improved

[tromey]

Suppose you have a "non-trivial" enum like:

enum MoreComplicated {
    One,
    Two(i32),
}

The DWARF looks like:

 <2><a7>: Abbrev Number: 6 (DW_TAG_union_type)
    <a8>   DW_AT_name        : (indirect string, offset: 0x16e): MoreComplicated
    <ac>   DW_AT_byte_size   : 24
 <3><ad>: Abbrev Number: 7 (DW_TAG_member)
    <ae>   DW_AT_type        : <0xc0>
    <b2>   DW_AT_data_member_location: 0
 <3><b3>: Abbrev Number: 7 (DW_TAG_member)
    <b4>   DW_AT_type        : <0xd1>
    <b8>   DW_AT_data_member_location: 0

Then looking at the type of One, aka DIE 0xc0:

 <2><c0>: Abbrev Number: 8 (DW_TAG_structure_type)
    <c1>   DW_AT_name        : (indirect string, offset: 0x160): One
    <c5>   DW_AT_byte_size   : 1
 <3><c6>: Abbrev Number: 9 (DW_TAG_member)
    <c7>   DW_AT_name        : (indirect string, offset: 0x1b7): RUST$ENUM$DISR
    <cb>   DW_AT_type        : <0x6d>
    <cf>   DW_AT_data_member_location: 0

This is peculiar DWARF in a few ways.

First, this emits a separate structure type for each branch. DWARF has a notion of a discriminated union that might be a closer fit semantically. See DW_TAG_variant_part, DW_AT_discr, etc. (gdb's DWARF reader doesn't handle these at all, but it certainly could be made to.)

Second, I think at the very least the discriminant should be marked DW_AT_artificial.

[michaelwoerister]

Yes, that would be nice. This strange encoding was chosen because it only contains things that also occur in C, so that existing debuggers can work with it. LLDB (and maybe GDB too) don't allow discriminated unions to be accessed via the Python API we use for pretty printing.

Once we get proper support for Rust in debuggers, it would definitely better to encode enums differently.

About DW_AT_artificial: Saidly, for some reason, LLDB completely ignores fields with this attribute. It doesn't even load them from DWARF (at least that was the case when this enum encoding was implemented).

[michaelwoerister]

I'd like to explore how DWARF's discriminated unions would work for Rust. Sticking to what DWARF currently specifies, we could make enums look like this:

enum RegularEnum {
    One { field1: u32, field2: f32 },
    Two(i32),
}

would be represented as

DW_TAG_structure_type
  DW_AT_name <RegularEnum>
  DW_AT_byte_size <12>

  DW_TAG_variant_part
    DW_AT_discr <ref to disr member below>

    DW_TAG_member // the discriminant
      DW_AT_type <u32>
      DW_AT_artificial <true>
      DW_AT_data_member_location <0>

    DW_TAG_variant
      DW_AT_name <One>
      DW_AT_discr_value <0>

      DW_TAG_member
        DW_AT_name <field1>
        DW_AT_type <u32>
        DW_AT_data_member_location <4>

      DW_TAG_member
        DW_AT_name <field2>
        DW_AT_type <f32>
        DW_AT_data_member_location <8>

    DW_TAG_variant
      DW_AT_name <Two>
      DW_AT_discr_value <1>

      DW_TAG_member
        DW_AT_type <i32>
        DW_AT_data_member_location <4>

This representation would also scale to enums with optimized memory layout:

enum OptimizedEnum {
    Some(i64, &u32),
    None,
}

would be represented as

DW_TAG_structure_type
  DW_AT_name <OptimizedEnum>
  DW_AT_byte_size <16>

  DW_TAG_variant_part
    DW_AT_discr <ref to disr member below>

    DW_TAG_member // the discriminant
      DW_AT_type <&u32>
      DW_AT_artificial <true>
      DW_AT_data_member_location <8> // <-- just point to where the value is

    DW_TAG_variant
      DW_AT_name <Some>
      // Note the omitted DW_AT_discr_value, making this the default case

      DW_TAG_member
        DW_AT_type <i32>
        DW_AT_data_member_location <0>

      DW_TAG_member
        DW_AT_type <&u32>
        DW_AT_data_member_location <8>

    DW_TAG_variant
      DW_AT_name <None>
      DW_AT_discr_value <0>

To stay consistent with that, C-like enums could also be represented like this:

enum CLikeEnum {
    One,
    Two,
    Three,
}

would become

DW_TAG_structure_type
  DW_AT_name <CLikeEnum>
  DW_AT_byte_size <1>

  DW_TAG_variant_part
    DW_AT_discr <ref to disr member below>

    DW_TAG_member // the discriminant
      DW_AT_type <u8>
      DW_AT_artificial <true>
      DW_AT_data_member_location <0>

    DW_TAG_variant
      DW_AT_name <One>
      DW_AT_discr_value <0>

    DW_TAG_variant
      DW_AT_name <Two>
      DW_AT_discr_value <1>

    DW_TAG_variant
      DW_AT_name <Three>
      DW_AT_discr_value <2>

Now, this would work, but it would make some concessions to what is allowed in DWARF right now:

1. The outermost DIE is a DW_TAG_structure_type because that's the only kind of DIE that is allowed to contain DW_TAG_variant_part children (as far as I interpret the standard).
2. At least for now, all of an enums is the "variant part" of the datatype, so it's redundant to have the DW_TAG_variant_part sub-DIE -- the various DW_TAG_variant sub-DIEs could be direct children of the outermost DIE.

Regarding (1), we could use DW_TAG_enumeration_type but that would pretty clearly be against DWARF's definition:

An “enumeration type” is a scalar that can assume one of a fixed number of symbolic values.

Regarding (2), in a future version of Rust, where structs and enums are unified in one way or another, it might be possible that only part of a data-type is variant, so omitting the DW_TAG_variant_part might be an optimization that would have to be undone in a later revision.

With these restrictions in mind, I would be OK with saying that enums are represented in DWARF as structs that have a variant part.

Another question would be whether to represent Rust's C-like enums as actual DW_TAG_enumeration_type DIEs, since that would be a more compact representation. (Right now, I'd lean towards keeping things consistent by representing all kinds of enums the same way).

cc @Manishearth

[tromey]

This seems reasonable to me.

I think having the variant part refer to one of the members is an extension to DWARF. Currently I think this has to refer to part of the structure (well, there is also some confusing text about a type, that didn't make sense to me). But, I think this is no big deal, we can document this as an extension and perhaps file a DWARF issue about it (DWARF issues being one of the two main non-coding deliverables from this project... do we need some kind of tag for this?)

This approach seems to handle the non-zero optimzation reasonably well -- in fact it's nicer than what we have today, since it's a simple offset and dereference.

[michaelwoerister]

I think having the variant part refer to one of the members is an extension to DWARF.

I think it should be supported even without any extensions. From the DWARF 4 standard, "5.5.9 Variant Entries":

If the variant part has a discriminant, the discriminant is represented by a separate debugging
information entry which is a child of the variant part entry. This entry has the form of a structure
data member entry. The variant part entry will have a DW_AT_discr attribute whose value is a
reference to the member entry for the discriminant.

It's surprisingly unambiguous :)

[tromey]

Ok, I can see how that could be read that way. I think the discriminant is supposed to come from outside the variants. See the introductory text to 5.5:

Pascal and other languages have a “discriminated union,” also called a “variant record.” Here, selection of a number of alternative substructures (“variants”) is based on the value of a component that is not part of any of those substructures (the “discriminant”).

Also, in the text you quoted, there is a requirement for DW_AT_discr to reference the member entry; but your example shows something else -- something more useful for Rust, but not, I think, something included in the DWARF text as it stands.

A note to the DWARF list asking for clarification would be a good idea.

[tromey]

Also, in the text you quoted, there is a requirement for DW_AT_discr to reference the member entry; but your example shows something else

Or, no, I just wasn't reading it properly.

[michaelwoerister]

I think the discriminant is supposed to come from outside the variants. See the introductory text to 5.5

You're right, I didn't see that. At least it's just in the informal description, not in the normative part. We could try to have that changed to something like:

Pascal and other languages have a “discriminated union,” also called a “variant record.” Here, selection of a number of alternative substructures (“variants”) is based on the value of a component (the “discriminant”) at a well-known location within the enclosing structure.

That would seem unobjectionable to me.

[Manishearth]

For the NonZero thing, the actual location of the discriminant can be several structures deep -- does this new scheme let us specify that?

[michaelwoerister]

Yes, I would think so: The discriminant has its own DW_TAG_member entry that does not depend on anything else and explicitly specifies its memory location. That that memory location coincides with the location of a value in a nested value should not be a problem.

[Manishearth]

Will this be prone to LLVM's optimizations reshuffling fields?

Not sure if it does that now, but IIRC it reserves the power to do so.

[michaelwoerister]

Will this be prone to LLVM's optimizations reshuffling fields?

That's definitely something that needs to be investigated. I doubt that LLVM is allowed to change data-type layouts but there are optimizations like SROA that might cause problems.

[tromey]

I have been looking into this, now that rustc has more space optimizations for enums.

LLVM doesn't have debuginfo support for variant records, so adding that is the first step.

I found this thread asking for clarifications about the DWARF standard in this area: http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2006-August/thread.html#3086

Also, while neither gdb nor gcc currently read DW_TAG_variant_part, gcc can emit it; see: https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01365.html. Anyway, all this means is that there will need to be a gdb patch as well.

[tromey]

Started LLVM work here: https://github.com/tromey/llvm/tree/discriminated-unions. I'll post a link to the Phabricator request when that is ready; but I'll probably be writing all the patches first before trying to land any of them, to make sure it all works ok.

[tromey]

Started the rust compiler work here: https://github.com/tromey/rust/tree/enum-debug-info

[tromey]

Today I got the compiler generating the output I want. Next up, tests and the gdb patch.

 <2><66>: Abbrev Number: 6 (DW_TAG_structure_type)
    <67>   DW_AT_name        : (indirect string, offset: 0x85712): F
    <6b>   DW_AT_byte_size   : 16
    <6c>   DW_AT_alignment   : 8
 <3><6d>: Abbrev Number: 7 (DW_TAG_member)
    <6e>   DW_AT_type        : <0xb4>
    <72>   DW_AT_alignment   : 8
    <73>   DW_AT_data_member_location: 0
    <74>   DW_AT_artificial  : 1
 <3><74>: Abbrev Number: 8 (DW_TAG_variant_part)
    <75>   DW_AT_discr       : <0x6d>
 <4><79>: Abbrev Number: 9 (DW_TAG_variant)
 <5><7a>: Abbrev Number: 10 (DW_TAG_member)
    <7b>   DW_AT_type        : <0x8e>
    <7f>   DW_AT_alignment   : 8
    <80>   DW_AT_data_member_location: 0
 <5><81>: Abbrev Number: 0
 <4><82>: Abbrev Number: 11 (DW_TAG_variant)
    <83>   DW_AT_discr_value : 0
 <5><84>: Abbrev Number: 10 (DW_TAG_member)
    <85>   DW_AT_type        : <0xac>
    <89>   DW_AT_alignment   : 8
    <8a>   DW_AT_data_member_location: 0
...

[tromey]

I'm going to be doing the gdb work here: https://github.com/tromey/gdb/tree/variant-parts

[tromey]

It all seems to be working, so next I'm going to write an LLVM test and submit the LLVM patch.

[tromey]

LLVM review request here: https://reviews.llvm.org/D42082

This differs from the branch I published, because it is based on LLVM master, not the branch Rust uses. Once LLVM accepts something, I will back-port it for submission to rust-llvm.

[michaelwoerister]

Great to see you making progress :)

[tromey]

The LLVM patch has landed. I've back-ported it to the 4.x branch of rust-llvm (though I can't seem to link tests there...). Now I'm updating the rustc patch to work with the changes I made to the patch during the LLVM review process.

One unknown to me is whether I should also backport this to the rust-llvm 6.x branch.

[tromey]

I have rustc working again. However I'm having some second thoughts about trying to land this in the near term. On the one hand, debuginfo for some enums is currently broken; but on the other hand, because lldb doesn't understand DWARF variants, landing this will break more cases for lldb (while fixing all cases in gdb once those patches land).

[tromey]

gdb patches submitted for review: https://sourceware.org/ml/gdb-patches/2018-02/msg00269.html

[tromey]

The gdb patches have landed upstream now.

[michaelwoerister]

I don't really know this code either. It is a "pretty printer" for Visual Studio's debugger -- which you probably already knew. Do your DWARF extension already map to a CodeView/PDB equivalent?

[tromey]

LLVM doesn't emit pdb for this construct. It seems like pdb might have something relevant here, since F# has a discriminated union type, but so far I haven't been able to find it. So maybe the principled approach is out of reach - I will see if I can find someone to ask. I suppose if there's no good way, then removing these visualizers is at least reflecting the underlying reality.

Another possibility is to fall back to the current approach when not generating DWARF. However, the current approach is also already known to be broken in some cases.

[tromey]

Actually I think we need the fallback regardless, because rustc still supports LLVM 5.0.

[vadimcn]

@tromey: I've finally gotten around to figure out what's up with https://bugs.llvm.org/show_bug.cgi?id=36654. Looks like what is throwing LLVM off is union variants that are smaller than the entire union. This patch appears to fix it.
You've mentioned a while back you were going to revamp enum debug info, so I'm not sure if this is worth committing... Then again, you say above that we'll need a fallback to stay compatible with older LLVM.
What's is the current plan for this stuff?

[tromey]

What's is the current plan for this stuff?

I have a patch to change this to use DW_TAG_variant, etc. This is the best approach since it doesn't rely on magic field names or the like.

However, there are some drawbacks:

• This requires a patched lldb (and gdb, but the gdb patches went in a while ago); so this has been blocked by Add lldb to the build #52716.
• The variant part changes in LLVM aren't hooked up to the pdb output.
• The variant part changes are only in rust-llvm and in LLVM 7 -- but rust supports LLVM 5 and 6.

So, one idea is to have a fallback mode -- basically, keep the existing code (even though it is broken in some situations), and use it for Windows and for LLVM 5/6.

Recently, though, my manager was telling me not to bother and that I should try to only support LLVM 7. This is somewhat attractive since it would mean not having to rewrite my patch again. But, I don't know if it is really a good idea, certainly my natural inclination is more toward the fallback idea.

What do you think we should do?

Just today I rebased my existing patch. I plan to submit it very soon, assuming the above decision can is made. All that really remains is fixing up the testing situation -- mostly updating existing tests (and if the fallback route is not taken, removing some obsolete code from tests).

[cuviper]

So, one idea is to have a fallback mode -- basically, keep the existing code (even though it is broken in some situations), and use it for Windows and for LLVM 5/6.

Recently, though, my manager was telling me not to bother and that I should try to only support LLVM 7.

Would that mean dropping Rust support for LLVM 5/6 altogether? Or just degrading their enum debuginfo? Seems preferable to keep the imperfect status quo while improving LLVM 7, if that's feasible.

[vadimcn]

What do you think we should do?

I would prefer to keep the option of generating old-style enum debug info, at least until rustc ships with a customized version of lldb. And even then, people might have a reason to keep using whatever gdb/lldb version comes with their distro.
Also, yeah, what will happen when emitting PDB info with your patch applied?

On the flip side, rustc would need flag(s) for choosing version of the emitted debug info :-/

[tromey]

Would that mean dropping Rust support for LLVM 5/6 altogether? Or just degrading their enum debuginfo? Seems preferable to keep the imperfect status quo while improving LLVM 7, if that's feasible.

In this scenario it would mean degraded DWARF for LLVM 5/6 users.

Also, yeah, what will happen when emitting PDB info with your patch applied?

It will not emit correct information about enums. I don't really know what exactly will be produced. As far as I am aware there is no way to represent Rust enums in pdb.

On the flip side, rustc would need flag(s) for choosing version of the emitted debug info :-/

I really do not want to do this. lldb will be in rustup soon. The needed gdb support is already committed and will be in gdb 8.2, which is due out soon and which anyway is trivial to build. So, if this is a blocker, I'd rather just wait until the debugger situation is considered good enough... this will not be any worse than the fallback plan anyway.

I guess I'm ok with choosing the fallback path for pdb and for older versions of LLVM. Though I question if it's better to emit bad debug info rather than just not emit it at all. The upside of missing debug info is that at least the debugger doesn't lie to you.

Future debuginfo changes are going to face these same issues. However in some of those cases, the fallback is more clearly to do nothing, since rustc already doesn't emit some kinds of debuginfo. Still, this is something to consider when choosing our path.

[tromey]

@vadimcn My patch already includes something close to what your patch has, and so I think as part of the fallback I will be fixing this as well.