-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: cleanup code paths that aren't needed now that -p is required #51734
Comments
I think point 2 will be the right fix to revert/improve what CL 389474 did. |
I'm going to send a CL for this. |
Hmm, simply changing
It seems to need more works than I expect, so feel free to go ahead if you have one, otherwise, I will take another look when I'm back with my laptop. |
@cuonglm At least step 1 needs to happen before we can do step 2. :) |
Change https://go.dev/cl/393715 mentions this issue: |
Change https://go.dev/cl/393716 mentions this issue: |
Change https://go.dev/cl/393896 mentions this issue: |
bug302 compiles p.go with -p=p, and then manually creates a pp.a archive, and imports it as both "p" and "pp". This is a misuse of cmd/compile's -p flag, and it isn't representative of how any actual Go build systems work anyway. This test made sense back when cmd/compile still wrote out bare object files, which was then split into separate __.PKGDEF and _go_.o archive entries when added to a pack archive. But since CL 102236, cmd/compile always writes out pack files. Updates #51734. Change-Id: I4b5de22d348ecc0a72c98b512351c2d267c77736 Reviewed-on: https://go-review.googlesource.com/c/go/+/393896 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Change https://go.dev/cl/403757 mentions this issue: |
Change https://go.dev/cl/406059 mentions this issue: |
Change https://go.dev/cl/406056 mentions this issue: |
Change https://go.dev/cl/406057 mentions this issue: |
Change https://go.dev/cl/406055 mentions this issue: |
Change https://go.dev/cl/406058 mentions this issue: |
Change https://go.dev/cl/405995 mentions this issue: |
Change https://go.dev/cl/405899 mentions this issue: |
Change https://go.dev/cl/405900 mentions this issue: |
Change https://go.dev/cl/405901 mentions this issue: |
When the local package has an explicit name instead of "", this is necessary to get past a cgo plugin test that fails because of a package signature mismatch. There's something questionable going on in the package hash generation, and in particular it went wrong here. Updating the sort order helps. This CL is a prerequisite for a pending code cleanup, https://go-review.googlesource.com/c/go/+/393715 Updates #51734. The failure: GOROOT/misc/cgo/testplugin$ go test . mkdir -p $TMPDIR/src/testplugin rsync -a testdata/ $TMPDIR/src/testplugin echo 'module testplugin' > $TMPDIR/src/testplugin/go.mod mkdir -p $TMPDIR/alt/src/testplugin rsync -a altpath/testdata/ $TMPDIR/alt/src/testplugin echo 'module testplugin' > $TMPDIR/alt/src/testplugin/go.mod cd $TMPDIR/alt/src/testplugin ( PWD=$TMPDIR/alt/src/testplugin GOPATH=$TMPDIR/alt go build -gcflags '' -buildmode=plugin -o $TMPDIR/src/testplugin/plugin-mismatch.so ./plugin-mismatch ) cd $TMPDIR/src/testplugin ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go build -gcflags '' -buildmode=plugin ./plugin1 ) ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go build -gcflags '' -buildmode=plugin ./plugin2 ) cp plugin2.so plugin2-dup.so ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go build -gcflags '' -buildmode=plugin -o=sub/plugin1.so ./sub/plugin1 ) ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go build -gcflags '' -buildmode=plugin -o=unnamed1.so ./unnamed1/main.go ) ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go build -gcflags '' -buildmode=plugin -o=unnamed2.so ./unnamed2/main.go ) ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go build -gcflags '' -o host.exe ./host ) ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go run -gcflags '' ./checkdwarf/main.go plugin2.so plugin2.UnexportedNameReuse ) ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin go run -gcflags '' ./checkdwarf/main.go ./host.exe main.main ) ( PWD=$TMPDIR/src/testplugin GOPATH=$TMPDIR LD_LIBRARY_PATH=$TMPDIR/src/testplugin ./host.exe ) --- FAIL: TestRunHost (0.02s) plugin_test.go:187: ./host.exe: exit status 1 2022/05/13 11:26:37 plugin.Open failed: plugin.Open("plugin1"): plugin was built with a different version of package runtime and many more after that. Change-Id: I0780decc5bedeea640ed0b3710867aeda5b3f725 Reviewed-on: https://go-review.googlesource.com/c/go/+/405995 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This (1) "just makes sense" and (2) avoids a weird bug in some name-dependent calling conventions in wasm code generation, when the local pkg has a real name instead of "". The calling conventions are triggered for a "wrapper" function, and somehow an abiwrapper was taken to be a "wrapper" function, resulting in the use of an invalid register. But abiwrapping has no business being in js/wasm code generation, so just turn that off. Updates #51734. For posterity, that crash is: GOSSAFUNC=wasmTruncU GOMAXPROCS=1 \ GOOS=js GOARCH=wasm GOEXPERIMENT=regabi,regabiargs /Users/drchase/work/go-quick/bin/go build \ -gcflags=all=-d=abiwrap -o a.exe \ GOROOT/test/abi/bad_select_crash.go <autogenerated>:1: internal compiler error: panic: bad Get: invalid register goroutine 1 [running]: runtime/debug.Stack() runtime/debug/stack.go:24 +0x65 cmd/compile/internal/base.FatalfAt({0xc80?, 0x0?}, {0x195c85e, 0x9}, {0xc005ef72c8, 0x1, 0x1}) /Users/drchase/work/go-quick/src/cmd/compile/internal/base/print.go:227 +0x1d7 cmd/compile/internal/base.Fatalf(...) /Users/drchase/work/go-quick/src/cmd/compile/internal/base/print.go:196 cmd/compile/internal/gc.handlePanic() /Users/drchase/work/go-quick/src/cmd/compile/internal/gc/main.go:48 +0x85 panic({0x18bf3c0, 0x1ad0430}) runtime/panic.go:854 +0x26d cmd/internal/obj/wasm.assemble(0xc0000f8200, 0xc001c74880, 0x0?) /Users/drchase/work/go-quick/src/cmd/internal/obj/wasm/wasmobj.go:920 +0x1958 cmd/internal/obj.Flushplist(0xc0000f8200, 0xc005ef79a8, 0xc0022264c0, {0x7ff7bfefdd17, 0x7}) /Users/drchase/work/go-quick/src/cmd/internal/obj/plist.go:151 +0x784 cmd/compile/internal/objw.(*Progs).Flush(...) /Users/drchase/work/go-quick/src/cmd/compile/internal/objw/prog.go:124 cmd/compile/internal/ssagen.Compile(0xc000707e00, 0xc001b4d620?) /Users/drchase/work/go-quick/src/cmd/compile/internal/ssagen/pgen.go:208 +0x495 cmd/compile/internal/gc.compileFunctions.func4.1(0xc005ef7a01?) /Users/drchase/work/go-quick/src/cmd/compile/internal/gc/compile.go:153 +0x3a cmd/compile/internal/gc.compileFunctions.func2(0x0?) /Users/drchase/work/go-quick/src/cmd/compile/internal/gc/compile.go:125 +0x1e cmd/compile/internal/gc.compileFunctions.func4({0xc004685000, 0x79f, 0xa00?}) /Users/drchase/work/go-quick/src/cmd/compile/internal/gc/compile.go:152 +0x53 cmd/compile/internal/gc.compileFunctions() /Users/drchase/work/go-quick/src/cmd/compile/internal/gc/compile.go:163 +0x162 cmd/compile/internal/gc.Main(0x198d3d8) /Users/drchase/work/go-quick/src/cmd/compile/internal/gc/main.go:297 +0x108a main.main() /Users/drchase/work/go-quick/src/cmd/compile/main.go:55 +0xdd Change-Id: I79f039e2494f78efba60e52ab1110d62656fb7ef Reviewed-on: https://go-review.googlesource.com/c/go/+/405899 Run-TryBot: David Chase <drchase@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Panic avoids any write barriers in the runtime by checking first and throwing if called inappropriately, so it is "okay". Adding this annotation repairs recursive write barrier checking, which becomes more thorough when the local package naming convention is changed from "" to the actual package name. This CL is a prerequisite for a pending code cleanup, https://go-review.googlesource.com/c/go/+/393715 Updates #51734. Change-Id: If831a3598c6c8cd37a8e9ba269f822cd81464a13 Reviewed-on: https://go-review.googlesource.com/c/go/+/405900 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: David Chase <drchase@google.com>
runtime code for js contains possible write barriers that fail the nowritebarrierrec check when internal local package naming conventions are changed. The problem was there all already; this allows the code to compile, and it seems to work anyway in the (single-threaded) js/wasm environment. The offending operations are noted with TODO, which is an improvement. runtime code for plan9 contained an apparent allocation that was not really an allocation; rewrite to remove the potential allocation to avoid nowritebarrierrec problems. This CL is a prerequisite for a pending code cleanup, https://go-review.googlesource.com/c/go/+/393715 Updates #51734. Change-Id: I93f31831ff9b92632137dd7b0055eaa721c81556 Reviewed-on: https://go-review.googlesource.com/c/go/+/405901 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This CL moves the call to base.ParseFlags() earlier in compiler startup. This is necessary so CL 393715 can use base.Ctxt.Pkgpath to construct types.LocalPkg. Updates #51734. Change-Id: I9f5f75dc9d5fd1b1d22e98523efc95e6cec64385 Reviewed-on: https://go-review.googlesource.com/c/go/+/406055 Reviewed-by: David Chase <drchase@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
….Path == "" Replace `pkg.Path == ""` check with `pkg == types.LocalPkg`. This is a prep refactoring for CL 393715, which will properly initialize types.LocalPkg. Updates #51734. Change-Id: I7a5428ef1f422de396762b6bc6d323992834b27c Reviewed-on: https://go-review.googlesource.com/c/go/+/406056 Reviewed-by: David Chase <drchase@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
….Path == "" The indexed export data format encodes the local package's path as "", because that's historically how we've represented it within cmd/compile. The format also requires the local package to be first in the exported list of packages, and was implicitly relying on "" sorting before other, non-empty package paths. We can't change the format without breaking existing importers (e.g., go/internal/gcimporter), but we can at least remove the dependency on LocalPkg.Path being "". Prep refactoring for CL 393715. Updates #51734. Change-Id: I6dd4eafd2d538f4e81376948ef9e92fc44a5462a Reviewed-on: https://go-review.googlesource.com/c/go/+/406057 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: David Chase <drchase@google.com>
…ymbols Not strictly necessary for CL 393715, but this is necessary if we want to remove the logic from cmd/internal/obj for substituting `""` in linker symbol names. Updates #51734. Change-Id: Ib13cb12fa3973389ca0c1c9a9209e00c30dc9431 Reviewed-on: https://go-review.googlesource.com/c/go/+/406058 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com>
…Path == "" Prep refactoring for CL 393715, after which LocalPkg.Path will no longer be the empty string. Instead of testing `pkg.Path == ""`, we can just test `pkg == LocalPkg`. Updates #51734. Change-Id: I74fff7fb383e275c9f294389d30b2220aced19e0 Reviewed-on: https://go-review.googlesource.com/c/go/+/406059 Reviewed-by: David Chase <drchase@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Change https://go.dev/cl/406315 mentions this issue: |
Change https://go.dev/cl/406674 mentions this issue: |
This test is currently overly sensitive to compiler optimizations, because inlining can affect the order in which cmd/link emits field references. The order doesn't actually matter though, so this CL just tweaks the test to sort the tracked fields before printing them. Updates #51734. Change-Id: I3b65ca265856b2e1102f40406d5ce34610c70d40 Reviewed-on: https://go-review.googlesource.com/c/go/+/406674 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: David Chase <drchase@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Historically, the compiler set types.LocalPkg.Path to "", so a lot of compiler code checks for this, and then falls back to using base.Ctxt.Pkgpath instead. Since CL 393715, we now initialize types.LocalPkg.Path to base.Ctxt.Pkgpath, so these code paths can now simply rely on Pkg.Path always being meaningful. Updates #51734. Change-Id: I0aedbd7cf8e14edbfef781106a9510344d468f2c Reviewed-on: https://go-review.googlesource.com/c/go/+/406317 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Auto-Submit: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: David Chase <drchase@google.com>
This hack is no longer needed since CL 393715, because LocalPkg.Prefix is set correctly, so when we write out instantiated objects/types into the export data, they'll already have a proper name. Updates #51734. Change-Id: I26cfa522f1bfdfd162685509757f51093b8b92e0 Reviewed-on: https://go-review.googlesource.com/c/go/+/406318 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Now that Type.LinkString always returns a fully unique string ID, we can use it in TypeHash to avoid collisions between instantiations of the same generic type. Updates #51734. Change-Id: I38cb396c88259be7afa44bd4333124ca98666c3f Reviewed-on: https://go-review.googlesource.com/c/go/+/393716 Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Okay, all of the outstanding cleanup CLs for this have landed. In particular, LinkString is now used for type hashing. The only remaining cleanup is removing package height. That can wait until Go 1.20. |
Change https://go.dev/cl/406775 mentions this issue: |
Change https://go.dev/cl/410337 mentions this issue: |
Change https://go.dev/cl/410342 mentions this issue: |
Change https://go.dev/cl/410347 mentions this issue: |
Change https://go.dev/cl/410654 mentions this issue: |
Since CL 393715, the path of package being compiled is now always known, so symbols can be sorted by package path instead of package height. Updates #51734 Change-Id: Ie543e2fdef4b93f3f0b97c6bcec0a4dcff788f2c Reviewed-on: https://go-review.googlesource.com/c/go/+/410654 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
After CL 410654, symbols are now sorted by package path, package height is not necessary anymore. Updates #51734 Change-Id: I976edd2e574dda68eb5c76cf95645b9dce051393 Reviewed-on: https://go-review.googlesource.com/c/go/+/410342 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
Same as CL 410342, but for types2. Updates #51734 Change-Id: I6d6cb8fbb7567d3acf0b8cec0fa74f1344b56a1c Reviewed-on: https://go-review.googlesource.com/c/go/+/410347 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@google.com> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Since CL 393715, the path of package being compiled is now always known, so symbols can be sorted by package path instead of package height. Updates golang#51734 Change-Id: Ie543e2fdef4b93f3f0b97c6bcec0a4dcff788f2c Reviewed-on: https://go-review.googlesource.com/c/go/+/410654 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
After CL 410654, symbols are now sorted by package path, package height is not necessary anymore. Updates golang#51734 Change-Id: I976edd2e574dda68eb5c76cf95645b9dce051393 Reviewed-on: https://go-review.googlesource.com/c/go/+/410342 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
Same as CL 410342, but for types2. Updates golang#51734 Change-Id: I6d6cb8fbb7567d3acf0b8cec0fa74f1344b56a1c Reviewed-on: https://go-review.googlesource.com/c/go/+/410347 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@google.com> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
I think this is more or less done now. This issue has served its purpose. |
Since CL 391014 (a987aaf), cmd/compile now requires the -p flag. There's a lot of code to handle code paths that aren't needed any more.
At least a few cleanups I can think of:
""
."".
in any linker symbols.Perhaps other opportunities.
The iexport file format uses
""
for the current package too, which probably needs to be retained for compatibility with legacy importers./cc @randall77 @dr2chase @cuonglm
The text was updated successfully, but these errors were encountered: