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 · 8 comments
Open
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mitchellh
Copy link

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

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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 1, 2020
@ianlancetaylor ianlancetaylor added this to the Backlog milestone May 1, 2020
TheCount added 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 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 reflect: NamedOf #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

@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

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

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
TheCount added a commit to TheCount/go that referenced this issue Sep 3, 2023
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

Update 2023/09/03: Commit rebased onto current master.
However, ·dispatchLabel assumes the old ABI0, which is wrong,
causing this patch to not work.
@TheCount
Copy link

TheCount commented Sep 3, 2023

Sorry for dropping the ball on this. FWIW, in TheCount/go@155eedb I adapted my initial patch to the current version of Go. However, the simple little dispatch hack in ·dispatchLabel doesn't work any more because of the internal Go ABI change. Possibly it would now require special treatment similar to ·makeFuncStub or ·methodValueCall.

I'll see whether I can find the time to write that design doc. No promises, though. Sorry.

TheCount added a commit to TheCount/go that referenced this issue Sep 4, 2023
TheCount added a commit to TheCount/go that referenced this issue Sep 4, 2023
Use abi.FuncPCABIInternal instead of going through assembly or
unsafe.Pointer hacks.

For golang#38783
TheCount added a commit to TheCount/go that referenced this issue Sep 4, 2023
Move amd64-specific definitions from general makefunc.go to
amd64-specific makefunc_amd64.go.

For golang#38783
TheCount added a commit to TheCount/go that referenced this issue Sep 7, 2023
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

Update 2023/09/03: Commit rebased onto current master.
However, ·dispatchLabel assumes the old ABI0, which is wrong,
causing this patch to not work.
TheCount added a commit to TheCount/go that referenced this issue Sep 7, 2023
TheCount added a commit to TheCount/go that referenced this issue Sep 7, 2023
Use abi.FuncPCABIInternal instead of going through assembly or
unsafe.Pointer hacks.

For golang#38783
TheCount added a commit to TheCount/go that referenced this issue Sep 7, 2023
Move amd64-specific definitions from general makefunc.go to
amd64-specific makefunc_amd64.go.

For golang#38783
TheCount added a commit to TheCount/go that referenced this issue Sep 7, 2023
TheCount added a commit to TheCount/go that referenced this issue Sep 9, 2023
Implement struct type creation for struct types with one embedded
interface. No limit on the number of such struct types that can be
created.

For golang#38783
TheCount added a commit to TheCount/go that referenced this issue Sep 12, 2023
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

Update 2023/09/03: Commit rebased onto current master.
However, ·dispatchLabel assumes the old ABI0, which is wrong,
causing this patch to not work.
TheCount added a commit to TheCount/go that referenced this issue Sep 12, 2023
TheCount added a commit to TheCount/go that referenced this issue Sep 12, 2023
Use abi.FuncPCABIInternal instead of going through assembly or
unsafe.Pointer hacks.

For golang#38783
TheCount added a commit to TheCount/go that referenced this issue Sep 12, 2023
Move amd64-specific definitions from general makefunc.go to
amd64-specific makefunc_amd64.go.

For golang#38783
TheCount added a commit to TheCount/go that referenced this issue Sep 12, 2023
@mumbleskates
Copy link

It looks like this no longer segfaults as of 1.22.0, the playground link returns a coherent panic message instead.

@TheCount
Copy link

It looks like this no longer segfaults as of 1.22.0, the playground link returns a coherent panic message instead.

Yes, the faulty code was removed shortly after I polished up my patches 😭

I mean, good that there's clean behavior now. Before I touch that code again, I should find out whether the MMU/page aliasing magic I have in mind even works in this context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: Triage Backlog
Development

No branches or pull requests

6 participants