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
Open
Labels
NeedsFix

Comments

@zhouguangyuan0718
Copy link
Contributor

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

@gopherbot gopherbot commented Sep 23, 2021

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

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

@zhouguangyuan0718
Copy link
Contributor Author

@zhouguangyuan0718 zhouguangyuan0718 commented Sep 24, 2021

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

@randall77 randall77 commented Sep 24, 2021

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 label Sep 24, 2021
@zhouguangyuan0718
Copy link
Contributor Author

@zhouguangyuan0718 zhouguangyuan0718 commented Sep 28, 2021

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

@zhouguangyuan0718
Copy link
Contributor Author

@zhouguangyuan0718 zhouguangyuan0718 commented Oct 10, 2021

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

@zhouguangyuan0718 zhouguangyuan0718 commented Oct 11, 2021

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

@cherrymui cherrymui commented Oct 11, 2021

If we had our own dynamic linker...

@cherrymui
Copy link
Member

@cherrymui cherrymui commented Oct 11, 2021

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

@gopherbot gopherbot commented Nov 10, 2021

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

@gopherbot gopherbot commented Nov 10, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 10, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 10, 2021

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

@zhouguangyuan0718 zhouguangyuan0718 commented Nov 10, 2021

@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

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

@zhouguangyuan0718
Copy link
Contributor Author

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

@cherrymui cherrymui commented Nov 10, 2021

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

@rguilmont
Copy link

@rguilmont rguilmont commented Feb 17, 2022

Had the same issue while working with this library, more specificaly with this function :

func Fold[A, B, C any](e Either[A, B], fa func(A) C, fb func(B) C) C {
	if e.IsRight() {
		return fa(e.(right[A, B]).Value)
	}
	return fb(e.(left[A, B]).Value)
}

i had a type A declared and used in packageB and main. It caused exactly this issue.

Solution was to move the type A to a packageA, and now i have to import it to packageB and main. It's now working flawlessly.
I hope it's clear enough, i know it won't solve most of related issues but at least in my case it was good enough.

@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 17, 2022

@rguilmont Are you using plugins?

If not, then it is not this issue. If you can open a new issue with the problem you're describing, and instructions on how to replicate it, that would be great.

If so, then what parts are in plugins and what parts are in the main binary?

@rguilmont
Copy link

@rguilmont rguilmont commented Feb 17, 2022

Sorry, i'm indeed not using plugins at all, you're totally right.

I will open an issue with the issue i'm describing, and everything to replicate :) hoping the problem is not between the chair and the keyboard :')

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

No branches or pull requests

6 participants