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

Jump to bottom

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

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

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

jcajka opened this issue Jan 8, 2019 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.12

Comments

@jcajka
Copy link
Contributor

jcajka 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 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jan 8, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jan 8, 2019
@ianlancetaylor
Copy link
Contributor

CC @aclements

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link

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
Copy link
Contributor Author

jcajka 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
Copy link
Contributor

dmitshur 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
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
5 participants
@dmitshur @aclements @ianlancetaylor @jcajka @gopherbot