Skip to content

Commit

Permalink
[release-branch.go1.16] cmd/compile, cmd/link: dynamically export wri…
Browse files Browse the repository at this point in the history
…table 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 <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
(cherry picked from commit de012bc095359e1b552d4ea6fb6b2995f3ab04f5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/302449
  • Loading branch information
cherrymui authored and toothrot committed Mar 25, 2021
1 parent 33fb479 commit ac59d7a
Show file tree
Hide file tree
Showing 7 changed files with 93 additions and 5 deletions.
7 changes: 7 additions & 0 deletions misc/cgo/testplugin/plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
7 changes: 7 additions & 0 deletions misc/cgo/testplugin/testdata/issue44956/base/base.go
Original file line number Diff line number Diff line change
@@ -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}
47 changes: 47 additions & 0 deletions misc/cgo/testplugin/testdata/issue44956/main.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
9 changes: 9 additions & 0 deletions misc/cgo/testplugin/testdata/issue44956/plugin1.go
Original file line number Diff line number Diff line change
@@ -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() {}
11 changes: 11 additions & 0 deletions misc/cgo/testplugin/testdata/issue44956/plugin2.go
Original file line number Diff line number Diff line change
@@ -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() {}
4 changes: 2 additions & 2 deletions src/cmd/compile/internal/gc/sinit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
13 changes: 10 additions & 3 deletions src/cmd/link/internal/ld/symtab.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
package ld

import (
"cmd/internal/obj"
"cmd/internal/objabi"
"cmd/link/internal/loader"
"cmd/link/internal/sym"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}

0 comments on commit ac59d7a

Please sign in to comment.