code review 2273041: Initial Plan9 runtime support for 386.

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.

[paulzhol, 2010-09-23 17:54:11]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[paulzhol, 2010-09-23 23:11:08]

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

[rsc1, 2010-09-24 22:58:47]

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

[paulzhol, 2010-09-25 22:31:16]

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

[rsc, 2010-09-28 16:08:29]

> 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

[paulzhol, 2010-09-29 22:21:18]

> 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

[rsc, 2010-09-30 01:55:47]

> 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

[paulzhol, 2010-09-30 15:11:44]

>
> 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

[paulzhol, 2010-10-06 08:12:42]

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

[paulzhol, 2010-10-07 06:59:14]

> 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

[rsc, 2010-10-07 07:06:26]

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

[rsc1, 2010-10-07 08:12:41]

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

[paulzhol, 2010-10-07 22:52:23]

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.

[paulzhol, 2010-10-07 22:54:43]

Hi Russ,
 Thank you for the CR, I have updated my patches.

Pavel

[rsc, 2010-10-11 01:11:49]

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

[paulzhol, 2010-10-11 07:48:10]

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.

[rsc1, 2010-10-12 02:50:08]

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

[paulzhol, 2010-10-12 08:06:59]

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.

[rsc, 2010-10-12 13:40:01]

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

[rsc, 2010-10-12 13:41:30]

Also it appears you haven't completed the CLA as described
at http://golang.org/doc/contribute.html#copyright .  Please do.

Thanks.

[paulzhol, 2010-10-12 22:15:09]

I've updated the patches against tip and signed the individual CLA.

Thanks for your patience,
Pavel

[paulzhol, 2010-10-16 08:17:47]

synced against 6537:31e328e0c972.

[rsc, 2010-10-18 16:32:59]

*** 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>