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: panic: runtime error: invalid memory address or nil pointer dereference [dev.regabi] #43701

Closed
ALTree opened this issue Jan 14, 2021 · 15 comments

Comments

@ALTree
Copy link
Member

@ALTree ALTree commented Jan 14, 2021

dev.regabi branch

$ gotip version
go version devel +35b9c66601 Thu Jan 14 17:35:39 2021 +0000 linux/amd64
package p

func f() {
	var st struct {
		s string
		i int16
	}
	_ = func() {
		var m map[int16]int
		m[st.i] = 0
	}
}
$ gotip tool compile crash.go 
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x51bd59]

goroutine 21 [running]:
cmd/internal/obj.(*LSym).prepwrite(0x0, 0xc00031c000, 0x0, 0x8)
	/home/alberto/go/src/cmd/internal/obj/data.go:71 +0x119
cmd/internal/obj.(*LSym).WriteInt(0x0, 0xc00031c000, 0x0, 0x8, 0x1)
	/home/alberto/go/src/cmd/internal/obj/data.go:100 +0x4d
cmd/compile/internal/objw.UintN(0x0, 0x0, 0x1, 0x8, 0x1)
	/home/alberto/go/src/cmd/compile/internal/objw/objw.go:34 +0x6e
cmd/compile/internal/objw.Uintptr(...)
	/home/alberto/go/src/cmd/compile/internal/objw/objw.go:27
cmd/compile/internal/ssagen.emitStackObjects(0xc000389da0, 0xc0000ea150)
	/home/alberto/go/src/cmd/compile/internal/ssagen/ssa.go:6492 +0x22a
cmd/compile/internal/ssagen.genssa(0xc000083500, 0xc0000ea150)
	/home/alberto/go/src/cmd/compile/internal/ssagen/ssa.go:6525 +0xf2
cmd/compile/internal/ssagen.Compile(0xc0000fa420, 0x0)
	/home/alberto/go/src/cmd/compile/internal/ssagen/pgen.go:178 +0x2a5
cmd/compile/internal/gc.compileFunctions.func2.1(0xc000308310, 0xc0000fa420, 0xc0000b8550, 0xc0000b6ce0)
	/home/alberto/go/src/cmd/compile/internal/gc/compile.go:143 +0x65
created by cmd/compile/internal/gc.compileFunctions.func2
	/home/alberto/go/src/cmd/compile/internal/gc/compile.go:141 +0x8e

cc @mdempsky @cuonglm

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jan 15, 2021

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 15, 2021

Thanks. I'm guessing the test case tickles some mismatch between the logic in gc.prepareFunc for deciding whether to create the stkobj symbol, and the code in ssagen.emitStackObjects.

I wonder if we can just move that code from gc.prepareFunc to ssagen.emitStackObjects? The comments say that code isn't safe to run during SSA generation, but I don't think that's true anymore. At least looking at them briefly, I see mutexes being used. I'd recommend trying to relocate the code and see if it passes the racecompile trybot.

It would also be interesting to know what variables are being caught by ssagen.emitStackObjects but missed by gc.prepareFunc. Not because it's necessary for the fix per se, but to double-check that we understand the failure.

@cuonglm Do you want to look into fixing this?

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jan 15, 2021

@mdempsky I did some investigation already, the problem is after CL 281352, the closure var &st isn't pushed to fn.Dcl in frontend anymore, so gc.prepareFunc won't see it. In SSA, we did add it to fn.Dcl in buildssa, then calling emitStackObjects inside genssa.

I will be AFK for couple of hours, I can try a fix when I'm back.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 15, 2021

@cuonglm Thanks. No rush on the fix if you're interested in it.

Hm, I can believe that's the issue, but I'm not immediately seeing how it led to this issue. Do you mind getting the output from ir.Dump("dcl", fn.Dcl) in both gc.prepareFunc and ssagen.emitStackObjects, both before and after that CL and maybe at tip of dev.regabi too? (I've made further closure changes since then, so it's possible there are multiple CLs in play here.)

