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

cmd/link: include hint about embedded struct field in DWARF #20037

Closed
hyangah opened this issue Apr 19, 2017 · 9 comments

Comments

Projects
None yet
7 participants
@hyangah
Copy link
Contributor

commented Apr 19, 2017

For the following code:

type A struct {  val int   }
type B struct {  A }

This is the dwarf type info for B. From this info, it's not possible to distinguish whether the field A is embedded or, a field explicitly named as A.

 <1><37954>: Abbrev Number: 21 (DW_TAG_structure_type)
    <37955>   DW_AT_name        : main.B        
    <3795c>   DW_AT_byte_size   : 8     
    <3795d>   Unknown AT value: 2900: 25        
 <2><3795e>: Abbrev Number: 6 (DW_TAG_member)
    <3795f>   DW_AT_name        : A     
    <37961>   DW_AT_data_member_location: 2 byte block: 23 0    (DW_OP_plus_uconst: 0)
    <37964>   DW_AT_type        : <0x37944>     
 <2><3796c>: Abbrev Number: 0
 <1><3796d>: Abbrev Number: 22 (DW_TAG_typedef)
    <3796e>   DW_AT_name        : main.B        
    <37975>   DW_AT_type        : <0x37954>  

Delve or others have been using heuristics to determine whether a field is an embedded one, e.g.,
https://github.com/derekparker/delve/blob/master/pkg/proc/variables.go#L711
But those heuristics are fragile at best (delve's heuristic doesn't work after 1.9, and also with type alias).

We need an explicit mechanism to tell embedded field.

@alandonovan @dr2chase @mdempsky @heschik

@hyangah hyangah changed the title cmd/link: include hint about embedded struct field cmd/link: include hint about embedded struct field in DWARF Apr 19, 2017

@hyangah

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

Perhaps the best way to represent the additional bit that distinguishes these two declarations:

   type T { A } 
   type T { A A } 

is to use the DW_TAG_inheritance mechanism, which can record that in the first declaration, "T inherits from A, and the A can be found at offset zero within T". Although this terminology has a bias towards C++ and Java, it's not a bad fit for Go, and you can use multiple 'inheritance' tags to describe a struct type that contains multiple "anonymous" fields.

@hyangah

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

Embedded field can be accessed via its implicit name (T.A.val or T.val). So, we need a way to infer the implicit name.

If DW_TAG_inheritance allows DW_AT_name attribute, the name can be encoded there.
Otherwise, DW_TAG_inheritance has the type attribute, and the type's name can be used as the field's name.

One minor issue to sort out is that currently DWARF info doesn't have type entry for aliased types. In the following case, the field name "AA" should be encoded somewhere.

type AA = A
type T struct { AA }
@heschik

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

I tried adding an additional DW_TAG_inheritance to the struct, so that the embedded field is defined both as inheritance and a member. That lets you omit or include the field name as you like, and GDB was fine with it, though it did print a bit strangely:
{<> = {val = 2}, A = {val = 2}}

The problem is that you can embed non-structs, and GDB doesn't like that at all:

../../gdb-7.9.x/gdb/gnu-v3-abi.c:207: internal-error: gnuv3_dynamic_class: Assertion `TYPE_CODE (type) == TYPE_CODE_STRUCT || TYPE_CODE (type) == TYPE_CODE_UNION' failed.

And it really doesn't like embedding a struct pointer -- it core dumped.

I suppose we could try to claim this is a GDB bug, but my feeling is that embedding and inheritance are different. I would be happier just adding a Go-specific attribute to the member tag.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 26, 2017

CL https://golang.org/cl/41873 mentions this issue.

@hyangah

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@derekparker, what do you think about cl/41873?
If ok, I can make a change to delve as well after the cl is submitted.

@derekparker

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

@hyangah that change LGTM -- and yes, please do submit a patch to Delve as well. Thanks!

gopherbot pushed a commit that referenced this issue Apr 27, 2017

dwarf: add marker for embedded fields in dwarf
Currently, the following two codes generate the identical dwarf info
for type Foo.

prog 1)
type Foo struct {
   Bar
}

prog 2)
type Foo struct {
   Bar Bar
}

This change adds a go-specific attribute DW_AT_go_embedded_field
to annotate each member entry. Its absence or false value indicates
the corresponding member is not an embedded field.

Update #20037

Change-Id: Ibcbd2714f3e4d97c7b523d7398f29ab2301cc897
Reviewed-on: https://go-review.googlesource.com/41873
Reviewed-by: David Chase <drchase@google.com>
@aarzilli

This comment has been minimized.

Copy link
Contributor

commented May 3, 2017

@hyangah I've written the patch for delve, I'm waiting on this CL to submit it.

gopherbot pushed a commit to golang/debug that referenced this issue May 5, 2017

x/debug/dwarf: support new AttrGoEmbed field of struct members
The new custom attribute 0x2903 was added to DIEs of struct members to
distinguish struct embeds from regular fields.

CL: https://golang.org/cl/41873
Issue: golang/go#20037

Change-Id: Ia676c6ef0d49e4e824e28d5cb53cad590c3b4f4a
Reviewed-on: https://go-review.googlesource.com/42512
Reviewed-by: John Dethridge <jcd@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@AlekSi

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

Anything else should be done to fix this issue or it should be closed?

@hyangah hyangah closed this May 17, 2017

@golang golang locked and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.