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#newcode6 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#newcode104 ...
11 years, 1 month ago
(2014-01-17 21:16:19 UTC)
#2
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#newcode6 src/cmd/link/pclntab.go:6: // See runtime.go. On 2014/01/17 21:16:19, iant wrote: ...
11 years, 1 month ago
(2014-01-21 16:42:18 UTC)
#3
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.
Issue 53820043: code review 53820043: cmd/link: pclntab generation
(Closed)
Created 11 years, 1 month ago by rsc
Modified 11 years, 1 month ago
Reviewers:
Base URL:
Comments: 18