code review 129510043: runtime: convert Stack to Go.

runtime: convert Stack to Go.

[remyoudompheng, 2014-08-22 06:10:28]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2014-08-22 06:12:25]

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

https://codereview.appspot.com/129510043/diff/40001/src/pkg/runtime/mprof.go#...
src/pkg/runtime/mprof.go:148: func Stack(buf []byte, all bool) (n int) {
why is n named here? It wasn't before: http://golang.org/pkg/runtime/#Stack

And the token 'n' isn't mentioned in the documentation text above.

[remyoudompheng, 2014-08-22 06:12:45]

I added traceback and tracebackothers to stubs.go without a *_m variant, I'm not
sure if this is the right thing to do.

[remyoudompheng, 2014-08-22 06:23:53]

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

Please take another look.

[josharian, 2014-08-22 15:35:07]

Missing asm_amd64p32.s?

[remyoudompheng, 2014-08-22 19:16:45]

Hello khr@golang.org, josharian@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[remyoudompheng, 2014-08-22 19:16:54]

On 2014/08/22 15:35:07, josharian wrote:
> Missing asm_amd64p32.s?

Yes, fixed.

[remyoudompheng, 2014-08-22 19:17:04]

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

https://codereview.appspot.com/129510043/diff/40001/src/pkg/runtime/mprof.go#...
src/pkg/runtime/mprof.go:148: func Stack(buf []byte, all bool) (n int) {
On 2014/08/22 06:12:25, bradfitz wrote:
> why is n named here? It wasn't before: http://golang.org/pkg/runtime/#Stack
> 
> And the token 'n' isn't mentioned in the documentation text above.

You are right, I think the n got here through copy-pasting from mprof.goc

[remyoudompheng, 2014-08-24 09:36:30]

ping?

[khr, 2014-08-25 01:03:37]

LGTM.

traceback and tracebackothers still need to be translated.  That can be a
separate CL.  They should probably run on the M stack.

https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:865: MOVL	sp+0(FP),AX		// addr of first arg
sp -> p

https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_amd64...
File src/pkg/runtime/asm_amd64p32.s (right):

https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64p32.s:751: TEXT runtime·gogetcallersp(SB),NOSPLIT,$0-12
I think this should be $0-8?

[remyoudompheng, 2014-08-26 06:40:05]

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

runtime: convert Stack to Go.

LGTM=khr
R=khr, josharian
CC=golang-codereviews
https://codereview.appspot.com/129510043

https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_amd64...
File src/pkg/runtime/asm_amd64p32.s (right):

https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64p32.s:751: TEXT runtime·gogetcallersp(SB),NOSPLIT,$0-12
On 2014/08/25 01:03:36, khr wrote:
> I think this should be $0-8?

The boundary between args and return values is 64-bit aligned. It should be like
gogetcallerpc

[mpl, 2014-08-27 15:59:07]

I have a question.
As far as I can see, the C version was calling runtime·goroutineheader(g);
(which happens to interest me because it was doing "runtime·printf("goroutine %D
[%S", gp->goid, status);"), and I don't see any equivalent done in the Go
version. What am I missing? Or is it omitted on purpose?

Thanks.

On 2014/08/26 06:40:05, remyoudompheng wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=6138449ef60f ***
> 
> runtime: convert Stack to Go.
> 
> LGTM=khr
> R=khr, josharian
> CC=golang-codereviews
> https://codereview.appspot.com/129510043
> 
>
https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_amd64...
> File src/pkg/runtime/asm_amd64p32.s (right):
> 
>
https://codereview.appspot.com/129510043/diff/80001/src/pkg/runtime/asm_amd64...
> src/pkg/runtime/asm_amd64p32.s:751: TEXT
runtime·gogetcallersp(SB),NOSPLIT,$0-12
> On 2014/08/25 01:03:36, khr wrote:
> > I think this should be $0-8?
> 
> The boundary between args and return values is 64-bit aligned. It should be
like
> gogetcallerpc

[bradfitz, 2014-08-27 16:16:32]

https://codereview.appspot.com/129510043/diff/120001/src/pkg/runtime/mprof.goc
File src/pkg/runtime/mprof.goc (left):

https://codereview.appspot.com/129510043/diff/120001/src/pkg/runtime/mprof.go...
src/pkg/runtime/mprof.goc:276: runtime·goroutineheader(g);
you forgot to include this header when you moved it to Go. Sent
https://codereview.appspot.com/136870043

[gobot, 2014-08-28 00:53:04]

This CL appears to have broken the linux-386-temp builder.
See http://build.golang.org/log/8602514c5ec9db3d0e79fe676ef15795cd335391