]> Cypherpunks repositories - gostls13.git/commitdiff
[dev.typeparams] Revert "[dev.typeparams] runtime: remove unnecessary split-preventio...
authorAustin Clements <austin@google.com>
Fri, 30 Jul 2021 20:41:11 +0000 (16:41 -0400)
committerAustin Clements <austin@google.com>
Fri, 30 Jul 2021 21:51:50 +0000 (21:51 +0000)
This reverts CL 337651.

This causes `go test -count 1000 -run TestDeferHeapAndStack runtime`
to fail with a SIGSEGV freedefer
[https://build.golang.org/log/c113b366cc6d51146db02a07b4d7dd931133efd5]
and possibly sometimes a GC bad pointer panic
[https://build.golang.org/log/5b1cef7a9ad68704e9ef3ce3ad2fefca3ba86998].

Change-Id: Ie56c274b78603c81191213b302225ae19de27fb9
Reviewed-on: https://go-review.googlesource.com/c/go/+/338710
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
src/runtime/panic.go

index e66fe27be0bfb1d62e1d378cd98221c847007f3f..35f3b44a4d513f04adf02b60b21b290333bdfd7a 100644 (file)
@@ -261,8 +261,10 @@ func deferproc(fn func()) {
 // deferprocStack queues a new deferred function with a defer record on the stack.
 // The defer record must have its fn field initialized.
 // All other fields can contain junk.
-// Nosplit because of the uninitialized pointer fields on the stack.
-//
+// The defer record must be immediately followed in memory by
+// the arguments of the defer.
+// Nosplit because the arguments on the stack won't be scanned
+// until the defer record is spliced into the gp._defer list.
 //go:nosplit
 func deferprocStack(d *_defer) {
        gp := getg()
@@ -311,14 +313,18 @@ func newdefer() *_defer {
        gp := getg()
        pp := gp.m.p.ptr()
        if len(pp.deferpool) == 0 && sched.deferpool != nil {
-               lock(&sched.deferlock)
-               for len(pp.deferpool) < cap(pp.deferpool)/2 && sched.deferpool != nil {
-                       d := sched.deferpool
-                       sched.deferpool = d.link
-                       d.link = nil
-                       pp.deferpool = append(pp.deferpool, d)
-               }
-               unlock(&sched.deferlock)
+               // Take the slow path on the system stack so
+               // we don't grow newdefer's stack.
+               systemstack(func() {
+                       lock(&sched.deferlock)
+                       for len(pp.deferpool) < cap(pp.deferpool)/2 && sched.deferpool != nil {
+                               d := sched.deferpool
+                               sched.deferpool = d.link
+                               d.link = nil
+                               pp.deferpool = append(pp.deferpool, d)
+                       }
+                       unlock(&sched.deferlock)
+               })
        }
        if n := len(pp.deferpool); n > 0 {
                d = pp.deferpool[n-1]
@@ -335,6 +341,11 @@ func newdefer() *_defer {
 
 // Free the given defer.
 // The defer cannot be used after this call.
+//
+// This must not grow the stack because there may be a frame without a
+// stack map when this is called.
+//
+//go:nosplit
 func freedefer(d *_defer) {
        if d._panic != nil {
                freedeferpanic()
@@ -348,23 +359,28 @@ func freedefer(d *_defer) {
        pp := getg().m.p.ptr()
        if len(pp.deferpool) == cap(pp.deferpool) {
                // Transfer half of local cache to the central cache.
-               var first, last *_defer
-               for len(pp.deferpool) > cap(pp.deferpool)/2 {
-                       n := len(pp.deferpool)
-                       d := pp.deferpool[n-1]
-                       pp.deferpool[n-1] = nil
-                       pp.deferpool = pp.deferpool[:n-1]
-                       if first == nil {
-                               first = d
-                       } else {
-                               last.link = d
+               //
+               // Take this slow path on the system stack so
+               // we don't grow freedefer's stack.
+               systemstack(func() {
+                       var first, last *_defer
+                       for len(pp.deferpool) > cap(pp.deferpool)/2 {
+                               n := len(pp.deferpool)
+                               d := pp.deferpool[n-1]
+                               pp.deferpool[n-1] = nil
+                               pp.deferpool = pp.deferpool[:n-1]
+                               if first == nil {
+                                       first = d
+                               } else {
+                                       last.link = d
+                               }
+                               last = d
                        }
-                       last = d
-               }
-               lock(&sched.deferlock)
-               last.link = sched.deferpool
-               sched.deferpool = first
-               unlock(&sched.deferlock)
+                       lock(&sched.deferlock)
+                       last.link = sched.deferpool
+                       sched.deferpool = first
+                       unlock(&sched.deferlock)
+               })
        }
 
        // These lines used to be simply `*d = _defer{}` but that
@@ -404,6 +420,12 @@ func freedeferfn() {
 // to have been called by the caller of deferreturn at the point
 // just before deferreturn was called. The effect is that deferreturn
 // is called again and again until there are no more deferred functions.
+//
+// Declared as nosplit, because the function should not be preempted once we start
+// modifying the caller's frame in order to reuse the frame to call the deferred
+// function.
+//
+//go:nosplit
 func deferreturn() {
        gp := getg()
        d := gp._defer
@@ -424,6 +446,13 @@ func deferreturn() {
                return
        }
 
+       // Moving arguments around.
+       //
+       // Everything called after this point must be recursively
+       // nosplit because the garbage collector won't know the form
+       // of the arguments until the jmpdefer can flip the PC over to
+       // fn.
+       argp := getcallersp() + sys.MinFrameSize
        fn := d.fn
        d.fn = nil
        gp._defer = d.link
@@ -433,9 +462,6 @@ func deferreturn() {
        // called with a callback on an LR architecture and jmpdefer is on the
        // stack, because jmpdefer manipulates SP (see issue #8153).
        _ = **(**funcval)(unsafe.Pointer(&fn))
-       // We must not split the stack between computing argp and
-       // calling jmpdefer because argp is a uintptr stack pointer.
-       argp := getcallersp() + sys.MinFrameSize
        jmpdefer(fn, argp)
 }