From ac59d7abb9ab5ec31cb0fe1eaccbdf3b17edbde0 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Fri, 12 Mar 2021 23:21:09 -0500 Subject: [release-branch.go1.16] cmd/compile, cmd/link: dynamically export writable static tmps Static tmps are private to a package, but with plugins a package can be shared among multiple DSOs. They need to have a consistent view of the static tmps, especially for writable ones. So export them. (Read-only static tmps have the same values anyway, so it doesn't matter. Also Mach-O doesn't support dynamically exporting read-only symbols anyway.) Updates #44956. Fixes #45030. Change-Id: I921e25b7ab73cd5d5347800eccdb7931e3448779 Reviewed-on: https://go-review.googlesource.com/c/go/+/301793 Trust: Cherry Zhang Run-TryBot: Cherry Zhang TryBot-Result: Go Bot Reviewed-by: Than McIntosh (cherry picked from commit de012bc095359e1b552d4ea6fb6b2995f3ab04f5) Reviewed-on: https://go-review.googlesource.com/c/go/+/302449 --- misc/cgo/testplugin/plugin_test.go | 7 ++++ .../testplugin/testdata/issue44956/base/base.go | 7 ++++ misc/cgo/testplugin/testdata/issue44956/main.go | 47 ++++++++++++++++++++++ misc/cgo/testplugin/testdata/issue44956/plugin1.go | 9 +++++ misc/cgo/testplugin/testdata/issue44956/plugin2.go | 11 +++++ src/cmd/compile/internal/gc/sinit.go | 4 +- src/cmd/link/internal/ld/symtab.go | 13 ++++-- 7 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 misc/cgo/testplugin/testdata/issue44956/base/base.go create mode 100644 misc/cgo/testplugin/testdata/issue44956/main.go create mode 100644 misc/cgo/testplugin/testdata/issue44956/plugin1.go create mode 100644 misc/cgo/testplugin/testdata/issue44956/plugin2.go diff --git a/misc/cgo/testplugin/plugin_test.go b/misc/cgo/testplugin/plugin_test.go index 2d991012c8..8869528015 100644 --- a/misc/cgo/testplugin/plugin_test.go +++ b/misc/cgo/testplugin/plugin_test.go @@ -209,3 +209,10 @@ func TestMethod2(t *testing.T) { goCmd(t, "build", "-o", "method2.exe", "./method2/main.go") run(t, "./method2.exe") } + +func TestIssue44956(t *testing.T) { + goCmd(t, "build", "-buildmode=plugin", "-o", "issue44956p1.so", "./issue44956/plugin1.go") + goCmd(t, "build", "-buildmode=plugin", "-o", "issue44956p2.so", "./issue44956/plugin2.go") + goCmd(t, "build", "-o", "issue44956.exe", "./issue44956/main.go") + run(t, "./issue44956.exe") +} diff --git a/misc/cgo/testplugin/testdata/issue44956/base/base.go b/misc/cgo/testplugin/testdata/issue44956/base/base.go new file mode 100644 index 0000000000..609aa0dff4 --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/base/base.go @@ -0,0 +1,7 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package base + +var X = &map[int]int{123: 456} diff --git a/misc/cgo/testplugin/testdata/issue44956/main.go b/misc/cgo/testplugin/testdata/issue44956/main.go new file mode 100644 index 0000000000..287a60585e --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/main.go @@ -0,0 +1,47 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Issue 44956: writable static temp is not exported correctly. +// In the test below, package base is +// +// X = &map{...} +// +// which compiles to +// +// X = &stmp // static +// stmp = makemap(...) // in init function +// +// plugin1 and plugin2 both import base. plugin1 doesn't use +// base.X, so that symbol is deadcoded in plugin1. +// +// plugin1 is loaded first. base.init runs at that point, which +// initialize base.stmp. +// +// plugin2 is then loaded. base.init already ran, so it doesn't run +// again. When base.stmp is not exported, plugin2's base.X points to +// its own private base.stmp, which is not initialized, fail. + +package main + +import "plugin" + +func main() { + _, err := plugin.Open("issue44956p1.so") + if err != nil { + panic("FAIL") + } + + p2, err := plugin.Open("issue44956p2.so") + if err != nil { + panic("FAIL") + } + f, err := p2.Lookup("F") + if err != nil { + panic("FAIL") + } + x := f.(func() *map[int]int)() + if x == nil || (*x)[123] != 456 { + panic("FAIL") + } +} diff --git a/misc/cgo/testplugin/testdata/issue44956/plugin1.go b/misc/cgo/testplugin/testdata/issue44956/plugin1.go new file mode 100644 index 0000000000..499fa31abf --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/plugin1.go @@ -0,0 +1,9 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import _ "testplugin/issue44956/base" + +func main() {} diff --git a/misc/cgo/testplugin/testdata/issue44956/plugin2.go b/misc/cgo/testplugin/testdata/issue44956/plugin2.go new file mode 100644 index 0000000000..a73542ca71 --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/plugin2.go @@ -0,0 +1,11 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import "testplugin/issue44956/base" + +func F() *map[int]int { return base.X } + +func main() {} diff --git a/src/cmd/compile/internal/gc/sinit.go b/src/cmd/compile/internal/gc/sinit.go index 212fcc022d..4d0837bc74 100644 --- a/src/cmd/compile/internal/gc/sinit.go +++ b/src/cmd/compile/internal/gc/sinit.go @@ -363,15 +363,15 @@ func staticname(t *types.Type) *Node { n := newname(lookup(fmt.Sprintf("%s%d", obj.StaticNamePref, statuniqgen))) statuniqgen++ addvar(n, t, PEXTERN) - n.Sym.Linksym().Set(obj.AttrLocal, true) return n } -// readonlystaticname returns a name backed by a (writable) static data symbol. +// readonlystaticname returns a name backed by a read-only static data symbol. func readonlystaticname(t *types.Type) *Node { n := staticname(t) n.MarkReadonly() n.Sym.Linksym().Set(obj.AttrContentAddressable, true) + n.Sym.Linksym().Set(obj.AttrLocal, true) return n } diff --git a/src/cmd/link/internal/ld/symtab.go b/src/cmd/link/internal/ld/symtab.go index c98e4de03f..f54cf9ea2f 100644 --- a/src/cmd/link/internal/ld/symtab.go +++ b/src/cmd/link/internal/ld/symtab.go @@ -31,6 +31,7 @@ package ld import ( + "cmd/internal/obj" "cmd/internal/objabi" "cmd/link/internal/loader" "cmd/link/internal/sym" @@ -102,10 +103,14 @@ func putelfsym(ctxt *Link, x loader.Sym, typ elf.SymType, curbind elf.SymBind) { elfshnum = xosect.Elfsect.(*ElfShdr).shnum } + sname := ldr.SymExtname(x) + // One pass for each binding: elf.STB_LOCAL, elf.STB_GLOBAL, // maybe one day elf.STB_WEAK. bind := elf.STB_GLOBAL - if ldr.IsFileLocal(x) || ldr.AttrVisibilityHidden(x) || ldr.AttrLocal(x) { + if ldr.IsFileLocal(x) && !isStaticTmp(sname) || ldr.AttrVisibilityHidden(x) || ldr.AttrLocal(x) { + // Static tmp is package local, but a package can be shared among multiple DSOs. + // They need to have a single view of the static tmp that are writable. bind = elf.STB_LOCAL } @@ -140,8 +145,6 @@ func putelfsym(ctxt *Link, x loader.Sym, typ elf.SymType, curbind elf.SymBind) { other |= 3 << 5 } - sname := ldr.SymExtname(x) - // When dynamically linking, we create Symbols by reading the names from // the symbol tables of the shared libraries and so the names need to // match exactly. Tools like DTrace will have to wait for now. @@ -823,3 +826,7 @@ func setCarrierSize(typ sym.SymKind, sz int64) { } CarrierSymByType[typ].Size = sz } + +func isStaticTmp(name string) bool { + return strings.Contains(name, "."+obj.StaticNamePref) +} -- cgit v1.2.3-54-g00ecf