|
|
Created:
11 years, 6 months ago by remyoudompheng Modified:
11 years, 4 months ago Reviewers:
minux1 CC:
dave_cheney.net, rsc, golang-dev Visibility:
Public. |
Descriptioncmd/5g: introduce componentgen for better registerization.
It is essentially identical to the version in 6g.
Patch Set 1 #Patch Set 2 : diff -r 6b67a925abcc https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 11f0ba3b7006 https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 4 : diff -r 8df088298a0c https://go.googlecode.com/hg/ #
MessagesTotal messages: 21
Please test.
Sign in to reply to this message.
On 2012/10/16 06:12:27, remyoudompheng wrote: > Please test. cmd/go panic: runtime error: invalid memory address or nil pointer dereference [signal 0xb code=0x1 addr=0xe pc=0x146670] goroutine 1 [running]: regexp/syntax.Parse(0x1df30c, 0xbb, 0x2d00d4, 0x2, 0x2, ...) /home/dfc/go/src/pkg/regexp/syntax/parse.go:750 +0x168 regexp.compile(0x1df30c, 0xbb, 0xd4, 0xffffffff, 0x603c4, ...) /home/dfc/go/src/pkg/regexp/regexp.go:134 +0x38 regexp.Compile(0x1df30c, 0xbb, 0x0, 0xb6efce78) /home/dfc/go/src/pkg/regexp/regexp.go:107 +0x4c regexp.MustCompile(0x1df30c, 0xbb, 0x1, 0x1) /home/dfc/go/src/pkg/regexp/regexp.go:195 +0x30 go/doc.init() /home/dfc/go/src/pkg/go/doc/synopsis.go:-1866 +0xb0 main.init() /home/dfc/go/src/cmd/go/vet.go:37 +0x7c goroutine 2 [runnable]: created by runtime.main /home/dfc/go/src/pkg/runtime/proc.c:224
Sign in to reply to this message.
On 2012/10/16 06:49:20, dfc wrote: > On 2012/10/16 06:12:27, remyoudompheng wrote: > > Please test. > > cmd/go > panic: runtime error: invalid memory address or nil pointer dereference > [signal 0xb code=0x1 addr=0xe pc=0x146670] > > goroutine 1 [running]: > regexp/syntax.Parse(0x1df30c, 0xbb, 0x2d00d4, 0x2, 0x2, ...) > /home/dfc/go/src/pkg/regexp/syntax/parse.go:750 +0x168 > regexp.compile(0x1df30c, 0xbb, 0xd4, 0xffffffff, 0x603c4, ...) > /home/dfc/go/src/pkg/regexp/regexp.go:134 +0x38 > regexp.Compile(0x1df30c, 0xbb, 0x0, 0xb6efce78) > /home/dfc/go/src/pkg/regexp/regexp.go:107 +0x4c > regexp.MustCompile(0x1df30c, 0xbb, 0x1, 0x1) > /home/dfc/go/src/pkg/regexp/regexp.go:195 +0x30 > go/doc.init() > /home/dfc/go/src/pkg/go/doc/synopsis.go:-1866 +0xb0 > main.init() > /home/dfc/go/src/cmd/go/vet.go:37 +0x7c > > goroutine 2 [runnable]: > created by runtime.main > /home/dfc/go/src/pkg/runtime/proc.c:224 comparing componentgen from the previous CL, this stands out as being removed @@ -1502,7 +1501,7 @@ int componentgen(Node *nr, Node *nl) { - Node nodl, nodr, tmp; + Node nodl, nodr; int freel, freer; freel = 0; @@ -1534,12 +1533,6 @@ igen(nr, &nodr, N); freer = 1; } - } else { - // When zeroing, prepare a register containing zero. - nodconst(&tmp, nl->type, 0); - regalloc(&nodr, types[TUINT], N); - gmove(&tmp, &nodr); - freer = 1; }
Sign in to reply to this message.
It seems to give different results when compiling with -N (tried running the produced go tool on a friends's Raspberry). I think I can say the bug is in the optimizer (regopt), but not in the peephole optimizations. I see blocks of code disappearing mysteriously, and it only happens when the componentgen change you mention is used [also this patch uses componentgen in clearfat, which it wasn't doing before]
Sign in to reply to this message.
Hello dave@cheney.net, minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
The bug was that mkvar was ignoring strings and slices (unlike 6g/8g) so it wasn't treated as an outvar and they fell into "set but not used" and discarded. Can you try again?
Sign in to reply to this message.
Will do. Thank you On 18 Oct 2012 08:09, <remyoudompheng@gmail.com> wrote: > The bug was that mkvar was ignoring strings and slices (unlike 6g/8g) so > it wasn't treated as an outvar and they fell into "set but not used" and > discarded. > > Can you try again? > > http://codereview.appspot.com/**6710043/<http://codereview.appspot.com/6710043/> >
Sign in to reply to this message.
LGTM. Please note, the following benchmark has CL 6717043 (fast _div) applied in both old and new cases. This is for two reasons, 1. it's got a LGTM, so will be committed soon, 2. I believe without it, the effects of any improvements to codegen are difficult to observe due to the increase of division operations in the malloc codepath. pando(~/go/test/bench/go1) % ./go1.5749e60d2b6b -test.bench=. > old.txt && ./go1.test -test.bench=. > new.txt && ~/go/misc/benchcmp {old,new}.txt testing: warning: no tests to run testing: warning: no tests to run benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 45597656000 45800415000 +0.44% BenchmarkFannkuch11 34541229000 33243561000 -3.76% BenchmarkGobDecode 117576600 113871750 -3.15% BenchmarkGobEncode 65834340 57223500 -13.08% BenchmarkGzip 6355713000 5444794000 -14.33% BenchmarkGunzip 1197907000 1063690000 -11.20% BenchmarkJSONEncode 799755800 658972200 -17.60% BenchmarkJSONDecode 1728577000 1597717000 -7.57% BenchmarkMandelbrot200 45700680 45696420 -0.01% BenchmarkParse 59507440 57044060 -4.14% BenchmarkRevcomp 132467600 128094500 -3.30% BenchmarkTemplate 1848328000 1707550000 -7.62% benchmark old MB/s new MB/s speedup BenchmarkGobDecode 6.53 6.74 1.03x BenchmarkGobEncode 11.66 13.41 1.15x BenchmarkGzip 3.05 3.56 1.17x BenchmarkGunzip 16.20 18.24 1.13x BenchmarkJSONEncode 2.43 2.94 1.21x BenchmarkJSONDecode 1.12 1.21 1.08x BenchmarkParse 0.97 1.02 1.05x BenchmarkRevcomp 19.19 19.84 1.03x BenchmarkTemplate 1.05 1.14 1.09x
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
http://codereview.appspot.com/6710043/diff/9001/src/cmd/5g/reg.c File src/cmd/5g/reg.c (right): http://codereview.appspot.com/6710043/diff/9001/src/cmd/5g/reg.c#newcode989 src/cmd/5g/reg.c:989: case TFUNC: comparing to 6g, this looks correct.
Sign in to reply to this message.
ping any opinion on where there is going?
Sign in to reply to this message.
Still LGTM. On Fri, Oct 26, 2012 at 5:41 PM, <remyoudompheng@gmail.com> wrote: > ping > any opinion on where there is going? > > http://codereview.appspot.com/6710043/
Sign in to reply to this message.
I'll submit this tomorrow unless there are remarks.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=8e87cb8dca7d *** cmd/5g: introduce componentgen for better registerization. It is essentially identical to the version in 6g. R=dave, minux.ma, rsc CC=golang-dev http://codereview.appspot.com/6710043
Sign in to reply to this message.
Message was sent while issue was closed.
Though strange, I just determined that this change is responsible for a test failure of Go on Darwin/ARM. Without this change, fixedbugs/bug348.go could get this traceback (I just printed out pc, file, line from bug348.go): 8488 /var/mobile/go/src/bug348.go 28 59468 /var/mobile/go/src/pkg/runtime/panic.c 134 61048 /var/mobile/go/src/pkg/runtime/panic.c 384 87056 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431 8428 /var/mobile/go/src/bug348.go 47 8232 /var/mobile/go/src/bug348.go 17 With this change, the traceback becomes: 8472 /var/mobile/go/src/bug348.go 28 59364 /var/mobile/go/src/pkg/runtime/panic.c 134 60944 /var/mobile/go/src/pkg/runtime/panic.c 384 86952 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431 8412 /var/mobile/go/src/bug348.go 47 8224 /var/mobile/go/src/bug348.go 15 BUG: bug348: panic at /var/mobile/go/src/bug348.go:15 in main.f Remy, do you have any ideas why this change could affect the pc->line number mapping? Thank you. The weird thing is that it doesn't affect Linux/ARM, only Darwin/ARM. (The port is here: http://bitbucket.org/minux/goios/, it doesn't make changes to 5g)
Sign in to reply to this message.
Are you sync'ed past http://code.google.com/p/go/source/detail?r=0f491e663a44?
Sign in to reply to this message.
On Wednesday, December 19, 2012, Russ Cox wrote: > Are you sync'ed past > http://code.google.com/p/go/source/detail?r=0f491e663a44? > yes. i just merged default branch of today, noticed the problem, and backtracked to this cl. 5g -S shows the line number is correct with this cl, but the only factor changed is just 5g, so i'm pretty confused.
Sign in to reply to this message.
Can you post the 5g -S output? I don't have a working 5g here.
Sign in to reply to this message.
On Wednesday, December 19, 2012, minux wrote: > > On Wednesday, December 19, 2012, Russ Cox wrote: > >> Are you sync'ed past >> http://code.google.com/p/go/source/detail?r=0f491e663a44? >> > yes. i just merged default branch of today, noticed the problem, > and backtracked to this cl. > > 5g -S shows the line number is correct with this cl, but the only factor > changed is just 5g, > so i'm pretty confused. > the pc -> line number conversion code seems to have a off-by-one error only on darwin/arm. bad.s --- prog list "f" --- 0000 (bug348.go:14) TEXT f+0(SB),$36-0 0001 (bug348.go:15) MOVW $0,R0 0002 (bug348.go:17) MOVW 0(R0),R1. // segfault happens here 0003 (bug348.go:17) MOVW R1,autotmp_0000+-16(SP) 0004 (bug348.go:17) MOVW 4(R0),R1 0005 (bug348.go:17) MOVW R1,autotmp_0000+-12(SP) 0006 (bug348.go:17) MOVW $0,R2 0007 (bug348.go:17) MOVW R2,autotmp_0001+-8(SP) good.s: --- prog list "f" --- 0000 (bug348.go:14) TEXT f+0(SB),$36-0 0001 (bug348.go:15) MOVW $0,R2 0002 (bug348.go:17) MOVW R2,R1 0003 (bug348.go:17) MOVW $autotmp_0000+-16(SP),R0 0004 (bug348.go:17) MOVW.P 4(R1),R2. // sigfault happens here 0005 (bug348.go:17) MOVW.P R2,4(R0) 0006 (bug348.go:17) MOVW.P 4(R1),R2 0007 (bug348.go:17) MOVW.P R2,4(R0) i can confirm the code generated for linux/arm is the same, so this cl only exposes the error in runtime. however, i don't remember i touched pcln code.....
Sign in to reply to this message.
Normally the return PC for a call leads to the instruction after the call, so the traceback code moves backward one instruction, in order to print the line of the call instead of the line of the code that follows the call. A signal like this is made to look like a call returning to the signaling instruction, so that if it did get handled and the signal handler returned, the instruction would be given a second chance to execute. If the traceback code shows the line of the previous instruction instead of the line of the actual instruction, that would explain the behavior you are seeing. runtime/traceback_arm.c says if(runtime·showframe(f)) { // Print during crash. // main(0x1, 0x2, 0x3) // /home/rsc/go/src/runtime/x.go:23 +0xf tracepc = pc; // back up to CALL instruction for funcline. if(n > 0 && pc > f->entry && !waspanic) tracepc -= sizeof(uintptr); It sounds like maybe in your case waspanic is 0 here but should be 1. waspanic is set during the previous frame, as waspanic = f->entry == (uintptr)runtime·sigpanic. That's the thread_darwin.c:431 frame have on your stack, but it is strange that there is a bug348.go:47 frame between thread_darwin.c:431 and bug348.go:15/17. Why does the panic appear to have happened in main.main called from main.f, instead of the other way around? That is: 8488 /var/mobile/go/src/bug348.go 28 59468 /var/mobile/go/src/pkg/runtime/panic.c 134 61048 /var/mobile/go/src/pkg/runtime/panic.c 384 87056 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431 8428 /var/mobile/go/src/bug348.go 47 <<< 8232 /var/mobile/go/src/bug348.go 17 <<< The two <<< lines seem backward to me. If you can arrange for them to end up in the right order then I think everything will start working. I expect the bug is in runtime.sighandler in your signal_darwin_arm.c. Russ
Sign in to reply to this message.
thank you for your detailed explanation. On Wednesday, December 19, 2012, Russ Cox wrote: > Normally the return PC for a call leads to the instruction after the > call, so the traceback code moves backward one instruction, in order > to print the line of the call instead of the line of the code that > follows the call. A signal like this is made to look like a call > returning to the signaling instruction, so that if it did get handled > and the signal handler returned, the instruction would be given a > second chance to execute. If the traceback code shows the line of the > previous instruction instead of the line of the actual instruction, > that would explain the behavior you are seeing. > > runtime/traceback_arm.c says > > if(runtime·showframe(f)) { > // Print during crash. > // main(0x1, 0x2, 0x3) > // > /home/rsc/go/src/runtime/x.go:23 +0xf > tracepc = pc; // back up to CALL > instruction for funcline. > if(n > 0 && pc > f->entry && !waspanic) > tracepc -= sizeof(uintptr); > > It sounds like maybe in your case waspanic is 0 here but should be 1. > > waspanic is set during the previous frame, as waspanic = f->entry == > (uintptr)runtime·sigpanic. That's the thread_darwin.c:431 frame have > on your stack, but it is strange that there is a bug348.go:47 frame > between thread_darwin.c:431 and bug348.go:15/17. Why does the panic > appear to have happened in main.main called from main.f, instead of > the other way around? > > That is: > 8488 /var/mobile/go/src/bug348.go 28 > 59468 /var/mobile/go/src/pkg/runtime/panic.c 134 > 61048 /var/mobile/go/src/pkg/runtime/panic.c 384 > 87056 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431 > 8428 /var/mobile/go/src/bug348.go 47 <<< > 8232 /var/mobile/go/src/bug348.go 17 <<< > > The two <<< lines seem backward to me. If you can arrange for them to > end up in the right order then I think everything will start working. > I expect the bug is in runtime.sighandler in your signal_darwin_arm.c. i figure out the stupid error i made in the signal handler. thank you russ for pointing me directly to the error. thank you remy for exposing the error. i will make a test to catch this kind of error in the future.
Sign in to reply to this message.
|