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

cmd/compile: confusion between variable name "init" in method wrapper and function "init" #29610

Closed
jcajka opened this issue Jan 8, 2019 · 10 comments

Comments

Projects
None yet
5 participants
@jcajka
Copy link
Contributor

commented Jan 8, 2019

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

$ go version
go version devel +a1d5e8adfa Tue Jan 8 02:10:40 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Affects master/1.12, 1.11 and older are not affected

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

go get github.com/openshift/source-to-image
go build -buildmode pie github.com/openshift/source-to-image/cmd/s2i

What did you expect to see?

Build passing and producing working binary

What did you see instead?

go build -buildmode pie github.com/openshift/source-to-image/cmd/s2i
# github.com/openshift/source-to-image/cmd/s2i
github.com/openshift/source-to-image/pkg/scm.init: relocation target github.com/openshift/source-to-image/pkg/scm/git.init not defined for ABI0 (but is defined for ABIInternal)

@ianlancetaylor ianlancetaylor changed the title Unresolved relocation in buildmode pie on linux/amd64 while building source-to-image binary cmd/link: unresolved relocation in buildmode pie on linux/amd64 while building source-to-image binary Jan 8, 2019

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jan 8, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

The key difference with -buildmode=pie is that it disables concurrent compilation. I can recreate the problem without -buildmode=pie if I set GO19CONCURRENTCOMPILATION=0 in the environment.

CC @josharian

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

I have a small test case. Put this file into cmd/go/testdata/script/gc_init_confusion.txt and run go test -test.run=TestScript/gc_init_confusion cmd/go:

# Test for issue 29610: the compiler got confused about a wrapper
# variable named "init" and the actual "init" function.
env GO19CONCURRENTCOMPILATION=0
go build -o x main

-- a/a.go --
package a

type I interface {
	M(init bool)
}

var V I

func init() {
	V = nil
}

-- b/b.go --
package b

import "a"

type S struct {
	a.I
}

var V a.I

func init() {
	V = S{}
}

-- main/main.go --
package main

import "b"

var v b.S

func main() {}
@aclements

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Thanks. This indicates that the compiler is creating init's LSym before it gets marked as a function symbol. init is a weird symbol. There were several bugs like this early in the split ABI implementation. I guess I didn't get them all.

@aclements

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

When compiling package b, we construct the LSym for a.init before calling Sym.SetFunc to mark it as a function symbol. We call Linksym here:

cmd/compile/internal/types.(*Sym).Linksym(0xc0003541e0, 0xc0003ac2d0)
	/home/austin/go.dev/src/cmd/compile/internal/types/sym.go:96 +0x38b
cmd/compile/internal/gc.AddrAuto(0xc0000de970, 0xc00035e2a0)
	/home/austin/go.dev/src/cmd/compile/internal/gc/ssa.go:5530 +0x59
cmd/compile/internal/amd64.ssaGenValue(0xc000333300, 0xc00035e930)
	/home/austin/go.dev/src/cmd/compile/internal/amd64/ssa.go:909 +0x5e45
cmd/compile/internal/gc.genssa(0xc0000c46e0, 0xc0003ae620)
	/home/austin/go.dev/src/cmd/compile/internal/gc/ssa.go:5184 +0x541
cmd/compile/internal/gc.compileSSA(0xc0000c4580, 0x0)
	/home/austin/go.dev/src/cmd/compile/internal/gc/pgen.go:301 +0x378
cmd/compile/internal/gc.compile(0xc0000c4580)
	/home/austin/go.dev/src/cmd/compile/internal/gc/pgen.go:270 +0x566
cmd/compile/internal/gc.funccompile(0xc0000c4580)
	/home/austin/go.dev/src/cmd/compile/internal/gc/pgen.go:221 +0xc2
cmd/compile/internal/gc.genwrapper(0xc00035ccc0, 0xc00032ee40, 0xc00035ce40)
	/home/austin/go.dev/src/cmd/compile/internal/gc/subr.go:1607 +0x5b4
cmd/compile/internal/gc.methods(0xc00035ccc0, 0x0, 0x0, 0x0)
	/home/austin/go.dev/src/cmd/compile/internal/gc/reflect.go:415 +0x4d2
cmd/compile/internal/gc.uncommonSize(0xc00035ccc0, 0x10)
	/home/austin/go.dev/src/cmd/compile/internal/gc/reflect.go:68 +0x45
cmd/compile/internal/gc.dcommontype(0xc0003ae230, 0xc00035ccc0, 0x6300000000000010)
	/home/austin/go.dev/src/cmd/compile/internal/gc/reflect.go:846 +0x20d
