From 3d1699ea787f38be6088f9a098d6e08dafca9387 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Wed, 31 May 2017 08:45:10 -0700 Subject: runtime: new itab lookup table Keep itabs in a growable hash table. Use a simple open-addressable hash table, quadratic probing, power of two sized. Synchronization gets a bit more tricky. The common read path now has two atomic reads, one to get the table pointer and one to read the entry out of the table. I set the max load factor to 75%, kind of arbitrarily. There's a space-speed tradeoff here, and I'm not sure where we should land. Because we use open addressing the itab.link field is no longer needed. I'll remove it in a separate CL. Fixes #20505 Change-Id: Ifb3d9a337512d6cf968c1fceb1eeaf89559afebf Reviewed-on: https://go-review.googlesource.com/44472 Run-TryBot: Keith Randall TryBot-Result: Gobot Gobot Reviewed-by: Josh Bleecher Snyder --- src/runtime/plugin.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'src/runtime/plugin.go') diff --git a/src/runtime/plugin.go b/src/runtime/plugin.go index 682caacb21..34b306ae25 100644 --- a/src/runtime/plugin.go +++ b/src/runtime/plugin.go @@ -54,13 +54,11 @@ func plugin_lastmoduleinit() (path string, syms map[string]interface{}, mismatch pluginftabverify(md) moduledataverify1(md) - lock(&ifaceLock) + lock(&itabLock) for _, i := range md.itablinks { - if !i.inhash { - additab(i, true, false) - } + itabAddStartup(i) } - unlock(&ifaceLock) + unlock(&itabLock) // Build a map of symbol names to symbols. Here in the runtime // we fill out the first word of the interface, the type. We -- cgit v1.3 From 89d74f54168619cf1f36b6868626fbb1237c1deb Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Tue, 30 May 2017 12:59:25 -0700 Subject: cmd/compile: set itab function pointers at compile time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I noticed that we don't set an itab's function pointers at compile time. Instead, we currently do it at executable startup. Set the function pointers at compile time instead. This shortens startup time. It has no effect on normal binary size. Object files will have more relocations, but that isn't a big deal. For PIE there are additional pointers that will need to be adjusted at load time. There are already other pointers in an itab that need to be adjusted, so the cache line will already be paged in. There might be some binary size overhead to mark these pointers. The "go test -c -buildmode=pie net/http" binary is 0.18% bigger. Update #20505 Change-Id: I267c82489915b509ff66e512fc7319b2dd79b8f7 Reviewed-on: https://go-review.googlesource.com/44341 Run-TryBot: Keith Randall TryBot-Result: Gobot Gobot Reviewed-by: Martin Möhrmann --- src/cmd/compile/internal/gc/reflect.go | 15 +++++++-------- src/runtime/iface.go | 9 +-------- src/runtime/plugin.go | 2 +- 3 files changed, 9 insertions(+), 17 deletions(-) (limited to 'src/runtime/plugin.go') diff --git a/src/cmd/compile/internal/gc/reflect.go b/src/cmd/compile/internal/gc/reflect.go index e3d8b1537e..a08ea0f73b 100644 --- a/src/cmd/compile/internal/gc/reflect.go +++ b/src/cmd/compile/internal/gc/reflect.go @@ -1463,14 +1463,13 @@ func dumptabs() { // } o := dsymptr(i.lsym, 0, dtypesym(i.itype).Linksym(), 0) o = dsymptr(i.lsym, o, dtypesym(i.t).Linksym(), 0) - o = duint32(i.lsym, o, typehash(i.t)) // copy of type hash - o += 4 // skip unused field - o += len(imethods(i.itype)) * Widthptr // skip fun method pointers - // at runtime the itab will contain pointers to types, other itabs and - // method functions. None are allocated on heap, so we can use obj.NOPTR. - ggloblsym(i.lsym, int32(o), int16(obj.DUPOK|obj.NOPTR)) - // TODO: mark readonly after we pre-add the function pointers - + o = duint32(i.lsym, o, typehash(i.t)) // copy of type hash + o += 4 // skip unused field + for _, fn := range genfun(i.t, i.itype) { + o = dsymptr(i.lsym, o, fn, 0) // method pointer for each method + } + // Nothing writes static itabs, so they are read only. + ggloblsym(i.lsym, int32(o), int16(obj.DUPOK|obj.RODATA)) ilink := itablinkpkg.Lookup(i.t.ShortString() + "," + i.itype.ShortString()).Linksym() dsymptr(ilink, 0, i.lsym, 0) ggloblsym(ilink, int32(Widthptr), int16(obj.DUPOK|obj.RODATA)) diff --git a/src/runtime/iface.go b/src/runtime/iface.go index 665dbdbc16..1f31bcae6d 100644 --- a/src/runtime/iface.go +++ b/src/runtime/iface.go @@ -167,13 +167,6 @@ func itabAdd(m *itab) { } } -// Adds m to the set of initial itabs. -// itabLock must be held. -func itabAddStartup(m *itab) { - m.init() // TODO: remove after CL 44341 - itabAdd(m) -} - // init fills in the m.fun array with all the code pointers for // the m.inter/m._type pair. If the type does not implement the interface, // it sets m.fun[0] to 0 and returns the name of an interface function that is missing. @@ -230,7 +223,7 @@ func itabsinit() { lock(&itabLock) for _, md := range activeModules() { for _, i := range md.itablinks { - itabAddStartup(i) + itabAdd(i) } } unlock(&itabLock) diff --git a/src/runtime/plugin.go b/src/runtime/plugin.go index 34b306ae25..caecba67f8 100644 --- a/src/runtime/plugin.go +++ b/src/runtime/plugin.go @@ -56,7 +56,7 @@ func plugin_lastmoduleinit() (path string, syms map[string]interface{}, mismatch lock(&itabLock) for _, i := range md.itablinks { - itabAddStartup(i) + itabAdd(i) } unlock(&itabLock) -- cgit v1.3 From d8ae2156fe08f31f9b20a79b6971638c5bf203b5 Mon Sep 17 00:00:00 2001 From: David Crawshaw Date: Sat, 2 Sep 2017 12:05:35 -0400 Subject: runtime, plugin: error not throw on duplicate open Along the way, track bad modules. Make sure they don't end up on the active modules list, and aren't accidentally reprocessed as new plugins. Fixes #19004 Change-Id: I8a5e7bb11f572f7b657a97d521a7f84822a35c07 Reviewed-on: https://go-review.googlesource.com/61171 Run-TryBot: David Crawshaw TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- misc/cgo/testplugin/src/host/host.go | 17 +++++++++++++++++ misc/cgo/testplugin/test.bash | 1 + src/plugin/plugin.go | 1 + src/plugin/plugin_dlopen.go | 26 ++++++++++++++++---------- src/runtime/plugin.go | 24 ++++++++++++++++-------- src/runtime/symtab.go | 9 +++++++-- 6 files changed, 58 insertions(+), 20 deletions(-) (limited to 'src/runtime/plugin.go') diff --git a/misc/cgo/testplugin/src/host/host.go b/misc/cgo/testplugin/src/host/host.go index bba9a62166..0ca17da3de 100644 --- a/misc/cgo/testplugin/src/host/host.go +++ b/misc/cgo/testplugin/src/host/host.go @@ -136,6 +136,14 @@ func main() { log.Fatalf("after loading plugin2, common.X=%d, want %d", got, want) } + _, err = plugin.Open("plugin2-dup.so") + if err == nil { + log.Fatal(`plugin.Open("plugin2-dup.so"): duplicate open should have failed`) + } + if s := err.Error(); !strings.Contains(s, "already loaded") { + log.Fatal(`plugin.Open("plugin2.so"): error does not mention "already loaded"`) + } + _, err = plugin.Open("plugin-mismatch.so") if err == nil { log.Fatal(`plugin.Open("plugin-mismatch.so"): should have failed`) @@ -144,6 +152,15 @@ func main() { log.Fatalf(`plugin.Open("plugin-mismatch.so"): error does not mention "different version": %v`, s) } + _, err = plugin.Open("plugin2-dup.so") + if err == nil { + log.Fatal(`plugin.Open("plugin2-dup.so"): duplicate open after bad plugin should have failed`) + } + _, err = plugin.Open("plugin2.so") + if err != nil { + log.Fatalf(`plugin.Open("plugin2.so"): second open with same name failed: %v`, err) + } + // Test that unexported types with the same names in // different plugins do not interfere with each other. // diff --git a/misc/cgo/testplugin/test.bash b/misc/cgo/testplugin/test.bash index 7e982c8961..f64be9b0ff 100755 --- a/misc/cgo/testplugin/test.bash +++ b/misc/cgo/testplugin/test.bash @@ -25,6 +25,7 @@ mkdir sub GOPATH=$(pwd) go build -buildmode=plugin plugin1 GOPATH=$(pwd) go build -buildmode=plugin plugin2 +cp plugin2.so plugin2-dup.so GOPATH=$(pwd)/altpath go build -buildmode=plugin plugin-mismatch GOPATH=$(pwd) go build -buildmode=plugin -o=sub/plugin1.so sub/plugin1 GOPATH=$(pwd) go build -buildmode=plugin -o=unnamed1.so unnamed1/main.go diff --git a/src/plugin/plugin.go b/src/plugin/plugin.go index c774465812..c37b65fd82 100644 --- a/src/plugin/plugin.go +++ b/src/plugin/plugin.go @@ -20,6 +20,7 @@ package plugin // Plugin is a loaded Go plugin. type Plugin struct { pluginpath string + err string // set if plugin failed to load loaded chan struct{} // closed when loaded syms map[string]interface{} } diff --git a/src/plugin/plugin_dlopen.go b/src/plugin/plugin_dlopen.go index ce66c036c9..37380989d7 100644 --- a/src/plugin/plugin_dlopen.go +++ b/src/plugin/plugin_dlopen.go @@ -87,7 +87,7 @@ func open(name string) (*Plugin, error) { if C.realpath( (*C.char)(unsafe.Pointer(&cRelName[0])), (*C.char)(unsafe.Pointer(&cPath[0]))) == nil { - return nil, errors.New("plugin.Open(" + name + "): realpath failed") + return nil, errors.New(`plugin.Open("` + name + `"): realpath failed`) } filepath := C.GoString((*C.char)(unsafe.Pointer(&cPath[0]))) @@ -95,6 +95,9 @@ func open(name string) (*Plugin, error) { pluginsMu.Lock() if p := plugins[filepath]; p != nil { pluginsMu.Unlock() + if p.err != "" { + return nil, errors.New(`plugin.Open("` + name + `"): ` + p.err + ` (previous failure)`) + } <-p.loaded return p, nil } @@ -102,22 +105,25 @@ func open(name string) (*Plugin, error) { h := C.pluginOpen((*C.char)(unsafe.Pointer(&cPath[0])), &cErr) if h == 0 { pluginsMu.Unlock() - return nil, errors.New("plugin.Open: " + C.GoString(cErr)) + return nil, errors.New(`plugin.Open("` + name + `"): ` + C.GoString(cErr)) } // TODO(crawshaw): look for plugin note, confirm it is a Go plugin // and it was built with the correct toolchain. if len(name) > 3 && name[len(name)-3:] == ".so" { name = name[:len(name)-3] } - - pluginpath, syms, mismatchpkg := lastmoduleinit() - if mismatchpkg != "" { - pluginsMu.Unlock() - return nil, errors.New("plugin.Open: plugin was built with a different version of package " + mismatchpkg) - } if plugins == nil { plugins = make(map[string]*Plugin) } + pluginpath, syms, errstr := lastmoduleinit() + if errstr != "" { + plugins[filepath] = &Plugin{ + pluginpath: pluginpath, + err: errstr, + } + pluginsMu.Unlock() + return nil, errors.New(`plugin.Open("` + name + `"): ` + errstr) + } // This function can be called from the init function of a plugin. // Drop a placeholder in the map so subsequent opens can wait on it. p := &Plugin{ @@ -153,7 +159,7 @@ func open(name string) (*Plugin, error) { p := C.pluginLookup(h, (*C.char)(unsafe.Pointer(&cname[0])), &cErr) if p == nil { - return nil, errors.New("plugin.Open: could not find symbol " + symName + ": " + C.GoString(cErr)) + return nil, errors.New(`plugin.Open("` + name + `"): could not find symbol ` + symName + `: ` + C.GoString(cErr)) } valp := (*[2]unsafe.Pointer)(unsafe.Pointer(&sym)) if isFunc { @@ -184,4 +190,4 @@ var ( ) // lastmoduleinit is defined in package runtime -func lastmoduleinit() (pluginpath string, syms map[string]interface{}, mismatchpkg string) +func lastmoduleinit() (pluginpath string, syms map[string]interface{}, errstr string) diff --git a/src/runtime/plugin.go b/src/runtime/plugin.go index caecba67f8..5e05be71ec 100644 --- a/src/runtime/plugin.go +++ b/src/runtime/plugin.go @@ -7,22 +7,29 @@ package runtime import "unsafe" //go:linkname plugin_lastmoduleinit plugin.lastmoduleinit -func plugin_lastmoduleinit() (path string, syms map[string]interface{}, mismatchpkg string) { - md := firstmoduledata.next +func plugin_lastmoduleinit() (path string, syms map[string]interface{}, errstr string) { + var md *moduledata + for pmd := firstmoduledata.next; pmd != nil; pmd = pmd.next { + if pmd.bad { + md = nil // we only want the last module + continue + } + md = pmd + } if md == nil { throw("runtime: no plugin module data") } - for md.next != nil { - md = md.next + if md.pluginpath == "" { + throw("runtime: plugin has empty pluginpath") } if md.typemap != nil { - throw("runtime: plugin already initialized") + return "", nil, "plugin already loaded" } for _, pmd := range activeModules() { if pmd.pluginpath == md.pluginpath { - println("plugin: plugin", md.pluginpath, "already loaded") - throw("plugin: plugin already loaded") + md.bad = true + return "", nil, "plugin already loaded" } if inRange(pmd.text, pmd.etext, md.text, md.etext) || @@ -43,7 +50,8 @@ func plugin_lastmoduleinit() (path string, syms map[string]interface{}, mismatch } for _, pkghash := range md.pkghashes { if pkghash.linktimehash != *pkghash.runtimehash { - return "", nil, pkghash.modulename + md.bad = true + return "", nil, "plugin was built with a different version of package " + pkghash.modulename } } diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index e1b41ca4ff..4a68f4eaa0 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -338,8 +338,8 @@ const ( // moduledata records information about the layout of the executable // image. It is written by the linker. Any changes here must be // matched changes to the code in cmd/internal/ld/symtab.go:symtab. -// moduledata is stored in read-only memory; none of the pointers here -// are visible to the garbage collector. +// moduledata is stored in statically allocated non-pointer memory; +// none of the pointers here are visible to the garbage collector. type moduledata struct { pclntable []byte ftab []functab @@ -371,6 +371,8 @@ type moduledata struct { typemap map[typeOff]*_type // offset to *_rtype in previous module + bad bool // module failed to load and should be ignored + next *moduledata } @@ -443,6 +445,9 @@ func activeModules() []*moduledata { func modulesinit() { modules := new([]*moduledata) for md := &firstmoduledata; md != nil; md = md.next { + if md.bad { + continue + } *modules = append(*modules, md) if md.gcdatamask == (bitvector{}) { md.gcdatamask = progToPointerMask((*byte)(unsafe.Pointer(md.gcdata)), md.edata-md.data) -- cgit v1.3