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

plugin: convert iface to type failed with "types from different scopes" #48532

Open
zhouguangyuan0718 opened this issue Sep 22, 2021 · 21 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@zhouguangyuan0718
Copy link
Contributor

zhouguangyuan0718 commented Sep 22, 2021

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

$ go version
go version devel go1.18-14e812bfc5 Fri Sep 17 00:31:49 2021 +0000 linux/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="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/mnt/d/00.Tool/00.golang/go-master"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/mnt/d/00.Tool/00.golang/go-master/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.18-14e812bfc5 Fri Sep 17 00:31:49 2021 +0000"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/mnt/d/01.Project/03.corego_performance/demo/go.mod"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build691445209=. -gno-record-gcc-switches"

What did you do?

pluginassert/go.mod:

module pluginassert

go 1.18

pluginassert/inter/iface.go:

package inter

type A interface {
	Method1()
}

type B struct{}

func (b *B) Method1() {
	println("Method1 in B")
}

pluginassert/main.go:

package main

import (
	"log"
	"plugin"
	"pluginassert/inter"
)

func main() {
	var a interface{}
	a = &inter.B{}
	switch x := a.(type) {
	case inter.A:
		x.Method1()
	default:
		log.Fatalln("assert x to A failed")
	}

	p, err := plugin.Open("./pluginassert.so")
	if err != nil {
		log.Fatalln(err)
	}
	sym, err := p.Lookup("TestFunc")
	if err != nil {
		log.Fatalln(err)
	}

	testFunc := sym.(func(inter.A))
	switch x := a.(type) {
	case inter.A:
		testFunc(x)
	default:
		log.Fatalln("assert x to A failed")
	}
}

pluginassert/plugin.go:

package main

import (
	"pluginassert/inter"
)

func TestFunc(a inter.A) {
	b := a.(*inter.B)
	b.Method1()
}

~/pluginassert $ go build -buildmode=plugin
~/pluginassert $ go build
~/pluginassert $ ./pluginassert

What did you expect to see?

Method1 in B
Method1 in B

What did you see instead?

Method1 in B
panic: interface conversion: inter.A is *inter.B, not *inter.B (types from different scopes)

goroutine 1 [running]:
pluginassert.TestFunc({0x7fe0643fdff8, 0x6406b8})
        pluginassert/plugin.go:8 +0x67
main.main()
        pluginassert/main.go:31 +0x204

There is no symbol go.itab.*pluginassert/inter.B,pluginassert/inter.A in main binary. So it will invoke persistentalloc in function getitab in first switch-case to allocate an itab.

But unfortunately, after plugin is loaded, the itab go.itab.*pluginassert/inter.B,pluginassert/inter.A was added to itabTable. When the TestFunc was invoked in second switch-case, x is a iface with itab which was allocated in first switch-case.

In plugin, the implement of a.(*inter.B) is to compare the address of a.tab and go.itab.*pluginassert/inter.B,pluginassert/inter.A. Certainly they are different. So we get this panic.

@zhouguangyuan0718 zhouguangyuan0718 changed the title plugin: assert iface to type failed with "types from different scopes" plugin: conwert iface to type failed with "types from different scopes" Sep 22, 2021
@zhouguangyuan0718 zhouguangyuan0718 changed the title plugin: conwert iface to type failed with "types from different scopes" plugin: convert iface to type failed with "types from different scopes" Sep 22, 2021
@gopherbot
Copy link

Change https://golang.org/cl/351850 mentions this issue: cmd/compile: recheck _type of itab when the address of itabs are not equal.

@randall77
Copy link
Contributor

Here's an even trickier case:
plugin.go:

package main

func F(x interface{}) {
	if _, ok := x.([37]int); !ok {
		panic("bad")
	}
}

main.go:

package main

import (
	"plugin"
	"reflect"
)

func main() {
	at := reflect.ArrayOf(37, reflect.TypeOf(int(0)))
	a := reflect.New(at).Elem().Interface()

	p, err := plugin.Open("plugin.so")
	if err != nil {
		panic(err)
	}
	s, err := p.Lookup("F")
	if err != nil {
		panic(err)
	}
	f := s.(func(interface{}))
	f(a)
}

