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: REFLECTMETHOD attribute is not set correctly #44207

Closed
cherrymui opened this issue Feb 10, 2021 · 2 comments
Closed

cmd/compile: REFLECTMETHOD attribute is not set correctly #44207

cherrymui opened this issue Feb 10, 2021 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@cherrymui
Copy link
Member

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

tip (e9c9683)

Does this issue reproduce with the latest release?

Not in Go 1.15.

What operating system and processor architecture are you using (go env)?

darwin/amd64

What did you do?

Run this program.

package main

import "reflect"

type S int

func (s S) M() { println(s) }

func main() {
	t := reflect.TypeOf(S(0))
	fn, _ := reflect.PtrTo(t).MethodByName("M")
	fn.Func.Call([]reflect.Value{reflect.New(t)})
}

What did you expect to see?

Print 0.

What did you see instead?

Crash.

unexpected fault address 0xffffffffffffffff
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x1 addr=0xffffffffffffffff pc=0xffffffffffffffff]

goroutine 1 [running]:
runtime.throw(0x109ea7c, 0x5)
	/Users/cherryyz/src/go-tip/src/runtime/panic.go:1117 +0x72 fp=0xc000092b50 sp=0xc000092b20 pc=0x1030b12
runtime.sigpanic()
	/Users/cherryyz/src/go-tip/src/runtime/signal_unix.go:741 +0x276 fp=0xc000092b88 sp=0xc000092b50 pc=0x10458f6
runtime.call16(0xc000098180, 0xc0000ae018, 0xc0000ae030, 0x800000008)
	/Users/cherryyz/src/go-tip/src/runtime/asm_amd64.s:550 +0x3e fp=0xc000092ba8 sp=0xc000092b88 pc=0x105ffbe
reflect.Value.call(0xc0000aa000, 0xc0000ae018, 0x13, 0x109e902, 0x4, 0xc000092ec0, 0x1, 0x1, 0xc000092e20, 0x107c50b, ...)
	/Users/cherryyz/src/go-tip/src/reflect/value.go:476 +0x8e7 fp=0xc000092db0 sp=0xc000092ba8 pc=0x1079ee7
reflect.Value.Call(0xc0000aa000, 0xc0000ae018, 0x13, 0xc000092ec0, 0x1, 0x1, 0x0, 0x10bb440, 0xc0000aa000)
	/Users/cherryyz/src/go-tip/src/reflect/value.go:337 +0xb9 fp=0xc000092e30 sp=0xc000092db0 pc=0x1079539
main.main()
	/tmp/r.go:12 +0x1d6 fp=0xc000092f88 sp=0xc000092e30 pc=0x107fbb6
runtime.main()
	/Users/cherryyz/src/go-tip/src/runtime/proc.go:225 +0x256 fp=0xc000092fe0 sp=0xc000092f88 pc=0x1033336
runtime.goexit()
	/Users/cherryyz/src/go-tip/src/runtime/asm_amd64.s:1371 +0x1 fp=0xc000092fe8 sp=0xc000092fe0 pc=0x10618a1

What happens is that the main function used to (as in Go 1.15) have REFLECTMETHOD attribute set, so the linker knows it and marks all exported methods, but not now.

        0x0000 00000 (r.go:9)   TEXT    "".main(SB), ABIInternal, $336-0

(tip) vs.

        0x0000 00000 (r.go:9)   TEXT    "".main(SB), REFLECTMETHOD|ABIInternal, $256-0

(1.15)

It is because in cmd/compile/internal/gc/walk.go, it only marks reflect.Type.Method/MethodByName as interface calls (i.e. expecting a CALLINTER). But now it devirtualizes to a concrete call (CALLMETH).

.   AS2FUNC l(11) colas(true) tc(1)
.   .   CALLMETH l(11) tc(1) STRUCT-(reflect.Method, bool)
.   .   .   DOTMETH l(11) tc(1) reflect.(*rtype).MethodByName FUNC-method(*reflect.rtype) func(string) (reflect.Method, bool)
.   .   .   .   DOTTYPE l(11) tc(1) PTR-*reflect.rtype
.   .   .   .   .   CONVNOP l(11) tc(1) hascall reflect.Type
.   .   .   .   .   .   NAME-main.~R0 l(11) x(0) class(PAUTO) esc(no) tc(1) used reflect.Type
.   .   CALLMETH-list
.   .   .   LITERAL-"M" l(11) tc(1) string

(tip) vs.

.   AS2FUNC l(11) colas(true) tc(1)
.   .   CALLINTER l(11) tc(1) STRUCT-@0
.   .   .   DOTINTER l(11) x(152) tc(1) main.MethodByName FUNC-@0
.   .   .   .   CONVNOP l(11) tc(1) hascall reflect.Type
.   .   .   .   .   NAME-main.~R0 l(11) x(0) class(PAUTO) esc(no) tc(1) assigned used reflect.Type
.   .   CALLINTER-list
.   .   .   LITERAL-"M" l(11) tc(1) string

(1.15)

Without the REFLECTMETHOD attribute set, the linker prunes methods more aggressively. In this case, S.M is discarded.

@cherrymui cherrymui added this to the Go1.16 milestone Feb 10, 2021
@mdempsky
Copy link
Member

Nice find.

It looks like just expanding walk to call usemethod for both ODOTINTER and ODOTMETH nodes should work. usemethod doesn't look at the receiver type at all, just the name and signature of the method called.

@gopherbot
Copy link

Change https://golang.org/cl/290950 mentions this issue: cmd/compile: mark concrete call of reflect.(*rtype).Method as REFLECTMETHOD

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 10, 2021
@golang golang locked and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants