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: variable not initialized properly #62430

Closed
oszika opened this issue Sep 3, 2023 · 12 comments
Closed

plugin: variable not initialized properly #62430

oszika opened this issue Sep 3, 2023 · 12 comments
Assignees
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

@oszika
Copy link

oszika commented Sep 3, 2023

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

$ go version
go version go1.21.0 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
❯ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/oszika/.cache/go-build'
GOENV='/home/oszika/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/oszika/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/oszika/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/oszika/example/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1329397681=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I tried to use regexp on a plugin and it fails. After a little investigation, I saw unicode.Categories is not properly initialized.

Plugin: p1/main.go

package main

import (
	"unicode"
)

func F(s string) *unicode.RangeTable {
	return unicode.Categories[s]
}

func main() {}

main.go:

package main

import (
	"fmt"
	"plugin"
	"unicode"
)

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

	f := s.(func(string) *unicode.RangeTable)
	if f("C") == nil {
		fmt.Println("unicode.Categories not properly initialized")
	} else {
		fmt.Println("unicode.Categories properly initialized")
	}
}

Execution:
$ go build -buildmode=plugin ./p1 && go run main.go unicode.Categories not properly initialized

If I add a use of unicode.Categories in the ./main.go, it is properly initialized. For example:
_ = unicode.Categories["C"].

Note: when unicode.Categories is not properly initialized, It is not nil but an empty map.

What did you expect to see?

The unicode.Categories properly initialized.

What did you see instead?

An empty map.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 3, 2023
@candlerb
Copy link

candlerb commented Sep 3, 2023

I observe that if you add this line anywhere inside your top-level main() function, it works:

fmt.Println(len(unicode.Categories))

Or even just add this outside the main function:

var z = unicode.Categories

Hence it appears to be something to do with the triggering (or suppression) of package initialization. The documentation says:

When a plugin is first opened, the init functions of all packages not already part of the program are called.

You have already imported "unicode" to access the type unicode.RangeTable, but possibly because you're not accessing any values this has suppressed initialization?

@oszika
Copy link
Author

oszika commented Sep 3, 2023

Hum, I have not been able to reproduce this behavior with another variable.

If I add a init function using unicode.Categories in the plugin, there is no reason to remove its initialization.

func init() {
	fmt.Println(len(unicode.Categories))
}

But it prints 0.

$ go build -buildmode=plugin ./p1 && go run main.go
0
unicode.Categories not properly initialized

@candlerb
Copy link

candlerb commented Sep 3, 2023

Yes, what matters is whether the main program initializes unicode (which you can do using "var z = unicode.Categories" in the top-level main.go)

As a complete guess:

  • Main program thinks: "I'm not using any variables or functions from unicode, I don't need to initialize it"
  • Plugin thinks: "main program already links with unicode, I don't need to initialize it"

@candlerb
Copy link

candlerb commented Sep 5, 2023

I modified your reproducer to demonstrate a similar issue with the regexp library as reported on golang-nuts:

==> go.mod <==
module example.com

go 1.20

==> main.go <==
package main

import (
	"plugin"
	"regexp/syntax"
)

func main() {
	if syntax.Perl != 212 {
		panic("Unexpected flags")
	}

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

	f := s.(func())
	f()
}

==> p1/plugin.go <==
package main

import (
	"regexp"
)

func F() {
	_ = regexp.MustCompile(`\w+`)
}

func main() {}
$ go version
go version go1.21.0 darwin/amd64
$ go build -buildmode=plugin ./p1 && go run main.go
panic: regexp: Compile(`\w+`): error parsing regexp: invalid escape sequence: `\w`

goroutine 1 [running]:
regexp.MustCompile({0x107aa6eea, 0x3})
	/usr/local/go/src/regexp/regexp.go:319 +0xb4
example.com/p1.F()
	/Users/brian/tmp/go/62430/p1/plugin.go:8 +0x1f
main.main()
	/Users/brian/tmp/go/62430/main.go:23 +0x103
exit status 2

@cherrymui
Copy link
Member

cc @thanm

I haven't looked into this carefully, but I guess this is related to the map initialization change. My guess (without looking into details) is that as the main program imports the unicode package but not explicitly use the map, it initialize the unicode package but not the map. When the plugin is loaded, it sees the unicode package is already initialized by the main program, it doesn't initialize it again, leaving the map uninitialized. Maybe we need to initialize maps for programs that can use plugins.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 5, 2023
@cherrymui cherrymui added this to the Go1.22 milestone Sep 5, 2023
@tarm
Copy link

tarm commented Sep 5, 2023

Supporting what @cherrymui said, I bisected to e2ca417

https://go.dev/cl/463395

@thanm thanm self-assigned this Sep 6, 2023
@thanm
Copy link
Contributor

thanm commented Sep 6, 2023

Thanks for the report. I will take a look.

@gopherbot
Copy link

Change https://go.dev/cl/526115 mentions this issue: cmd/link: avoid deadcode of global map vars for programs using plugins

@candlerb
Copy link

candlerb commented Sep 7, 2023

The regexp case is almost certainly also due to an uninitialized map, in regexp/syntax/perl_groups.go

var perlGroup = map[string]charGroup{
        `\d`: {+1, code1},
        `\D`: {-1, code1},
        `\s`: {+1, code2},
        `\S`: {-1, code2},
        `\w`: {+1, code3},
        `\W`: {-1, code3},
}

@cherrymui
Copy link
Member

@gopherbot please backport this to Go 1.21. This is a regression that causes valid programs to run incorrectly.

@gopherbot
Copy link

Backport issue(s) opened: #62505 (for 1.21).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

Change https://go.dev/cl/526575 mentions this issue: cmd/link: avoid deadcode of global map vars for programs using plugins

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 20, 2023
gopherbot pushed a commit that referenced this issue Sep 20, 2023
…or programs using plugins

If a program imports the plugin package, the mechanisms in place for
detecting and deleting unused global map variables are no longer safe,
since it's possibly for a given global map var to be unreferenced in
the main program but referenced by a plugin. This patch changes the
linker to test for plugin use and to avoid removing any unused global
map variables if the main program could possibly load up a plugin.

Fixes #62505.
Updates #62430.

Change-Id: Ie00b18b681cb0d259e3c859ac947ade5778cd6c8
(cherry picked from commit 660620d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/526575
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
bradfitz pushed a commit to tailscale/go that referenced this issue Sep 25, 2023
…or programs using plugins

If a program imports the plugin package, the mechanisms in place for
detecting and deleting unused global map variables are no longer safe,
since it's possibly for a given global map var to be unreferenced in
the main program but referenced by a plugin. This patch changes the
linker to test for plugin use and to avoid removing any unused global
map variables if the main program could possibly load up a plugin.

Fixes golang#62505.
Updates golang#62430.

Change-Id: Ie00b18b681cb0d259e3c859ac947ade5778cd6c8
(cherry picked from commit 660620d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/526575
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
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
None yet
Development

No branches or pull requests

8 participants
@candlerb @tarm @dmitshur @gopherbot @thanm @cherrymui @oszika and others