Code review - Issue 2273041: code review 2273041: Initial Plan9 runtime support for 386.https://codereview.appspot.com/2010-10-18T16:32:59+00:00rietveld
Message from unknown
2010-09-23T17:53:55+00:00paulzholurn:md5:9d72ccb93f2457bb18135a7fa125bb65
Message from paulzhol@gmail.com
2010-09-23T17:54:11+00:00paulzholurn:md5:a0d68c42b02207c694f7eeff6767fbca
Hello golang-dev@googlegroups.com,
I'd like you to review this change.
Message from paulzhol@gmail.com
2010-09-23T23:11:08+00:00paulzholurn:md5:720d07ab56d2f395fd69ef069f0848d2
Hi,
I've tried to get a basic google go runtime for plan9 the (b) part working and would really love to get your feedback.
Thanks
Message from rsc@google.com
2010-09-24T22:58:47+00:00rsc1urn:md5:3f5baaf4c0877d9bd92a12761595df46
Very cool!
You changed the TLS story while I was halfway
through the review. I don't think either is quite right.
The top of the stack is a good place. I'm not sure
whether you can use the very top or if that part will
contain program arguments. For now it's a fine choice
and we can always move the program arguments down
or something like that. So I would hard code
0(gs) -> 0xdfffeff8
4(gs) -> 0xdfffeffc
on both the to and from sides in the linker.
Then the get_tls(r) is a no-op and g(r) is 0xdfffeff8
and m(r) is 0xdfffeffc. Or maybe I have them
backwards; good to double-check but you get
the idea.
The part about all goroutines being asleep could
be due to not implementing threads. Getting to 439
is a good sign.
http://codereview.appspot.com/2273041/diff/2001/src/cmd/8l/pass.c
File src/cmd/8l/pass.c (right):
http://codereview.appspot.com/2273041/diff/2001/src/cmd/8l/pass.c#newcode687
src/cmd/8l/pass.c:687: case 2: // Plan9
blank line above this one to separate the cases
also // Plan 9
(with a space)
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/mkasmh.sh
File src/pkg/runtime/mkasmh.sh (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/mkasmh.sh#newcode28
src/pkg/runtime/mkasmh.sh:28: echo '#define get_tls(r) MOVL 0xdfffeffc, r'
This is putting a pointer to the m, g word pair
at that location but instead I would put m, g themselves there,
avoiding the indirection:
#define get_tls(r)
#define g(r) 0xdfffeffc
#define m(r) 0xdfffeff8
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/defs.h
File src/pkg/runtime/plan9/386/defs.h (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/defs.h#newcode2
src/pkg/runtime/plan9/386/defs.h:2: #define PLAN9_TLS (UTOPSTCK-4)
Is this used somewhere?
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/rt0.s
File src/pkg/runtime/plan9/386/rt0.s (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/rt0.s#newcode5
src/pkg/runtime/plan9/386/rt0.s:5: // Darwin and Linux use the same linkage to main
Can delete this comment. I'm not sure this is correct, either.
The environment, for example, is passed in a different way,
although the arguments are probably the same.
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/sys.s
File src/pkg/runtime/plan9/386/sys.s (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/sys.s#newcode6
src/pkg/runtime/plan9/386/sys.s:6: MOVL address+4(FP), CX
I think you can just make setldt a no-op.
Each thread will get its own copy of the stack.
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/mem.c
File src/pkg/runtime/plan9/mem.c (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/mem.c#newcode32
src/pkg/runtime/plan9/mem.c:32: USED(v, n);
It's probably a good idea to implement this
at least partially. See tiny/mem.c for an idea.
http://codereview.appspot.com/2273041/diff/7001/src/cmd/8l/pass.c
File src/cmd/8l/pass.c (right):
http://codereview.appspot.com/2273041/diff/7001/src/cmd/8l/pass.c#newcode441
src/cmd/8l/pass.c:441: p->from.sym = lookup("tls0", 0);
The top of the stack is per-thread memory.
tls0 is part of the data segment and not per-thread memory.
I would store these directly on the stack.
Make this refer to the constant address 0xdfffeff8+p->from.offset
http://codereview.appspot.com/2273041/diff/7001/src/cmd/8l/pass.c#newcode691
src/cmd/8l/pass.c:691: p->from.sym = lookup("tls0", 0);
Same
Message from unknown
2010-09-25T22:06:55+00:00paulzholurn:md5:99462257a04b5fb44f71269a4a30fae6
Message from paulzhol@gmail.com
2010-09-25T22:31:16+00:00paulzholurn:md5:7589df24b004a3c0036e31bbfb6a36bb
Hi!
I saw some Plan 9 src/9/prot/sysproc.c code for the exec? syscall, and a struct Tos is placed at the top of the stack for profiling information and clock.
What makes me even more cautious in using 0xdfffeff8 and 0xdfffeffc directly is that I see the pid of the process at 0xdfffeff8 with acid.
So instead, I reserved 8 bytes right before the Tos struct by moving the arguments with memmove.
Also I had to comment out the line at runtime/386/asm.s which tries to store 0x123 through g and then checks the value through tls0, which now always aborts.
My hello go works but the sieve is giving me:
throw: mark - world not stopped
panic PC=0x40f90
goroutine 1 [2]:
sieve 247: suicide: sys: breakpoint pc=0x00005e4c
Message from rsc@golang.org
2010-09-28T16:08:29+00:00rscurn:md5:ca5fd5bae09291730347c35ddfe075a8
> My hello go works but the sieve is giving me:
> throw: mark - world not stopped
> panic PC=0x40f90
That suggests there are references to invalid
goroutine states. If you find that string in
package runtime it would be good to print
out what the offending g->status is.
Russ
Message from paulzhol@gmail.com
2010-09-29T22:21:18+00:00paulzholurn:md5:671c417eea38baba549eab7d40981d2f
> That suggests there are references to invalid
> goroutine states. If you find that string in
> package runtime it would be good to print
> out what the offending g->status is.
>
> Russ
Hi Russ,
I've updated my patches to disable env copying for now, need to do a dirread on /env instead of copying them from the stack anyhow.
I also turned on some debug prints in newproc1 and added a print in pkg/runtime/mgc0.c:/mark(void) at the location throwing the "mark - world not stopped" exception.
Now my sieve run looks like this:
term% sieve
newproc1 0x5eb4(<-mainstart) 0xdfffef20 narg=0 nret=0
goid=1
gp: 0x3f000 gp->status:2(<-Grunning) gp->goid: 1 entry:0x5eb4(<-mainstart) pc:0x1ebc(<-goexit)
g:0x40100 g->status:0(<-Gidle) g->goid:0 entry:0x5eb4(<-mainstart) pc:0x0
throw: mark - world not stopped
panic PC=0x40f70(<-this seems similar the address of g)
goroutine 1 [2]:
sieve 401: suicide: sys: breakpoint pc=0x00005ecf(<-looks like the middle of INT $3 opcode in breakpoint()??)
Do you think it could be caused by storing the g and m pointers directly on the stack, or me not implementing threading/rfork and locks (although runtime/tiny also assumes it is running in a single process)?
Thanks,
Pavel
Message from rsc@golang.org
2010-09-30T01:55:47+00:00rscurn:md5:43416d49bde5776d5366fccab7402a28
> term% sieve
> newproc1 0x5eb4(<-mainstart) 0xdfffef20 narg=0 nret=0
> goid=1
> gp: 0x3f000 gp->status:2(<-Grunning) gp->goid: 1
> entry:0x5eb4(<-mainstart) pc:0x1ebc(<-goexit)
> g:0x40100 g->status:0(<-Gidle) g->goid:0 entry:0x5eb4(<-mainstart)
> pc:0x0
The global variable g should never have g->status == Gidle.
I suspect that the global variable g is actually m: that is,
somewhere, maybe in the linker, maybe in the compiler,
maybe in the assembly in the runtime, the addresses for
g and m got swapped. I'm not 100% sure of that, though,
because m should be == &m0, and &m0 would not be
such a large number. Still, the only valid g with goid == 0
is &g0 (== m0.g0), and 0x40100 is too big to be &g0 too.
Russ
Message from unknown
2010-09-30T14:54:08+00:00paulzholurn:md5:8da9922e22b246f24312f4fc3eef51e1
Message from paulzhol@gmail.com
2010-09-30T15:11:44+00:00paulzholurn:md5:2b3ea790507ae27368ed3661e427652c
>
> The global variable g should never have g->status == Gidle.
> I suspect that the global variable g is actually m: that is,
> somewhere, maybe in the linker, maybe in the compiler,
> maybe in the assembly in the runtime, the addresses for
> g and m got swapped. I'm not 100% sure of that, though,
> because m should be == &m0, and &m0 would not be
> such a large number. Still, the only valid g with goid == 0
> is &g0 (== m0.g0), and 0x40100 is too big to be &g0 too.
>
> Russ
>
You are absolutely right! I had left an extra dereference in the patch and
dostkoff at 8l/pass.c from the first attempt, so instead of g, I was getting
g->stackguard.
I've updated my changes and now sieve runs:
newproc1 0x5d1c 0xdfffef20 narg=0 nret=0
goid=1
newproc1 0x1020 0x410a8 narg=4 nret=0
goid=2
2
newproc1 0x1059 0x410a8 narg=12 nret=0
goid=3
3
newproc1 0x1059 0x410a8 narg=12 nret=0
goid=4
5
newproc1 0x1059 0x410a8 narg=12 nret=0
goid=5
7
newproc1 0x1059 0x410a8 narg=12 nret=0
goid=6
11
.
.
.
newproc1 0x1059 0x410a8 narg=12 nret=0
goid=85
433
newproc1 0x1059 0x410a8 narg=12 nret=0
goid=86
439
newproc1 0x1059 0x410a8 narg=12 nret=0
goid=87
throw: all goroutines are asleep - deadlock!
panic PC=0xdfffeed0
goroutine 87 [4]:
goroutine 86 [4]:
Just as with runtime/tiny. Besides getting environment variables working
what else is needed to be done to get merged in (is os thread support
mandatory)?
Thanks,
Pavel
Message from unknown
2010-09-30T20:48:00+00:00paulzholurn:md5:b09cb7ba234ace288f57d0b7a94aa4ce
Message from paulzhol@gmail.com
2010-10-06T08:12:42+00:00paulzholurn:md5:74a69c190290107a361cd87f7240bd34
Update: Got newosproc working with rfork, implemented locking with plan9 semaphores (Sape Mullender's paper), also notewakeup/sleep with plan9 semaphores - almost the same code in darwin.
I had to add runtime.GOMAXPROCS(2) to sieve's main.main as I still have no environment variables and it works!!
I see 2 processes with ps: one is in Semacqui and the second in _write. I got to 14737 at one time and it kept running :)
I did notice a something strange: without the Paranoia checking in src/pkg/runtime/plan9/386/sys.s (Patchset 5 vs 6) the multithreaded sieve will go up until ~2100/~2700 and throw: all goroutines are asleep - deadlock!
Also I had started working on pkg/syscall to later get os.File read /env but I'm not sure if it should go in this CL or not.
Thanks,
Pavel
Message from unknown
2010-10-07T06:15:05+00:00paulzholurn:md5:b5d711c4ad4666818f42b8e61886bcd9
Message from unknown
2010-10-07T06:17:32+00:00paulzholurn:md5:9a75b4163c1384989b8cfde09b754ed1
Message from paulzhol@gmail.com
2010-10-07T06:59:14+00:00paulzholurn:md5:91e8a48a92e2b6358e2843f4dbd00b12
> I did notice a something strange: without the Paranoia checking in
> src/pkg/runtime/plan9/386/sys.s (Patchset 5 vs 6) the multithreaded sieve will
> go up until ~2100/~2700 and throw: all goroutines are asleep - deadlock!
>
Ok it seems it was to early to rejoice. The paranoia checks have nothing to do with it: sometimes the deadlock will show up when I run inside an rc rio window and sometimes it will just keep running. Also when running inside an Acme win terminal I almost always get the deadlock.
Russ, could you please have another look or point me in the right direction.
Thanks,
Pavel
Message from rsc@golang.org
2010-10-07T07:06:26+00:00rscurn:md5:84e19c136ec489681b9ebb4f4ec524d5
On Thu, Oct 7, 2010 at 02:59, <paulzhol@gmail.com> wrote:
>> I did notice a something strange: without the Paranoia checking in
>> src/pkg/runtime/plan9/386/sys.s (Patchset 5 vs 6) the multithreaded
>
> sieve will
>>
>> go up until ~2100/~2700 and throw: all goroutines are asleep -
>
> deadlock!
>
>
> Ok it seems it was to early to rejoice. The paranoia checks have nothing
> to do with it: sometimes the deadlock will show up when I run inside an
> rc rio window and sometimes it will just keep running. Also when running
> inside an Acme win terminal I almost always get the deadlock.
Let's get what you have reviewed and checked in.
You've gotten quite a bit done.
Russ
Message from rsc@google.com
2010-10-07T08:12:41+00:00rsc1urn:md5:559ce31097c536824f324ff0e6a196ba
Looks pretty good.
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/386/asm.s
File src/pkg/runtime/386/asm.s (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/386/asm.s#newcode408
src/pkg/runtime/386/asm.s:408: TEXT tlscheck(SB), 7, $0
GLOBL isplan9(SB), $4
TEXT tlscheck(SB), 7, $0
CMPL isplan9(SB), $1
JZ ok
at this point might as well put the code back where it was
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s
File src/pkg/runtime/plan9/386/rt0.s (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s#newcode5
src/pkg/runtime/plan9/386/rt0.s:5: TEXT _rt0_386_plan9(SB),7, $0
This should be the same as what's here but shorter.
TEXT _rt0_386_plan9(SB), 7, $8
MOVL AX, _tos(SB)
// move arguments down to SP
MOVL SP, DI
LEAL 0(FP), SI
CLD
REP; MOVSB
// adjust argv
SUBL SI, DI
MOVL 0(SP), CX // argc
MOVL 4(SP), BP // argv
argv:
ADDL DI, 0(BP)
ADDL $4, BP
LOOP argv
// standard startup
JMP _rt0_386(SB)
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s#newcode36
src/pkg/runtime/plan9/386/rt0.s:36: GLOBL _tos(SB), $4
DATA isplan9(SB)+0/4, $1
GLOBL isplan9(SB), $4
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/mem.c
File src/pkg/runtime/plan9/mem.c (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/mem.c#newcode8
src/pkg/runtime/plan9/mem.c:8: extern int8 end[];
,s/int8/byte/g
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/runtime.c#newcode157
src/pkg/runtime/runtime.c:157: if(goos != nil && strcmp((uint8*)goos, (uint8*)"plan9") == 0)
extern int isplan9;
if(isplan9)
envc = 0;
else
Message from unknown
2010-10-07T22:50:05+00:00paulzholurn:md5:37526d1d72333055f01fe8cb1d33a8af
Message from paulzhol@gmail.com
2010-10-07T22:52:23+00:00paulzholurn:md5:91ffc46d43dd2e0698dfb82efc3275d7
http://codereview.appspot.com/2273041/diff/2001/src/cmd/8l/pass.c
File src/cmd/8l/pass.c (right):
http://codereview.appspot.com/2273041/diff/2001/src/cmd/8l/pass.c#newcode687
src/cmd/8l/pass.c:687: case 2: // Plan9
On 2010/09/24 22:58:47, rsc1 wrote:
> blank line above this one to separate the cases
>
> also // Plan 9
> (with a space)
>
Done.
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/mkasmh.sh
File src/pkg/runtime/mkasmh.sh (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/mkasmh.sh#newcode28
src/pkg/runtime/mkasmh.sh:28: echo '#define get_tls(r) MOVL 0xdfffeffc, r'
On 2010/09/24 22:58:47, rsc1 wrote:
> This is putting a pointer to the m, g word pair
> at that location but instead I would put m, g themselves there,
> avoiding the indirection:
>
> #define get_tls(r)
> #define g(r) 0xdfffeffc
> #define m(r) 0xdfffeff8
>
Done.
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/defs.h
File src/pkg/runtime/plan9/386/defs.h (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/defs.h#newcode2
src/pkg/runtime/plan9/386/defs.h:2: #define PLAN9_TLS (UTOPSTCK-4)
On 2010/09/24 22:58:47, rsc1 wrote:
> Is this used somewhere?
>
Done.
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/rt0.s
File src/pkg/runtime/plan9/386/rt0.s (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/rt0.s#newcode5
src/pkg/runtime/plan9/386/rt0.s:5: // Darwin and Linux use the same linkage to main
On 2010/09/24 22:58:47, rsc1 wrote:
> Can delete this comment. I'm not sure this is correct, either.
> The environment, for example, is passed in a different way,
> although the arguments are probably the same.
>
Done.
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/sys.s
File src/pkg/runtime/plan9/386/sys.s (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/sys.s#newcode6
src/pkg/runtime/plan9/386/sys.s:6: MOVL address+4(FP), CX
On 2010/09/24 22:58:47, rsc1 wrote:
> I think you can just make setldt a no-op.
> Each thread will get its own copy of the stack.
>
Done.
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/mem.c
File src/pkg/runtime/plan9/mem.c (right):
http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/mem.c#newcode32
src/pkg/runtime/plan9/mem.c:32: USED(v, n);
On 2010/09/24 22:58:47, rsc1 wrote:
> It's probably a good idea to implement this
> at least partially. See tiny/mem.c for an idea.
>
Done.
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/386/asm.s
File src/pkg/runtime/386/asm.s (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/386/asm.s#newcode408
src/pkg/runtime/386/asm.s:408: TEXT tlscheck(SB), 7, $0
Done, is this alright or should I keep tlscheck ?
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s
File src/pkg/runtime/plan9/386/rt0.s (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s#newcode5
src/pkg/runtime/plan9/386/rt0.s:5: TEXT _rt0_386_plan9(SB),7, $0
Done, I had to tweak it a bit to make it work.
Is it expected that if I do
MOVL argc+0(SP), CX
instead of
MOVL 0(SP), CX // argc
the final output will have 8(SP) - the 8 being due to the TEXT pseudo-op ?
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s#newcode36
src/pkg/runtime/plan9/386/rt0.s:36: GLOBL _tos(SB), $4
Should _tos be made os·_tos like os·Args ?
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s#newcode36
src/pkg/runtime/plan9/386/rt0.s:36: GLOBL _tos(SB), $4
Done.
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/mem.c
File src/pkg/runtime/plan9/mem.c (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/mem.c#newcode8
src/pkg/runtime/plan9/mem.c:8: extern int8 end[];
Done.
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):
http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/runtime.c#newcode157
src/pkg/runtime/runtime.c:157: if(goos != nil && strcmp((uint8*)goos, (uint8*)"plan9") == 0)
Done, had to make isplan9 an int32 because of the runtime.h defines prohibiting plain types.
Message from paulzhol@gmail.com
2010-10-07T22:54:43+00:00paulzholurn:md5:c6d1ed2160f3a3de3d9d7eb9df9e4ff9
Hi Russ,
Thank you for the CR, I have updated my patches.
Pavel
Message from rsc@golang.org
2010-10-11T01:11:49+00:00rscurn:md5:6b10da70cec1d23aa29302b967a1d416
On Thu, Oct 7, 2010 at 18:52, <paulzhol@gmail.com> wrote:
>
> http://codereview.appspot.com/2273041/diff/2001/src/cmd/8l/pass.c
> File src/cmd/8l/pass.c (right):
>
> http://codereview.appspot.com/2273041/diff/2001/src/cmd/8l/pass.c#newcode687
> src/cmd/8l/pass.c:687: case 2: // Plan9
> On 2010/09/24 22:58:47, rsc1 wrote:
>>
>> blank line above this one to separate the cases
>
>> also // Plan 9
>> (with a space)
>
>
> Done.
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/mkasmh.sh
> File src/pkg/runtime/mkasmh.sh (right):
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/mkasmh.sh#newcode28
> src/pkg/runtime/mkasmh.sh:28: echo '#define get_tls(r) MOVL
> 0xdfffeffc,
> r'
> On 2010/09/24 22:58:47, rsc1 wrote:
>>
>> This is putting a pointer to the m, g word pair
>> at that location but instead I would put m, g themselves there,
>> avoiding the indirection:
>
>> #define get_tls(r)
>> #define g(r) 0xdfffeffc
>> #define m(r) 0xdfffeff8
>
>
> Done.
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/defs.h
> File src/pkg/runtime/plan9/386/defs.h (right):
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/defs.h#newcode2
> src/pkg/runtime/plan9/386/defs.h:2: #define PLAN9_TLS (UTOPSTCK-4)
> On 2010/09/24 22:58:47, rsc1 wrote:
>>
>> Is this used somewhere?
>
>
> Done.
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/rt0.s
> File src/pkg/runtime/plan9/386/rt0.s (right):
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/rt0.s#newcode5
> src/pkg/runtime/plan9/386/rt0.s:5: // Darwin and Linux use the same
> linkage to main
> On 2010/09/24 22:58:47, rsc1 wrote:
>>
>> Can delete this comment. I'm not sure this is correct, either.
>> The environment, for example, is passed in a different way,
>> although the arguments are probably the same.
>
>
> Done.
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/sys.s
> File src/pkg/runtime/plan9/386/sys.s (right):
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/386/sys.s#newcode6
> src/pkg/runtime/plan9/386/sys.s:6: MOVL address+4(FP), CX
> On 2010/09/24 22:58:47, rsc1 wrote:
>>
>> I think you can just make setldt a no-op.
>> Each thread will get its own copy of the stack.
>
>
> Done.
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/mem.c
> File src/pkg/runtime/plan9/mem.c (right):
>
> http://codereview.appspot.com/2273041/diff/2001/src/pkg/runtime/plan9/mem.c#newcode32
> src/pkg/runtime/plan9/mem.c:32: USED(v, n);
> On 2010/09/24 22:58:47, rsc1 wrote:
>>
>> It's probably a good idea to implement this
>> at least partially. See tiny/mem.c for an idea.
>
>
> Done.
>
> http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/386/asm.s
> File src/pkg/runtime/386/asm.s (right):
>
> http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/386/asm.s#newcode408
> src/pkg/runtime/386/asm.s:408: TEXT tlscheck(SB), 7, $0
> Done, is this alright or should I keep tlscheck ?
>
> http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s
> File src/pkg/runtime/plan9/386/rt0.s (right):
>
> http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s#newcode5
> src/pkg/runtime/plan9/386/rt0.s:5: TEXT _rt0_386_plan9(SB),7, $0
> Done, I had to tweak it a bit to make it work.
> Is it expected that if I do
> MOVL argc+0(SP), CX
> instead of
> MOVL 0(SP), CX // argc
>
> the final output will have 8(SP) - the 8 being due to the TEXT pseudo-op
No, it should still be 0(SP). The $0 means do not subtract from
the stack pointer on entry to allocate a frame, so 0(SP) should
be the return pc. I think you want argc+0(FP), which is the first
word in the argument frame (above the return pc).
> http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/plan9/386/rt0.s#newcode36
> src/pkg/runtime/plan9/386/rt0.s:36: GLOBL _tos(SB), $4
> Should _tos be made os·_tos like os·Args ?
No, it's fine. It's os.Args so that package os can use it
but nothing is using tos yet.
> http://codereview.appspot.com/2273041/diff/93001/src/pkg/runtime/runtime.c#newcode157
> src/pkg/runtime/runtime.c:157: if(goos != nil && strcmp((uint8*)goos,
> (uint8*)"plan9") == 0)
> Done, had to make isplan9 an int32 because of the runtime.h defines
> prohibiting plain types.
Indeed, sorry about that.
I'll take a closer look at the new changes tomorrow.
Russ
Message from unknown
2010-10-11T07:35:48+00:00paulzholurn:md5:217298e95ac03e46e2fb8edb7da623ba
Message from paulzhol@gmail.com
2010-10-11T07:48:10+00:00paulzholurn:md5:34211bfe45f3ab9c51f91914491e04aa
Hi Russ,
I've changed rt0.s a bit to make it more readable, I'm not sure its necessary. It is the only change in the last patchset.
Pavel
http://codereview.appspot.com/2273041/diff/87002/src/pkg/runtime/plan9/386/rt0.s
File src/pkg/runtime/plan9/386/rt0.s (right):
http://codereview.appspot.com/2273041/diff/87002/src/pkg/runtime/plan9/386/rt0.s#newcode10
src/pkg/runtime/plan9/386/rt0.s:10: LEAL argc-4(FP), SI
argc+0(FP) doesn't work here because we have no return address on the stack after the exec, so using argc-4(SP).
http://codereview.appspot.com/2273041/diff/87002/src/pkg/runtime/plan9/386/rt0.s#newcode19
src/pkg/runtime/plan9/386/rt0.s:19: LEAL 4(SP), BP // argv
My question was referring to the two instructions above.
The required effect is reading from the word at address SP,
but if I replace MOVL 0(SP), CX with MOVL tmp+0(SP), CX I see a MOV 0x8(SP), CX in Acid.
Using FP instead of SP generates a MOV 0xc(SP), CX.
I can get the required MOVL 0(SP) by doing a MOVL tmp-8(SP), CX which is consistent with what 8c seems to do when it allocates stack space for locals.
I'll make another patch with this method having $0 instead of $8 automatic storage and do the SUBL $8, SP by hand to make it clear when reading whats going on.
Message from rsc@google.com
2010-10-12T02:50:08+00:00rsc1urn:md5:edd2b14dbafa5e24f6baa4c3ee17471e
LGTM
Can take or leave the rt0.s comment but please
fix the jump in asm.s. JZ is particularly confusing
after a CMPL with $1.
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/386/asm.s
File src/pkg/runtime/386/asm.s (right):
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/386/asm.s#newcode30
src/pkg/runtime/386/asm.s:30: JZ ok
usually spelled JEQ
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/rt0.s
File src/pkg/runtime/plan9/386/rt0.s (right):
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/rt0.s#newcode5
src/pkg/runtime/plan9/386/rt0.s:5: #define ARGS_OFFSET 8
only used once; can go away
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/rt0.s#newcode11
src/pkg/runtime/plan9/386/rt0.s:11: LEAL argc+0(SP), SI
// move arguments down to make room for
// m and g at top of stack.
MOVL SP, SI
SUBL $8, SP
MOVL SP, DI
seems pretty clear to me
Message from unknown
2010-10-12T07:55:44+00:00paulzholurn:md5:fa42fb174b503b7720e94432b458ccd2
Message from paulzhol@gmail.com
2010-10-12T08:06:59+00:00paulzholurn:md5:3f0d08622e4c9448db861027971ba1f2
ok, updated.
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/386/asm.s
File src/pkg/runtime/386/asm.s (right):
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/386/asm.s#newcode30
src/pkg/runtime/386/asm.s:30: JZ ok
On 2010/10/12 02:50:08, rsc1 wrote:
> usually spelled JEQ
Done.
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/rt0.s
File src/pkg/runtime/plan9/386/rt0.s (right):
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/rt0.s#newcode5
src/pkg/runtime/plan9/386/rt0.s:5: #define ARGS_OFFSET 8
On 2010/10/12 02:50:08, rsc1 wrote:
> only used once; can go away
Done.
http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/rt0.s#newcode11
src/pkg/runtime/plan9/386/rt0.s:11: LEAL argc+0(SP), SI
On 2010/10/12 02:50:08, rsc1 wrote:
> // move arguments down to make room for
> // m and g at top of stack.
> MOVL SP, SI
> SUBL $8, SP
> MOVL SP, DI
>
>
> seems pretty clear to me
Done.
Message from rsc@golang.org
2010-10-12T13:40:01+00:00rscurn:md5:1443652cd00d8e4f4646ac142845059f
Sorry for dragging this out, but could you please
hg sync
hg upload 2273041
?
Your current patch doesn't apply against the tip of the repository.
Thanks.
Russ
Message from rsc@golang.org
2010-10-12T13:41:30+00:00rscurn:md5:664435c3c6c01501ac0580c6a2602408
Also it appears you haven't completed the CLA as described
at http://golang.org/doc/contribute.html#copyright . Please do.
Thanks.
Message from unknown
2010-10-12T22:02:44+00:00paulzholurn:md5:3f340370afada0200e386961afc52690
Message from paulzhol@gmail.com
2010-10-12T22:15:09+00:00paulzholurn:md5:225a3c21c79daaf03a9d2f1e1634a843
I've updated the patches against tip and signed the individual CLA.
Thanks for your patience,
Pavel
Message from unknown
2010-10-16T08:14:13+00:00paulzholurn:md5:1ae28e3feeab58feda9d06313b24950e
Message from paulzhol@gmail.com
2010-10-16T08:17:47+00:00paulzholurn:md5:7c7b02858c8a9876b5654ce673942bbb
synced against 6537:31e328e0c972.
Message from rsc@golang.org
2010-10-18T16:32:59+00:00rscurn:md5:bdb4cd292e9e9b003747e8f74c40c4e9
*** Submitted as 8d16ce8ee935 ***
8l, runtime: initial support for Plan 9
No multiple processes/locks, managed to compile
and run a hello.go (with print not fmt). Also test/sieve.go
seems to run until 439 and stops with a
'throw: all goroutines are asleep - deadlock!'
- just like runtime/tiny.
based on Russ's suggestions at:
http://groups.google.com/group/comp.os.plan9/browse_thread/thread/cfda8b82535d2d68/243777a597ec1612
Build instructions:
cd src/pkg/runtime
make clean && GOOS=plan9 make install
this will build and install the runtime.
When linking with 8l, you should pass -s to suppress symbol
generation in the a.out, otherwise the generated executable will not run.
This is runtime only, the porting of the toolchain has already
been done: http://code.google.com/p/go-plan9/source/browse
in the plan9-quanstro branch.
R=rsc
CC=golang-dev
http://codereview.appspot.com/2273041
Committer: Russ Cox <rsc@golang.org>