From 9eb1d5317b2b87c379797edf0dea48e59c9ebc7d Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Thu, 27 Jul 2023 16:20:36 -0700 Subject: [PATCH] runtime: refactor defer processing This CL refactors gopanic, Goexit, and deferreturn to share a common state machine for processing pending defers. The new state machine removes a lot of redundant code and does overall less work. It should also make it easier to implement further optimizations (e.g., TODOs added in this CL). Change-Id: I71d3cc8878a6f951d8633505424a191536c8e6b3 Reviewed-on: https://go-review.googlesource.com/c/go/+/513837 Reviewed-by: Keith Randall Run-TryBot: Matthew Dempsky Reviewed-by: Keith Randall TryBot-Result: Gopher Robot --- src/cmd/internal/objabi/funcid.go | 4 +- src/runtime/panic.go | 673 +++++++++--------------- src/runtime/runtime-seh_windows_test.go | 2 +- src/runtime/runtime2.go | 69 +-- src/runtime/stack.go | 3 - 5 files changed, 287 insertions(+), 464 deletions(-) diff --git a/src/cmd/internal/objabi/funcid.go b/src/cmd/internal/objabi/funcid.go index afe9deb4f1b..007107e778f 100644 --- a/src/cmd/internal/objabi/funcid.go +++ b/src/cmd/internal/objabi/funcid.go @@ -32,9 +32,7 @@ var funcIDs = map[string]abi.FuncID{ "systemstack": abi.FuncID_systemstack, // Don't show in call stack but otherwise not special. - "deferreturn": abi.FuncIDWrapper, - "runOpenDeferFrame": abi.FuncIDWrapper, - "deferCallSave": abi.FuncIDWrapper, + "deferreturn": abi.FuncIDWrapper, } // Get the function ID for the named function in the named file. diff --git a/src/runtime/panic.go b/src/runtime/panic.go index 64fa2723854..d3aaa20cbce 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -276,9 +276,6 @@ func deferproc(fn func()) { } d := newdefer() - if d._panic != nil { - throw("deferproc: d.panic != nil after newdefer") - } d.link = gp._defer gp._defer = d d.fn = fn @@ -314,13 +311,9 @@ func deferprocStack(d *_defer) { // fn is already set. // The other fields are junk on entry to deferprocStack and // are initialized here. - d.started = false d.heap = false - d.openDefer = false d.sp = getcallersp() d.pc = getcallerpc() - d.framepc = 0 - d.varp = 0 // The lines below implement: // d.panic = nil // d.fd = nil @@ -332,8 +325,6 @@ func deferprocStack(d *_defer) { // The fourth write does not require a write barrier because we // explicitly mark all the defer structures, so we don't need to // keep track of pointers to them with a write barrier. - *(*uintptr)(unsafe.Pointer(&d._panic)) = 0 - *(*uintptr)(unsafe.Pointer(&d.fd)) = 0 *(*uintptr)(unsafe.Pointer(&d.link)) = uintptr(unsafe.Pointer(gp._defer)) *(*uintptr)(unsafe.Pointer(&gp._defer)) = uintptr(unsafe.Pointer(d)) @@ -390,9 +381,6 @@ func freedefer(d *_defer) { d.link = nil // After this point we can copy the stack. - if d._panic != nil { - freedeferpanic() - } if d.fn != nil { freedeferfn() } @@ -433,11 +421,6 @@ func freedefer(d *_defer) { // Separate function so that it can split stack. // Windows otherwise runs out of stack space. -func freedeferpanic() { - // _panic must be cleared before d is unlinked from gp. - throw("freedefer with d._panic != nil") -} - func freedeferfn() { // fn must be cleared before d is unlinked from gp. throw("freedefer with d.fn != nil") @@ -447,33 +430,15 @@ func freedeferfn() { // The compiler inserts a call to this at the end of any // function which calls defer. func deferreturn() { - gp := getg() - for { - d := gp._defer - if d == nil { - return - } - sp := getcallersp() - if d.sp != sp { - return - } - if d.openDefer { - done := runOpenDeferFrame(d) - if !done { - throw("unfinished open-coded defers in deferreturn") - } - gp._defer = d.link - freedefer(d) - // If this frame uses open defers, then this - // must be the only defer record for the - // frame, so we can just return. - return - } + var p _panic + p.deferreturn = true - fn := d.fn - d.fn = nil - gp._defer = d.link - freedefer(d) + p.start(getcallerpc(), unsafe.Pointer(getcallersp())) + for { + fn, ok := p.nextDefer() + if !ok { + break + } fn() } } @@ -487,78 +452,20 @@ func deferreturn() { // the program continues execution of other goroutines. // If all other goroutines exit, the program crashes. func Goexit() { - // Run all deferred functions for the current goroutine. - // This code is similar to gopanic, see that implementation - // for detailed comments. - gp := getg() - // Create a panic object for Goexit, so we can recognize when it might be // bypassed by a recover(). var p _panic p.goexit = true - p.link = gp._panic - gp._panic = (*_panic)(noescape(unsafe.Pointer(&p))) - addOneOpenDeferFrame(gp, getcallerpc(), unsafe.Pointer(getcallersp())) + p.start(getcallerpc(), unsafe.Pointer(getcallersp())) for { - d := gp._defer - if d == nil { + fn, ok := p.nextDefer() + if !ok { break } - if d.started { - if d._panic != nil { - d._panic.aborted = true - d._panic = nil - } - if !d.openDefer { - d.fn = nil - gp._defer = d.link - freedefer(d) - continue - } - } - d.started = true - d._panic = (*_panic)(noescape(unsafe.Pointer(&p))) - if d.openDefer { - done := runOpenDeferFrame(d) - if !done { - // We should always run all defers in the frame, - // since there is no panic associated with this - // defer that can be recovered. - throw("unfinished open-coded defers in Goexit") - } - if p.aborted { - // Since our current defer caused a panic and may - // have been already freed, just restart scanning - // for open-coded defers from this frame again. - addOneOpenDeferFrame(gp, getcallerpc(), unsafe.Pointer(getcallersp())) - } else { - addOneOpenDeferFrame(gp, 0, nil) - } - } else { - // Save the pc/sp in deferCallSave(), so we can "recover" back to this - // loop if necessary. - deferCallSave(&p, d.fn) - } - if p.aborted { - // We had a recursive panic in the defer d we started, and - // then did a recover in a defer that was further down the - // defer chain than d. In the case of an outstanding Goexit, - // we force the recover to return back to this loop. d will - // have already been freed if completed, so just continue - // immediately to the next defer on the chain. - p.aborted = false - continue - } - if gp._defer != d { - throw("bad defer entry in Goexit") - } - d._panic = nil - d.fn = nil - gp._defer = d.link - freedefer(d) - // Note: we ignore recovers here because Goexit isn't a panic + fn() } + goexit1() } @@ -607,117 +514,6 @@ func printpanics(p *_panic) { print("\n") } -// addOneOpenDeferFrame scans the stack (in gentraceback order, from inner frames to -// outer frames) for the first frame (if any) with open-coded defers. If it finds -// one, it adds a single entry to the defer chain for that frame. The entry added -// represents all the defers in the associated open defer frame, and is sorted in -// order with respect to any non-open-coded defers. -// -// addOneOpenDeferFrame stops (possibly without adding a new entry) if it encounters -// an in-progress open defer entry. An in-progress open defer entry means there has -// been a new panic because of a defer in the associated frame. addOneOpenDeferFrame -// does not add an open defer entry past a started entry, because that started entry -// still needs to finished, and addOneOpenDeferFrame will be called when that started -// entry is completed. The defer removal loop in gopanic() similarly stops at an -// in-progress defer entry. Together, addOneOpenDeferFrame and the defer removal loop -// ensure the invariant that there is no open defer entry further up the stack than -// an in-progress defer, and also that the defer removal loop is guaranteed to remove -// all not-in-progress open defer entries from the defer chain. -// -// If sp is non-nil, addOneOpenDeferFrame starts the stack scan from the frame -// specified by sp. If sp is nil, it uses the sp from the current defer record (which -// has just been finished). Hence, it continues the stack scan from the frame of the -// defer that just finished. It skips any frame that already has a (not-in-progress) -// open-coded _defer record in the defer chain. -// -// Note: All entries of the defer chain (including this new open-coded entry) have -// their pointers (including sp) adjusted properly if the stack moves while -// running deferred functions. Also, it is safe to pass in the sp arg (which is -// the direct result of calling getcallersp()), because all pointer variables -// (including arguments) are adjusted as needed during stack copies. -func addOneOpenDeferFrame(gp *g, pc uintptr, sp unsafe.Pointer) { - var prevDefer *_defer - if sp == nil { - prevDefer = gp._defer - pc = prevDefer.framepc - sp = unsafe.Pointer(prevDefer.sp) - } - systemstack(func() { - var u unwinder - frames: - for u.initAt(pc, uintptr(sp), 0, gp, 0); u.valid(); u.next() { - frame := &u.frame - if prevDefer != nil && prevDefer.sp == frame.sp { - // Skip the frame for the previous defer that - // we just finished (and was used to set - // where we restarted the stack scan) - continue - } - f := frame.fn - fd := funcdata(f, abi.FUNCDATA_OpenCodedDeferInfo) - if fd == nil { - continue - } - // Insert the open defer record in the - // chain, in order sorted by sp. - d := gp._defer - var prev *_defer - for d != nil { - dsp := d.sp - if frame.sp < dsp { - break - } - if frame.sp == dsp { - if !d.openDefer { - throw("duplicated defer entry") - } - // Don't add any record past an - // in-progress defer entry. We don't - // need it, and more importantly, we - // want to keep the invariant that - // there is no open defer entry - // passed an in-progress entry (see - // header comment). - if d.started { - break frames - } - continue frames - } - prev = d - d = d.link - } - if frame.fn.deferreturn == 0 { - throw("missing deferreturn") - } - - d1 := newdefer() - d1.openDefer = true - d1._panic = nil - // These are the pc/sp to set after we've - // run a defer in this frame that did a - // recover. We return to a special - // deferreturn that runs any remaining - // defers and then returns from the - // function. - d1.pc = frame.fn.entry() + uintptr(frame.fn.deferreturn) - d1.varp = frame.varp - d1.fd = fd - // Save the SP/PC associated with current frame, - // so we can continue stack trace later if needed. - d1.framepc = frame.pc - d1.sp = frame.sp - d1.link = d - if prev == nil { - gp._defer = d1 - } else { - prev.link = d1 - } - // Stop stack scanning after adding one open defer record - break - } - }) -} - // readvarintUnsafe reads the uint32 in varint format starting at fd, and returns the // uint32 and a pointer to the byte following the varint. // @@ -742,66 +538,6 @@ func readvarintUnsafe(fd unsafe.Pointer) (uint32, unsafe.Pointer) { } } -// runOpenDeferFrame runs the active open-coded defers in the frame specified by -// d. It normally processes all active defers in the frame, but stops immediately -// if a defer does a successful recover. It returns true if there are no -// remaining defers to run in the frame. -func runOpenDeferFrame(d *_defer) bool { - done := true - fd := d.fd - - deferBitsOffset, fd := readvarintUnsafe(fd) - nDefers, fd := readvarintUnsafe(fd) - deferBits := *(*uint8)(unsafe.Pointer(d.varp - uintptr(deferBitsOffset))) - - for i := int(nDefers) - 1; i >= 0; i-- { - // read the funcdata info for this defer - var closureOffset uint32 - closureOffset, fd = readvarintUnsafe(fd) - if deferBits&(1< 0 { + p.openDefers-- + + // Find the closure offset for the next deferred call. + var closureOffset uint32 + closureOffset, p.closureOffsets = readvarintUnsafe(p.closureOffsets) + + bit := uint8(1 << p.openDefers) + if *p.deferBitsPtr&bit == 0 { + continue + } + *p.deferBitsPtr &^= bit + + if *p.deferBitsPtr == 0 { + p.openDefers = 0 // short circuit: no more active defers + } + + return *(*func())(add(p.varp, -uintptr(closureOffset))), true + } + + if d := gp._defer; d != nil && d.sp == uintptr(p.sp) { + fn := d.fn + d.fn = nil + + // TODO(mdempsky): Instead of having each deferproc call have + // its own "deferreturn(); return" sequence, we should just make + // them reuse the one we emit for open-coded defers. + p.retpc = d.pc + + // Unlink and free. + gp._defer = d.link + freedefer(d) + + return fn, true + } + + if !p.nextFrame() { + return nil, false + } + } +} + +// nextFrame finds the next frame that contains deferred calls, if any. +func (p *_panic) nextFrame() (ok bool) { + if p.lr == 0 { + return false + } + + gp := getg() + systemstack(func() { + var limit uintptr + if p.deferreturn { + limit = uintptr(p.fp) + } else if d := gp._defer; d != nil { + limit = uintptr(d.sp) + } + + var u unwinder + u.initAt(p.lr, uintptr(p.fp), 0, gp, 0) + for { + if !u.valid() { + p.lr = 0 + return // ok == false + } + + // TODO(mdempsky): If we populate u.frame.fn.deferreturn for + // every frame containing a defer (not just open-coded defers), + // then we can simply loop until we find the next frame where + // it's non-zero. + + if fd := funcdata(u.frame.fn, abi.FUNCDATA_OpenCodedDeferInfo); fd != nil { + if u.frame.fn.deferreturn == 0 { + throw("missing deferreturn") + } + p.retpc = u.frame.fn.entry() + uintptr(u.frame.fn.deferreturn) + + var deferBitsOffset uint32 + deferBitsOffset, fd = readvarintUnsafe(fd) + deferBitsPtr := (*uint8)(add(unsafe.Pointer(u.frame.varp), -uintptr(deferBitsOffset))) + + if *deferBitsPtr != 0 { + var openDefers uint32 + openDefers, fd = readvarintUnsafe(fd) + + p.openDefers = uint8(openDefers) + p.deferBitsPtr = deferBitsPtr + p.closureOffsets = fd + break // found a frame with open-coded defers + } + } + + if u.frame.sp == limit { + break // found a frame with linked defers, or deferreturn with no defers + } + + u.next() + } + + if p.deferreturn { + p.lr = 0 // prevent unwinding past this frame + } else { + p.lr = u.frame.lr + } + p.sp = unsafe.Pointer(u.frame.sp) + p.fp = unsafe.Pointer(u.frame.fp) + p.varp = unsafe.Pointer(u.frame.varp) + + ok = true + }) + + return } // The implementation of the predeclared function recover. @@ -1110,12 +870,73 @@ var paniclk mutex // Unwind the stack after a deferred function calls recover // after a panic. Then arrange to continue running as though // the caller of the deferred function returned normally. +// +// However, if unwinding the stack would skip over a Goexit call, we +// return into the Goexit loop instead, so it can continue processing +// defers instead. func recovery(gp *g) { - // Info about defer passed in G struct. - sp := gp.sigcode0 - pc := gp.sigcode1 + p := gp._panic + pc, sp := p.retpc, uintptr(p.sp) - // d's arguments need to be in the stack. + // Unwind the panic stack. + for ; p != nil && uintptr(p.startSP) < sp; p = p.link { + // Don't allow jumping past a pending Goexit. + // Instead, have its _panic.start() call return again. + // + // TODO(mdempsky): In this case, Goexit will resume walking the + // stack where it left off, which means it will need to rewalk + // frames that we've already processed. + // + // There's a similar issue with nested panics, when the inner + // panic supercedes the outer panic. Again, we end up needing to + // walk the same stack frames. + // + // These are probably pretty rare occurrences in practice, and + // they don't seem any worse than the existing logic. But if we + // move the unwinding state into _panic, we could detect when we + // run into where the last panic started, and then just pick up + // where it left off instead. + // + // With how subtle defer handling is, this might not actually be + // worthwhile though. + if p.goexit { + pc, sp = p.startPC, uintptr(p.startSP) + break + } + + runningPanicDefers.Add(-1) + } + gp._panic = p + + if p == nil { // must be done with signal + gp.sig = 0 + } + + // TODO(mdempsky): Currently, we rely on frames containing "defer" + // to end with "CALL deferreturn; RET". This allows deferreturn to + // finish running any pending defers in the frame. + // + // But we should be able to tell whether there are still pending + // defers here. If there aren't, we can just jump directly to the + // "RET" instruction. And if there are, we don't need an actual + // "CALL deferreturn" instruction; we can simulate it with something + // like: + // + // if usesLR { + // lr = pc + // } else { + // sp -= sizeof(pc) + // *(*uintptr)(sp) = pc + // } + // pc = funcPC(deferreturn) + // + // So that we effectively tail call into deferreturn, such that it + // then returns to the simple "RET" epilogue. That would save the + // overhead of the "deferreturn" call when there aren't actually any + // pending defers left, and shrink the TEXT size of compiled + // binaries. (Admittedly, both of these are modest savings.) + + // Ensure we're recovering within the appropriate stack. if sp != 0 && (sp < gp.stack.lo || gp.stack.hi < sp) { print("recover: ", hex(sp), " not in [", hex(gp.stack.lo), ", ", hex(gp.stack.hi), "]\n") throw("bad recovery") diff --git a/src/runtime/runtime-seh_windows_test.go b/src/runtime/runtime-seh_windows_test.go index 27e4f497411..42509532be5 100644 --- a/src/runtime/runtime-seh_windows_test.go +++ b/src/runtime/runtime-seh_windows_test.go @@ -112,7 +112,7 @@ func testSehCallersEqual(t *testing.T, pcs []uintptr, want []string) { } name := fn.Name() switch name { - case "runtime.deferCallSave", "runtime.runOpenDeferFrame", "runtime.panicmem": + case "runtime.panicmem": // These functions are skipped as they appear inconsistently depending // whether inlining is on or off. continue diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index f4c76abd1c2..75f009388eb 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -999,29 +999,18 @@ func extendRandom(r []byte, n int) { // initialize them are not required. All defers must be manually scanned, // and for heap defers, marked. type _defer struct { - started bool - heap bool - // openDefer indicates that this _defer is for a frame with open-coded - // defers. We have only one defer record for the entire frame (which may - // currently have 0, 1, or more defers active). - openDefer bool - sp uintptr // sp at time of defer - pc uintptr // pc at time of defer - fn func() // can be nil for open-coded defers - _panic *_panic // panic that is running defer - link *_defer // next defer on G; can point to either heap or stack! - - // If openDefer is true, the fields below record values about the stack - // frame and associated function that has the open-coded defer(s). sp - // above will be the sp for the frame, and pc will be address of the - // deferreturn call in the function. - fd unsafe.Pointer // funcdata for the function associated with the frame - varp uintptr // value of varp for the stack frame - // framepc is the current pc associated with the stack frame. Together, - // with sp above (which is the sp associated with the stack frame), - // framepc/sp can be used as pc/sp pair to continue a stack trace via - // gentraceback(). - framepc uintptr + // TODO(mdempsky): Remove blank fields and update cmd/compile. + _ bool // was started + heap bool + _ bool // was openDefer + sp uintptr // sp at time of defer + pc uintptr // pc at time of defer + fn func() // can be nil for open-coded defers + _ unsafe.Pointer // was _panic + link *_defer // next defer on G; can point to either heap or stack! + _ unsafe.Pointer // was fd + _ uintptr // was varp + _ uintptr // was framepc } // A _panic holds information about an active panic. @@ -1033,14 +1022,32 @@ type _defer struct { // _panic values only live on the stack, regular stack pointer // adjustment takes care of them. type _panic struct { - argp unsafe.Pointer // pointer to arguments of deferred call run during panic; cannot move - known to liblink - arg any // argument to panic - link *_panic // link to earlier panic - pc uintptr // where to return to in runtime if this panic is bypassed - sp unsafe.Pointer // where to return to in runtime if this panic is bypassed - recovered bool // whether this panic is over - aborted bool // the panic was aborted - goexit bool + argp unsafe.Pointer // pointer to arguments of deferred call run during panic; cannot move - known to liblink + arg any // argument to panic + link *_panic // link to earlier panic + + // startPC and startSP track where _panic.start was called. + startPC uintptr + startSP unsafe.Pointer + + // The current stack frame that we're running deferred calls for. + sp unsafe.Pointer + lr uintptr + fp unsafe.Pointer + varp unsafe.Pointer + + // retpc stores the PC where the panic should jump back to, if the + // function last returned by _panic.next() recovers the panic. + retpc uintptr + + // Extra state for handling open-coded defers. + deferBitsPtr *uint8 + closureOffsets unsafe.Pointer + openDefers uint8 // count of pending open-coded defers + + recovered bool // whether this panic has been recovered + goexit bool + deferreturn bool } // ancestorInfo records details of where a goroutine was started. diff --git a/src/runtime/stack.go b/src/runtime/stack.go index 45d66da91f6..903b096f082 100644 --- a/src/runtime/stack.go +++ b/src/runtime/stack.go @@ -763,10 +763,7 @@ func adjustdefers(gp *g, adjinfo *adjustinfo) { for d := gp._defer; d != nil; d = d.link { adjustpointer(adjinfo, unsafe.Pointer(&d.fn)) adjustpointer(adjinfo, unsafe.Pointer(&d.sp)) - adjustpointer(adjinfo, unsafe.Pointer(&d._panic)) adjustpointer(adjinfo, unsafe.Pointer(&d.link)) - adjustpointer(adjinfo, unsafe.Pointer(&d.varp)) - adjustpointer(adjinfo, unsafe.Pointer(&d.fd)) } }