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

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

cmd/compile: variable read should not be optimized when using -gcflags="all=-N -l" #29977

dlsniper opened this issue Jan 29, 2019 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@dlsniper
Copy link
Contributor

dlsniper commented Jan 29, 2019 •
edited

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
Copy link
Contributor

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
@dlsniper
Copy link
Contributor Author

dlsniper commented Jan 31, 2019 •
edited

@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
Copy link
Contributor

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.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 1, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@aarzilli aarzilli mentioned this issue Jul 28, 2019
Closed
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@dwneumann
Copy link

using go version go1.15.3 linux/amd64 this bug still exists and prevents assignments to variables in delve.
The thread above mentions "When I explicitly mark the opt and late opt compiler passes as not required, that fixes the problem."
What is the syntax to allow users to do that marking of compiler passes not required... until this bug is fixed?
The attached file reproduces the problem.
rrm.go.txt

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @andybons @dlsniper @bcmills @randall77 @gopherbot @dwneumann