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.

Sign up for GitHub

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: sanity check in makePreinlineDclMap can fail #38698

Closed
josharian opened this issue Apr 27, 2020 · 5 comments
Closed

cmd/compile: sanity check in makePreinlineDclMap can fail #38698

josharian opened this issue Apr 27, 2020 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@josharian
Copy link
Contributor 

package p

func ff( /*line :10*/ x string) bool {
	{
		var _ /*line :10*/, x int
		_ = x
	}
	return x == ""
}

func h(a string) bool {
	return ff(a)
}
$ go tool compile -S bug.go
:17: internal compiler error: child dcl collision on symbol x within "".ff


goroutine 1 [running]:
runtime/debug.Stack(0x19892e0, 0xc0000ac008, 0x0)
	/Users/josh/go/tip/src/runtime/debug/stack.go:24 +0x9d
cmd/compile/internal/gc.Fatalf(0x1829099, 0x2b, 0xc0003c2f68, 0x2, 0x2)
	/Users/josh/go/tip/src/cmd/compile/internal/gc/subr.go:193 +0x1aa
cmd/compile/internal/gc.makePreinlineDclMap(0xc0003b4580, 0xc0003c31b8)
	/Users/josh/go/tip/src/cmd/compile/internal/gc/dwinl.go:225 +0x337
cmd/compile/internal/gc.assembleInlines(0xc0003b4580, 0xc0003dc4c0, 0x3, 0x4, 0x4, 0xc0000b2cc0, 0x3)
	/Users/josh/go/tip/src/cmd/compile/internal/gc/dwinl.go:106 +0x52d
cmd/compile/internal/gc.debuginfo(0xc0003b4580, 0xc0003b4680, 0x18056c0, 0xc00039e3c0, 0x20, 0x30, 0x30, 0xc0003d31d0, 0xc00008ed60, 0x10)
	/Users/josh/go/tip/src/cmd/compile/internal/gc/pgen.go:434 +0x9b3
cmd/internal/obj.(*Link).DwarfAbstractFunc(0xc0001701a0, 0x18056c0, 0xc00039e3c0, 0xc0003b4580, 0x0, 0x0)
	/Users/josh/go/tip/src/cmd/internal/obj/objfile.go:675 +0xb6
cmd/compile/internal/gc.genAbstractFunc(0xc0003b4580)
	/Users/josh/go/tip/src/cmd/compile/internal/gc/dwinl.go:195 +0xb8
cmd/internal/obj.(*DwarfFixupTable).Finalize(0xc0000b1140, 0x0, 0x0, 0x0)
	/Users/josh/go/tip/src/cmd/internal/obj/objfile.go:926 +0x224
cmd/compile/internal/gc.Main(0x18357e0)
	/Users/josh/go/tip/src/cmd/compile/internal/gc/main.go:769 +0x3b67
main.main()
	/Users/josh/go/tip/src/cmd/compile/main.go:52 +0xac

This code is obviously contrived/ridiculous, but the point remains--in the presence of line directives, we cannot safely assume that the contents of any given source positions are unique.

cc @thanm
@josharian josharian changed the title cmd/compile: sanity check in makePreinlineDclMap can fail on real code cmd/compile: sanity check in makePreinlineDclMap can fail Apr 27, 2020
@thanm
Copy link
Contributor

thanm commented Apr 27, 2020

Yes, this is a drawback of the current implementation -- it assumes that variables and parameters have unit source positions.

@thanm thanm self-assigned this Apr 27, 2020
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 27, 2020
@andybons andybons added this to the Unplanned milestone Apr 27, 2020
josharian added a commit to josharian/go that referenced this issue Apr 30, 2020
@josharian
cmd/compile: make all positions x.go:1:1
1a3e66c 
This shrinks binaries by about 6%, if you're using -ldflags=-w.

Backtraces should still work, with function names and PCs and arguments,
but all line numbers will display as x.go:1.

This breaks any tests that depend on correct line numbers.
For example, net/http:

--- FAIL: TestWriteHeaderNoCodeCheck_h1hijack (0.00s)
    x.go:1: stderr output = "http: response.WriteHeader on hijacked connection from net/http_test.testWriteHeaderAfterWrite.func1 (x.go:1)"; want "http: response.WriteHeader on hijacked connection from net/http_test.testWriteHeaderAfterWrite.func1 (clientserver_test.go:"
--- FAIL: TestWriteHeaderNoCodeCheck_h1 (0.01s)
    x.go:1: stderr output = "http: superfluous response.WriteHeader call from net/http_test.testWriteHeaderAfterWrite.func1 (x.go:1)"; want "http: superfluous response.WriteHeader call from net/http_test.testWriteHeaderAfterWrite.func1 (clientserver_test.go:"
FAIL
FAIL	net/http	13.940s

Remove an invariant check in dwinl.go that's broken by this.
See golang#38698.
That may break debugging. But if you care enough about binary
size to use this, you're already stripping binaries with -ldflags=-w.

Remove a security check around cgo pragma placements.
Unfortunate, but if you're using a hacked toolchain,
you should always make sure your code still builds and passes tests
with a regular toolchain.

Change-Id: I56c7540ea856a3bdccf8d44b5c3dbd4803545015
@josharian
Copy link
Contributor Author

@thanm does https://golang.org/cl/294289 by chance fix this issue as well?

@josharian
Copy link
Contributor Author

(If so, it’d still be good to add it as an additional test.)

@thanm
Copy link
Contributor

thanm commented Feb 21, 2021

Yes, the change from https://golang.org/cl/294289 does indeed fix this issue. Thanks for spotting that.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/295129 mentions this issue: test: add test for issue 38698

@golang golang locked and limited conversation to collaborators Feb 22, 2022
@rsc rsc unassigned thanm Jun 23, 2022
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.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@josharian @andybons @gopherbot @thanm