Sorry for asking so much. Also, if simply moving the sym generation to emitStackObjects fixes the issue and passes the racecompile slowbot, I think it's reasonable to go ahead and commit that while we continue understanding why it failed in the first place.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 15, 2021

Oh, and finally: are we crashing trying to write out stack objects for f or for f.func1?

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 15, 2021

@ALTree Thanks for the issue report and the reproducer. I'm assuming that's minimized down from some real world code? If so, are you able to share that? (Not important, just curious.)

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jan 15, 2021

Oh, and finally: are we crashing trying to write out stack objects for f or for f.func1?

it's f.func1.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 15, 2021

Oh, that's surprising. I didn't expect f.func1 needed stack objects. I assumed f was crashing.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 15, 2021

Change https://golang.org/cl/284112 mentions this issue: [dev.regabi] cmd/compile: create stkobj symbol inside emitStackObjects

Loading

@ALTree
Copy link
Member Author

@ALTree ALTree commented Jan 15, 2021

Thanks for the issue report and the reproducer. I'm assuming that's minimized down from some real world code? If so, are you able to share that? (Not important, just curious.)

@mdempsky Sorry, there's no real world code; the crasher was found by a fuzzer.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 15, 2021

@ALTree No apology needed. I'd rather a fuzzer discover compiler issues than an end user. :)

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 15, 2021

Change https://golang.org/cl/284075 mentions this issue: [dev.regabi] cmd/compile: don't promote Byval CaptureVars if Addrtaken

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 15, 2021

Change https://golang.org/cl/284076 mentions this issue: [dev.regabi] cmd/compile: move stkobj symbol generation to SSA

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 15, 2021

Change https://golang.org/cl/284077 mentions this issue: [dev.regabi] cmd/compile: unexport reflectdata.WriteType

Loading

gopherbot pushed a commit that referenced this issue Jan 15, 2021
We decide during escape analysis whether to pass closure variables by
value or reference. One of the factors that's considered is whether a
variable has had its address taken.

However, this analysis is based only on the user-written source code,
whereas order+walk may introduce rewrites that take the address of a
variable (e.g., passing a uint16 key by reference to the size-generic
map runtime builtins).

Typically this would be harmless, albeit suboptimal. But in #43701 it
manifested as needing a stack object for a function where we didn't
realize we needed one up front when we generate symbols.

Probably we should just generate symbols on demand, now that those
routines are all concurrent-safe, but this is a first fix.

Thanks to Alberto Donizetti for reporting the issue, and Cuong Manh Le
for initial investigation.

Fixes #43701.

Change-Id: I16d87e9150723dcb16de7b43f2a8f3cd807a9437
Reviewed-on: https://go-review.googlesource.com/c/go/+/284075
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
gopherbot pushed a commit that referenced this issue Jan 15, 2021
The code for allocating linksyms and recording that we need runtime
type descriptors is now concurrent-safe, so move it to where those
symbols are actually needed to reduce complexity and risk of failing
to generate all needed symbols in advance.

For #43701.

Change-Id: I759d2508213ac9a4e0b504b51a75fa10dfa37a8d
Reviewed-on: https://go-review.googlesource.com/c/go/+/284076
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Jan 15, 2021
WriteType isn't safe for direct concurrent use, and users should
instead use TypeLinksym or another higher-level API provided by
reflectdata. After the previous CL, there are no remaining uses of
WriteType elsewhere in the compiler, so unexport it to keep it that
way.

For #43701.

[git-generate]
cd src/cmd/compile/internal/reflectdata
rf '
	mv WriteType writeType
'

Change-Id: I294a78be570a47feb38a1ad4eaae7723653d5991
Reviewed-on: https://go-review.googlesource.com/c/go/+/284077
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jan 15, 2021

@mdempsky @ALTree we can close this issue now.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants