code review 11533043: runtime: handle morestack/lessstack in stack trace

runtime: handle morestack/lessstack in stack trace

If we start a garbage collection on g0 during a
stack split or unsplit, we'll see morestack or lessstack
at the top of the stack. Record an argument frame size
for those, and record that they terminate the stack.

[rsc, 2013-07-18 17:18:12]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-07-18 20:15:13]

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:181: runtime·Gosched();
why is this?

[rsc, 2013-07-18 20:17:00]

That was leftover debugging. I had the scavenger in a tight loop sleeping and
stopping the world, and I wanted to give it a chance to run. Gone.

[dvyukov, 2013-07-18 20:17:42]

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2512: f->entry == (uintptr)runtime·morestack ||
if a goroutine calls e.g. runtime.Gosched() which calls runtime.mcall(gosched0),
and then on g0 it triggers GC and unwinds stack; what will be the top frame?

[dvyukov, 2013-07-18 20:18:46]

On 2013/07/18 20:17:42, dvyukov wrote:
> https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c#newc...
> src/pkg/runtime/proc.c:2512: f->entry == (uintptr)runtime·morestack ||
> if a goroutine calls e.g. runtime.Gosched() which calls
runtime.mcall(gosched0),
> and then on g0 it triggers GC and unwinds stack; what will be the top frame?

I mean that we have lots of "entry points" to g0, why only lessstack/morestack
are here?

[rsc, 2013-07-18 20:24:29]

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2512: f->entry == (uintptr)runtime·morestack ||
On 2013/07/18 20:17:42, dvyukov wrote:
> if a goroutine calls e.g. runtime.Gosched() which calls
runtime.mcall(gosched0),
> and then on g0 it triggers GC and unwinds stack; what will be the top frame?

The top frame will be mcall. mcall calls its argument.
Any assembly function that switches to the top of the m stack needs to be listed
here.
mcall is here.

[dvyukov, 2013-07-18 20:32:16]

