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

@gopherbot please backport to 1.12.

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.

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)

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

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

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?

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

Thanks. That seems way more plausible. @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.

gccgo correctly prints 1 then 2.

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.

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