This panics in the plugin. The [37]int in the plugin doesn't look like the same type created by ArrayOf in the main program.

Note that if you move the at and a assignments to after the plugin.Open call, then it works fine.

This issue is about itabs, but the same underlying problem occurs - dynamically created types (created explicitly with reflect) or itabs (often created implicitly in type conversions) won't match with statically declared types or itabs in a plugin.

I think plugins are going to have to indirect all of their types and itabs, and resolve all of them at plugin load time, deduplicating with their dynamically created doppelgangers.

@zhouguangyuan0718
Copy link
Contributor Author

This case can reproduce,too.

pluginassert/main.go:

package main

import (
	"log"
	"plugin"
	"pluginassert/inter"
)

func main() {
	var c inter.C
	c = &inter.B{}
	a := c.(inter.A)
	a.Method1()
	p, err := plugin.Open("./pluginassert.so")
	if err != nil {
		log.Fatalln(err)
	}
	sym, err := p.Lookup("TestFunc")
	if err != nil {
		log.Fatalln(err)
	}

	testFunc := sym.(func(inter.A))
	testFunc(c.(inter.A))
}

The itab created by assertI2I , assertE2I in main process has the same problem.

@zhouguangyuan0718
Copy link
Contributor Author

zhouguangyuan0718 commented Sep 24, 2021

Here's an even trickier case:
plugin.go:

package main

func F(x interface{}) {
	if _, ok := x.([37]int); !ok {
		panic("bad")
	}
}

main.go:

package main

import (
	"plugin"
	"reflect"
)

func main() {
	at := reflect.ArrayOf(37, reflect.TypeOf(int(0)))
	a := reflect.New(at).Elem().Interface()

	p, err := plugin.Open("plugin.so")
	if err != nil {
		panic(err)
	}
	s, err := p.Lookup("F")
	if err != nil {
		panic(err)
	}
	f := s.(func(interface{}))
	f(a)
}

This panics in the plugin. The [37]int in the plugin doesn't look like the same type created by ArrayOf in the main program.

Note that if you move the at and a assignments to after the plugin.Open call, then it works fine.

This issue is about itabs, but the same underlying problem occurs - dynamically created types (created explicitly with reflect) or itabs (often created implicitly in type conversions) won't match with statically declared types or itabs in a plugin.

I think plugins are going to have to indirect all of their types and itabs, and resolve all of them at plugin load time, deduplicating with their dynamically created doppelgangers.

genssa
# /plugin.go
00000 (3) TEXT "".F(SB), ABIInternal
00001 (3) FUNCDATA $0, gclocals·09cf9819fc716118c209c2d2155a3632(SB)
00002 (3) FUNCDATA $1, gclocals·69c1753bd5f81501d95132d08af04464(SB)
00003 (3) FUNCDATA $5, "".F.arginfo1(SB)
00004 (+4) LEAQ type.[37]int(SB), BX
00005 (4) CMPQ AX, BX
00006 (4) JNE 8
00007 (5) RET
00008 (4) LEAQ type.interface {}(SB), CX
00009 (4) PCDATA $1, $1
00010 (4) CALL runtime.panicdottypeE(SB)
00011 (4) XCHGL AX, AX
00012 (?) END

In this case, it compare the type field of eface and address of type.[37]int. This behavior is same with itab.
But it seems that in this case, we can't fix it by recheck.

@randall77
Copy link
Contributor

Yes, I think for plugins we'll need to have an array of *runtime._type in the plugin's RW memory, and every place in the plugin that needs a type must load an entry from that array instead of using a direct LEAQ. The plugin starts with each of those entries pointing to a runtime.type in its own RO memory, but the plugin loader will need to iterate over that array and see if an equivalent type already exists in the runtime (or reflect?) type lists, and replace the array entry if so. Same for itabs.

@dr2chase dr2chase added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 24, 2021
@zhouguangyuan0718
Copy link
Contributor Author

