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/link: missing arg stack map for functions defined in different package #24419

Closed
cherrymui opened this issue Mar 16, 2018 · 10 comments

Comments

Projects
None yet
6 participants
@cherrymui
Copy link
Contributor

commented Mar 16, 2018

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

tip (86a3389)

Does this issue reproduce with the latest release?

yes

What did you do?

https://play.golang.org/p/4-OeiC68PO4

package main

import "bytes"

func growstack(n int) {
	if n > 0 {
		growstack(n - 1)
	}
}

func main() {
	var x, y []byte
	defer bytes.Compare(x, y) // on AMD64 bytes.Compare is implemented in internal/bytealg
	growstack(10000)          // trigger a stack grow
}
$ go run b.go
runtime: frame bytes.Compare untyped args 824634277936+56
fatal error: missing stackmap

runtime stack:
runtime.throw(0x1073aff, 0x10)
	/Users/cherryyz/src/go-tip/src/runtime/panic.go:598 +0x72
runtime.adjustframe(0x7ffeefbff518, 0x7ffeefbff630, 0x10b3be0)
	/Users/cherryyz/src/go-tip/src/runtime/stack.go:698 +0x578
runtime.tracebackdefers(0xc000000180, 0x1077300, 0x7ffeefbff630)
	/Users/cherryyz/src/go-tip/src/runtime/traceback.go:73 +0xda
runtime.adjustdefers(0xc000000180, 0x7ffeefbff630)
	/Users/cherryyz/src/go-tip/src/runtime/stack.go:733 +0x45
runtime.copystack(0xc000000180, 0x4000, 0x10c5f01)
	/Users/cherryyz/src/go-tip/src/runtime/stack.go:878 +0x17e
runtime.newstack()
	/Users/cherryyz/src/go-tip/src/runtime/stack.go:1063 +0x30e
runtime.morestack()
	/Users/cherryyz/src/go-tip/src/runtime/asm_amd64.s:481 +0x8f

The stack map of bytes.Compare is missing.

The compiler does generate the stack map

$ go build -gcflags -S bytes
...
"".Compare.args_stackmap SRODATA size=12
        0x0000 02 00 00 00 0e 00 00 00 09 00 09 00              ............
...

But it doesn't make into the binary. Apparently the linker dropped it on the floor?

This doesn't seem to be the case for assembly function defined in the same package.

@cherrymui cherrymui added this to the Go1.11 milestone Mar 16, 2018

@cherrymui

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

Ok, for normal assembly functions, the obj package will insert a FUNCDATA which refers to the args_stackmap generated by the compiler from the Go declaration.
https://go.googlesource.com/go/+/master/src/cmd/internal/obj/plist.go#78
But this is only done for functions whose name starts with ""., which means, defined in the same package.

Maybe we can make the obj package also insert FUNCDATA for functions not defined in the same package as the declaration? Or the assembly writer is expected to write a GO_ARGS pseudo-instruction (https://go.googlesource.com/go/+/master/src/runtime/funcdata.h#34) for this kind of functions?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

I'd rather avoid the GO_ARGS solution. We can certainly do that for internal/bytealg, but this problem would occur for anyone writing an assembly routine that is exported. I don't want them writing a GO_ARGS declaration when we have all the information we need to construct it for them.

What happens if we just remove the "". restriction? Does everything still work? How much extra linker work is it? Extra binary size?

Maybe we could restrict to functions which have a declaration but no body. I'm not sure whether we have that bit in the data exported from a package, though.

@cherrymui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

What happens if we just remove the "". restriction? Does everything still work?

This doesn't work, since not all assembly functions have Go declarations. For them, it will result in undefined relocation targets at link time. I tried restricting to only functions whose name has a ".", but still some functions don't have Go declarations. Maybe we can establish a convention that if a function doesn't have Go declaration (therefore cannot be called from Go), it should not have "." (written as "·" in assembly source code) in its name? We could fix up the exceptions in std, but not sure what to do with code outside our repo.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

Can we restrict to exported functions? I think it would be reasonable to require that exported assembly functions have Go declarations. In fact, I don't think it would work otherwise (unless assembly was calling assembly?).

@aclements

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

I'd rather avoid the GO_ARGS solution. We can certainly do that for internal/bytealg, but this problem would occur for anyone writing an assembly routine that is exported.

I may be misunderstanding, but doesn't this also require that the assembly routine be implemented in a different package than its Go declaration? I'm okay with saying that end users don't get to do that.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

@aclements Yes, I guess that's true.
In any case, I have a patch that restricts to exported functions, and it seems to work. I'll mail it in a sec.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 6, 2018

Change https://golang.org/cl/122519 mentions this issue: cmd/compile: use Go declaration to make GO_ARGS for assembly functions.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

My patch didn't quite work, due to shared libraries.
When building runtime as a shared library, that shared library actually includes the implementations of bytes.IndexByte, for example. That implementation now references bytes.IndexByte.args_stackmap as its args pointer map funcdata. But that symbol doesn't exist when compiling runtime, and won't exist if you never use the bytes package.

Adding something like:

FUNCDATA $0, ·IndexByte·args_stackmap(SB)

to the bytes.IndexByte assembly fixes things. Of course, that's probably the original GO_ARGS fix Cherry suggested.
I guess that's good enough to fix this bug, I'll whip up a CL.
There's the remaining question of whether my CL 122519 should be submitted. I think it might still be a good fix for people deferring exported assembly functions which don't have package "". (Presumably people use pkg·Func as well as ·Func to declare their assembly functions.)

@gopherbot

This comment has been minimized.

Copy link

commented Jul 9, 2018

Change https://golang.org/cl/122676 mentions this issue: internal/bytealg: specify argmaps for exported functions

@gopherbot gopherbot closed this in be9c994 Jul 10, 2018

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