FAQ
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:
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_plan9_386.c
File src/pkg/runtime/signal_plan9_386.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan9_386.c#newcode33
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_plan9_386.c#newcode39
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_plan9_386.c#newcode43
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_plan9_386.c#newcode57
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_plan9_386.c#newcode67
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_plan9_amd64.c
File src/pkg/runtime/signal_plan9_amd64.c (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signal_plan9_amd64.c#newcode22
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_plan9.h
File src/pkg/runtime/signals_plan9.h (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/signals_plan9.h#newcode9
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#newcode58
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#newcode64
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_386.s
File src/pkg/runtime/sys_plan9_386.s (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_386.s#newcode135
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_amd64.s
File src/pkg/runtime/sys_plan9_amd64.s (right):

https://codereview.appspot.com/6569068/diff/18001/src/pkg/runtime/sys_plan9_amd64.s#newcode146
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.

https://codereview.appspot.com/6569068/

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 1 of 1 | next ›
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 8, '13 at 6:03a
activeJan 8, '13 at 6:03a
posts1
users1
websitegolang.org

1 user in discussion

Seed: 1 post

People

Translate

site design / logo © 2022 Grokbase