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: cleanup code paths that aren't needed now that -p is required #51734

Open
mdempsky opened this issue Mar 16, 2022 · 25 comments
Open

cmd/compile: cleanup code paths that aren't needed now that -p is required #51734

mdempsky opened this issue Mar 16, 2022 · 25 comments
Labels
NeedsFix
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 16, 2022

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:

  1. Initialize types.LocalPkg with base.Ctxt.Pkgpath instead of "".
  2. Change types.TypeHash to use t.LinkString instead of fmtTypeIDHash.
  3. Get rid of "package height" and just sort method sets on identifier and package path. (We can just write package height as 0 in the iexport data format; public importers ignore it anyway.)
  4. Change cmd/link (or cmd/internal/obj?) to Fatalf if it sees "". 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

@mdempsky mdempsky added the NeedsFix label Mar 16, 2022
@mdempsky mdempsky added this to the Go1.19 milestone Mar 16, 2022
@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 16, 2022

I think point 2 will be the right fix to revert/improve what CL 389474 did.

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Mar 17, 2022

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.

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Mar 17, 2022

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 types.TypeHash to use t.LinkString() make go run run.go panic with:

panic: interface conversion: reflect.Type is *reflect.rtype, not *reflect.rtype (types from different scopes)

goroutine 1 [running]:
reflect.PointerTo(...)
	/home/cuonglm/sources/go/src/reflect/type.go:1442
encoding/json.newTypeEncoder({0x566c30?, 0x516440?}, 0x1?)
	/home/cuonglm/sources/go/src/encoding/json/encode.go:422 +0x3e5
encoding/json.typeEncoder({0x566c30?, 0x516440})
	/home/cuonglm/sources/go/src/encoding/json/encode.go:404 +0x16b
encoding/json.typeFields({0x566c30, 0x528440})
	/home/cuonglm/sources/go/src/encoding/json/encode.go:1385 +0x668
encoding/json.cachedTypeFields({0x566c30?, 0x528440})
	/home/cuonglm/sources/go/src/encoding/json/encode.go:1417 +0xab
encoding/json.(*decodeState).object(0xc0000022a8, {0x5123c0?, 0xc00002e5c0?, 0xc000061980?})
	/home/cuonglm/sources/go/src/encoding/json/decode.go:654 +0x21e
encoding/json.(*decodeState).value(0xc0000022a8, {0x5123c0?, 0xc00002e5c0?, 0xc0000619d0?})
	/home/cuonglm/sources/go/src/encoding/json/decode.go:373 +0x45
encoding/json.(*decodeState).unmarshal(0xc0000022a8, {0x5123c0?, 0xc00002e5c0?})
	/home/cuonglm/sources/go/src/encoding/json/decode.go:180 +0x1de
encoding/json.(*Decoder).Decode(0xc000002280, {0x5123c0, 0xc00002e5c0})
	/home/cuonglm/sources/go/src/encoding/json/stream.go:73 +0x177
main.glob..func1()
	/home/cuonglm/sources/go/test/run.go:68 +0x1a5
main.init()
	/home/cuonglm/sources/go/test/run.go:75 +0x3f9
exit status 2

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.

cc @mdempsky @randall77

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Mar 17, 2022

@cuonglm At least step 1 needs to happen before we can do step 2. :)

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 18, 2022

Change https://go.dev/cl/393715 mentions this issue: cmd/compile: set LocalPkg.Path to -p flag

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 18, 2022

Change https://go.dev/cl/393716 mentions this issue: cmd/compile/internal/types: use Type.LinkString in TypeHash

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 18, 2022

Change https://go.dev/cl/393896 mentions this issue: test: remove obsolete test cases that misuse -p

gopherbot pushed a commit that referenced this issue Mar 24, 2022
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>
@gopherbot
Copy link

@gopherbot gopherbot commented May 3, 2022

Change https://go.dev/cl/403757 mentions this issue: cmd/compile: combo set LocalPkg.Path to -p flag

@gopherbot
Copy link

@gopherbot gopherbot commented May 12, 2022

Change https://go.dev/cl/406059 mentions this issue: cmd/compile/internal/ir: remove PkgFuncName assumption that LocalPkg.Path == ""

@gopherbot
Copy link

@gopherbot gopherbot commented May 12, 2022

Change https://go.dev/cl/406056 mentions this issue: cmd/compile/internal/noder: remove unified IR assumptions on LocalPkg.Path == ""

@gopherbot
Copy link

@gopherbot gopherbot commented May 12, 2022

Change https://go.dev/cl/406057 mentions this issue: cmd/compile/internal/typecheck: remove iexport assumption of LocalPkg.Path == ""

@gopherbot
Copy link

@gopherbot gopherbot commented May 12, 2022

Change https://go.dev/cl/406055 mentions this issue: cmd/compile/internal/gc: parse command-line flags earlier

@gopherbot
Copy link

@gopherbot gopherbot commented May 12, 2022

Change https://go.dev/cl/406058 mentions this issue: cmd/compile/internal/staticdata: remove use of "" in embed linker symbols

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/405995 mentions this issue: cmd/compile: sort named types before unnamed in reflect

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/405899 mentions this issue: buildcfg: disable regabiwrappers along with regabiargs

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/405900 mentions this issue: runtime: add go:yeswritebarrierrec to panic functions

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/405901 mentions this issue: runtime: tweak js and plan9 to avoid/disable write barrier & gc problems

