Joel Brobecker sent me an Ada test case so that I could see a real-life example of the use of DW_TAG_variant_part (in support of some Rust stuff I'm doing elsewhere). For this test case, GCC emits this DWARF: <1><7c0>: Abbrev Number: 4 (DW_TAG_structure_type) <7c1> DW_AT_name : (indirect string, offset: 0xb26): pck__rec <7c5> DW_AT_byte_size : 13 byte block: 97 94 1 99 95 0 0 0 23 7 9 fc 1a <7d3> DW_AT_decl_file : 2 <7d4> DW_AT_decl_line : 15 <7d5> DW_AT_sibling : <0x817> <2><7d9>: Abbrev Number: 5 (DW_TAG_member) <7da> DW_AT_name : (indirect string, offset: 0xaf8): discr <7de> DW_AT_decl_file : 2 <7df> DW_AT_decl_line : 15 <7e0> DW_AT_type : <0x7af> <7e4> DW_AT_data_member_location: 0 <2><7e5>: Abbrev Number: 6 (DW_TAG_variant_part) <7e6> DW_AT_discr : <0x7d9> <3><7ea>: Abbrev Number: 7 (DW_TAG_variant) <7eb> DW_AT_discr_value : 1 <7ec> DW_AT_sibling : <0x7fd> However, the DWARF standard says: > 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. So, I think that, according to the DWARF standard, that "discr" member must be a child (not a sibling) of the DW_TAG_variant_part.
Hello Tom, I agree with your interpretation of the standard. I did not notice that part when I implemented DW_TAG_variant_part generation several years ago. I’ll try to fix that. Note however that since GCC is in stage 4, it’s likely that the fix will not be accepted in trunk until we switch back to stage 5 (in April, I guess). In any case, thank you for reporting this!
Thinking more about it, the rule that the discriminant entry must be a child of the variant part entry sounds suspicious to me. In the case of two variant parts, which are nested and depend on the same discriminant, where should the discriminant entry go? If it’s in the outer one, then it’s not a child of the nested one, violating the rule, and conversely. We could duplicate the dicriminant entry, but that does not look friendly DWARF consumer at all: two member entries would have the same name. And that sounds like a waste of space. Here’s the Ada example: -- $ gcc -c -g -fgnat-encodings=minimal pck.ads). package Pck is type Rec (I : Integer) is record case I is when Positive => C : Character; case I is when 0 => null; when others => N : Natural; end case; when others => S : String (1 .. 10); end case; end record; R : Rec (1); end Pck; And now, even though it’s illegal in Ada (it could be legal in another language): what about two variant parts that have the same discriminant and that are not nested? I guess I should report these questions to the DWARF discussion mailing list. What do you think, Tom?
(In reply to Pierre-Marie de Rodat from comment #2) > Thinking more about it, the rule that the discriminant entry must be a child > of the variant part entry sounds suspicious to me. TBH this did not make sense to me, either, which is partly why I originally wrote my patch the "more natural" way -- then this got caught in review, see https://reviews.llvm.org/D42082 > I guess I should report these questions to the DWARF discussion mailing > list. What do you think, Tom? It's worth a shot, though I think it was tried before, see http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2006-August/001710.html
(In reply to Tom Tromey from comment #3) > TBH this did not make sense to me, either, which is partly why I originally > wrote my patch the "more natural" way -- then this got caught in review, > see https://reviews.llvm.org/D42082 Thanks for the pointer. So first, thanks for your efforts in adding support for these tags in GDB! During the review, Paul said: > Adding a TAG_variant_part that had no children could also work, although it > seems a little odd and might trip up an unwary non-Rust-aware debugger. Well, for now, we do generate child-less variant parts for Ada types such as: type Integer_Option (B : Boolean) is record case B is when False => null; when True => Value : Integer; end record; So I hope non-Rust-aware debuggers can cope with this. ;-) > It's worth a shot, though I think it was tried before, see > http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2006-August/ > 001710.html Ah, I see. Looks like I have the same reasoning that Ron and Todd with respect to variant parts nesting had 12 years ago! :-D Everybody seems to agree that having a top-level member to serve as a variant part’s discriminant is a good thing to have, and it’s even suggested to post a proposal to reword the troublesome paragraph. So I suggest we indeed post a submission on dwarf-discuss@ and leave GCC’s DWARF back-end as it is today. What do you think? It’s a bit late for me, so I’ll give it a try tomorrow.
I just submitted an Issue/Comment on dwarfstd.org, but unfortunately it is not yet publicly visible (http://dwarfstd.org/Issues.php). Waiting for feedback from there…
Just got a notification that it got assigned issue #180123.1: http://dwarfstd.org/ShowIssue.php?issue=180123.1
For Rust I ended up following the letter of the standard, so I'm going to follow this in the gdb patches as well. That said, gdb can be adapted to work with either approach, so it's not strictly necessary to change gcc here, perhaps depending on resolution of that DWARF issue.
Understood, thank you for the notice! As we have to tweak the spec one way or another for Ada, I suggest indeed we keep the way things are implemented in GCC today, waiting for the DWARF committee to state on this. This will probably take a while, so I’m not sure what to do with this PR. ;-) Can you please tell me when you managed to update GDB to work with variant? It could be interesting to see how it deals with GCC’s, and if it does not, how much work will be needed. Thank you in advance!
(In reply to Pierre-Marie de Rodat from comment #8) > Understood, thank you for the notice! As we have to tweak the spec one way > or another for Ada, I suggest indeed we keep the way things are implemented > in GCC today, waiting for the DWARF committee to state on this. This will > probably take a while, so I’m not sure what to do with this PR. ;-) I suppose we could leave it open pending resolution. > Can you please tell me when you managed to update GDB to work with variant? > It could be interesting to see how it deals with GCC’s, and if it does not, > how much work will be needed. Thank you in advance! It is here: https://github.com/tromey/gdb/tree/variant-parts I plan to submit it to gdb soon, like probably today. There are 3 patches; the first one introduces some minor changes to add discriminated unions to gdb's type system, and the third one adds the DWARF reading parts. I imagine there are some gaps between what I did and what Ada requires. Since I don't know Ada I'm not really sure how big the gaps are. I'd suggest reading it, then commenting on the gdb-patches post so that it can be discussed there. Dealing with the particular location of the discriminant might not be too awful. One simple idea would be to make a new artificial discriminant in the discriminated union, essentially copying the member from the outer struct. There are two missing bits I know about: one is that I didn't need DW_AT_discr_list, so I didn't add this. This should be straightforward. The other is that in Rust, a variant can only have a single member. Multiple members could be dealt with in the current model by interposing a new anonymous structure type, perhaps.
I have been looking at this again recently, for Ada, and now I think perhaps the approach that GCC takes should be preferred. At first I was thinking maybe the compiler could linearize the members of the emitted structure, but I tried modifying the example in comment #2 to add multiple variants with different discriminators: package Pck is type Rec (I : Integer; J: Integer) is record case I is when Positive => C : Character; case J is when 0 => null; when others => N : Natural; end case; when others => S : String (1 .. 10); end case; end record; R : Rec (1, 2); end Pck; ... this seems to work and I think means that nested variant parts is indeed a sensible way to go.
I'm fully on board with the GCC approach here, and I think this is something that should be changed in the DWARF standard. And, there's a DWARF bug for it. And finally, I have some patches to teach gdb to understand the GCC output here. So, I'm closing this as invalid.