in traceback we have 2 options:
1. "should never happen", then we need to print/throw always
2. can not happen only during GC, but can happen during SIGPROF and probably
during SIGSEGV; then we need to print if(printing||callback) and throw
if(callback)

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_a...
File src/pkg/runtime/traceback_arm.c (right):

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_a...
src/pkg/runtime/traceback_arm.c:71: runtime·printf("runtime: unknown pc %p after
stack split\n", frame.pc);
same as in traceback_x86

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
File src/pkg/runtime/traceback_x86.c (right):

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
src/pkg/runtime/traceback_x86.c:55: runtime·printf("runtime: unknown pc %p\n",
frame.pc);
perhaps the can be done if(callback != nil || printing) as below
can be useful especially during this churn

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
src/pkg/runtime/traceback_x86.c:85: runtime·printf("runtime: unknown pc %p after
stack split\n", frame.pc);
are you sure this can not be triggered during CPU profiling?
if this is "should never happen", then we need to always throw
otherwise, print if(callback||printing), throw if(callback)

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
src/pkg/runtime/traceback_x86.c:108: runtime·printf("runtime: unexpected return
pc for %s called from %p\n", runtime·funcname(f), frame.lr);
ditto

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
src/pkg/runtime/traceback_x86.c:151: runtime·printf("runtime: inconsistent
locals=%p frame=%p fp=%p sp=%p for %s\n", (uintptr)f->locals, (uintptr)f->frame,
frame.fp, frame.sp, runtime·funcname(f));
ditto

[dvyukov, 2013-07-18 20:34:26]

On 2013/07/18 20:24:29, rsc wrote:
> https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/proc.c#newc...
> src/pkg/runtime/proc.c:2512: f->entry == (uintptr)runtime·morestack ||
> On 2013/07/18 20:17:42, dvyukov wrote:
> > if a goroutine calls e.g. runtime.Gosched() which calls
> runtime.mcall(gosched0),
> > and then on g0 it triggers GC and unwinds stack; what will be the top frame?
> 
> The top frame will be mcall. mcall calls its argument.
> Any assembly function that switches to the top of the m stack needs to be
listed
> here.
> mcall is here.

OK

[rsc, 2013-07-18 20:40:26]

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
File src/pkg/runtime/traceback_x86.c (right):

https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
src/pkg/runtime/traceback_x86.c:85: runtime·printf("runtime: unknown pc %p after
stack split\n", frame.pc);
On 2013/07/18 20:32:16, dvyukov wrote:
> are you sure this can not be triggered during CPU profiling?
> if this is "should never happen", then we need to always throw
> otherwise, print if(callback||printing), throw if(callback)

This specific thing cannot happen.

"always throw" is unconditionally wrong. The problem is that throw will trigger
a new stack trace, and you don't want the second one to re-throw, or else it
will cause 'panic during panic' and you won't get to see the rest of the
context.

If we're garbage collecting, that's worth a throw.
If we're printing or doing a SIGPROF trace, then I don't mind the extra print
but we should probably not throw.

[dvyukov, 2013-07-18 20:45:51]

On 2013/07/18 20:40:26, rsc wrote:
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
> File src/pkg/runtime/traceback_x86.c (right):
> 
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
> src/pkg/runtime/traceback_x86.c:85: runtime·printf("runtime: unknown pc %p
after
> stack split\n", frame.pc);
> On 2013/07/18 20:32:16, dvyukov wrote:
> > are you sure this can not be triggered during CPU profiling?
> > if this is "should never happen", then we need to always throw
> > otherwise, print if(callback||printing), throw if(callback)
> 
> This specific thing cannot happen.
> 
> "always throw" is unconditionally wrong. The problem is that throw will
trigger
> a new stack trace, and you don't want the second one to re-throw, or else it
> will cause 'panic during panic' and you won't get to see the rest of the
> context.
> 
> If we're garbage collecting, that's worth a throw.
> If we're printing or doing a SIGPROF trace, then I don't mind the extra print
> but we should probably not throw.

I remember there were some cases where traceback may not work during SIGPROF, we
should not print in these cases.

[dvyukov, 2013-07-18 20:45:59]

LGTM

[rsc, 2013-07-18 20:46:20]

On 2013/07/18 20:45:51, dvyukov wrote:
> On 2013/07/18 20:40:26, rsc wrote:
> >
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
> > File src/pkg/runtime/traceback_x86.c (right):
> > 
> >
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
> > src/pkg/runtime/traceback_x86.c:85: runtime·printf("runtime: unknown pc %p
> after
> > stack split\n", frame.pc);
> > On 2013/07/18 20:32:16, dvyukov wrote:
> > > are you sure this can not be triggered during CPU profiling?
> > > if this is "should never happen", then we need to always throw
> > > otherwise, print if(callback||printing), throw if(callback)
> > 
> > This specific thing cannot happen.
> > 
> > "always throw" is unconditionally wrong. The problem is that throw will
> trigger
> > a new stack trace, and you don't want the second one to re-throw, or else it
> > will cause 'panic during panic' and you won't get to see the rest of the
> > context.
> > 
> > If we're garbage collecting, that's worth a throw.
> > If we're printing or doing a SIGPROF trace, then I don't mind the extra
print
> > but we should probably not throw.
> 
> I remember there were some cases where traceback may not work during SIGPROF,
we
> should not print in these cases.

What are those cases? Do you remember the details? If not, I'd like to keep the
print until we do know the details, and then record them in comments in the
file.

[rsc, 2013-07-18 20:53:49]

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

runtime: handle morestack/lessstack in stack trace

If we start a garbage collection on g0 during a
stack split or unsplit, we'll see morestack or lessstack
at the top of the stack. Record an argument frame size
for those, and record that they terminate the stack.

R=golang-dev, dvyukov
CC=golang-dev
https://codereview.appspot.com/11533043

[dvyukov, 2013-07-18 20:56:43]

On 2013/07/18 20:46:20, rsc wrote:
> On 2013/07/18 20:45:51, dvyukov wrote:
> > On 2013/07/18 20:40:26, rsc wrote:
> > >
> >
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
> > > File src/pkg/runtime/traceback_x86.c (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/11533043/diff/6001/src/pkg/runtime/traceback_x...
> > > src/pkg/runtime/traceback_x86.c:85: runtime·printf("runtime: unknown pc %p
> > after
> > > stack split\n", frame.pc);
> > > On 2013/07/18 20:32:16, dvyukov wrote:
> > > > are you sure this can not be triggered during CPU profiling?
> > > > if this is "should never happen", then we need to always throw
> > > > otherwise, print if(callback||printing), throw if(callback)
> > > 
> > > This specific thing cannot happen.
> > > 
> > > "always throw" is unconditionally wrong. The problem is that throw will
> > trigger
> > > a new stack trace, and you don't want the second one to re-throw, or else
it
> > > will cause 'panic during panic' and you won't get to see the rest of the
> > > context.
> > > 
> > > If we're garbage collecting, that's worth a throw.
> > > If we're printing or doing a SIGPROF trace, then I don't mind the extra
> print
> > > but we should probably not throw.
> > 
> > I remember there were some cases where traceback may not work during
SIGPROF,
> we
> > should not print in these cases.
> 
> What are those cases? Do you remember the details? If not, I'd like to keep
the
> print until we do know the details, and then record them in comments in the
> file.


That case is:

>>>I have not done the system call stubs in sys_*.s.
>>>I hope to avoid that, because those do not block, so those
>>>frames will not appear in stack traces during garbage
>>>collection.

>>But traceback currently crashes if can't derive argsize during cpu
>>profiling as well.

>I intend to disable arg size lookup during cpu profiling. The information is
unused there.

[rsc, 2013-07-18 20:57:06]

The arg size lookup is disabled during cpu profiling now.

[shamsahmadms, 2023-03-09 10:45:37]

Ok