code review 6569068: pkg/runtime: Plan 9: add support for recover() and pani...

runtime: Plan 9: add support for recover() and panic() in note handler

This change also resolves some issues with note handling: we now make sure that there is enough room at the bottom of every goroutine to execute the note handler, and the `exitstatus' is no longer a global entity, which resolves some race conditions.

[akumar, 2012-09-28 23:26:41]

Hello rminnich@gmail.com, npe@plan9.bell-labs.com, john@jfloren.net,
david.jakob.fritz@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

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

[rsc, 2012-10-06 22:22:37]

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c...
src/cmd/dist/buildruntime.c:111: "#define	tos_pid 48\n"
Should this be #define tos_pid(r) 48(r) to be like the others?

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_386.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:33: int8 *err[NERRMAX];
This is an array of pointers not an array of char.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:39: if(s && *s == 0)
I don't understand this. What is going on here?

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:43: if(len <= 4 || runtime·mcmp((byte*)s,
(byte*)"sys:", 4) != 0)
There are too many exits calls in this function. There shouldn't be any. In the
worst case there should be a noted(NDFLT) to let the kernel kill the proc (or
put it in broken state for debugging).

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:57: runtime·exits(s);
Here's another.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:67: runtime·memmove((void*)err, (void*)s,
len);
Should check len against ERRMAX. Also, when this function returns the stack
space is gone, so this seems dodgy. Probably you need to save into a per-m field
instead. Or throw it away and use sig->name instead.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_amd64.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_amd64.c:22: runtime·printf("r8      %X\n", u->r8);
Shouldn't all these %X be lined up?
Or even better do ax\t%X bx\t%X etc

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signals_pla...
File src/pkg/runtime/signals_plan9.h (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signals_pla...
src/pkg/runtime/signals_plan9.h:9: T, "sys: trap:",
I think you need some comments here explaining why this is a throw but the above
were panic.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/stack.h
File src/pkg/runtime/stack.h (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/stack.h#new...
src/pkg/runtime/stack.h:58: // purposes like signal handling. Used on Windows
because
Used on Window and on Plan 9
because they do not use a separate stack.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/stack.h#new...
src/pkg/runtime/stack.h:64: StackSystem = 512,
Where does 512 come from? You need space for the Ureg and the note and the call
frame.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_3...
File src/pkg/runtime/sys_plan9_386.s (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_3...
src/pkg/runtime/sys_plan9_386.s:135: SUBL	$90, SP
90 is a strange choice, especially since it is not a multiple of 4.
It looks like 24 would work.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_a...
File src/pkg/runtime/sys_plan9_amd64.s (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_a...
src/pkg/runtime/sys_plan9_amd64.s:146: SUBQ	$90, SP
Same comment. Looks like you need $40.

[akumar, 2013-01-08 06:03:09]

PTAL.

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c...
src/cmd/dist/buildruntime.c:111: "#define	tos_pid 48\n"
On 2012/10/06 22:22:38, rsc wrote:
> Should this be #define tos_pid(r) 48(r) to be like the others?

The semantics I had in mind were analogous to constants like gobuf_pc or
g_stackguard: tos_pid is just an offset of "pid"  from top of struct "tos".
Still, I can change it if you like.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_386.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:33: int8 *err[NERRMAX];
On 2012/10/06 22:22:38, rsc wrote:
> This is an array of pointers not an array of char.

Removed entirely.

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:39: if(s && *s == 0)
On 2012/10/06 22:22:38, rsc wrote:
> I don't understand this. What is going on here?

If the note == "", then exit. Booger from debugging. Removed.

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:43: if(len <= 4 || runtime·mcmp((byte*)s,
(byte*)"sys:", 4) != 0)
On 2012/10/06 22:22:38, rsc wrote:
> There are too many exits calls in this function. There shouldn't be any. In
the
> worst case there should be a noted(NDFLT) to let the kernel kill the proc (or
> put it in broken state for debugging).

Replaced this with a `return NDFLT'. This case is supposed to account for
messages that didn't come from the kernel (i.e., exit strings reported from
other go threads).

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:57: runtime·exits(s);
On 2012/10/06 22:22:38, rsc wrote:
> Here's another.

In this case, we have a message from the kernel that is not declared in the
SigTab.

