|
|
|
Created:
15 years, 1 month ago by paulzhol Modified:
15 years, 1 month ago Reviewers:
CC:
rsc, golang-dev Visibility:
Public. |
Initial Plan9 runtime support for 386.
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 Russes suggestions at:
http://groups.google.com/group/comp.os.plan9/browse_thread/thread/cfda8b82535d2d68/243777a597ec1612
(I'm not sure I can use the 0xdffffefc address like that but it seems to run...)
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.
Patch Set 1 : code review 2273041: Initial Plan9 runtime support for 386. #
Total comments: 12
Patch Set 2 : code review 2273041: Initial Plan9 runtime support for 386. #Patch Set 3 : code review 2273041: Initial Plan9 runtime support for 386. #Patch Set 4 : code review 2273041: Initial Plan9 runtime support for 386 [update to release.2010年09月29日]. #Patch Set 5 : code review 2273041: Initial Plan9 runtime support for 386. #Patch Set 6 : code review 2273041: Initial Plan9 runtime support for 386. #
Total comments: 11
Patch Set 7 : code review 2273041: Initial Plan9 runtime support for 386. #
Total comments: 2
Patch Set 8 : code review 2273041: Initial Plan9 runtime support for 386. #
Total comments: 6
Patch Set 9 : code review 2273041: Initial Plan9 runtime support for 386. #Patch Set 10 : code review 2273041: Initial Plan9 runtime support for 386. #Patch Set 11 : code review 2273041: Initial Plan9 runtime support for 386. #
Total messages: 23
|
paulzhol
Hello golang-dev@googlegroups.com, I'd like you to review this change.
|
15 years, 1 month ago (2010年09月23日 17:54:11 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hello golang-dev@googlegroups.com, I'd like you to review this change.
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
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#new... 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/def... 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... 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... 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#n... 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
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
> 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
> 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
> 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
> > 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
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
> 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
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
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#ne... 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/rt... 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/rt... 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#... 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#ne... src/pkg/runtime/runtime.c:157: if(goos != nil && strcmp((uint8*)goos, (uint8*)"plan9") == 0) extern int isplan9; if(isplan9) envc = 0; else
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#new... 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/def... 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... 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... 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#n... 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#ne... 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/rt... 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/rt... 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/rt... 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#... 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#ne... 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.
Hi Russ, Thank you for the CR, I have updated my patches. Pavel
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#new... > 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/def... > 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... > 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... > 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#n... > 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#ne... > 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/rt... > 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/rt... > 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#ne... > 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
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/rt... 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/rt... 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.
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#n... src/pkg/runtime/386/asm.s:30: JZ ok usually spelled JEQ http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/r... File src/pkg/runtime/plan9/386/rt0.s (right): http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/r... 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/r... 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
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#n... 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/r... File src/pkg/runtime/plan9/386/rt0.s (right): http://codereview.appspot.com/2273041/diff/102001/src/pkg/runtime/plan9/386/r... 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/r... 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.
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
Also it appears you haven't completed the CLA as described at http://golang.org/doc/contribute.html#copyright . Please do. Thanks.
I've updated the patches against tip and signed the individual CLA. Thanks for your patience, Pavel
synced against 6537:31e328e0c972.
*** 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/cfda8b82535... 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>