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: asm for results says (SP) not (FP) #19458

Closed
rsc opened this issue Mar 8, 2017 · 4 comments
Closed

cmd/compile: asm for results says (SP) not (FP) #19458

rsc opened this issue Mar 8, 2017 · 4 comments

Comments

@rsc
Copy link
Contributor

rsc commented Mar 8, 2017

$ cat /tmp/x.go
package p

func f(x int) (y int) {
	return x
}
$ go tool compile -S /tmp/x.go
"".f t=1 size=11 args=0x10 locals=0x0
	0x0000 00000 (/tmp/x.go:3)	TEXT	"".f(SB), $0-16
	0x0000 00000 (/tmp/x.go:3)	FUNCDATA	$0, gclocals·f207267fbf96a0178e8758c6e3e0ce28(SB)
	0x0000 00000 (/tmp/x.go:3)	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (/tmp/x.go:4)	MOVQ	"".x+8(FP), AX
	0x0005 00005 (/tmp/x.go:4)	MOVQ	AX, "".y+16(SP)
	0x000a 00010 (/tmp/x.go:4)	RET
	0x0000 48 8b 44 24 08 48 89 44 24 10 c3                 H.D$.H.D$..
$ 

Note that the result is described as y+16(SP) instead of y+16(FP). The use of SP instead of FP is not semantically meaningful in the pseudo-assembly of the -S output (in that output, both use offsets relative to the real hardware SP), but the distinction may matter inside the compiler. I wonder if there are any bugs lurking due to the confusion that causes SP to be printed instead of FP.

Worth checking.

/cc @mdempsky @randall77

@rsc rsc added this to the Go1.9Early milestone Mar 8, 2017
@cherrymui
Copy link
Member

I believe this is related to https://go-review.googlesource.com/c/37255/.

@randall77
Copy link
Contributor

randall77 commented Mar 8, 2017

Yes, probably 37255. I changed the disassembler to print the real base register if it is provided. For the example, the SSA is:

f <nil>
  b1:
    v1 = InitMem <mem>
    v8 = VarDef <mem> {y} v1
    v2 = SP <uintptr> : SP
    v6 = Arg <int> {x} : x[int]
    v5 = LoadReg <int> v6 : AX
    v9 = MOVQstore <mem> {y} v2 v5 v8
    Ret v9

v5 is the load of the input x, and compiles to a move with no base register.
v9 is the output to y, and compiles to a move with the SP base register.

The reason for 37255 is that if the associated symbol is a PPARAM we always printed FP even though the base register was something bad, like CX. So the assembly looked ok and the code was actually bad.

We should probably fix v5 to use the SP base register (by recording the base register explicitly). Then we will have SP as the base throughout.
I think that is the correct end state, as it is confusing to have FP=SP as people might expect to have FP = SP + framesize.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/37971 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38156 mentions this issue.

@golang golang locked and limited conversation to collaborators Mar 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants