code review 107630043: ogle/dwarf/debug: add PCToSPOffset

ogle/dwarf/debug: add PCToSPOffset
Allows us to discover the offset from the stack pointer to the
virtual frame pointer.

The DWARF machine does a lot more than this. Our implementation
handles only what gc'ed Go binaries include.

[r, 2014-07-08 20:49:57]

Hello nigeltao,

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

[nigeltao, 2014-07-09 06:31:48]

LGTM.

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

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode42
debug/dwarf/frame.go:42: // Call Frame instructions.Figure 40, page 181.
s/s.F/s. F/

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode66
debug/dwarf/frame.go:66: frameOffsetExtended_sf = 0<<6 + 0x11 // op: ULEB128
register SLEB128 offset
s/_sf/Sf/

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode102
debug/dwarf/frame.go:102: var offset int64
You could avoid the vars:

for {
  offset, found, err := m.scanFDE(b, pc)
  if err != nil {
    return 0, err
  }
  if found {
    return offset, nil
  }
}

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode138
debug/dwarf/frame.go:138: // Unused. TODO?
Return an error "unsupported foobar" instead of continuing silently?

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode182
debug/dwarf/frame.go:182: if pc < m.location || m.location+addressRange < pc {
Just double-checking that the second < should be < and not <=.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode225
debug/dwarf/frame.go:225: continue
No need for a continue after a return.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode233
debug/dwarf/frame.go:233: continue
You could drop the "continue"s...

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode249
debug/dwarf/frame.go:249: m.cfaOffset = int64(b.uint())
...for example, you don't "continue" here.

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

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:83: t.Fatal(err)
Delete.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:88: t.Fatal(err)
Delete.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:101: t.Fatal("expected %d at start of function; got
%d", addrSize, offset)
s/Fatal/Fatalf/

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:108: for pc := startPC; pc <= endPC; pc++ {
s/<=/</ right?

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/testdata/pcsptest.go
File debug/dwarf/testdata/pcsptest.go (right):

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/testdata/pcsptest...
debug/dwarf/testdata/pcsptest.go:14: // pulled down 1*addr-size for the return
PC plus 3*8 for the three
s/addr-size/addrSize/ unless I'm reading it wrong.

[r, 2014-07-09 17:57:11]

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

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode42
debug/dwarf/frame.go:42: // Call Frame instructions.Figure 40, page 181.
On 2014/07/09 06:31:48, nigeltao wrote:
> s/s.F/s. F/

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode66
debug/dwarf/frame.go:66: frameOffsetExtended_sf = 0<<6 + 0x11 // op: ULEB128
register SLEB128 offset
On 2014/07/09 06:31:48, nigeltao wrote:
> s/_sf/Sf/

Done. Aren't these names lovely? Believe it or not, they're a significant
improvement on the names in the standard.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode102
debug/dwarf/frame.go:102: var offset int64
On 2014/07/09 06:31:48, nigeltao wrote:
> You could avoid the vars:
> 
> for {
>   offset, found, err := m.scanFDE(b, pc)
>   if err != nil {
>     return 0, err
>   }
>   if found {
>     return offset, nil
>   }
> }

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode138
debug/dwarf/frame.go:138: // Unused. TODO?
On 2014/07/09 06:31:48, nigeltao wrote:
> Return an error "unsupported foobar" instead of continuing silently?

No. Version 3 is actually what we generate, so it's not an error.
Adding a comment.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode182
debug/dwarf/frame.go:182: if pc < m.location || m.location+addressRange < pc {
On 2014/07/09 06:31:48, nigeltao wrote:
> Just double-checking that the second < should be < and not <=.

Good catch.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode225
debug/dwarf/frame.go:225: continue
On 2014/07/09 06:31:48, nigeltao wrote:
> No need for a continue after a return.

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode233
debug/dwarf/frame.go:233: continue
On 2014/07/09 06:31:48, nigeltao wrote:
> You could drop the "continue"s...

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame.go#newcode249
debug/dwarf/frame.go:249: m.cfaOffset = int64(b.uint())
On 2014/07/09 06:31:48, nigeltao wrote:
> ...for example, you don't "continue" here.

Done.

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

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:83: t.Fatal(err)
On 2014/07/09 06:31:48, nigeltao wrote:
> Delete.

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:88: t.Fatal(err)
On 2014/07/09 06:31:48, nigeltao wrote:
> Delete.

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:101: t.Fatal("expected %d at start of function; got
%d", addrSize, offset)
On 2014/07/09 06:31:48, nigeltao wrote:
> s/Fatal/Fatalf/

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/frame_test.go#new...
debug/dwarf/frame_test.go:108: for pc := startPC; pc <= endPC; pc++ {
On 2014/07/09 06:31:48, nigeltao wrote:
> s/<=/</ right?

Done.

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/testdata/pcsptest.go
File debug/dwarf/testdata/pcsptest.go (right):

https://codereview.appspot.com/107630043/diff/1/debug/dwarf/testdata/pcsptest...
debug/dwarf/testdata/pcsptest.go:14: // pulled down 1*addr-size for the return
PC plus 3*8 for the three
On 2014/07/09 06:31:48, nigeltao wrote:
> s/addr-size/addrSize/ unless I'm reading it wrong.

Done.

[r, 2014-07-09 17:57:27]

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

ogle/dwarf/debug: add PCToSPOffset
Allows us to discover the offset from the stack pointer to the
virtual frame pointer.

The DWARF machine does a lot more than this. Our implementation
handles only what gc'ed Go binaries include.

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