code review 89060043: ogle: use the DWARF PC/line table instead of the gosym one

ogle: use the DWARF PC/line table instead of the gosym one
New code in debug/dwarf/line.go to interpret the .debug_line section.

[r, 2014-04-17 20:56:37]

Hello nigeltao,

I'd like you to review this change to
https://code.google.com/p/ogle

[nigeltao, 2014-04-17 21:40:22]

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go
File debug/dwarf/line.go (right):

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode5
debug/dwarf/line.go:5: // Mapping from PC to lines.
I like to put non-doc comments under the package clause:

----
// Copyright etc
// etc

package dwarf

// Mapping etc
// http://etc

// TODO: etc

import (
----

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode69
debug/dwarf/line.go:69: unit_length                        int
Is it worth renaming all_of_these_underscores to GoStyle?

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode83
debug/dwarf/line.go:83: // lineFIle represents a file name stored in the PC/line
table, usually the prologue.
s/FIle/File/

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode170
debug/dwarf/line.go:170: log.Printf("DWARF: bad header length: %d should be <=
%d", m.prologue.unit_length, len(data))
Return error instead of bool? Similarly below.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode261
debug/dwarf/line.go:261: // fmt.Printf("%#8x: % x\n", data[offset-1:offset+20])
Delete.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode264
debug/dwarf/line.go:264: offset, ok = m.parseLinePrologue(data, offset)
You could use := here.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode270
debug/dwarf/line.go:270: // fmt.Println(m)
Delete.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode273
debug/dwarf/line.go:273: if op == start_extended_opcode {
Move this into the "switch op" (and drop the continue)?

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode275
debug/dwarf/line.go:275: log.Printf("DWARF: short extended opcode (1)")
Again, return error instead of bool?

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode317
debug/dwarf/line.go:317: if op >= m.prologue.opcode_base {
Move this into the default case below?

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode376
debug/dwarf/line.go:376: panic("not reached")
This could be not so drastic if you returned error instead of bool.

[r, 2014-04-17 22:49:36]

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go
File debug/dwarf/line.go (right):

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode5
debug/dwarf/line.go:5: // Mapping from PC to lines.
On 2014/04/17 21:40:22, nigeltao wrote:
> I like to put non-doc comments under the package clause:
> 
> ----
> // Copyright etc
> // etc
> 
> package dwarf
> 
> // Mapping etc
> // http://etc
> 
> // TODO: etc
> 
> import (
> ----

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode69
debug/dwarf/line.go:69: unit_length                        int
On 2014/04/17 21:40:22, nigeltao wrote:
> Is it worth renaming all_of_these_underscores to GoStyle?

i prefer to be able to search for these words in the document.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode83
debug/dwarf/line.go:83: // lineFIle represents a file name stored in the PC/line
table, usually the prologue.
On 2014/04/17 21:40:22, nigeltao wrote:
> s/FIle/File/

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode170
debug/dwarf/line.go:170: log.Printf("DWARF: bad header length: %d should be <=
%d", m.prologue.unit_length, len(data))
On 2014/04/17 21:40:22, nigeltao wrote:
> Return error instead of bool? Similarly below.

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode261
debug/dwarf/line.go:261: // fmt.Printf("%#8x: % x\n", data[offset-1:offset+20])
On 2014/04/17 21:40:22, nigeltao wrote:
> Delete.

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode264
debug/dwarf/line.go:264: offset, ok = m.parseLinePrologue(data, offset)
On 2014/04/17 21:40:22, nigeltao wrote:
> You could use := here.

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode270
debug/dwarf/line.go:270: // fmt.Println(m)
On 2014/04/17 21:40:22, nigeltao wrote:
> Delete.

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode273
debug/dwarf/line.go:273: if op == start_extended_opcode {
On 2014/04/17 21:40:22, nigeltao wrote:
> Move this into the "switch op" (and drop the continue)?

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode275
debug/dwarf/line.go:275: log.Printf("DWARF: short extended opcode (1)")
On 2014/04/17 21:40:22, nigeltao wrote:
> Again, return error instead of bool?

Done.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode317
debug/dwarf/line.go:317: if op >= m.prologue.opcode_base {
On 2014/04/17 21:40:22, nigeltao wrote:
> Move this into the default case below?

it must be before the switch because the number of switch entries that are valid
depends on prologue.opcode_base.
see the comment on line 37.

https://codereview.appspot.com/89060043/diff/1/debug/dwarf/line.go#newcode376
debug/dwarf/line.go:376: panic("not reached")
On 2014/04/17 21:40:22, nigeltao wrote:
> This could be not so drastic if you returned error instead of bool.

It's really not reached.

[r, 2014-04-17 22:49:40]

Hello nigeltao@golang.org,

Please take another look.

[nigeltao, 2014-04-18 02:14:06]

LGTM.

[r, 2014-04-18 17:38:42]

*** Submitted as https://code.google.com/p/ogle/source/detail?r=2747377222b1 ***

ogle: use the DWARF PC/line table instead of the gosym one
New code in debug/dwarf/line.go to interpret the .debug_line section.

LGTM=nigeltao
R=nigeltao
https://codereview.appspot.com/89060043