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

Comments

Projects
None yet
6 participants
@rsc
Copy link
Contributor

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

Bisected to CL 36030, which fixed #18860.

@brauner

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

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

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

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

@mdempsky

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

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

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

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

This comment has been minimized.

Copy link

commented Mar 13, 2017

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.
You can’t perform that action at this time.