Go: cmd/compile: function inlining produces incorrect results in certain conditions

[siddharthab]

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]

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]

@gopherbot please backport to 1.12.

[gopherbot]

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]

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]

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

[mvdan]

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

[josharian]

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]

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

[josharian]

Thanks. That seems way more plausible. @randall77

[randall77]

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]

gccgo correctly prints 1 then 2.

[randall77]

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]

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