My understanding is that we don't ever want go threads to be in the broken state
in isolation, while others in the group keep running; is that correct? In which
case I suppose we ought to first kill all the other threads and only afterwards
call noted(NDFLT). I would think a `goexitsall(); return NDFLT;' would suffice
here, but perhaps we want to be more careful with regards to the state of other
threads (i.e., they may also be handling other notes). What would you suggest?

For now, I've changed this to a `return NDFLT' as per your initial suggestion.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:67: runtime·memmove((void*)err, (void*)s,
len);
On 2012/10/06 22:22:38, rsc wrote:
> Should check len against ERRMAX.

Done.

> Also, when this function returns the stack
> space is gone, so this seems dodgy. Probably you need to save into a per-m
field
> instead. Or throw it away and use sig->name instead.

It would be nice to have the full information during a panic. I've allocated the
space in m->gsignal->sigcode0 on thread start-up.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_amd64.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_amd64.c:22: runtime·printf("r8      %X\n", u->r8);
On 2012/10/06 22:22:38, rsc wrote:
> Shouldn't all these %X be lined up?
> Or even better do ax\t%X bx\t%X etc

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signals_pla...
File src/pkg/runtime/signals_plan9.h (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signals_pla...
src/pkg/runtime/signals_plan9.h:9: T, "sys: trap:",
On 2012/10/06 22:22:38, rsc wrote:
> I think you need some comments here explaining why this is a throw but the
above
> were panic.

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/stack.h
File src/pkg/runtime/stack.h (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/stack.h#new...
src/pkg/runtime/stack.h:58: // purposes like signal handling. Used on Windows
because
On 2012/10/06 22:22:38, rsc wrote:
> Used on Window and on Plan 9
> because they do not use a separate stack.

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/stack.h#new...
src/pkg/runtime/stack.h:64: StackSystem = 512,
On 2012/10/06 22:22:38, rsc wrote:
> Where does 512 come from? You need space for the Ureg and the note and the
call
> frame.

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_3...
File src/pkg/runtime/sys_plan9_386.s (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_3...
src/pkg/runtime/sys_plan9_386.s:135: SUBL	$90, SP
On 2012/10/06 22:22:38, rsc wrote:
> 90 is a strange choice, especially since it is not a multiple of 4.
> It looks like 24 would work.

Yes; during testing, I chose an arbitrary large number.

Done.

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_a...
File src/pkg/runtime/sys_plan9_amd64.s (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_a...
src/pkg/runtime/sys_plan9_amd64.s:146: SUBQ	$90, SP
On 2012/10/06 22:22:38, rsc wrote:
> Same comment. Looks like you need $40.

Done.

[ality, 2013-01-10 03:19:21]

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c...
src/cmd/dist/buildruntime.c:111: "#define	tos_pid 48\n"
On 2013/01/08 06:03:09, akumar wrote:
> On 2012/10/06 22:22:38, rsc wrote:
> > Should this be #define tos_pid(r) 48(r) to be like the others?
> 
> The semantics I had in mind were analogous to constants like gobuf_pc or
> g_stackguard: tos_pid is just an offset of "pid"  from top of struct "tos".
> Still, I can change it if you like.

I would just do #define procid(r) 48(r) to match the amd64 definition.
It's clear that we get it from the tos in this case.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/os_plan9.h
File src/pkg/runtime/os_plan9.h (right):

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/os_plan9.h#...
src/pkg/runtime/os_plan9.h:86: #define	NERRMAX	128	/* max length of note string
*/
Why not just ERRMAX?

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_386.c (right):

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:29: runtime·sighandler(void *v, int8 *s, G
*gp)
I think this is mostly fine for now but we're going to have
to change the logic in this function once we add support
for os/signal.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/stack.h
File src/pkg/runtime/stack.h (right):

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/stack.h#new...
src/pkg/runtime/stack.h:64: // Maximum size of NFrame (386 vs amd64) + call
frame.
NFrame is not present (at least by name) in the 386 kernels and
it's size actually contains the call frame. Also, the 32-bit
kernels leave 256 bytes between the old stack pointer and the
note handler frame as an aid for debugging. Shouldn't we take
that into account?

So on amd64 we need at least 368 bytes whereas on 386 we need
at least 476. I don't know about the arm systems.

Maybe the comment should read something like:

// We need enough space for the note handler frame setup
// by the kernel. This varies among the architectures we
// support. 512 bytes ought to be enough for anybody.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan9.c
File src/pkg/runtime/thread_plan9.c (right):

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:18: m->gsignal = runtime·malg(32*1024);
Why 32Kb?

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:223: int8 *exitstatus;
Too much typing. Just make this "status".

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:295: runtime·panicstring("call of nil func
value");
One too many tabs.

[akumar, 2013-01-10 03:33:04]

Thanks for the review!

On 10 January 2013 03:19,  <ality@pbrane.org> wrote:
https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/signal_plan...
> src/pkg/runtime/signal_plan9_386.c:29: runtime·sighandler(void *v, int8
> *s, G *gp)
> I think this is mostly fine for now but we're going to have
> to change the logic in this function once we add support
> for os/signal.

Yes, but one thing at a time. I have some code for the rest,
but it builds upon this stuff and this is already quite a bit to
review and get right, for now. A subsequent CL will add to
this, in order to properly handle asynchronous signals.

I'll fix up the rest soon.


Best,
Akshat

[akumar, 2013-01-13 01:40:51]

PTAL.

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/cmd/dist/buildruntime.c...
src/cmd/dist/buildruntime.c:111: "#define	tos_pid 48\n"
On 2013/01/10 03:19:22, ality wrote:
> On 2013/01/08 06:03:09, akumar wrote:
> > On 2012/10/06 22:22:38, rsc wrote:
> > > Should this be #define tos_pid(r) 48(r) to be like the others?
> > 
> > The semantics I had in mind were analogous to constants like gobuf_pc or
> > g_stackguard: tos_pid is just an offset of "pid"  from top of struct "tos".
> > Still, I can change it if you like.
> 
> I would just do #define procid(r) 48(r) to match the amd64 definition.
> It's clear that we get it from the tos in this case.
> 

Done.

Also updated runtime·rfork code in sys_plan9_386.s to match.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/os_plan9.h
File src/pkg/runtime/os_plan9.h (right):

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/os_plan9.h#...
src/pkg/runtime/os_plan9.h:86: #define	NERRMAX	128	/* max length of note string
*/
On 2013/01/10 03:19:22, ality wrote:
> Why not just ERRMAX?

Done.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/stack.h
File src/pkg/runtime/stack.h (right):

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/stack.h#new...
src/pkg/runtime/stack.h:64: // Maximum size of NFrame (386 vs amd64) + call
frame.
On 2013/01/10 03:19:22, ality wrote:
> NFrame is not present (at least by name) in the 386 kernels and
> it's size actually contains the call frame. Also, the 32-bit
> kernels leave 256 bytes between the old stack pointer and the
> note handler frame as an aid for debugging. Shouldn't we take
> that into account?
> 
> So on amd64 we need at least 368 bytes whereas on 386 we need
> at least 476. I don't know about the arm systems.
> 
> Maybe the comment should read something like:
> 
> // We need enough space for the note handler frame setup
> // by the kernel. This varies among the architectures we
> // support. 512 bytes ought to be enough for anybody.

Done.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan9.c
File src/pkg/runtime/thread_plan9.c (right):

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:18: m->gsignal = runtime·malg(32*1024);
On 2013/01/10 03:19:22, ality wrote:
> Why 32Kb?

Should be enough for panic routines.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:223: int8 *exitstatus;
On 2013/01/10 03:19:22, ality wrote:
> Too much typing. Just make this "status".

Done.

https://codereview.appspot.com/6569068/diff/43003/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:295: runtime·panicstring("call of nil func
value");
On 2013/01/10 03:19:22, ality wrote:
> One too many tabs.

Done.

[rsc, 2013-01-18 20:32:42]

LGTM

Leaving for ality to submit once he's happy.

[ality, 2013-01-20 08:44:46]

Very close.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_386.c (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:69: 
You need to push the PC onto the stack here
for the traceback code. See any of the other
signal_$GOOS_$GOARCH.c files for an example.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_amd64.c (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_amd64.c:77: 
Likewise.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_3...
File src/pkg/runtime/sys_plan9_386.s (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_3...
src/pkg/runtime/sys_plan9_386.s:144: SUBL	$24, SP
Any reason this can't be 16?

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_a...
File src/pkg/runtime/sys_plan9_amd64.s (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_a...
src/pkg/runtime/sys_plan9_amd64.s:170: SUBQ	$40, SP
Likewise, can this just be 32?

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan9.c
File src/pkg/runtime/thread_plan9.c (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:298: if(g->sig == 0 || g->sig == 1)
This no longer matches the indices in runtime·sigtab.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:341: runtime·badsignal(int32 sig)
You can make this runtime·badsignal(void)
and simplifiy the call in runtime·sigtramp.

[akumar, 2013-01-22 01:11:37]

PTAL.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_386.c (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_386.c:69: 
On 2013/01/20 08:44:46, ality wrote:
> You need to push the PC onto the stack here
> for the traceback code. See any of the other
> signal_$GOOS_$GOARCH.c files for an example.

Done.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
File src/pkg/runtime/signal_plan9_amd64.c (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
src/pkg/runtime/signal_plan9_amd64.c:77: 
On 2013/01/20 08:44:46, ality wrote:
> Likewise.

Done.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_3...
File src/pkg/runtime/sys_plan9_386.s (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_3...
src/pkg/runtime/sys_plan9_386.s:144: SUBL	$24, SP
On 2013/01/20 08:44:46, ality wrote:
> Any reason this can't be 16?

Done.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_a...
File src/pkg/runtime/sys_plan9_amd64.s (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_a...
src/pkg/runtime/sys_plan9_amd64.s:170: SUBQ	$40, SP
On 2013/01/20 08:44:46, ality wrote:
> Likewise, can this just be 32?

Done.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan9.c
File src/pkg/runtime/thread_plan9.c (right):

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:298: if(g->sig == 0 || g->sig == 1)
On 2013/01/20 08:44:46, ality wrote:
> This no longer matches the indices in runtime·sigtab.

Done.

https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan...
src/pkg/runtime/thread_plan9.c:341: runtime·badsignal(int32 sig)
On 2013/01/20 08:44:46, ality wrote:
> You can make this runtime·badsignal(void)
> and simplifiy the call in runtime·sigtramp.

Done.

[akumar, 2013-01-23 19:23:06]

ping

On 22 January 2013 01:11,  <seed@mail.nanosouffle.net> wrote:
> PTAL.
>
>
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
> File src/pkg/runtime/signal_plan9_386.c (right):
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
> src/pkg/runtime/signal_plan9_386.c:69:
> On 2013/01/20 08:44:46, ality wrote:
>>
>> You need to push the PC onto the stack here
>> for the traceback code. See any of the other
>> signal_$GOOS_$GOARCH.c files for an example.
>
>
> Done.
>
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
> File src/pkg/runtime/signal_plan9_amd64.c (right):
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan...
> src/pkg/runtime/signal_plan9_amd64.c:77:
> On 2013/01/20 08:44:46, ality wrote:
>>
>> Likewise.
>
>
> Done.
>
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_3...
> File src/pkg/runtime/sys_plan9_386.s (right):
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_3...
> src/pkg/runtime/sys_plan9_386.s:144: SUBL       $24, SP
> On 2013/01/20 08:44:46, ality wrote:
>>
>> Any reason this can't be 16?
>
>
> Done.
>
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_a...
> File src/pkg/runtime/sys_plan9_amd64.s (right):
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/sys_plan9_a...
> src/pkg/runtime/sys_plan9_amd64.s:170: SUBQ     $40, SP
> On 2013/01/20 08:44:46, ality wrote:
>>
>> Likewise, can this just be 32?
>
>
> Done.
>
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan9.c
> File src/pkg/runtime/thread_plan9.c (right):
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan...
> src/pkg/runtime/thread_plan9.c:298: if(g->sig == 0 || g->sig == 1)
> On 2013/01/20 08:44:46, ality wrote:
>>
>> This no longer matches the indices in runtime·sigtab.
>
>
> Done.
>
>
>
https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/thread_plan...
> src/pkg/runtime/thread_plan9.c:341: runtime·badsignal(int32 sig)
> On 2013/01/20 08:44:46, ality wrote:
>>
>> You can make this runtime·badsignal(void)
>> and simplifiy the call in runtime·sigtramp.
>
>
> Done.
>
> https://codereview.appspot.com/6569068/

[akumar, 2013-01-24 13:16:21]

After some testing, I realized that storing the note-string at
m->gsignal->sigcode0 causes stack corruption after sufficiently many
panic-recover sequences that come from synchronous traps. To address this, I've
updated the code by adding a Plan 9-specific field to M where I now store the
note-string. Now, arbitrarily many panic-recover sequences can run without
causing GP faults.

PTAL.

[ality, 2013-01-30 10:51:48]

LGTM

Sorry for the long delay. Thanks for working on this.

It even works well on 9vx (after fixing a few bugs there).

[ality, 2013-01-30 10:54:09]

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

runtime: add support for panic/recover in Plan 9 note handler

This change also resolves some issues with note handling: we now make
sure that there is enough room at the bottom of every goroutine to
execute the note handler, and the `exitstatus' is no longer a global
entity, which resolves some race conditions.

R=rminnich, npe, rsc, ality
CC=golang-dev
https://codereview.appspot.com/6569068

Committer: Anthony Martin <ality@pbrane.org>