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

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 *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#newcode64
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_plan9.c#newcode18
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_plan9.c#newcode223
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_plan9.c#newcode295
src/pkg/runtime/thread_plan9.c:295: runtime·panicstring("call of nil
func value");
One too many tabs.

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

Search Discussions

  • Akshat Kumar at Jan 10, 2013 at 3:33 am
    Thanks for the review!

    On 10 January 2013 03:19, 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 *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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 10, '13 at 3:19a
activeJan 10, '13 at 3:33a
posts2
users2
websitegolang.org

2 users in discussion

Ality: 1 post Akshat Kumar: 1 post

People

Translate

site design / logo © 2022 Grokbase