Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: incorrect liveness information #19078

Closed
rsc opened this issue Feb 14, 2017 · 9 comments
Closed

cmd/compile: incorrect liveness information #19078

rsc opened this issue Feb 14, 2017 · 9 comments

Comments

@rsc
Copy link
Contributor

rsc commented Feb 14, 2017

This bug is not present in release-branch.go1.8 nor earlier versions of Go. It is the root cause for some failures reported on #19029 (but not the ones using Go 1.7.5).

Here's a source file:

package p

func g()
func h(*error)

func f(err error) error {
	defer g()
	h(&err)
	return err
}

The liveness debugging output begins:

$ go tool compile -live=2 /tmp/xxx.go
/tmp/xxx.go:6: live at entry to f: err
/tmp/xxx.go:6: live at call to newobject: err ~r1
/tmp/xxx.go:6: live at call to writebarrierptr: ~r1 &err
/tmp/xxx.go:7: live at call to deferproc: ~r1 &err
/tmp/xxx.go:8: live at call to h: ~r1 &err
/tmp/xxx.go:9: live at call to deferreturn: ~r1
/tmp/xxx.go:7: live at call to deferreturn: ~r1
liveness: f
bb#0 pred= succ=2,1
00000 (/tmp/xxx.go:6)	TEXT	"".f(SB)
	varkill=err
	live=err
00001 (/tmp/xxx.go:6)	FUNCDATA	$0, "".gcargs·0(SB)
00002 (/tmp/xxx.go:6)	FUNCDATA	$1, "".gclocals·1(SB)
00003 (/tmp/xxx.go:6)	LEAQ	type.error(SB), AX
00004 (/tmp/xxx.go:6)	MOVQ	AX, (SP)
00047 (/tmp/xxx.go:6)	PCDATA	$0, $1
00005 (/tmp/xxx.go:6)	CALL	runtime.newobject(SB)
	live=err,~r1
00006 (/tmp/xxx.go:6)	MOVQ	8(SP), AX
00007 (/tmp/xxx.go:6)	MOVQ	AX, "".&err-8(SP)
	varkill=&err
00008 (/tmp/xxx.go:6)	MOVQ	"".err(FP), CX
	uevar=err
00009 (/tmp/xxx.go:6)	MOVQ	CX, (AX)
00010 (/tmp/xxx.go:6)	MOVL	runtime.writeBarrier(SB), CX
00011 (/tmp/xxx.go:6)	LEAQ	8(AX), DX
00012 (/tmp/xxx.go:6)	TESTL	CX, CX
00013 (/tmp/xxx.go:6)	JNE	$0, 40
end
	varkill=err,&err liveout=err,&err

bb#1 pred=0 succ=3
	uevar=err livein=err,&err
00014 (/tmp/xxx.go:6)	MOVQ	"".err+8(FP), CX
	uevar=err
00015 (/tmp/xxx.go:6)	MOVQ	CX, 8(AX)
end
	liveout=&err

bb#2 pred=0 succ=3
	uevar=err,&err livein=err,&err
00040 (/tmp/xxx.go:6)	MOVQ	DX, (SP)
00041 (/tmp/xxx.go:6)	MOVQ	"".err+8(FP), CX
	uevar=err
00042 (/tmp/xxx.go:6)	MOVQ	CX, 8(SP)
00048 (/tmp/xxx.go:6)	PCDATA	$0, $2
00043 (/tmp/xxx.go:6)	CALL	runtime.writebarrierptr(SB)
	live=~r1,&err
00044 (/tmp/xxx.go:8)	MOVQ	"".&err-8(SP), AX
	uevar=&err
00045 (/tmp/xxx.go:6)	JMP	16
end
	liveout=&err

bb#3 pred=2,1 succ=5,4
	livein=&err
00016 (/tmp/xxx.go:6)	VARDEF	"".~r1+16(FP)
	varkill=~r1
00017 (/tmp/xxx.go:6)	MOVQ	$0, "".~r1+16(FP)
00018 (/tmp/xxx.go:6)	MOVQ	$0, "".~r1+24(FP)
00019 (/tmp/xxx.go:7)	MOVL	$0, (SP)
...

Note that in bb 0~r1 has magically become live between the start of the function and the call to newobject. That's wrong. It is not initialized until bb 3.

My guess is this was introduced by Keith's change to the handling of results in liveness bitmaps, but that's just a guess.

Again, not present in Go 1.8rc3, nor in Go 1.7.5. Those correctly delay ~r1 being live until after it is initialized.

/cc @mdempsky @randall77

@brauner
Copy link

brauner commented Feb 14, 2017

@rsc, cool. Glad that this helps! In summary:

  • Go 1.7 not reproducible
  • Go 1.7.5 not reproducible
  • current Go master (2017-02-14) reproducible
  • Go 1.8.rc3 (currently running)

@josharian
Copy link
Contributor

josharian commented Feb 14, 2017

Bisected to CL 36030, which fixed #18860.

@brauner
Copy link

brauner commented Feb 14, 2017

@rsc, fwiw, also not reproducible on Go 1.8.rc3 for us. So again, this is only reproducible on current Go master (2017-02-14).

@josharian
Copy link
Contributor

This keeps popping up on the issue tracker under various guises. I think it'd be good to prioritize it.

@randall77
Copy link
Contributor

I'm working on it now. The fix is pretty easy. The test is hard.

@mdempsky
Copy link
Contributor

mdempsky commented Mar 9, 2017

@randall77 What's your planned fix? Only add Addrtaken PPARAMOUT variables to livedefer?

@randall77
Copy link
Contributor

My planned fix is to zero results earlier. Currently we walk down the results list once, issuing zeroings and move-to-heaps as we go. I'm changing it to walk down the list twice, all the zeroings first and then all the move-to-heaps after that.

@randall77
Copy link
Contributor

I thought for a while about a more complicated fix, which was to be more precise about when result slots were live. We'd need to mark every instruction that could panic so we could mark all the result slots as live at each of those points. It is a more precise fix but it is also more complicated and fragile.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38133 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants