code review 53820043: cmd/link: pclntab generation

cmd/link: pclntab generation

[rsc, 2014-01-17 19:17:43]

Hello iant (cc: golang-codereviews@googlegroups.com),

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

[iant, 2014-01-17 21:16:19]

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go
File src/cmd/link/pclntab.go (right):

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:6: // See runtime.go.
Why should I see runtime.go?

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:104: // variable-sized data. end gives the current write
position for
This comment does not seem correct.  It seems that end is the offset after the
Func encoding and all the variable-sized data.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:106: end := off + p.ptrsize + 3*4 + 5*4 +
len(f.PCData)*4 + len(f.FuncData)*p.ptrsize
http://golang.org/s/go12symtab doesn't match this.  It looks like the code has
args and frame fields that are not on the web page.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:117: off = buf.Uint32(off, uint32(addString(buf,
sym.Name)))
According to http://golang.orgs/go12symtab this should be an offset from the
start of the Func structure.  The code is actually storing an offset from the
start of the pclntab buffer.  That looks consistent with what runtime/symtab.c
does, so I guess go12symtab should be updated.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:119: off = buf.Uint32(off, uint32(f.Frame+p.ptrsize))
Why add p.ptrsize here?  Probably deserves a comment.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:133: off += off & 4 // must be pointer-aligned and we're
only int32-aligned.
Pointer-aligned is not int64-aligned on an int32 system.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:166: buf.Uint32(off, uint32(len(files)+1))
I don't understand why you add 1 here.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:388: Error     bool
Experience suggests that either this should be an error value or we should print
out information when an error occurs.

[rsc, 2014-01-21 16:42:18]

PTAL

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go
File src/cmd/link/pclntab.go (right):

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:6: // See runtime.go.
On 2014/01/17 21:16:19, iant wrote:
> Why should I see runtime.go?

Only because that's how you end up in this code.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:104: // variable-sized data. end gives the current write
position for
On 2014/01/17 21:16:19, iant wrote:
> This comment does not seem correct.  It seems that end is the offset after the
> Func encoding and all the variable-sized data.

Rephrased, thanks.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:106: end := off + p.ptrsize + 3*4 + 5*4 +
len(f.PCData)*4 + len(f.FuncData)*p.ptrsize
On 2014/01/17 21:16:19, iant wrote:
> http://golang.org/s/go12symtab doesn't match this.  It looks like the code has
> args and frame fields that are not on the web page.

Done.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:117: off = buf.Uint32(off, uint32(addString(buf,
sym.Name)))
On 2014/01/17 21:16:19, iant wrote:
> According to http://golang.orgs/go12symtab this should be an offset from the
> start of the Func structure.  The code is actually storing an offset from the
> start of the pclntab buffer.  That looks consistent with what runtime/symtab.c
> does, so I guess go12symtab should be updated.

Done.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:119: off = buf.Uint32(off, uint32(f.Frame+p.ptrsize))
On 2014/01/17 21:16:19, iant wrote:
> Why add p.ptrsize here?  Probably deserves a comment.

Done (and a TODO).

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:133: off += off & 4 // must be pointer-aligned and we're
only int32-aligned.
On 2014/01/17 21:16:19, iant wrote:
> Pointer-aligned is not int64-aligned on an int32 system.

Done.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:166: buf.Uint32(off, uint32(len(files)+1))
On 2014/01/17 21:16:19, iant wrote:
> I don't understand why you add 1 here.

I changed the comment; maybe it will help.
The array is like
   var files [n+1]uint32
files[0] = n+1
files[1] = file name for #1
files[2] = file name for #2
files[n] = file name for #n

The numbering here (using 1..n instead of 0..n-1) is probably a mistake, but it
is easier to keep using it than to have to write readers that figure out whether
they are reading a Go 1.2 or a Go 1.3 file list.

https://codereview.appspot.com/53820043/diff/70001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:388: Error     bool
On 2014/01/17 21:16:19, iant wrote:
> Experience suggests that either this should be an error value or we should
print
> out information when an error occurs.

I agree in general. But my experience with this particular data structure is
that by far the most common reason you see a corrupt table is that the caller
passed in a completely wrong bunch of bytes (for example, loaded from the wrong
place in the file, because you got the offset wrong somehow). That's why the
result here is just one bit: "this is not a valid pcdata table".
I've renamed it to Corrupt bool, just so that it the name isn't so suggestive.

[iant, 2014-01-21 17:24:37]

LGTM

https://codereview.appspot.com/53820043/diff/90001/src/cmd/link/pclntab.go
File src/cmd/link/pclntab.go (right):

https://codereview.appspot.com/53820043/diff/90001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:168: // The first entry gives 1+n, the size of the
array.
I don't see how to reconcile this with http://golang.org/s/go12symtab, which
says this table is
N name1 name2 name3 … nameN
If this is correct then I guess go12symtab should say
N+1 name1 name2 name3 … nameN

[rsc, 2014-01-21 18:47:51]

*** Submitted as https://code.google.com/p/go/source/detail?r=95018395386a ***

cmd/link: pclntab generation

R=iant
CC=golang-codereviews
https://codereview.appspot.com/53820043

https://codereview.appspot.com/53820043/diff/90001/src/cmd/link/pclntab.go
File src/cmd/link/pclntab.go (right):

https://codereview.appspot.com/53820043/diff/90001/src/cmd/link/pclntab.go#ne...
src/cmd/link/pclntab.go:168: // The first entry gives 1+n, the size of the
array.
On 2014/01/21 17:24:37, iant wrote:
> I don't see how to reconcile this with http://golang.org/s/go12symtab, which
> says this table is
> N name1 name2 name3 … nameN
> If this is correct then I guess go12symtab should say
> N+1 name1 name2 name3 … nameN

Done.