code review 140880043: runtime: convert symtab.c into symtab.go

runtime: convert symtab.c into symtab.go

Because symtab.c was partially converted before,
the diffs are not terribly useful.

The earlier conversion was trying to refactor or
clean up the code in addition to doing the translation.
It also made a mistake by redefining Func to be something
users could overwrite.

I undid those changes, making symtab.go a more
literal line-for-line translation of symtab.c instead.

[rsc, 2014-09-02 23:24:28]

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

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

[dave_cheney.net, 2014-09-02 23:34:27]

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/symtab.go
File src/pkg/runtime/symtab.go (right):

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:50: pcln := (*[100]byte)(unsafe.Pointer(&pclntab))
Where does the 100 come from? 
Can it be a constant defined above with a reference to other places that need it
to be 100 ?

[rsc, 2014-09-02 23:43:11]

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/symtab.go
File src/pkg/runtime/symtab.go (right):

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:50: pcln := (*[100]byte)(unsafe.Pointer(&pclntab))
On 2014/09/02 23:34:26, dfc wrote:
> Where does the 100 come from? 
> Can it be a constant defined above with a reference to other places that need
it
> to be 100 ?

it's just a number that is more than enough.
the code only looks at the first 8 bytes.

[bradfitz, 2014-09-02 23:45:43]

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/traceback.go
File src/pkg/runtime/traceback.go (right):

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/traceback...
src/pkg/runtime/traceback.go:527: if s[i] == t[0] && hasprefix(s, t) {
this is wrong. s[i:] instead. But I'd just bail out when t is too long to
possibly fit.

[bradfitz, 2014-09-02 23:46:25]

On Tue, Sep 2, 2014 at 4:43 PM, <rsc@golang.org> wrote:

>
> https://codereview.appspot.com/140880043/diff/40001/src/
> pkg/runtime/symtab.go
> File src/pkg/runtime/symtab.go (right):
>
> https://codereview.appspot.com/140880043/diff/40001/src/
> pkg/runtime/symtab.go#newcode50
> src/pkg/runtime/symtab.go:50: pcln :=
> (*[100]byte)(unsafe.Pointer(&pclntab))
> On 2014/09/02 23:34:26, dfc wrote:
>
>> Where does the 100 come from?
>> Can it be a constant defined above with a reference to other places
>>
> that need it
>
>> to be 100 ?
>>
>
> it's just a number that is more than enough.
> the code only looks at the first 8 bytes.


so why not 8?

[rsc, 2014-09-02 23:49:02]

On Tue, Sep 2, 2014 at 7:46 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> On Tue, Sep 2, 2014 at 4:43 PM, <rsc@golang.org> wrote:
>
>> it's just a number that is more than enough.
>> the code only looks at the first 8 bytes.
>
>
> so why not 8?
>

so i don't have to update it when it starts looking at 9.

[rsc, 2014-09-02 23:50:45]

PTAL

I painted the shed 8.

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/traceback.go
File src/pkg/runtime/traceback.go (right):

https://codereview.appspot.com/140880043/diff/40001/src/pkg/runtime/traceback...
src/pkg/runtime/traceback.go:527: if s[i] == t[0] && hasprefix(s, t) {
On 2014/09/02 23:45:43, bradfitz wrote:
> this is wrong. s[i:] instead. But I'd just bail out when t is too long to
> possibly fit.

Thanks. Will fix the bug. The optimization isn't worthwhile in this case (and
I'm translating the C code line for line).

[josharian, 2014-09-03 16:10:52]

LGTM as a line-by-line translation

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/runtime.c...
src/pkg/runtime/runtime.c:123: #pragma textflag NOSPLIT
Seems unrelated to this CL.

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go
File src/pkg/runtime/symtab.go (right):

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:75: println("function symbol table not sorted by
program counter:", hex(ftab[i].entry), gofuncname(f1), ">",
hex(ftab[i+1].entry), gofuncname(f2))
The C version checks that f2 is valid before calling gofuncname(f2):

i+1 == runtime·nftab ? "end" : runtime·funcname(f2)

Looks to me like that check is still needed.

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:201: return gostringnocopy(funcname(f))
If f is nil we will return a string with nil pointer and length 0. Are there
assumptions anywhere that strings always have non-nil pointers?

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:232: off :=
*(*int32)(add(unsafe.Pointer(&f.nfuncdata), (1+uintptr(table))*4))
Suggest s/4/unsafe.Sizeof(f.nfuncdata)/

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/traceback.go
File src/pkg/runtime/traceback.go (right):

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/traceback...
src/pkg/runtime/traceback.go:509: traceback := gotraceback(nil)
Naive question: traceback was static. It is now local. Does that matter?

[rsc, 2014-09-03 16:47:03]

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/runtime.c...
src/pkg/runtime/runtime.c:123: #pragma textflag NOSPLIT
On 2014/09/03 16:10:52, josharian wrote:
> Seems unrelated to this CL.

removed (and forgot to upload). thanks.

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go
File src/pkg/runtime/symtab.go (right):

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:75: println("function symbol table not sorted by
program counter:", hex(ftab[i].entry), gofuncname(f1), ">",
hex(ftab[i+1].entry), gofuncname(f2))
On 2014/09/03 16:10:52, josharian wrote:
> The C version checks that f2 is valid before calling gofuncname(f2):
> 
> i+1 == runtime·nftab ? "end" : runtime·funcname(f2)
> 
> Looks to me like that check is still needed.

Nice catch. Done.

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:201: return gostringnocopy(funcname(f))
On 2014/09/03 16:10:52, josharian wrote:
> If f is nil we will return a string with nil pointer and length 0. Are there
> assumptions anywhere that strings always have non-nil pointers?

no, that's fine.

https://codereview.appspot.com/140880043/diff/60001/src/pkg/runtime/symtab.go...
src/pkg/runtime/symtab.go:232: off :=
*(*int32)(add(unsafe.Pointer(&f.nfuncdata), (1+uintptr(table))*4))
On 2014/09/03 16:10:52, josharian wrote:
> Suggest s/4/unsafe.Sizeof(f.nfuncdata)/

changed to unsafe.Sizeof(f.nfuncdata)+uintptr(table)*4
the 4 is because this is an array of int32s, which is mostly
unrelated to the type of nfuncdata.

[rsc, 2014-09-03 17:02:52]

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

runtime: convert symtab.c into symtab.go

Because symtab.c was partially converted before,
the diffs are not terribly useful.

The earlier conversion was trying to refactor or
clean up the code in addition to doing the translation.
It also made a mistake by redefining Func to be something
users could overwrite.

I undid those changes, making symtab.go a more
literal line-for-line translation of symtab.c instead.

LGTM=josharian
R=golang-codereviews, dave, bradfitz, josharian
CC=golang-codereviews, iant, khr, r
https://codereview.appspot.com/140880043

[gobot, 2014-09-03 17:27:30]

This CL appears to have broken the windows-amd64-perf builder.
See http://build.golang.org/log/5e7d5a7fe7af7db286e404711be33c008094a5bc