code review 9666047: cmd/ld, runtime: emit pointer maps for nosplits identif...

cmd/ld, runtime: emit pointer maps for nosplits identified by the linker

A nosplits was assumed to have no argument information and no
pointer map.  However, nosplits created by the linker often
have both.  This change uses the pointer map size as an
alternate source of argument size when processing a nosplit.

In addition, the symbol table construction pointer map size
and argument size consistency check is strengthened.  If a
nptrs is greater than 0 it must be equal to the number of
argument words.

[cshapiro1, 2013-05-29 02:49:50]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[khr, 2013-05-29 19:07:03]

https://codereview.appspot.com/9666047/diff/5001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/9666047/diff/5001/src/cmd/ld/lib.c#newcode1917
src/cmd/ld/lib.c:1917: if(s->text->textflag & NOSPLIT) {
I don't understand why NOSPLIT implies that we don't have any information about
args.  For most (all?) NOSPLIT routines we do.

https://codereview.appspot.com/9666047/diff/5001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9666047/diff/5001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:237: if(sym->value != 0 && sym->value !=
func[nfunc-1].args/sizeof(uintptr)) {
This will change when we have pointer info for locals, right?

[cshapiro1, 2013-05-29 20:06:40]

https://codereview.appspot.com/9666047/diff/5001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/9666047/diff/5001/src/cmd/ld/lib.c#newcode1917
src/cmd/ld/lib.c:1917: if(s->text->textflag & NOSPLIT) {
If we are talking about 6g compiled code, most nosplits will have argument
information.  The exception is the trampoline functions built by the
genembedtramp function.  I am working on a fix for that right now.

However, the linker is not limited to processing 6g compiled code.  It also sees
6c compiled code.  There are routinely hundreds of routines that pass through
the linker without argument information.  Some of these are nosplits by virtue
of allocating no stack space.  Others are nosplits because of an optional
textflag annotation or a required textflag annotation because of varargs usage.

Here's a list...

runtime.memequal0
runtime.memcopy0
runtime.memequal8
runtime.memcopy8
runtime.memequal16
runtime.memcopy16
runtime.memequal32
runtime.memcopy32
runtime.memequal64
runtime.memcopy64
runtime.memequal128
runtime.memcopy128
runtime.f32equal
runtime.f64equal
runtime.c64equal
runtime.c128equal
runtime.slicecopy
runtime.strcopy
runtime.intercopy
runtime.nilintercopy
runtime.atomicload
runtime.atomicload64
runtime.atomicloadp
runtime.chansend1
runtime.chanrecv1
runtime.selectnbsend
runtime.selectnbrecv
reflect.chansend
runtime.closechan
reflect.chanlen
reflect.chancap
enqueue
runtime.mapaccess2_fast32
runtime.mapaccess1_fast64
runtime.mapaccess2_fast64
runtime.mapaccess1_faststr
runtime.mapaccess2_faststr
hash_gciter_init
runtime.mapaccess1
runtime.mapaccess2
runtime.mapassign1
runtime.mapdelete
runtime.mapiter1
reflect.maplen
runtime.mapiter2
runtime.typ2Itab
runtime.convT2I
runtime.convT2E
runtime.assertI2T
runtime.assertI2T2
runtime.assertI2TOK
runtime.assertE2T
runtime.assertE2T2
runtime.assertE2TOK
runtime.convI2E
runtime.ifacethash
runtime.efacethash
runtime.noteclear
runtime.MGetSizeClassInfo
runtime.FixAlloc_Init
runtime.FixAlloc_Free
runtime.blockspecial
BestFit
runtime.MHeap_Lookup
runtime.MHeap_LookupMaybe
runtime.MSpan_Init
runtime.MSpanList_Init
runtime.MSpanList_Remove
runtime.MSpanList_IsEmpty
runtime.setprof
runtime.badsignal
runtime.deferproc
runtime.deferreturn
runtime.recover
runtime.printf
runtime.typestring
mhelpgc
mspinning
runtime.entersyscall
runtime.entersyscallblock
runtime.newproc
LockOSThread
UnlockOSThread
runtime.mcount
mget
globrunqput
runtime.charntorune
runtime.runetochar
runtime.mcmp
runtime.mchr
runtime.atoi
runtime.fastrand1
runtime.appendslice
runtime.appendstr
runtime.copy
runtime.slicestringcopy
readword
splitpcln
runtime.funcline
hasprefix
vdso_init_from_sysinfo_ehdr
runtime.purgecachedstats
runtime.new
MProf_GC
getaddrbucket
netpollunblock
semroot
semqueue
semdequeue
runtime.findnull
runtime.concatstring
runtime.strcmp
_rt0_amd64
runtime.asminit
runtime.gosave
runtime.gogo
runtime.gogocall
runtime.gogocallfn
runtime.mcall
runtime.morestack
reflect.call
runtime.lessstack
runtime.morestack00
runtime.morestack01
runtime.morestack10
runtime.morestack11
runtime.morestack8
runtime.morestack16
runtime.morestack24
runtime.morestack32
runtime.morestack40
runtime.morestack48
morestack
runtime.cas
runtime.cas64
runtime.casp
runtime.xadd
runtime.xadd64
runtime.xchg
runtime.xchg64
runtime.procyield
runtime.atomicstorep
runtime.atomicstore
runtime.atomicstore64
runtime.jmpdefer
return
runtime.asmcgocall
setmg_gcc
runtime.stackcheck
runtime.memclr
runtime.getcallerpc
runtime.getcallersp
runtime.cputicks
runtime.aeshash
runtime.aeshashstr
runtime.aeshashbody
runtime.aeshash32
runtime.aeshash64
masks
shifts
runtime.memeq
bytes.Equal
runtime.memeqbody
runtime.cmpstring
runtime.cmpbody
runtime.memmove
_rt0_amd64_linux
main
runtime.exit
runtime.open
runtime.close
runtime.write
runtime.read
runtime.usleep
runtime.raise
runtime.setitimer
runtime.mincore
time.now
runtime.nanotime
runtime.rtsigprocmask
runtime.rt_sigaction
runtime.sigtramp
runtime.sigreturn
runtime.mmap
runtime.munmap
runtime.madvise
runtime.futex
runtime.clone
runtime.sigaltstack
runtime.settls
runtime.osyield
runtime.sched_getaffinity
runtime.epollwait
bytes.IndexByte
crypto/md5.block
reflect.methodValueCall
math.Abs
math.Ldexp
syscall.Syscall
syscall.Syscall6
syscall.RawSyscall
syscall.RawSyscall6
sync/atomic.CompareAndSwapInt32
sync/atomic.CompareAndSwapUint32
sync/atomic.AddInt32
sync/atomic.AddUint32
sync/atomic.LoadUint32
sync/atomic.StoreInt32
sync/atomic.StoreUint32

https://codereview.appspot.com/9666047/diff/5001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9666047/diff/5001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:237: if(sym->value != 0 && sym->value !=
func[nfunc-1].args/sizeof(uintptr)) {
Yes, the condition and the error message will have to change.

[khr1, 2013-05-29 21:21:21]

On Wed, May 29, 2013 at 1:06 PM, <cshapiro@google.com> wrote:

>
>
https://codereview.appspot.**com/9666047/diff/5001/src/cmd/**ld/lib.c<https:/...
> File src/cmd/ld/lib.c (right):
>
> https://codereview.appspot.**com/9666047/diff/5001/src/cmd/**
>
ld/lib.c#newcode1917<https://codereview.appspot.com/9666047/diff/5001/src/cmd/ld/lib.c#newcode1917>
> src/cmd/ld/lib.c:1917: if(s->text->textflag & NOSPLIT) {
> If we are talking about 6g compiled code, most nosplits will have
> argument information.  The exception is the trampoline functions built
> by the genembedtramp function.  I am working on a fix for that right
> now.
>
> However, the linker is not limited to processing 6g compiled code.  It
> also sees 6c compiled code.  There are routinely hundreds of routines
> that pass through the linker without argument information.  Some of
> these are nosplits by virtue of allocating no stack space.  Others are
> nosplits because of an optional textflag annotation or a required
> textflag annotation because of varargs usage.
>
>
What I'm saying here is that you are using NOSPLIT as a proxy for "no arg
info" when they are not in fact the same thing.  Can't we just check if the
arg info is there directly?  Maybe it's not that easy...


> Here's a list...
>
> runtime.memequal0
> runtime.memcopy0
> runtime.memequal8
> runtime.memcopy8
> runtime.memequal16
> runtime.memcopy16
> runtime.memequal32
> runtime.memcopy32
> runtime.memequal64
> runtime.memcopy64
> runtime.memequal128
> runtime.memcopy128
> runtime.f32equal
> runtime.f64equal
> runtime.c64equal
> runtime.c128equal
> runtime.slicecopy
> runtime.strcopy
> runtime.intercopy
> runtime.nilintercopy
> runtime.atomicload
> runtime.atomicload64
> runtime.atomicloadp
> runtime.chansend1
> runtime.chanrecv1
> runtime.selectnbsend
> runtime.selectnbrecv
> reflect.chansend
> runtime.closechan
> reflect.chanlen
> reflect.chancap
> enqueue
> runtime.mapaccess2_fast32
> runtime.mapaccess1_fast64
> runtime.mapaccess2_fast64
> runtime.mapaccess1_faststr
> runtime.mapaccess2_faststr
> hash_gciter_init
> runtime.mapaccess1
> runtime.mapaccess2
> runtime.mapassign1
> runtime.mapdelete
> runtime.mapiter1
> reflect.maplen
> runtime.mapiter2
> runtime.typ2Itab
> runtime.convT2I
> runtime.convT2E
> runtime.assertI2T
> runtime.assertI2T2
> runtime.assertI2TOK
> runtime.assertE2T
> runtime.assertE2T2
> runtime.assertE2TOK
> runtime.convI2E
> runtime.ifacethash
> runtime.efacethash
> runtime.noteclear
> runtime.MGetSizeClassInfo
> runtime.FixAlloc_Init
> runtime.FixAlloc_Free
> runtime.blockspecial
> BestFit
> runtime.MHeap_Lookup
> runtime.MHeap_LookupMaybe
> runtime.MSpan_Init
> runtime.MSpanList_Init
> runtime.MSpanList_Remove
> runtime.MSpanList_IsEmpty
> runtime.setprof
> runtime.badsignal
> runtime.deferproc
> runtime.deferreturn
> runtime.recover
> runtime.printf
> runtime.typestring
> mhelpgc
> mspinning
> runtime.entersyscall
> runtime.entersyscallblock
> runtime.newproc
> LockOSThread
> UnlockOSThread
> runtime.mcount
> mget
> globrunqput
> runtime.charntorune
> runtime.runetochar
> runtime.mcmp
> runtime.mchr
> runtime.atoi
> runtime.fastrand1
> runtime.appendslice
> runtime.appendstr
> runtime.copy
> runtime.slicestringcopy
> readword
> splitpcln
> runtime.funcline
> hasprefix
> vdso_init_from_sysinfo_ehdr
> runtime.purgecachedstats
> runtime.new
> MProf_GC
> getaddrbucket
> netpollunblock
> semroot
> semqueue
> semdequeue
> runtime.findnull
> runtime.concatstring
> runtime.strcmp
> _rt0_amd64
> runtime.asminit
> runtime.gosave
> runtime.gogo
> runtime.gogocall
> runtime.gogocallfn
> runtime.mcall
> runtime.morestack
> reflect.call
> runtime.lessstack
> runtime.morestack00
> runtime.morestack01
> runtime.morestack10
> runtime.morestack11
> runtime.morestack8
> runtime.morestack16
> runtime.morestack24
> runtime.morestack32
> runtime.morestack40
> runtime.morestack48
> morestack
> runtime.cas
> runtime.cas64
> runtime.casp
> runtime.xadd
> runtime.xadd64
> runtime.xchg
> runtime.xchg64
> runtime.procyield
> runtime.atomicstorep
> runtime.atomicstore
> runtime.atomicstore64
> runtime.jmpdefer
> return
> runtime.asmcgocall
> setmg_gcc
> runtime.stackcheck
> runtime.memclr
> runtime.getcallerpc
> runtime.getcallersp
> runtime.cputicks
> runtime.aeshash
> runtime.aeshashstr
> runtime.aeshashbody
> runtime.aeshash32
> runtime.aeshash64
> masks
> shifts
> runtime.memeq
> bytes.Equal
> runtime.memeqbody
> runtime.cmpstring
> runtime.cmpbody
> runtime.memmove
> _rt0_amd64_linux
> main
> runtime.exit
> runtime.open
> runtime.close
> runtime.write
> runtime.read
> runtime.usleep
> runtime.raise
> runtime.setitimer
> runtime.mincore
> time.now
> runtime.nanotime
> runtime.rtsigprocmask
> runtime.rt_sigaction
> runtime.sigtramp
> runtime.sigreturn
> runtime.mmap
> runtime.munmap
> runtime.madvise
> runtime.futex
> runtime.clone
> runtime.sigaltstack
> runtime.settls
> runtime.osyield
> runtime.sched_getaffinity
> runtime.epollwait
> bytes.IndexByte
> crypto/md5.block
> reflect.methodValueCall
> math.Abs
> math.Ldexp
> syscall.Syscall
> syscall.Syscall6
> syscall.RawSyscall
> syscall.RawSyscall6
> sync/atomic.**CompareAndSwapInt32
> sync/atomic.**CompareAndSwapUint32
> sync/atomic.AddInt32
> sync/atomic.AddUint32
> sync/atomic.LoadUint32
> sync/atomic.StoreInt32
> sync/atomic.StoreUint32
>
>
> https://codereview.appspot.**com/9666047/diff/5001/src/pkg/**
>
runtime/symtab.c<https://codereview.appspot.com/9666047/diff/5001/src/pkg/runtime/symtab.c>
> File src/pkg/runtime/symtab.c (right):
>
> https://codereview.appspot.**com/9666047/diff/5001/src/pkg/**
>
runtime/symtab.c#newcode237<https://codereview.appspot.com/9666047/diff/5001/src/pkg/runtime/symtab.c#newcode237>
> src/pkg/runtime/symtab.c:237: if(sym->value != 0 && sym->value !=
> func[nfunc-1].args/sizeof(**uintptr)) {
> Yes, the condition and the error message will have to change.
>
>
https://codereview.appspot.**com/9666047/<https://codereview.appspot.com/9666...
>

[cshapiro1, 2013-05-29 21:37:58]

On Wed, May 29, 2013 at 2:21 PM, Keith Randall <khr@google.com> wrote:

> What I'm saying here is that you are using NOSPLIT as a proxy for "no arg
> info" when they are not in fact the same thing.  Can't we just check if the
> arg info is there directly?  Maybe it's not that easy...


At present, no.  The arg info is 0 when it is not available.  This is
indistinguishable from a 0 argument procedure.

I am trying to solve this problem more generally by providing the missing
information where possible and eliminating the remaining, problematic
constructs.  It will take a few CLs to make this happen.

[khr1, 2013-05-29 21:46:27]

Maybe the condition should be (s->text->textflag & NOSPLIT) && s->args ==
0.  In any case, put a comment in there with your text above.

LGTM.


On Wed, May 29, 2013 at 2:37 PM, Carl Shapiro <cshapiro@google.com> wrote:

> On Wed, May 29, 2013 at 2:21 PM, Keith Randall <khr@google.com> wrote:
>
>> What I'm saying here is that you are using NOSPLIT as a proxy for "no arg
>> info" when they are not in fact the same thing.  Can't we just check if the
>> arg info is there directly?  Maybe it's not that easy...
>
>
> At present, no.  The arg info is 0 when it is not available.  This is
> indistinguishable from a 0 argument procedure.
>
> I am trying to solve this problem more generally by providing the missing
> information where possible and eliminating the remaining, problematic
> constructs.  It will take a few CLs to make this happen.
>

[cshapiro1, 2013-05-29 23:42:55]

On Wed, May 29, 2013 at 2:46 PM, Keith Randall <khr@google.com> wrote:

> Maybe the condition should be (s->text->textflag & NOSPLIT) && s->args ==
> 0.  In any case, put a comment in there with your text above.
>

I have changed the pointer map information to have nptrs always count the
size of the pointer map in words and not take any shortcuts when the map is
all zero.  This allows the condition to be simplified to

if(s->text->textflag & NOSPLIT) && (s->args == 0) && (s->nptrs < 0)


Which is another way of saying "is this a nosplit, is the argumnt size 0,
and is there no pointer map".

Of course, this will trigger for routines that are 6c compiled, marked by
the linker as NOSPLIT, and have 0 arguments.  There is not yet much I can
do about that.

[khr1, 2013-05-29 23:44:30]

Sounds good.


On Wed, May 29, 2013 at 4:42 PM, Carl Shapiro <cshapiro@google.com> wrote:

> On Wed, May 29, 2013 at 2:46 PM, Keith Randall <khr@google.com> wrote:
>
>> Maybe the condition should be (s->text->textflag & NOSPLIT) && s->args
>> == 0.  In any case, put a comment in there with your text above.
>>
>
> I have changed the pointer map information to have nptrs always count the
> size of the pointer map in words and not take any shortcuts when the map is
> all zero.  This allows the condition to be simplified to
>
> if(s->text->textflag & NOSPLIT) && (s->args == 0) && (s->nptrs < 0)
>
>
> Which is another way of saying "is this a nosplit, is the argumnt size 0,
> and is there no pointer map".
>
> Of course, this will trigger for routines that are 6c compiled, marked by
> the linker as NOSPLIT, and have 0 arguments.  There is not yet much I can
> do about that.
>

[cshapiro1, 2013-05-30 00:17:00]

*** Submitted as https://code.google.com/p/go/source/detail?r=44ea8c2938f1 ***

cmd/ld, runtime: emit pointer maps for nosplits identified by the linker

A nosplits was assumed to have no argument information and no
pointer map.  However, nosplits created by the linker often
have both.  This change uses the pointer map size as an
alternate source of argument size when processing a nosplit.

In addition, the symbol table construction pointer map size
and argument size consistency check is strengthened.  If a
nptrs is greater than 0 it must be equal to the number of
argument words.

R=golang-dev, khr, khr
CC=golang-dev
https://codereview.appspot.com/9666047

[r, 2013-05-30 01:36:57]

This piece of deep runtime was submitted without a core team member's
LGTM. It's always good to have external approval (thanks kr) but
especially for critical pieces please get a core LGTM. Keith Randall's
'sounds good' wasn't enough for my petty tyrannical tendencies.

-rob


On Thu, May 30, 2013 at 12:17 AM,  <cshapiro@google.com> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=44ea8c2938f1 ***
>
> cmd/ld, runtime: emit pointer maps for nosplits identified by the linker
>
> A nosplits was assumed to have no argument information and no
> pointer map.  However, nosplits created by the linker often
> have both.  This change uses the pointer map size as an
> alternate source of argument size when processing a nosplit.
>
> In addition, the symbol table construction pointer map size
> and argument size consistency check is strengthened.  If a
> nptrs is greater than 0 it must be equal to the number of
> argument words.
>
> R=golang-dev, khr, khr
> CC=golang-dev
> https://codereview.appspot.com/9666047
>
>
> https://codereview.appspot.com/9666047/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>