I'm working on it. I will try to implement it.

@zhouguangyuan0718
Copy link
Contributor Author

I sent a CL for itabs, can you review the CL when you're free?
I tried to implement it by following your proposal. And I use the itablinks as the array that you mentioned.
Maybe there is some mistake or risk,please tell me, thank you!
https://go-review.googlesource.com/c/go/+/351850
@randall77

@zhouguangyuan0718
Copy link
Contributor Author

a.go:

package min

type InterA interface {
	Method1()
}

type ImplB struct{}

func (b *ImplB) Method1() {
	println("Method1 in B")
}

func TestFunc(a InterA) {
	b := a.(*ImplB)
	b.Method1()
}

Before my CL(go 1.17):

$ go tool compile -S a.go
        ………………
"".TestFunc STEXT size=134 args=0x10 locals=0x20 funcid=0x0
        ………………
        0x001e 00030 (a.go:14)  LEAQ    go.itab.*"".ImplB,"".InterA(SB), DX
        0x0025 00037 (a.go:14)  CMPQ    AX, DX
        ………………

After my CL:

$ go tool compile -S a.go
        ………………
"".TestFunc STEXT size=134 args=0x10 locals=0x20 funcid=0x0
        ………………
        0x0020 00032 (a.go:14)  CMPQ    go.itabaddr.*"".ImplB,"".InterA(SB), AX
        ………………

go.itabaddr.*"".ImplB,"".InterA SNOPTRDATA dupok size=8
        0x0000 00 00 00 00 00 00 00 00                          ........
        rel 0+8 t=1 go.itab.*"".ImplB,"".InterA+0

And in linker, runtime.itablilnk will be container symbol for all the symbols prefix with "go.itabaddr".
The address in runtime.itablilnk will be replaced when loading the plugin.

@cherrymui
Copy link
Member

If we had our own dynamic linker...

@cherrymui
Copy link
Member

Plugins already load the address of a type descriptor from the GOT entry. Can we modify the GOT entry? (I know this sounds crazy...)

@gopherbot
Copy link

Change https://golang.org/cl/362635 mentions this issue: cmd/go,cmd/compile: add flags to support store itab and type address for plugin

@gopherbot
Copy link

Change https://golang.org/cl/362935 mentions this issue: cmd/link: use .itabaddr section as itablink for plugin

@gopherbot
Copy link

Change https://golang.org/cl/362936 mentions this issue: runtime: rewrite the entries in moduledata.itablinks when loading a plugin

@gopherbot
Copy link

Change https://golang.org/cl/362937 mentions this issue: cmd/go,cmd/compile: add flags to support load address of itab and type from table

@zhouguangyuan0718
Copy link
Contributor Author

@cherrymui @randall77
I have sent some CL for this issue. I want to add a new gcflag to distinguish we should load address of itab from the new table or not. Could you review these CL continue?
The same way to resolve this bug of types is diffcult. If the proposal for itabs can be accepted. I will work for types continue.

@cherrymui
Copy link
Member

I'm not sure that is the best way to resolve this. I'm still thinking if there is a better way (like, modifying the GOT?). And it is probably too late for 1.18.

@zhouguangyuan0718
Copy link
Contributor Author

zhouguangyuan0718 commented Nov 10, 2021

I'm not sure that is the best way to resolve this. I'm still thinking if there is a better way (like, modifying the GOT?). And it is probably too late for 1.18.

Emm.. Maybe modifying the GOT is not a normal way.
It's okay for 1.19. If you can review the code of this way and give some advice, I will backport it to our own go and test it for a long time. Thank you!
Or I can try implementing the way of modifying the GOT. I think it should work, too.

@cherrymui
Copy link
Member

I'm not sure modifying GOT is the best way, either. Maybe there is a simpler approach. We can still think about it.

@rguilmont

This comment was marked as off-topic.

@randall77

This comment was marked as off-topic.

@rguilmont

This comment was marked as off-topic.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
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. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Triage Backlog
Development

No branches or pull requests

7 participants