Issue 12785045: code review 12785045: cmd/gc, runtime: use type information to scan interface... (Closed)
Descriptioncmd/gc, runtime: use type information to scan interface values
Patch Set 1 #Patch Set 2 : diff -r e1afa89496ed https://code.google.com/p/go/ #Patch Set 3 : diff -r e1afa89496ed https://code.google.com/p/go/ #Patch Set 4 : diff -r e1afa89496ed https://code.google.com/p/go/ #Patch Set 5 : diff -r e1afa89496ed https://code.google.com/p/go/ #
Total comments: 6
Patch Set 6 : diff -r 8bf13ae02c8e https://code.google.com/p/go/ #Patch Set 7 : diff -r 8bf13ae02c8e https://code.google.com/p/go/ #Patch Set 8 : diff -r 8bf13ae02c8e https://code.google.com/p/go/ #
MessagesTotal messages: 13
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
LGTM assuming we do better later
Sign in to reply to this message.
https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:1449: scaninterfacedata(bits, scanp, inprologue); why only scanning of interfaces depends on inprologue? https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:1493: scanbitvector(frame->argp, args, !afterprologue); why do not we look at interface type if we are inprologue? I think it would be better to consistently use afterprologue predicate. It would remove negation here and in scaninterfacedata.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:1449: scaninterfacedata(bits, scanp, inprologue); Because only the interface scanning loads through the value being scanned. See lines 1416 and lines 1420. To scan an interface value we must know its type. To know an interface type, we must load through the first word. After the prologue we will have established the invariant that all pointer values in a frame are either zero or contain a valid pointer. As such, it should be safe to load through the first word of an interface to check its type. However, if we stop in the prologue the aforementioned invariant does not necessarily hold. There are no locals yet, so that is okay by default. The ins area will have been filled by the caller so that is okay to scan. The outs area, however, contains uninitialized data, and so it is not okay to scan if it contains interface values. https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:1493: scanbitvector(frame->argp, args, !afterprologue); I am not sure I understand what you perceive to be not consistent about the afterprologue value. Can you clarify?
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=71ce80dc4195 *** cmd/gc, runtime: use type information to scan interface values R=golang-dev, rsc, dvyukov CC=golang-dev https://codereview.appspot.com/12785045
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:1493: scanbitvector(frame->argp, args, !afterprologue); On 2013/08/19 17:07:25, cshapiro1 wrote: > I am not sure I understand what you perceive to be not consistent about the > afterprologue value. Can you clarify? In this function you use afterprologue, and in scanbitvector() and scaninterfacedata() you call it inprologue (with inversed value). I meant to rename inprologue to afterprologue, and remove negations.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/08/19 17:22:43, dvyukov wrote: > In this function you use afterprologue, and in scanbitvector() and > scaninterfacedata() you call it inprologue (with inversed value). > I meant to rename inprologue to afterprologue, and remove negations. That sounds good. I'll upload a change shortly. Thanks!
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/12785045/diff/10001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:1449: scaninterfacedata(bits, scanp, inprologue); On 2013/08/19 17:07:25, cshapiro1 wrote: > Because only the interface scanning loads through the value being scanned. See > lines 1416 and lines 1420. > > To scan an interface value we must know its type. To know an interface type, we > must load through the first word. After the prologue we will have established > the invariant that all pointer values in a frame are either zero or contain a > valid pointer. As such, it should be safe to load through the first word of an > interface to check its type. > > However, if we stop in the prologue the aforementioned invariant does not > necessarily hold. There are no locals yet, so that is okay by default. The ins > area will have been filled by the caller so that is okay to scan. The outs > area, however, contains uninitialized data, and so it is not okay to scan if it > contains interface values. Ah, I see. I thought that it's 100% precise now, and so there must be no such checks. It is not 100% precise yet because we do not know where input arguments and where outputs. So we have to scan all them, but part of that data is uninitialized, right?
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/08/19 17:28:01, dvyukov wrote: > I thought that it's 100% precise now, and so there must be no such checks. It is > not 100% precise yet because we do not know where input arguments and where > outputs. So we have to scan all them, but part of that data is uninitialized, > right? If by "it" you mean scanning of go frames then you are correct. We need a small amount of additional accounting to skip the outs area during scanning of functions that have stopped in the prologue. (As an aside, this is already handled by my now delayed change to scan go frames using liveness information as the first bitmap generated marks only pointer values in the ins area live.) Go frames aside, there is still a lot of stuff on the stack for which we do not have enough metadata to scan precisely yet.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/08/19 17:33:11, cshapiro1 wrote: > On 2013/08/19 17:28:01, dvyukov wrote: > > I thought that it's 100% precise now, and so there must be no such checks. It > is > > not 100% precise yet because we do not know where input arguments and where > > outputs. So we have to scan all them, but part of that data is uninitialized, > > right? > > If by "it" you mean scanning of go frames then you are correct. We need a small > amount of additional accounting to skip the outs area during scanning of > functions that have stopped in the prologue. (As an aside, this is already > handled by my now delayed change to scan go frames using liveness information as > the first bitmap generated marks only pointer values in the ins area live.) Go > frames aside, there is still a lot of stuff on the stack for which we do not > have enough metadata to scan precisely yet. Yes, I meant scanning of go frames. Now I understand why we need that 'if'. Thanks!
Sign in to reply to this message.
Message was sent while issue was closed.
Race builder is unhappy: http://build.golang.org/log/952a3ae2cc1e7594abc8c71d1fbc999acdea2176 There is that weird: goroutine 8 [runnable]: runtime/race._Cfunc___tsan_func_enter(0x1b373d8, 0x531156) runtime/race/_obj/_cgo_defun.c:79 +0x31 runtime/race.FuncEnter(0x1b373d8, 0x531156) /tmp/gobuilder/linux-amd64-race-71ce80dc4195/go/src/pkg/runtime/race/race.go:69 +0x31 net.convertErr(0x1, 0x72, 0x1) /usr/local/go/src/pkg/net/fd_poll_runtime.go:115 +0x27 I suspect that GC and traceback printing use different pc/sp for traceback. GC looks at g->syscallsp, and traceback looks as g->status==Gsyscall. So it may be not the stack GC observed.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/08/19 17:41:40, dvyukov wrote: > I suspect that GC and traceback printing use different pc/sp for traceback. GC > looks at g->syscallsp, and traceback looks as g->status==Gsyscall. So it may be > not the stack GC observed. Two stack contain something similar to the following SIGSEGV: segmentation violation PC=0x410d70 scaninterfacedata(0x2, 0x7fca5bbbd888, 0x7fca599ccb00) /usr/local/go/src/pkg/runtime/mgc0.c:1416 +0x30 scanbitvector(0x7fca5bbbd888, 0x8bd108, 0xaf4100) /usr/local/go/src/pkg/runtime/mgc0.c:1449 +0x10a addframeroots(0x7fca599ccbc8, 0x0) /usr/local/go/src/pkg/runtime/mgc0.c:1493 +0x137 runtime.gentraceback(0x40cc80, 0x7fca5bbbd768, 0x0, 0xc210047c60, 0x0, ...) For the record, this means that something tried to load the type of an interface pointer and loaded an invalid pointer instead.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
