code review 129240043: dwarf: differentiate DW_FORM_ref_alt from DW_FORM_strp_alt

dwarf: differentiate DW_FORM_ref_alt from DW_FORM_strp_alt "Val" isn't sufficient by itself to differentiate DW_FORM_ref_alt from DW_FORM_strp_alt. See http://www.dwarfstd.org/ShowIssue.php?issue=120604.1 DW_FORM_ref_alt represents offset in alternate .debug_info while DW_FORM_strp_alt represents offset in alternate .debug_str section. So we introduce two different types to handle such scenarios.

[dhiru, 2014-08-15 17:59:01]

Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com, iant@golang.org, rsc@golang.org), I'd like you to review this change to https://code.google.com/p/go

[iant, 2014-08-15 18:01:25]

Any chance of a test case? https://codereview.appspot.com/129240043/diff/40001/src/pkg/debug/dwarf/entry.go File src/pkg/debug/dwarf/entry.go (right): https://codereview.appspot.com/129240043/diff/40001/src/pkg/debug/dwarf/entry... src/pkg/debug/dwarf/entry.go:135: type Offset_ref_alt int64 Go names use FunkyCaps, not underscores (http://golang.org/doc/effective_go.html#mixed-caps). https://codereview.appspot.com/129240043/diff/40001/src/pkg/debug/dwarf/entry... src/pkg/debug/dwarf/entry.go:264: val = Offset_ref_alt(val.(int64)) This will crash if there was an error above. I think it might be better to set a new int64 variable local to this block, and then switch on the form to set val.

[dhiru, 2014-08-15 20:22:09]

Hello golang-codereviews@googlegroups.com, iant@golang.org (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.

[iant, 2014-08-15 20:45:09]

https://codereview.appspot.com/129240043/diff/230001/src/pkg/debug/dwarf/DW_F... File src/pkg/debug/dwarf/DW_FORM_strp_alt-HOWTO.txt (right): https://codereview.appspot.com/129240043/diff/230001/src/pkg/debug/dwarf/DW_F... src/pkg/debug/dwarf/DW_FORM_strp_alt-HOWTO.txt:1: Separate .debug files Instead of this file, please add debug/dwarf/testdata/README that lists the commands used to create typedef.gnu. We don't need the notes or anything, just the command sequence. Thanks. https://codereview.appspot.com/129240043/diff/230001/src/pkg/debug/dwarf/entr... File src/pkg/debug/dwarf/entry.go (right): https://codereview.appspot.com/129240043/diff/230001/src/pkg/debug/dwarf/entr... src/pkg/debug/dwarf/entry.go:135: type OffsetRefAlt int64 These two types need doc comments for godoc. Something like // An OffsetRefAlt represents the value of a formGnuRefAlt. It holds an offset in an alternate object file.

[dhiru, 2014-08-15 21:01:03]

Hello golang-codereviews@googlegroups.com, iant@golang.org (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.

[iant, 2014-08-15 21:12:22]

Looks pretty good but I just realized that you need to add typedef.gnu to type_test.go. Please make sure that "go test debug/dwarf" fails without your change and passes with your change. Thanks.

[dhiru, 2014-08-16 09:25:33]

On 2014/08/15 21:12:22, iant wrote: > Looks pretty good but I just realized that you need to add typedef.gnu to > type_test.go. Please make sure that "go test debug/dwarf" fails without your > change and passes with your change. Thanks. After following the steps in "README", typedef1.so.debug (typedef.gnu) will have DW_FORM_GNU_strp_alt but it will *not* have any DW_TAG_typedef stuff in it (which gets extracted out to typedef.dwz by the dwz utility). This causes "go test debug/dwarf" to fail. How do we get around this problem? Does it make *any* sense to keep typedef.dzw (instead of typedef1.so.debug) as "typedef.gnu" in the repository?

[iant, 2014-08-17 02:10:52]

On 2014/08/16 09:25:33, dhiru wrote: > On 2014/08/15 21:12:22, iant wrote: > > Looks pretty good but I just realized that you need to add typedef.gnu to > > type_test.go. Please make sure that "go test debug/dwarf" fails without your > > change and passes with your change. Thanks. > > After following the steps in "README", typedef1.so.debug (typedef.gnu) will > have DW_FORM_GNU_strp_alt but it will *not* have any DW_TAG_typedef stuff in it > (which gets extracted out to typedef.dwz by the dwz utility). > > This causes "go test debug/dwarf" to fail. How do we get around this problem? > > Does it make *any* sense to keep typedef.dzw (instead of typedef1.so.debug) as > "typedef.gnu" in the repository? Probably not as yet. In this case it seems to me we could just write a different test to look at typedef.gnu. Ian

[dhiru, 2014-08-27 13:17:36]

On 2014/08/17 02:10:52, iant wrote: > On 2014/08/16 09:25:33, dhiru wrote: > > Does it make *any* sense to keep typedef.dzw (instead of typedef1.so.debug) as > > "typedef.gnu" in the repository? > > Probably not as yet. > > In this case it seems to me we could just write a different test to look at > typedef.gnu. Hi Ian, It would great if this ticket could get some attention and love :-) Can I do something to move this forward? Dhiru

[iant, 2014-08-27 19:15:34]

On 2014/08/27 13:17:36, dhiru wrote: > > It would great if this ticket could get some attention and love :-) > > Can I do something to move this forward? I thought you were going to write a test. Just adding typedef.gnu to testdata doesn't do anything. We need to actually test that the data can be read, and that it arrives with the right types. I understand that it can't simply call the existing testTypedefs, but it should be possible to do something similar. Thanks. Ian

[gobot, 2014-12-19 05:12:07]

R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/129240043 is best) in the description in your new CL. Thanks very much.