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: removing deadcode check for reflect.Value.Call causes miscompilation #38515

Closed
mdempsky opened this issue Apr 18, 2020 · 23 comments
Closed

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Apr 18, 2020

This program now panics at master due to CL 228792:

package main

import "reflect"

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

var h = reflect.Type.Method

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

It should print "ok".

/cc @cherrymui @bradfitz

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

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))})
}
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

I think as a short-term workaround, we should revert CL 228792, and also add a check for reflect.Value.CallSlice, and add these as regress tests.

Longer term, we should figure out how to make the reflection-use analysis more precise without compromising soundness.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

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.

@mvdan
Copy link
Member

@mvdan mvdan commented Apr 18, 2020

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 18, 2020

This program failed even before CL 228792:

@mdempsky, thanks! Nice! That was the program I was trying to write that prompted my golang-dev post.

In playground form: https://play.golang.org/p/8L3UVmShhQf (showing that a Call anywhere else makes it work fine in Go 1.14)

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Apr 18, 2020

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

@mdempsky How about using Type.LongString():

diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go
index 06910450ff..7b9dee746a 100644
--- a/src/cmd/compile/internal/gc/walk.go
+++ b/src/cmd/compile/internal/gc/walk.go
@@ -3657,8 +3657,8 @@ func usemethod(n *Node) {
        }

        // Note: Don't rely on res0.Type.String() since its formatting depends on multiple factors
-       //       (including global variables such as numImports - was issue #19028).
-       if s := res0.Type.Sym; s != nil && s.Name == "Method" && s.Pkg != nil && s.Pkg.Path == "reflect" {
+       //       (including global variables such as numImports - was issue #19028)
+       if s := res0.Type.Sym; s != nil && res0.Type.LongString() == "reflect.Method" {
                Curfn.Func.SetReflectMethod(true)
        }
 }
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 18, 2020

Yeah, actually I was thinking about this the other day. It looks like the compiler only marks direct call to reflect.Type.Method, but not call by function pointers. And reflect.Value.Call doesn't really matter, as you could get a method and do other things with it without actually call it, and the method should be live in this case as well. I'm thinking there could be a different way of doing it.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 18, 2020

Change https://golang.org/cl/228879 mentions this issue: cmd/link: handle indrect call of reflect.Type.Method in deadcode pass

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

@cuonglm I wouldn't think that would fix the test cases. Does it?

@cherrymui I don't think the linker should be involved in checking for interface wrapper methods. There can be many wrappers semantically equivalent to reflect.Type.Method (as a method expression) in different packages, and only the compiler can detect them all correctly.

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Apr 18, 2020

@mdempsky it does, I have a patch in my local, do you want to take a look?

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Apr 18, 2020

@mdempsky it does, I have a patch in my local, do you want to take a look?

Ah, sorry, I haven’t tested with the CallSlice case.

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Apr 18, 2020

@mdempsky it does, I have a patch in my local, do you want to take a look?

Ah, sorry, I haven’t tested with the CallSlice case.

It does pass.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

@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.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 18, 2020

Yeah, I think you're probably right. There could be more method wrappers, like

type I interface { Method(int) reflect.Method }
I.Method

It seems those wrapper functions actually do have REFLECTMETHOD set. Only reflect.Type.Method itself. Maybe we just didn't check the package name correctly, then.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

@cherrymui Yeah, that's what the change I mentioned in #38515 (comment) was meant to address. With that change, reflect.Type.Method gets marked REFLECTMETHOD.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 18, 2020

Yeah, thanks! (I somehow missed that, sorry.) Do you want to send a CL? Or I can do that.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 18, 2020

(With that, the linker change actually makes it work. But it is clearly not the best way to do it.)

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

@cherrymui No problem, and either way works for me. I probably won't get to my desktop to create a CL until later today. Happy if you beat me to it. :)

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 18, 2020

Okay, I can do that. Thanks!

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 18, 2020

Just wanted to mention that, as @rsc pointed out in the email thread, Call or CallSlice is not important here. You could obtain a method and do things with it, without using Call or CallSlice. If the method is not retained by the linker, bad things will happen. E.g. https://play.golang.org/p/rSIx14FBMNs

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 18, 2020

@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.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 18, 2020

Yeah, I think so. Thanks.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 18, 2020

Change https://golang.org/cl/228880 mentions this issue: cmd/compile: when marking REFLECTMETHOD, check for reflect package itself

@gopherbot gopherbot closed this in a32262d Apr 19, 2020
bradfitz added a commit to tailscale/go that referenced this issue Apr 20, 2020
…self

reflect.Type.Method (and MethodByName) can be used to obtain a
reference of a method by reflection. The linker needs to know
if reflect.Type.Method is called, and retain all exported methods
accordingly. This is handled by the compiler, which marks the
caller of reflect.Type.Method with REFLECTMETHOD attribute. The
current code failed to handle the reflect package itself, so the
method wrapper reflect.Type.Method is not marked. This CL fixes
it.

Fixes golang#38515.

Change-Id: I12904d23eda664cf1794bc3676152f3218fb762b
Reviewed-on: https://go-review.googlesource.com/c/go/+/228880
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.