cmd/compile/internal/gc.dtypesym(0xc00035ccc0, 0xc00035ccc0)
	/home/austin/go.dev/src/cmd/compile/internal/gc/reflect.go:1323 +0x96b
cmd/compile/internal/gc.dcommontype(0xc0003ae0e0, 0xc0003552c0, 0x15d0901)
	/home/austin/go.dev/src/cmd/compile/internal/gc/reflect.go:820 +0xee
cmd/compile/internal/gc.dtypesym(0xc0003552c0, 0x11194ea6a0d01)
	/home/austin/go.dev/src/cmd/compile/internal/gc/reflect.go:1348 +0xd10
cmd/compile/internal/gc.compile(0xc0000c4160)
	/home/austin/go.dev/src/cmd/compile/internal/gc/pgen.go:258 +0x176
cmd/compile/internal/gc.funccompile(0xc0000c4160)
	/home/austin/go.dev/src/cmd/compile/internal/gc/pgen.go:221 +0xc2
cmd/compile/internal/gc.Main(0xdf21d8)
	/home/austin/go.dev/src/cmd/compile/internal/gc/main.go:671 +0x3193
main.main()
	/home/austin/go.dev/src/cmd/compile/main.go:51 +0xad

This appears to be when we're compiling a wrapper for the a.I embedding in b.S. The compiler creates an LSym for the argument just so its string name can appear in the assembly dump (it doesn't otherwise care about this name).

It later calls SetFunc for the actual b.init symbol here:

cmd/compile/internal/types.(*Sym).SetFunc(0xc0003541e0, 0xc000354101)
	/home/austin/go.dev/src/cmd/compile/internal/types/sym.go:68 +0x1aa
cmd/compile/internal/gc.importobj(0xc0000945f0, 0x100000000004, 0xc0003541e0, 0x601, 0xc0003df3e0, 0x0)
	/home/austin/go.dev/src/cmd/compile/internal/gc/export.go:137 +0x1e5
cmd/compile/internal/gc.importfunc(0xc0000945f0, 0x100000000004, 0xc0003541e0, 0xc0003df3e0)
	/home/austin/go.dev/src/cmd/compile/internal/gc/export.go:161 +0x77
cmd/compile/internal/gc.(*importReader).doDecl(0xc0003e28c0, 0xc000349860)
	/home/austin/go.dev/src/cmd/compile/internal/gc/iimport.go:291 +0x6ed
cmd/compile/internal/gc.expandDecl(0xc000349860)
	/home/austin/go.dev/src/cmd/compile/internal/gc/iimport.go:49 +0x83
cmd/compile/internal/gc.resolve(0xc000349860, 0x0)
	/home/austin/go.dev/src/cmd/compile/internal/gc/typecheck.go:93 +0x73
cmd/compile/internal/gc.fninit(0xc000056be0, 0x2, 0x2)
	/home/austin/go.dev/src/cmd/compile/internal/gc/init.go:121 +0x626
cmd/compile/internal/gc.Main(0xdf21d8)
	/home/austin/go.dev/src/cmd/compile/internal/gc/main.go:678 +0x39b0
main.main()
	/home/austin/go.dev/src/cmd/compile/main.go:51 +0xad
@aclements

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

I think the reordering is a consequence of on-demand imports: we don't actually process the a.init function in a's export data until after we've processed the a.I type and already compiled a wrapper for it.

Somehow we need to mark a.init as a function symbol before resolving it to an LSym. Or we need something more radical... Maybe a separate hash in Ctxt for function and non-function LSyms?

/cc @griesemer @mdempsky for thoughts and because LSyms make me sad.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 9, 2019

Change https://golang.org/cl/157017 mentions this issue: cmd/compile: separate data and function LSyms

@ianlancetaylor ianlancetaylor changed the title cmd/link: unresolved relocation in buildmode pie on linux/amd64 while building source-to-image binary cmd/compile: confusion between variable name "init" in method wrapper and function "init" Jan 9, 2019

@jcajka

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@aclements @ianlancetaylor thank you for quick fix

for the record the https://golang.org/cl/157017 CL is fixing the issue for me

@dmitshur

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

@aclements @ianlancetaylor This issue is currently labeled release-blocker with milestone Go 1.12. I can't tell whether it's meant to be a blocker for the final Go 1.12 release, or also for Go 1.12 RC 1.

Do you think it should block Go 1.12 RC 1 from being made? If not, I was planning on making Go 1.12 RC 1 tomorrow. /cc @andybons

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

This is a blocker for the final 1.12 release but it is not a blocker for 1.12rc1.

@gopherbot gopherbot closed this in a2e7957 Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.