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: reflect.TypeOf.Method.Func.Interface fails #14740

Closed
ianlancetaylor opened this issue Mar 10, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@ianlancetaylor
Copy link
Contributor

commented Mar 10, 2016

Running this program with Go 1.6 prints My unique method string. Running it with tip causes a crash. The recent CL https://golang.org/cl/20483 is too aggressive; as this program shows, you can call a method without naming it and without using reflect.Value.Call.

package main

import (
    "fmt"
    "reflect"
)

type MyType int

func (m MyType) UniqueMethodName() {
    fmt.Println("My unique method string")
}

var v MyType

func main() {
    reflect.TypeOf(v).Method(0).Func.Interface().(func (MyType))(v)
}

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Mar 10, 2016

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

Thanks for the careful review.

Given that reflect.Type is widely used and reflect.Method is always going to be used inside the reflect package, I think the thing to do here is look to see if the program ever calls an interface method with either of the signatures:

Method(int) reflect.Method
MethodByName(string) (reflect.Method, bool)

and treat the existence of those calls as equivalent to reflect.Value.Call in the linker.

Now, to work out how to send that fact neatly to cmd/link...

@mwhudson

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

reflect.makeMethodValue is also a canary here, but I guess too many things end up referring to that to be useful?

@gopherbot

This comment has been minimized.

Copy link

commented Mar 10, 2016

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2016

For the record, another test case:

package main

import (
    "fmt"
    "reflect"
)

type MyType int

func (m MyType) UniqueMethodName() {
    fmt.Println("My unique method string")
}

var v MyType

func main() {
    reflect.ValueOf(v).Method(0).Interface().(func())()
}
@gopherbot

This comment has been minimized.

Copy link

commented Mar 11, 2016

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

@rasky

This comment has been minimized.

Copy link
Member

commented Mar 11, 2016

This is a test case distilled from #14771 that still crashes after applying the patch in the CL:

package main

import (
    "fmt"
    "reflect"
)

type Foo struct {
    Ptr func()
}

func (f *Foo) Bar() {
    fmt.Println("Hello, world!")
}

func main() {
    f := &Foo{}

    val := reflect.ValueOf(f).Elem()

    meth := val.Addr().MethodByName("Bar")
    valueField := val.FieldByName("Ptr")
    valueField.Set(meth)

    f.Ptr()
}
@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2016

@rasky which CL are you applying? I would expect that case to be caught by cl/20562. (Because the implementation of reflect.Value.MethodByName calls reflect.Value.Method.)

gopherbot pushed a commit that referenced this issue Mar 11, 2016

cmd/compile: track reflect.Type.Method in deadcode
In addition to reflect.Value.Call, exported methods can be invoked
by the Func value in the reflect.Method struct. This CL has the
compiler track what functions get access to a legitimate reflect.Method
struct by looking for interface calls to either of:

	Method(int) reflect.Method
	MethodByName(string) (reflect.Method, bool)

This is a little overly conservative. If a user implements a type
with one of these methods without using the underlying calls on
reflect.Type, the linker will assume the worst and include all
exported methods. But it's cheap.

No change to any of the binary sizes reported in cl/20483.

For #14740

Change-Id: Ie17786395d0453ce0384d8b240ecb043b7726137
Reviewed-on: https://go-review.googlesource.com/20489
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

@gopherbot gopherbot closed this in e283693 Mar 11, 2016

@golang golang locked and limited conversation to collaborators Mar 13, 2017

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