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.

Sign up for GitHub

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

runtime, cmd/compile: add getclosureptr intrinsic #21258

Closed
josharian opened this issue Aug 1, 2017 · 6 comments
Closed

runtime, cmd/compile: add getclosureptr intrinsic #21258

josharian opened this issue Aug 1, 2017 · 6 comments
Labels
FrozenDueToAge
Milestone
Go1.10

Comments

@josharian
Copy link
Contributor

I suggest we add runtime.getclosureptr as a compiler intrinsic. It should be somewhat easy to implement, since all it is doing is identifying a register. Then we could use it to implement memhash_varlen in plain Go, rather than assembly.

Fairly low priority, but might be an interesting (if slightly non-trivial) starter project for someone who wants to learn a bit about the compiler.
@josharian josharian added this to the Unplanned milestone Aug 1, 2017
@choleraehyq
Copy link
Contributor

Hi, if no one is working on this, I'd like to take it. But I think I need some guides...
I've seen the implementation of memhash_varlen of amd64:

// memhash_varlen(p unsafe.Pointer, h seed) uintptr
// redirects to memhash(p, h, size) using the size
// stored in the closure.
TEXT runtime·memhash_varlen(SB),NOSPLIT,$32-24
	GO_ARGS
	NO_LOCAL_POINTERS
	MOVQ	p+0(FP), AX
	MOVQ	h+8(FP), BX
	MOVQ	8(DX), CX
	MOVQ	AX, 0(SP)
	MOVQ	BX, 8(SP)
	MOVQ	CX, 16(SP)
	CALL	runtime·memhash(SB)
	MOVQ	24(SP), AX
	MOVQ	AX, ret+16(FP)
	RET

It seems that the size store in closure can be accessed by DX+8. But as far as I know, closure will be represented by a struct contained a function pointer and all catched variables, why can we access the size store in closure by DX+8, and what is stored in DX?

@dr2chase
Copy link
Contributor

dr2chase commented Aug 2, 2017

You could probably use this CL as a model.
https://go-review.googlesource.com/c/31851
If you look, you'll see that the ssa backend already has an opcode for getting at the closure pointer, so it might be as simple as adding a stub and an intrinsic.

@josharian
Copy link
Contributor Author

The Go 1.1 Function Calls Design Doc should help explain the DX+8.

@choleraehyq
Copy link
Contributor

After I wrote everything like https://go-review.googlesource.com/c/31851, I changed memhash_varlen's implementation into pure go,

func memhash_varlen(p unsafe.Pointer, h uintptr) uintptr {
	return memhash(p, h, *(*uintptr)(unsafe.Pointer(getclosureptr(unsafe.Pointer(&p)) + unsafe.Sizeof(h))))
}

Then I run ./all.bash, I can compile all packages successfully, but some tests failed. Error messages are like:

crypto/x509/example_test.go:115:2: internal compiler error: panic during prove while compiling ExampleParsePKIXPublicKey:

runtime error: invalid memory address or nil pointer dereference

goroutine 18 [running]:
cmd/compile/internal/ssa.Compile.func1(0xc420755280, 0xc42079a000)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/compile.go:38 +0xc8
panic(0xb02ee0, 0xe86150)
	/home/cholerae/test/go/src/runtime/panic.go:491 +0x283
cmd/compile/internal/ssa.(*factsTable).get(0xc420751428, 0xc42062f4f8, 0xc42062f568, 0x4, 0x1)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/prove.go:190 +0x381
cmd/compile/internal/ssa.simplifyBlock(0xc420751428, 0xc420664878, 0x29)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/prove.go:712 +0x386
cmd/compile/internal/ssa.prove(0xc42079a000)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/prove.go:542 +0xafe
cmd/compile/internal/ssa.Compile(0xc42079a000)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/compile.go:70 +0x2bb
cmd/compile/internal/gc.buildssa(0xc4205cc160, 0x1, 0x0)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/ssa.go:212 +0xd89
cmd/compile/internal/gc.compileSSA(0xc4205cc160, 0x1)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/pgen.go:240 +0x3c
cmd/compile/internal/gc.compileFunctions.func2(0xc42072b6e0, 0xc420017b20, 0x1)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/pgen.go:289 +0x49
created by cmd/compile/internal/gc.compileFunctions
	/home/cholerae/test/go/src/cmd/compile/internal/gc/pgen.go:287 +0x10e



goroutine 18 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
	/home/cholerae/test/go/src/runtime/debug/stack.go:24 +0xa7
cmd/compile/internal/gc.Fatalf(0xb6d65e, 0x2c, 0xc4202def40, 0x4, 0x4)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/subr.go:181 +0x230
cmd/compile/internal/gc.(*ssafn).Fatalf(0xc420798000, 0x730200000002, 0xb6d65e, 0x2c, 0xc4202def40, 0x4, 0x4)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/ssa.go:5045 +0x67
cmd/compile/internal/ssa.(*Func).Fatalf(0xc42079a000, 0xb6d65e, 0x2c, 0xc4202def40, 0x4, 0x4)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/func.go:496 +0x78
cmd/compile/internal/ssa.Compile.func1(0xc420755280, 0xc42079a000)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/compile.go:40 +0x250
panic(0xb02ee0, 0xe86150)
	/home/cholerae/test/go/src/runtime/panic.go:491 +0x283
cmd/compile/internal/ssa.(*factsTable).get(0xc420751428, 0xc42062f4f8, 0xc42062f568, 0x4, 0x1)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/prove.go:190 +0x381
cmd/compile/internal/ssa.simplifyBlock(0xc420751428, 0xc420664878, 0x29)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/prove.go:712 +0x386
cmd/compile/internal/ssa.prove(0xc42079a000)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/prove.go:542 +0xafe
cmd/compile/internal/ssa.Compile(0xc42079a000)
	/home/cholerae/test/go/src/cmd/compile/internal/ssa/compile.go:70 +0x2bb
cmd/compile/internal/gc.buildssa(0xc4205cc160, 0x1, 0x0)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/ssa.go:212 +0xd89
cmd/compile/internal/gc.compileSSA(0xc4205cc160, 0x1)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/pgen.go:240 +0x3c
cmd/compile/internal/gc.compileFunctions.func2(0xc42072b6e0, 0xc420017b20, 0x1)
	/home/cholerae/test/go/src/cmd/compile/internal/gc/pgen.go:289 +0x49
created by cmd/compile/internal/gc.compileFunctions
	/home/cholerae/test/go/src/cmd/compile/internal/gc/pgen.go:287 +0x10e

Actually I don't have much experience dealing with compiler bug, but I think there must be some wrongs with my memhash_varlen implementation...What's wrong with memhash_varlen?

@josharian
Copy link
Contributor Author

It would help us help you if you pushed up a change so we can see what you did. Some observations, though:

  • getclosureptr shouldn't take an argument
  • you can add debugging to memhashvarlen, like: if pointer1 == nil { throw("oops") }, so you can find problems most directly; you can also add logging to memhashvarlen using println("etc")
  • the closure pointer is only well-defined at the beginning of the function, since that register can get re-used, so you might need to make sure getclosureptr is scheduled early (see schedule.go)

But without being able to look at your actual changes, all I can do is guess.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/53411 mentions this issue: cmd/compile: add intrinsic getclosureptr

@bradfitz bradfitz modified the milestones: Go1.10, Unplanned Aug 9, 2017
@golang golang locked and limited conversation to collaborators Aug 11, 2018
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
Go1.10
Development

No branches or pull requests
5 participants
@bradfitz @josharian @dr2chase @gopherbot @choleraehyq