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

reflect: can't call methods on StructOf with embedded interface field #38783

Open
mitchellh opened this issue May 1, 2020 · 5 comments
Open

reflect: can't call methods on StructOf with embedded interface field #38783

mitchellh opened this issue May 1, 2020 · 5 comments
Labels
NeedsInvestigation
Milestone

Comments

@mitchellh
Copy link

@mitchellh mitchellh commented May 1, 2020

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

$ go version
go version devel +4c78d54fdd Thu Apr 30 22:06:07 2020 +0000 darwin/amd64

Same behavior with:

$ go version
go version go1.14.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/mitchellh/Library/Caches/go-build"
GOENV="/Users/mitchellh/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY="github.com/mitchellh,github.com/hashicorp"
GONOSUMDB="github.com/mitchellh,github.com/hashicorp"
GOOS="darwin"
GOPATH="/Users/mitchellh/code/go"
GOPRIVATE="github.com/mitchellh,github.com/hashicorp"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.2_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.2_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xj/vc6mm2px1x5745hbvfpxkx5h0000gn/T/go-build768892661=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Use reflect.StructOf with one field with an embedded interface and then attempt to call a method on that interface (with a non-nil value set). https://play.golang.org/p/GwTyV_AFWv6

Note that using an embedded struct (not an interface) works fine: https://play.golang.org/p/jVFT3pcw7ia

What did you expect to see?

The function to be called and the result shown: "42".

Or, if this isn't supported, a panic probably.

What did you see instead?

A segfault.

@mitchellh
Copy link
Author

@mitchellh mitchellh commented May 1, 2020

I'm not sure if this is supposed to be supported or not. I did a bit of digging and this whole method table setup for embedded interface types stood out to me as suspect especially since there is no test coverage going over it, but I'm not familiar enough with the Go internals to really figure this out:

go/src/reflect/type.go

Lines 2466 to 2471 in 4209a9f

methods = append(methods, method{
name: resolveReflectName(ift.nameOff(m.name)),
mtyp: resolveReflectType(mtyp),
ifn: resolveReflectText(unsafe.Pointer(&ifn)),
tfn: resolveReflectText(unsafe.Pointer(&tfn)),
})

In particular, the ifn and tfn pointers are directly to address of reflect.Value values, which doesn't feel right to me.

This may also be related to #16522 but I hope not!

Also maybe related to #15924 because this comment found the same issue: #15924 (comment)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 1, 2020

This is not expected to work at present, but it should panic or fail rather than getting a segmentation fault.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation label May 1, 2020
@ianlancetaylor ianlancetaylor added this to the Backlog milestone May 1, 2020
TheCount pushed a commit to TheCount/go that referenced this issue Jul 3, 2020
DO NOT SUBMIT

INCOMPLETE PATCH

Previously, a call to a StructOf method from an embedded interface field
would cause a segfault, so fix the behaviour to either succeed in
calling the method or, if available method slots have been exhausted,
cause a clean panic.

Updates golang#38783
@TheCount
Copy link

@TheCount TheCount commented Jul 3, 2020

@mitchellh TheCount@b97bc37 contains a fix at least for your particular issue.

What's going on:

  • The previous code used the interface method type to create the implementation with MakeFunc. However, the interface method type has no receiver, so it has to be altered to include the receiver. For that reason, the generation code had to be farther down within StructOf because the receiver type has not yet been constructed in the old location.
  • I think the previous code checked the kindDirectIface flag on the wrong kind of object to determine how the tfn and ifn method members are to be set: it should be checked on the created type. I'm not actually sure if kindDirectIface can ever be set on a struct with an embedded interface field since an interface is already two words long, but to be safe, I left it in for the time being (see https://github.com/TheCount/go/blob/b97bc377d699b1c624ce5fe12163c890ac6398f7/src/reflect/type.go#L2750).
  • As you noticed, the previous code uses the addresses of the reflect.Values tfn and ifn directly, which is wrong. We need to call the closure created by MakeFunc (located at tfn.ptr and ifn.ptr). However, method expects a direct code pointer. This is indeed related to #16522: we cannot give method the MakeFunc generated actual code pointer because then it can't find its closure context. So we store the closure elsewhere and use some special dispatch code instead as the method code pointer. The dispatch code then retrieves the closure and calls it.

This patch has some deficiencies:

  • The current solution works with single embedded interfaces (your case), but probably not with more complex embeddings (I haven't checked). Maybe this can be left for later.
  • The current solution works only on AMD64. This is because the special dispatch code uses some assembly hackery. It shouldn't be too hard to extend it to the remaining architectures, though I could probably use some help here as I don't have access to some of the hardware Go supports (or maybe I could try QEMU).
  • The current solution can generate only a finite number of working methods (812 to be exact, number is AMD64 specific) within the whole process. This is because dispatch entry points need to be mutually different so some information is available as to which MakeFunc closure to use. The number 812 is such that the entire dispatch code section fits into one memory page (4096 bytes); I had a vague idea using virtual memory mapping to remove this limit without using more physical RAM, but I'd need some help from a compiler or runtime expert to pull this off. Alternatively, we could just say, 812 methods (or whatever number) is enough for almost everyone and accept that there is a limit. Once the limit is exceeded, a method which always panics is created instead.

@ianlancetaylor Can you give me some advice how to proceed from here? Should I create a pull request despite the deficiencies? Or connect to some people first (who?)? TIA!

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 4, 2020

@TheCount This seems complicated enough that it's probably worth writing a little design doc for how to implement this. I think the limitations you describe are a bit too extreme, as they will cause programs to work in limited cases but fail surprisingly in general. Perhaps the best person to review a design doc would be @randall77 . Thanks.

@randall77
Copy link
Contributor

@randall77 randall77 commented Jul 4, 2020

Sure, I can take a look when it is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

4 participants