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

Jump to bottom

cmd/compile: incorrect optimizations. #30339

Closed
lintanghui opened this issue Feb 21, 2019 · 5 comments
Closed

cmd/compile: incorrect optimizations. #30339

lintanghui opened this issue Feb 21, 2019 · 5 comments
Labels
FrozenDueToAge

Comments

@lintanghui
Copy link

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

$ go version
go version go1.11 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=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/work/go"
GOPROXY=""
GORACE=""

What did you do?

i write an example code as below and run with go run main.go

package main

import (
	"fmt"
)

var val = "aa"
var gDone = false

func setup() {
	val = "bb"
	fmt.Println("aa")
	gDone = true
	fmt.Println(gDone)
}
func main() {
	go setup()
	var finish = gDone
	for !finish {
		finish = gDone
	}
	fmt.Println(val)
}

What did you expect to see?

should output below and exit.

aa
true
bb

What did you see instead?

only output and never exit

aa
true

when i run this code with go run main.go,it's hang in the for loop and never exit.
instead,i run with go run -gcflags "-N -l" main.go it work ok as what i want to see.
compile this code into assemble with optimizations

	0x0000 00000 (main.go:10)	TEXT	"".setup(SB), $88-0
	0x0000 00000 (main.go:10)	MOVQ	(TLS), CX
	0x0009 00009 (main.go:10)	CMPQ	SP, 16(CX)
	0x000d 00013 (main.go:10)	JLS	250
	0x0013 00019 (main.go:10)	SUBQ	$88, SP
	0x0017 00023 (main.go:10)	MOVQ	BP, 80(SP)
	0x001c 00028 (main.go:10)	LEAQ	80(SP), BP
	0x0021 00033 (main.go:10)	FUNCDATA	$0, gclocals·7d2d5fca80364273fb07d5820a76fef4(SB)
	0x0021 00033 (main.go:10)	FUNCDATA	$1, gclocals·ef22736e31ca7e9ff10587cea75d17ff(SB)
	0x0021 00033 (main.go:10)	FUNCDATA	$3, gclocals·1cf923758aae2e428391d1783fe59973(SB)
	0x0021 00033 (main.go:11)	PCDATA	$2, $0
	0x0021 00033 (main.go:11)	PCDATA	$0, $0
	0x0021 00033 (main.go:11)	MOVQ	$2, "".val+8(SB)
	0x002c 00044 (main.go:11)	PCDATA	$2, $-2
	0x002c 00044 (main.go:11)	PCDATA	$0, $-2
	0x002c 00044 (main.go:11)	CMPL	runtime.writeBarrier(SB), $0
	0x0033 00051 (main.go:11)	JNE	226
	0x0039 00057 (main.go:11)	LEAQ	go.string."bb"(SB), AX
	0x0040 00064 (main.go:11)	MOVQ	AX, "".val(SB)
	0x0047 00071 (main.go:12)	PCDATA	$2, $0
	0x0047 00071 (main.go:12)	PCDATA	$0, $1
	0x0047 00071 (main.go:12)	XORPS	X0, X0
	0x004a 00074 (main.go:12)	MOVUPS	X0, ""..autotmp_0+64(SP)
	0x004f 00079 (main.go:12)	PCDATA	$2, $1
	0x004f 00079 (main.go:12)	LEAQ	type.string(SB), AX
	0x0056 00086 (main.go:12)	PCDATA	$2, $0
	0x0056 00086 (main.go:12)	MOVQ	AX, ""..autotmp_0+64(SP)
	0x005b 00091 (main.go:12)	PCDATA	$2, $1
	0x005b 00091 (main.go:12)	LEAQ	"".statictmp_0(SB), AX
	0x0062 00098 (main.go:12)	PCDATA	$2, $0
	0x0062 00098 (main.go:12)	MOVQ	AX, ""..autotmp_0+72(SP)
	0x0067 00103 (main.go:12)	PCDATA	$2, $1
	0x0067 00103 (main.go:12)	LEAQ	""..autotmp_0+64(SP), AX
	0x006c 00108 (main.go:12)	PCDATA	$2, $0
	0x006c 00108 (main.go:12)	MOVQ	AX, (SP)
	0x0070 00112 (main.go:12)	MOVQ	$1, 8(SP)
	0x0079 00121 (main.go:12)	MOVQ	$1, 16(SP)
	0x0082 00130 (main.go:12)	CALL	fmt.Println(SB)
	0x0087 00135 (main.go:13)	PCDATA	$0, $0
	0x0087 00135 (main.go:13)	MOVB	$1, "".gDone(SB)
	0x008e 00142 (main.go:14)	PCDATA	$0, $2
	0x008e 00142 (main.go:14)	XORPS	X0, X0
	0x0091 00145 (main.go:14)	MOVUPS	X0, ""..autotmp_1+48(SP)
	0x0096 00150 (main.go:14)	MOVBLZX	"".gDone(SB), AX
	0x009d 00157 (main.go:14)	PCDATA	$2, $2
	0x009d 00157 (main.go:14)	LEAQ	type.bool(SB), CX
	0x00a4 00164 (main.go:14)	PCDATA	$2, $0
	0x00a4 00164 (main.go:14)	MOVQ	CX, ""..autotmp_1+48(SP)
	0x00a9 00169 (main.go:14)	PCDATA	$2, $2
	0x00a9 00169 (main.go:14)	LEAQ	runtime.staticbytes(SB), CX
	0x00b0 00176 (main.go:14)	PCDATA	$2, $1
	0x00b0 00176 (main.go:14)	ADDQ	CX, AX
	0x00b3 00179 (main.go:14)	PCDATA	$2, $0
	0x00b3 00179 (main.go:14)	MOVQ	AX, ""..autotmp_1+56(SP)
	0x00b8 00184 (main.go:14)	PCDATA	$2, $1
	0x00b8 00184 (main.go:14)	LEAQ	""..autotmp_1+48(SP), AX
	0x00bd 00189 (main.go:14)	PCDATA	$2, $0
	0x00bd 00189 (main.go:14)	MOVQ	AX, (SP)
	0x00c1 00193 (main.go:14)	MOVQ	$1, 8(SP)
	0x00ca 00202 (main.go:14)	MOVQ	$1, 16(SP)
	0x00d3 00211 (main.go:14)	CALL	fmt.Println(SB)
	0x00d8 00216 (main.go:15)	PCDATA	$0, $0
	0x00d8 00216 (main.go:15)	MOVQ	80(SP), BP
	0x00dd 00221 (main.go:15)	ADDQ	$88, SP
	0x00e1 00225 (main.go:15)	RET
	0x00e2 00226 (main.go:11)	PCDATA	$2, $-2
	0x00e2 00226 (main.go:11)	PCDATA	$0, $-2
	0x00e2 00226 (main.go:11)	LEAQ	"".val(SB), DI
	0x00e9 00233 (main.go:11)	LEAQ	go.string."bb"(SB), AX
	0x00f0 00240 (main.go:11)	CALL	runtime.gcWriteBarrier(SB)
	0x00f5 00245 (main.go:11)	JMP	71
	0x00fa 00250 (main.go:11)	NOP
	0x00fa 00250 (main.go:10)	PCDATA	$0, $-1
	0x00fa 00250 (main.go:10)	PCDATA	$2, $-1
	0x00fa 00250 (main.go:10)	CALL	runtime.morestack_noctxt(SB)
	0x00ff 00255 (main.go:10)	JMP	0
	0x0000 00000 (main.go:16)	TEXT	"".main(SB), $72-0
	0x0000 00000 (main.go:16)	MOVQ	(TLS), CX
	0x0009 00009 (main.go:16)	CMPQ	SP, 16(CX)
	0x000d 00013 (main.go:16)	JLS	167
	0x0013 00019 (main.go:16)	SUBQ	$72, SP
	0x0017 00023 (main.go:16)	MOVQ	BP, 64(SP)
	0x001c 00028 (main.go:16)	LEAQ	64(SP), BP
	0x0021 00033 (main.go:16)	FUNCDATA	$0, gclocals·69c1753bd5f81501d95132d08af04464(SB)
	0x0021 00033 (main.go:16)	FUNCDATA	$1, gclocals·568470801006e5c0dc3947ea998fe279(SB)
	0x0021 00033 (main.go:16)	FUNCDATA	$3, gclocals·1cf923758aae2e428391d1783fe59973(SB)
	0x0021 00033 (main.go:17)	PCDATA	$2, $0
	0x0021 00033 (main.go:17)	PCDATA	$0, $0
	0x0021 00033 (main.go:17)	MOVL	$0, (SP)
	0x0028 00040 (main.go:17)	PCDATA	$2, $1
	0x0028 00040 (main.go:17)	LEAQ	"".setup·f(SB), AX
	0x002f 00047 (main.go:17)	PCDATA	$2, $0
	0x002f 00047 (main.go:17)	MOVQ	AX, 8(SP)
	0x0034 00052 (main.go:17)	CALL	runtime.newproc(SB)
	0x0039 00057 (main.go:18)	PCDATA	$2, $1
	0x0039 00057 (main.go:18)	LEAQ	"".gDone(SB), AX
	0x0040 00064 (main.go:18)	PCDATA	$2, $0
	0x0040 00064 (main.go:18)	CMPB	(AX), $0
	0x0043 00067 (main.go:19)	PCDATA	$2, $-2
	0x0043 00067 (main.go:19)	PCDATA	$0, $-2
	0x0043 00067 (main.go:19)	JEQ	67
	0x0045 00069 (main.go:22)	PCDATA	$2, $0
	0x0045 00069 (main.go:22)	PCDATA	$0, $1
	0x0045 00069 (main.go:22)	XORPS	X0, X0
	0x0048 00072 (main.go:22)	MOVUPS	X0, ""..autotmp_1+48(SP)
	0x004d 00077 (main.go:22)	PCDATA	$2, $1
	0x004d 00077 (main.go:22)	LEAQ	type.string(SB), AX
	0x0054 00084 (main.go:22)	PCDATA	$2, $0
	0x0054 00084 (main.go:22)	MOVQ	AX, (SP)
	0x0058 00088 (main.go:22)	PCDATA	$2, $1
	0x0058 00088 (main.go:22)	LEAQ	"".val(SB), AX
	0x005f 00095 (main.go:22)	PCDATA	$2, $0
	0x005f 00095 (main.go:22)	MOVQ	AX, 8(SP)
	0x0064 00100 (main.go:22)	CALL	runtime.convT2Estring(SB)
	0x0069 00105 (main.go:22)	MOVQ	16(SP), AX
	0x006e 00110 (main.go:22)	PCDATA	$2, $2
	0x006e 00110 (main.go:22)	MOVQ	24(SP), CX
	0x0073 00115 (main.go:22)	MOVQ	AX, ""..autotmp_1+48(SP)
	0x0078 00120 (main.go:22)	PCDATA	$2, $0
	0x0078 00120 (main.go:22)	MOVQ	CX, ""..autotmp_1+56(SP)
	0x007d 00125 (main.go:22)	PCDATA	$2, $1
	0x007d 00125 (main.go:22)	LEAQ	""..autotmp_1+48(SP), AX
	0x0082 00130 (main.go:22)	PCDATA	$2, $0
	0x0082 00130 (main.go:22)	MOVQ	AX, (SP)
	0x0086 00134 (main.go:22)	MOVQ	$1, 8(SP)
	0x008f 00143 (main.go:22)	MOVQ	$1, 16(SP)
	0x0098 00152 (main.go:22)	CALL	fmt.Println(SB)
	0x009d 00157 (main.go:23)	PCDATA	$0, $0
	0x009d 00157 (main.go:23)	MOVQ	64(SP), BP
	0x00a2 00162 (main.go:23)	ADDQ	$72, SP
	0x00a6 00166 (main.go:23)	RET
	0x00a7 00167 (main.go:23)	NOP
	0x00a7 00167 (main.go:16)	PCDATA	$0, $-1
	0x00a7 00167 (main.go:16)	PCDATA	$2, $-1
	0x00a7 00167 (main.go:16)	CALL	runtime.morestack_noctxt(SB)
	0x00ac 00172 (main.go:16)	JMP	0
	rel 28+4 t=29 gofile../work/go/src/examp/main.go+0
	rel 27+4 t=29 gofile../work/go/src/examp/main.go+0

we can see that the for loop is optimize into below. the assign code finish=gDone is optizime so this loop is always true and never exit.

	0x0043 00067 (main.go:19)	PCDATA	$2, $-2
	0x0043 00067 (main.go:19)	PCDATA	$0, $-2
	0x0043 00067 (main.go:19)	JEQ	67
@ALTree
Copy link
Member

ALTree commented Feb 21, 2019

Hi,

your code has a data race on gDone, so it's not a valid Go program. The -race flag can help you diagnose this kind of issues. Run go run -race main.go and you'll see a data race report.

@lintanghui
Copy link
Author

yeah, it's in data race. but if I init finish with var finish =0,it would not be hang. my question is that if the assemble code in optimized mode was expected? with optimized ,finish=gDone is deleted. should don't it be a dirty read even though in data race instead of completely deleted. with optimization, the change of gDone in setup func is never seen by main func.

@mvdan
Copy link
Member

mvdan commented Feb 21, 2019

A program with a data race is an invalid program, and it can behave in unexpected ways. You should just fix the data race. If you think you've found a compiler bug, file an issue with a reproducer that has no data races.

@mvdan mvdan closed this as completed Feb 21, 2019
@ALTree
Copy link
Member

ALTree commented Feb 21, 2019 •
edited

@lintanghui

yeah, it's in data race. but if I init finish with var finish =0,it would not be hang. my question is that if the assemble code in optimized mode was expected?

Since the compiler is allowed to assume that the code does not contain any data race (because if it does, it's not valid code), it's possible that it'll perform optimizations that don't really make sense (and completely break) for racy code.

To sum it up: don't look at the generated code for racy programs and expect it to make sense, because there's no guarantee that it'll do.

@ianlancetaylor
Copy link
Contributor

I think that a perhaps simpler way to describe the problem is to observe that this loop

	for !finish {
		finish = gDone
	}

has no synchronization. Therefore nothing will ever cause this goroutine to observe that gDone has changed.

You seem to be making the assumption that the program should check the value of gDone each time through the loop. But Go doesn't work that way. In Go a goroutine need only see changes to variables made by other goroutines when there is some synchronization step, such as a channel operation.

That is, I wouldn't say that the real problem is that your program is racy (although it is). The real problem is that you are assuming that one goroutine will see changes made by another goroutine without synchronization. That assumption is not valid in Go. For more details see https://golang.org/ref/mem.

@golang golang locked and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ianlancetaylor @mvdan @ALTree @gopherbot @lintanghui