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: variable read should not be optimized when using -gcflags="all=-N -l" #29977

Open
dlsniper opened this issue Jan 29, 2019 · 3 comments

Comments

@dlsniper
Copy link
Contributor

commented Jan 29, 2019

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

$ go version
go version go1.11.4 windows/amd64
go version go1.12beta2 windows/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
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\florin\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=D:\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Go1.12
set GOTMPDIR=
set GOTOOLDIR=C:\Go1.12\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=D:\awesomeProject9\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\florin\AppData\Local\Temp\go-build732947326=/tmp/go-build -gno-record-gcc-switches

What did you do?

  • create a project using Go Modules support and the code from https://play.golang.org/p/nnDfvOVbWk9
  • run go-delve/delve@b0056eb dlv debug blah.go
  • place a breakpoint on line 22
  • run the application and make a request with curl -i http://localhost:8080/
  • run set returnStatus = 404 in delve then resume the application

What did you expect to see?

The debugger should be able to change the value of the function parameter before the call happens.

What did you see instead?

The debugger was unable to change the value and the old value was used.

My expectation is that if I compile the application using -gcflags="all=-N -l" such optimization is removed and I can do what I need to do with my program to debug it.

I also noticed that if I swap lines 22 and 23 and keep the breakpoint on line 22 then the result is the expected one. So the optimization applied here is not consistently applied either (should I open a separate bug for it?).

According to @aarzilli in the original issue, go-delve/delve#1473 (comment):

that line compiles to:

  blah.go:22		0x750931		488b8424e0000000		MOVQ 0xe0(SP), AX		
  blah.go:22		0x750939		8400				TESTB AL, 0(AX)			
  blah.go:22		0x75093b		488b4028			MOVQ 0x28(AX), AX		
  blah.go:22		0x75093f		488b8c24e8000000		MOVQ 0xe8(SP), CX		
  blah.go:22		0x750947		48890c24			MOVQ CX, 0(SP)			
  blah.go:22		0x75094b		48c7442408c8000000		MOVQ $0xc8, 0x8(SP)		
  blah.go:22		0x750954		ffd0				CALL AX				

the instruction of interest is 0x75094b which sets one of the function arguments directly to the constant 0xc8 (200). The compiler noticed that the value of the variable is constant and optimized the read away.

@dlsniper dlsniper changed the title cmd/compile: variable read should not be optimized hen using -gcflags="all=-N -l" cmd/compile: variable read should not be optimized when using -gcflags="all=-N -l" Jan 29, 2019

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

This looks like it's a consequence of the fact that we haven't separated out required rewrites from optional rewrites in the opt passes. When I explicitly mark the opt and late opt compiler passes as not required, that fixes the problem.

There are only a few required rewrite rules; they shouldn't be hard to split out.

There's even a comment in the code:

	{name: "opt", fn: opt, required: true},              // TODO: split required rules and optimizing rules

@randall77 randall77 added this to the Go1.13 milestone Jan 29, 2019

@randall77 randall77 added the Debugging label Jan 29, 2019

@dlsniper

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@randall77 I do have a question for you. Why is it that when swapping the lines 22 and 23 then this doesn't happen? And, perhaps the bigger question, why doesn't this optimization apply to line 23 in the original code as well? I would expect the behavior to be consistently applied for function calls, but it seems it's not? Is it related to the parameter type that the function expects?

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Why is it that when swapping the lines 22 and 23 then this doesn't happen?

This is the difference between:

v := 5
f(v)
g()

and

v := 5
g()
f(v)

In the former, a simple rewrite rule can follow the flow of "5" to the argument slot of f. In the latter, it requires knowing that g does not modify v. That later knowledge is turned off with -N.

And, perhaps the bigger question, why doesn't this optimization apply to line 23 in the original code as well? I would expect the behavior to be consistently applied for function calls, but it seems it's not? Is it related to the parameter type that the function expects?

This is because Sprintf has a ... argument slot. With -N the backing array for the ... slice is always allocated on the heap, and the newobject call involved in that acts as the g above.

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