go: cmd/link: removing deadcode check for reflect.Value.Call causes miscompilation

Yeah, I think so. Thanks.

@cherrymui Thanks for pointing out that example.

Yeah, I don’t think the linker should need to check for Call or CallSlice. I only suggested restoring those checks as a short-term workaround.

I think if we just fix the compiler’s usemethod check so that reflect.Type.Method is marked REFLECTMETHOD, we should be in a good state. Do you agree with that assessment?

Edit: I do think the linker should / needs to continue checking for reflect.Value.Method.

Okay, I can do that. Thanks!

@cuonglm Oh, right, LongString uses package name, not package path.

No, I don’t think that’s a desirable change. It’s not likely to cause problems in practice, but that can lead to false positives. We always want to identify standard library packages by path, not name.

Observation: Changing

var h = reflect.Type.Method

to

var h = (interface { Method(int) reflect.Method }).Method

causes the test cases to pass, even with CL 228792.

It looks like what’s happening is the ReflectMethod check doesn’t work within package reflect itself. While compiling a package, localpkg.Path will be “”, not it’s actual package path (e.g., “reflect”).

Changing s.Pkg.Path == "reflect" to s.Pkg.Path == "reflect" || s.Pkg == localpkg && myimportpath == "reflect" seems like it might work.

This program failed even before CL 228792:

package main

import "reflect"

type foo struct {}
func (foo) X(x ...int) { println("ok") }

var h = reflect.Type.Method

func main() {
	v := reflect.ValueOf(foo{})
	m := h(v.Type(), 0)
	m.Func.CallSlice([]reflect.Value{v, reflect.ValueOf([]int(nil))})
}