gopherbot pushed a commit that referenced this issue May 13, 2022
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>
gopherbot pushed a commit that referenced this issue May 13, 2022
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>
gopherbot pushed a commit that referenced this issue May 13, 2022
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>
gopherbot pushed a commit that referenced this issue May 13, 2022
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>
gopherbot pushed a commit that referenced this issue May 13, 2022
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>
gopherbot pushed a commit that referenced this issue May 13, 2022
….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>
gopherbot pushed a commit that referenced this issue May 13, 2022
….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>
gopherbot pushed a commit that referenced this issue May 13, 2022
…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>
gopherbot pushed a commit that referenced this issue May 13, 2022
…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>
@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/406315 mentions this issue: cmd/compile/internal/test: don't initialize LocalPkg.Path to ""

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/406317 mentions this issue: cmd/compile: remove base.Ctxt.Pkgpath fallback paths

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/406318 mentions this issue: cmd/compile/internal/typecheck: remove "name" handling in iimport.go

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/406316 mentions this issue: runtime: mark panicshift and panicdivide as //go:yeswritebarrierrec

@gopherbot
Copy link

@gopherbot gopherbot commented May 14, 2022

Change https://go.dev/cl/406319 mentions this issue: cmd/compile/internal/noder: remove another unified IR assumption about types.LocalPkg.Path

gopherbot pushed a commit that referenced this issue May 16, 2022
Updates #51734.

Change-Id: I80c4e9ae7e17172f26cd32509ce0cb5b4d311819
Reviewed-on: https://go-review.googlesource.com/c/go/+/406315
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue May 16, 2022
When compiling package runtime, cmd/compile logically has two copies
of package runtime: the actual source files being compiled, and the
internal description used for emitting compiler-generated calls.

Notably, CL 393715 will cause the compiler's write barrier validation
to start recognizing that compiler-generated calls are actually calls
to the corresponding functions from the source package. And today,
there are some code paths in nowritebarrierrec code paths that
actually end up generating code to call panicshift or panicdivide.

In preparation, this CL marks those functions as
//go:yeswritebarrierrec. We probably want to actually cleanup those
code paths to avoid these calls actually (e.g., explicitly convert
shift count expressions to an unsigned integer type). But for now,
this at least unblocks CL 393715 while preserving the status quo.

Updates #51734.

Change-Id: I01f89adb72466c0260a9cd363e3e09246e39cff9
Reviewed-on: https://go-review.googlesource.com/c/go/+/406316
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue May 16, 2022
…ified

The TestForCompiler/LookupCustom test tries to read in the export data
for "math/big", but with a package path of "math/bigger" instead. This
has historically worked because the export data formats were designed
to not assume the package's own path, but I expect we can safely
remove support for this now.

However, since that would be a user-visible change, for now just
disable the test for GOEXPERIMENT=unified so we can land CL 393715. We
can revisit whether it's actually safe to break that go/importer use
case later.

Updates #51734.

Change-Id: I5e89314511bd1352a9f5e14a2e218a5ab00cab3c
Reviewed-on: https://go-review.googlesource.com/c/go/+/406319
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue May 16, 2022
Since CL 391014, cmd/compile now requires the -p flag to be set the
build system. This CL changes it to initialize LocalPkg.Path to the
provided path, rather than relying on writing out `"".` into object
files and expecting cmd/link to substitute them.

However, this actually involved a rather long tail of fixes. Many have
already been submitted, but a few notable ones that have to land
simultaneously with changing LocalPkg:

1. When compiling package runtime, there are really two "runtime"
packages: types.LocalPkg (the source package itself) and
ir.Pkgs.Runtime (the compiler's internal representation, for synthetic
references). Previously, these ended up creating separate link
symbols (`"".xxx` and `runtime.xxx`, respectively), but now they both
end up as `runtime.xxx`, which causes lsym collisions (notably
inittask and funcsyms).

2. test/codegen tests need to be updated to expect symbols to be named
`command-line-arguments.xxx` rather than `"".foo`.

3. The issue20014 test case is sensitive to the sort order of field
tracking symbols. In particular, the local package now sorts to its
natural place in the list, rather than to the front.

Thanks to David Chase for helping track down all of the fixes needed
for this CL.

Updates #51734.

Change-Id: Iba3041cf7ad967d18c6e17922fa06ba11798b565
Reviewed-on: https://go-review.googlesource.com/c/go/+/393715
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented May 16, 2022

Change https://go.dev/cl/406674 mentions this issue: test: fix issue20014 for noopt builder

gopherbot pushed a commit that referenced this issue May 16, 2022
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>
gopherbot pushed a commit that referenced this issue May 16, 2022
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>
gopherbot pushed a commit that referenced this issue May 16, 2022
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>
gopherbot pushed a commit that referenced this issue May 16, 2022
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>
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented May 16, 2022

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.

@mdempsky mdempsky removed this from the Go1.19 milestone May 16, 2022
@mdempsky mdempsky added this to the Go1.20 milestone May 16, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented May 17, 2022

Change https://go.dev/cl/406775 mentions this issue: cmd/compile: remove package height

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

4 participants