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: function inlining produces incorrect results in certain conditions #30566

Closed
siddharthab opened this issue Mar 4, 2019 · 13 comments

Comments

Projects
None yet
6 participants
@siddharthab
Copy link

commented Mar 4, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="_redacted_"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/_redacted_/go"
GOPROXY=""
GORACE=""
GOROOT="_redacted_"
GOTMPDIR=""
GOTOOLDIR="/Users/_redacted_/.gimme/versions/go1.12.darwin.amd64/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/kc/3s47lffn4550qy8s6hhrs0_r0000gp/T/go-build402200574=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/7xK7ufXbSA6

What did you expect to see?

Actual and expected values in the demo above to match.

What did you see instead?

Results of one function call are being unintentionally overwritten by the results of another function call.

@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Smaller repro:

package main

//go:noinline
func ident(s string) string { return s }

func returnSecond(_ bool, s string) string { return s }

func identWrapper(s string) string { return ident(s) }

func main() {
        got := returnSecond((false || identWrapper("bad") != ""), ident("good"))
        println(got)
}
$ go1.11.5 version
go version go1.11.5 linux/amd64
$ go1.11.5 run f.go
good
$ go1 version
go version go1.12 linux/amd64
$ go1 run f.go
bad
$ go version
go version devel +d1887676d9 Mon Mar 4 09:45:46 2019 +0000 linux/amd64
$ go run f.go
bad

@siddharthab if you could do a bisect, that would be very helpful.

/cc @randall77 @mdempsky @martisch

@mvdan mvdan added this to the Go1.13 milestone Mar 4, 2019

@mvdan mvdan changed the title Go 1.12 function inlining produces incorrect results in certain conditions cmd/compile: function inlining produces incorrect results in certain conditions Mar 4, 2019

@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@gopherbot please backport to 1.12.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 4, 2019

Backport issue(s) opened: #30567 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@siddharthab

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

Here's what I can understand. If you look at the assembly code, you will see that the return value from the non-inlined function call was not saved further down the stack before executing the inline function call.

In the actual code in our code base, the return value was moved to registers but then the execution immediately jumps to the inline function.

I am not familiar with the compiler code base, so can not point to the source of the bug.

go build -o f f.go
$ go tool objdump -S -s main.main f
TEXT main.main(SB) f.go
func main() {
  0x104e5a0		65488b0c2530000000	MOVQ GS:0x30, CX	
  0x104e5a9		483b6110		CMPQ 0x10(CX), SP	
  0x104e5ad		0f8687000000		JBE 0x104e63a		
  0x104e5b3		4883ec38		SUBQ $0x38, SP		
  0x104e5b7		48896c2430		MOVQ BP, 0x30(SP)	
  0x104e5bc		488d6c2430		LEAQ 0x30(SP), BP	
	got := returnSecond((false || identWrapper("bad") != ""), ident("good"))
  0x104e5c1		488d0508e60100		LEAQ go.string.*+240(SB), AX	
  0x104e5c8		48890424		MOVQ AX, 0(SP)			
  0x104e5cc		48c744240804000000	MOVQ $0x4, 0x8(SP)		
  0x104e5d5		e8a6ffffff		CALL main.ident(SB)		
  0x104e5da		90			NOPL				
func identWrapper(s string) string { return ident(s) }
  0x104e5db		488d055ee50100		LEAQ go.string.*+96(SB), AX	
  0x104e5e2		48890424		MOVQ AX, 0(SP)			
  0x104e5e6		48c744240803000000	MOVQ $0x3, 0x8(SP)		
  0x104e5ef		e88cffffff		CALL main.ident(SB)		
	got := returnSecond((false || identWrapper("bad") != ""), ident("good"))
  0x104e5f4		90			NOPL			
func identWrapper(s string) string { return ident(s) }
  0x104e5f5		488b442410		MOVQ 0x10(SP), AX	
  0x104e5fa		4889442428		MOVQ AX, 0x28(SP)	
  0x104e5ff		488b4c2418		MOVQ 0x18(SP), CX	
  0x104e604		48894c2420		MOVQ CX, 0x20(SP)
@marigonzes

This comment has been minimized.

Copy link

commented Mar 4, 2019

I bisected using @mvdan's repro and the first commit that exhibited the bad behavior was 13baf4b.

@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Thanks @marigonzes! Then we know to ping @randall77 @dr2chase.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Hmm. Then the bug may have existed prior to that CL, and that CL only uncovered. Any chance you could re-bisect using -gcflags=-l=4?

@marigonzes

This comment has been minimized.

Copy link

commented Mar 4, 2019

Using -gcflags=-l=4, the bad commit became 389e942.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Thanks. That seems way more plausible. @randall77

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

This appears to be a bug in the order pass. Here's a simple repro:

package main

func main() {
	_, _ = false || g(1), g(2)
}

//go:noinline
func g(x int) bool {
	println(x)
	return false
}

This should print 1 then 2. Instead, it prints 2 then 1.
This has been a bug since at least Go 1.2.

The problem is that order rewrites the code to:

   tmp2 := g(2)
   _, _ = (false || (tmp1 := g(1); tmp1)), tmp2

Order wants to extract all the calls to their own statements so it can ensure the function calls all happen in order. But it can't extract the call from the RHS of a ||, because it might not happen. So instead it uses exprInPlace to keep any introduced assignments inside the ||. I think exprInPlace might be fundamentally broken, but I need to think about it some more.

As far as I can tell, my autotmp reuse code just triggers this underlying bug. There's nothing wrong with the autotmp reuse otherwise.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

gccgo correctly prints 1 then 2.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

In particular, what happens in the OP's code is that the introduced temporaries are incorrectly reused because order processes the code in a different order than the code is emitted.

We first process (false || foo("bad") != "") This introduces a temporary for the result of the call, initializes it with the result of the call, compares it to "", and then issues a VARKILL for the temp.

A. VARDEF tmp
B. tmp = foo("bad")
C. ... = tmp != ""
D. VARKILL tmp

We then process foo("good"). That needs its own temporary. Order decides that it can reuse the temporary used in the first part, as the temporary has been killed. It assigns the result of foo("good") to the temporary, copies that temporary to the final destination and then issues a VARKILL for the temp.

E. VARDEF tmp
F. tmp = foo("good")
G. destination = tmp
H. VARKILL tmp

But then it issues the code out of order. We end up with

E. VARDEF tmp
F. tmp = foo("good")
A. VARDEF tmp
B. tmp = foo("bad")
C. ... = tmp != ""
D. VARKILL tmp
G. destination = tmp
H. VARKILL tmp

Boom, we end up with foo("bad") instead of foo("good") as the result.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 6, 2019

Change https://golang.org/cl/165617 mentions this issue: cmd/compile: fix ordering for short-circuiting ops

@gopherbot gopherbot closed this in 4a9064e Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.