Bug 83935 - DWARF for a variant part is incorrect
Summary: DWARF for a variant part is incorrect
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: debug (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Pierre-Marie de Rodat
URL:
Keywords: wrong-debug
Depends on:
Blocks:
 
Reported: 2018-01-18 21:41 UTC by Tom Tromey
Modified: 2020-03-12 16:26 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-01-22 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2018-01-18 21:41:48 UTC
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.
Comment 1 Pierre-Marie de Rodat 2018-01-22 10:05:45 UTC
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!
Comment 2 Pierre-Marie de Rodat 2018-01-22 10:24:33 UTC
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?
Comment 3 Tom Tromey 2018-01-22 17:58:34 UTC
(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
Comment 4 Pierre-Marie de Rodat 2018-01-22 18:41:44 UTC
(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.
Comment 5 Pierre-Marie de Rodat 2018-01-23 18:07:49 UTC
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…
Comment 6 Pierre-Marie de Rodat 2018-02-01 17:37:41 UTC
Just got a notification that it got assigned issue #180123.1:
http://dwarfstd.org/ShowIssue.php?issue=180123.1
Comment 7 Tom Tromey 2018-02-19 19:41:11 UTC
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.
Comment 8 Pierre-Marie de Rodat 2018-02-20 11:12:10 UTC
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!
Comment 9 Tom Tromey 2018-02-20 17:34:37 UTC
(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.
Comment 10 Tom Tromey 2019-05-01 20:44:51 UTC
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.
Comment 11 Tom Tromey 2020-03-12 16:26:10 UTC
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.