Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(235)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by akumar
Modified:
9 years, 3 months ago
Reviewers:
CC:
rminnich, npe1, rsc, ality, golang-dev
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r 8e69b6243f49 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 8e69b6243f49 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 8e69b6243f49 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r aa2f44cc25c0 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r aa2f44cc25c0 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r aa2f44cc25c0 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r aa2f44cc25c0 https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r ff25343e334e https://go.googlecode.com/hg/ #

Total comments: 26

Patch Set 10 : diff -r 43203aa69a8a https://code.google.com/p/go #

Total comments: 11

Patch Set 11 : diff -r c53ac9baac67 https://code.google.com/p/go #

Total comments: 12

Patch Set 12 : diff -r 6287072632c4 https://code.google.com/p/go #

Patch Set 13 : diff -r 5b5399bc3335 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+439 lines, -57 lines) Patch
M src/cmd/dist/buildruntime.c View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
M src/pkg/runtime/defs_plan9_386.h View 1 2 1 chunk +28 lines, -2 lines 0 comments Download
M src/pkg/runtime/defs_plan9_amd64.h View 1 2 3 4 5 6 7 8 9 1 chunk +33 lines, -1 line 0 comments Download
M src/pkg/runtime/os_plan9.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -3 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -0 lines 0 comments Download
M src/pkg/runtime/signal_plan9_386.c View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +101 lines, -0 lines 0 comments Download
M src/pkg/runtime/signal_plan9_amd64.c View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +108 lines, -0 lines 0 comments Download
M src/pkg/runtime/signals_plan9.h View 1 2 3 4 5 6 7 8 9 1 chunk +24 lines, -1 line 0 comments Download
M src/pkg/runtime/stack.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -3 lines 0 comments Download
M src/pkg/runtime/sys_plan9_386.s View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +50 lines, -4 lines 0 comments Download
M src/pkg/runtime/sys_plan9_amd64.s View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +48 lines, -2 lines 0 comments Download
M src/pkg/runtime/thread_plan9.c View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +22 lines, -41 lines 0 comments Download

Messages

Total messages: 13
akumar
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 ...
9 years, 7 months ago (2012-09-28 23:26:41 UTC) #1
rsc
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#newcode111 src/cmd/dist/buildruntime.c:111: "#define tos_pid 48\n" Should this be #define tos_pid(r) 48(r) ...
9 years, 7 months ago (2012-10-06 22:22:37 UTC) #2
akumar
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#newcode111 src/cmd/dist/buildruntime.c:111: "#define tos_pid 48\n" On 2012/10/06 22:22:38, rsc wrote: ...
9 years, 4 months ago (2013-01-08 06:03:09 UTC) #3
ality
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#newcode111 src/cmd/dist/buildruntime.c:111: "#define tos_pid 48\n" On 2013/01/08 06:03:09, akumar wrote: > ...
9 years, 4 months ago (2013-01-10 03:19:21 UTC) #4
akumar
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_plan9_386.c#newcode29 > src/pkg/runtime/signal_plan9_386.c:29: runtime·sighandler(void ...
9 years, 4 months ago (2013-01-10 03:33:04 UTC) #5
akumar
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#newcode111 src/cmd/dist/buildruntime.c:111: "#define tos_pid 48\n" On 2013/01/10 03:19:22, ality wrote: ...
9 years, 4 months ago (2013-01-13 01:40:51 UTC) #6
rsc
LGTM Leaving for ality to submit once he's happy.
9 years, 4 months ago (2013-01-18 20:32:42 UTC) #7
ality
Very close. https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan9_386.c File src/pkg/runtime/signal_plan9_386.c (right): https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan9_386.c#newcode69 src/pkg/runtime/signal_plan9_386.c:69: You need to push the PC onto ...
9 years, 4 months ago (2013-01-20 08:44:46 UTC) #8
akumar
PTAL. https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan9_386.c File src/pkg/runtime/signal_plan9_386.c (right): https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan9_386.c#newcode69 src/pkg/runtime/signal_plan9_386.c:69: On 2013/01/20 08:44:46, ality wrote: > You need ...
9 years, 4 months ago (2013-01-22 01:11:37 UTC) #9
akumar
ping On 22 January 2013 01:11, <seed@mail.nanosouffle.net> wrote: > PTAL. > > > > https://codereview.appspot.com/6569068/diff/52001/src/pkg/runtime/signal_plan9_386.c ...
9 years, 3 months ago (2013-01-23 19:23:06 UTC) #10
akumar
After some testing, I realized that storing the note-string at m->gsignal->sigcode0 causes stack corruption after ...
9 years, 3 months ago (2013-01-24 13:16:21 UTC) #11
ality
LGTM Sorry for the long delay. Thanks for working on this. It even works well ...
9 years, 3 months ago (2013-01-30 10:51:48 UTC) #12
ality
9 years, 3 months ago (2013-01-30 10:54:09 UTC) #13
*** 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>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b