code review 112270043: ogle/server: provide type-aware printer for DWARF-typed...

ogle/server: provide type-aware printer for DWARF-typed objects

[r, 2014-07-14 19:21:54]

Hello nigeltao,

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

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

Hello nigeltao@golang.org,

Please take another look.

[nigeltao, 2014-07-15 04:14:41]

https://codereview.appspot.com/112270043/diff/40001/debug/dwarf/symbol.go
File debug/dwarf/symbol.go (right):

https://codereview.appspot.com/112270043/diff/40001/debug/dwarf/symbol.go#new...
debug/dwarf/symbol.go:67: return nil, fmt.Errorf("symbol %q not found", name)
s/symbol/entry/ ?

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go
File program/server/print.go (right):

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:16: // Routines to print a value using DWARF type
descriptions.
s/Routines/Methods that don't take an implicit 'this' parameter/

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:49: // it is printed to printBuf, parenthesized for
discimination, and remembered.
Typo in "discrimination".

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:51: if p.err != nil && err != nil {
The first != should be ==.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:60: p.tmp = p.tmp[:len]
What happens if you try to peek at a large array? Would it be better to size tmp
on demand?

if int64(cap(p.tmp)) < len {
  p.tmp = make([]byte, len)
}

You could be fancier (powers of two; round up small lengths to 100) if you want.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:77: tmp:     make([]byte, 100), // Enough for a largish
string.
You could delete this line if you sized tmp on demand.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:124: p.errorf("(unimplemented location type %#x)",
data[0])
Drop the inner ()s?

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:202: p.printf("%g",
math.Float64frombits(p.arch.UintN(p.tmp)))
I don't know my hardware architectures that well... Is the float64 bit-level
representation the same as or independent from the uint64 endianness? I mean, is
float64(1.0) always going to be "3f f0 00 00 00 00 00 00" for both big and
little endians? p.arch.UintN will certainly return a different uint64 for that
byte sequence.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:250: p.errorf("unimplemented typedef type %T %v", typ,
typ)
Is the "%T" useful? Isn't it just "*dwarf.TypedefType"?

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:259: p.errorf("array has no defined size")
I might be misunderstanding the code, but "no defined size" doesn't sound right.
Similarly many times below.

Separately, I'm not sure if this is necessary in this instance. AFAICT you don't
look at p.tmp after you peek, and the later element-by-element peek will do any
bounds checking anyway, right?

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:276: if !first {
This could just be "if i != 0 {" and you could then drop the first variable.
Similarly below.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:370: default:
You could drop the default case:

switch typ.(type) {
case *dwarf.PtrType:
  return int64(p.arch.PointerSize)
}
p.errorf("unknown size for %s", typ)
return -1

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:371: p.errorf("unknown size for %s", typ.String())
The ".String()" is unnecessary.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:371: p.errorf("unknown size for %s", typ.String())
Add
size = -1
after this. Or drop the default case (see above).

[nigeltao, 2014-07-15 04:20:34]

LGTM.

[r, 2014-07-15 04:56:51]

https://codereview.appspot.com/112270043/diff/40001/debug/dwarf/symbol.go
File debug/dwarf/symbol.go (right):

https://codereview.appspot.com/112270043/diff/40001/debug/dwarf/symbol.go#new...
debug/dwarf/symbol.go:67: return nil, fmt.Errorf("symbol %q not found", name)
On 2014/07/15 04:14:40, nigeltao wrote:
> s/symbol/entry/ ?

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go
File program/server/print.go (right):

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:49: // it is printed to printBuf, parenthesized for
discimination, and remembered.
On 2014/07/15 04:14:40, nigeltao wrote:
> Typo in "discrimination".

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:51: if p.err != nil && err != nil {
On 2014/07/15 04:14:40, nigeltao wrote:
> The first != should be ==.

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:60: p.tmp = p.tmp[:len]
it's never large. the only big thing we read this way is strings, and that's
taken care of.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:77: tmp:     make([]byte, 100), // Enough for a largish
string.
On 2014/07/15 04:14:40, nigeltao wrote:
> You could delete this line if you sized tmp on demand.

see above

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:124: p.errorf("(unimplemented location type %#x)",
data[0])
On 2014/07/15 04:14:40, nigeltao wrote:
> Drop the inner ()s?

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:202: p.printf("%g",
math.Float64frombits(p.arch.UintN(p.tmp)))
On 2014/07/15 04:14:40, nigeltao wrote:
> I don't know my hardware architectures that well... Is the float64 bit-level
> representation the same as or independent from the uint64 endianness? I mean,
is
> float64(1.0) always going to be "3f f0 00 00 00 00 00 00" for both big and
> little endians? p.arch.UintN will certainly return a different uint64 for that
> byte sequence.

we'll find out, but i believe this is correct as written on all consistent
machines (i.e., not pdp-11).

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:250: p.errorf("unimplemented typedef type %T %v", typ,
typ)
On 2014/07/15 04:14:41, nigeltao wrote:
> Is the "%T" useful? Isn't it just "*dwarf.TypedefType"?

%T tells us what the dwarf type is. %s shows us what the type displays as to the
user.
depending who you are, one is more informative than the other.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:259: p.errorf("array has no defined size")
This code is wrong anyway. There's no point in reading the array, since it has
no header.
The code was copied from printSliceAt without proper testing.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:276: if !first {
On 2014/07/15 04:14:40, nigeltao wrote:
> This could just be "if i != 0 {" and you could then drop the first variable.
> Similarly below.

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:276: if !first {
On 2014/07/15 04:14:40, nigeltao wrote:
> This could just be "if i != 0 {" and you could then drop the first variable.
> Similarly below.

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:370: default:
On 2014/07/15 04:14:41, nigeltao wrote:
> You could drop the default case:
> 
> switch typ.(type) {
> case *dwarf.PtrType:
>   return int64(p.arch.PointerSize)
> }
> p.errorf("unknown size for %s", typ)
> return -1

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:371: p.errorf("unknown size for %s", typ.String())
On 2014/07/15 04:14:40, nigeltao wrote:
> The ".String()" is unnecessary.

Done.

https://codereview.appspot.com/112270043/diff/40001/program/server/print.go#n...
program/server/print.go:371: p.errorf("unknown size for %s", typ.String())
On 2014/07/15 04:14:40, nigeltao wrote:
> Add
> size = -1
> after this. Or drop the default case (see above).

Done.

[r, 2014-07-15 04:56:53]

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

ogle/server: provide type-aware printer for DWARF-typed objects

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