]> Cypherpunks repositories - gostls13.git/commitdiff
runtime: make systemstack tail call if already switched
authorAustin Clements <austin@google.com>
Fri, 27 Oct 2017 19:20:21 +0000 (15:20 -0400)
committerAustin Clements <austin@google.com>
Mon, 30 Oct 2017 16:33:55 +0000 (16:33 +0000)
Currently systemstack always calls its argument, even if we're already
on the system stack. Unfortunately, traceback with _TraceJump stops at
the first systemstack it sees, which often cuts off runtime stacks
early in profiles.

Fix this by performing a tail call if we're already on the system
stack. This eliminates it from the traceback entirely, so it won't
stop prematurely (or all get mushed into a single node in the profile
graph).

Change-Id: Ibc69e8765e899f8d3806078517b8c7314da196f4
Reviewed-on: https://go-review.googlesource.com/74050
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
src/runtime/asm_386.s
src/runtime/asm_amd64.s
src/runtime/asm_amd64p32.s
src/runtime/asm_arm.s
src/runtime/asm_arm64.s
src/runtime/asm_mips64x.s
src/runtime/asm_mipsx.s
src/runtime/asm_ppc64x.s
src/runtime/asm_s390x.s
src/runtime/export_test.go
src/runtime/stack_test.go

index 15d9ce9fdf5bc70efbd263ee8a9ba84209391cfc..80a145187c884bf18f14dd68d19d9319bb2bbce7 100644 (file)
@@ -474,11 +474,12 @@ switch:
        RET
 
 noswitch:
-       // already on system stack, just call directly
+       // already on system stack; tail call the function
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVL    DI, DX
        MOVL    0(DI), DI
-       CALL    DI
-       RET
+       JMP     DI
 
 /*
  * support for morestack
index 2ac879c31de8fcc08b9ae54152999bb16b005598..01a17100464dbd93b195770a3c12fba3644c65d3 100644 (file)
@@ -419,11 +419,12 @@ switch:
        RET
 
 noswitch:
-       // already on m stack, just call directly
+       // already on m stack; tail call the function
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVQ    DI, DX
        MOVQ    0(DI), DI
-       CALL    DI
-       RET
+       JMP     DI
 
 /*
  * support for morestack
index b7fcf2376ee65af0d3cbe155afd8c30a7e0c6739..7fee79aefbc6c86b553b52c025737bfd9eddfe24 100644 (file)
@@ -306,10 +306,11 @@ switch:
 
 noswitch:
        // already on m stack, just call directly
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVL    DI, DX
        MOVL    0(DI), DI
-       CALL    DI
-       RET
+       JMP     DI
 
 /*
  * support for morestack
index caa96cc4b364849100b146fb0ec05c64f012fced..306984e8f7376ae9f26cd9c469f73d9dd3f96256 100644 (file)
@@ -358,10 +358,12 @@ switch:
        RET
 
 noswitch:
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVW    R0, R7
        MOVW    0(R0), R0
-       BL      (R0)
-       RET
+       MOVW.P  4(R13), R14     // restore LR
+       B       (R0)
 
 /*
  * support for morestack
index b2aff1aab7fb618fc81227f5aac8ae811772cc17..5e202e7a8737da593245cac2bd19d5e758653e51 100644 (file)
@@ -239,9 +239,11 @@ switch:
 
 noswitch:
        // already on m stack, just call directly
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVD    0(R26), R3      // code pointer
-       BL      (R3)
-       RET
+       MOVD.P  16(RSP), R30    // restore LR
+       B       (R3)
 
 /*
  * support for morestack
index 3510853804eb7b9c3bf39d39a19af70943d9bef0..12cea00adc8413a453dbbcb4602895969d7759d2 100644 (file)
@@ -214,9 +214,12 @@ switch:
 
 noswitch:
        // already on m stack, just call directly
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVV    0(REGCTXT), R4  // code pointer
-       JAL     (R4)
-       RET
+       MOVV    0(R29), R31     // restore LR
+       ADDV    $8, R29
+       JMP     (R4)
 
 /*
  * support for morestack
index 334f259186d201e6323fd01d731be70d55d66676..bba6a9501d3a710e8b7ea7ae83747d67bb1ada77 100644 (file)
@@ -215,9 +215,12 @@ switch:
 
 noswitch:
        // already on m stack, just call directly
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVW    0(REGCTXT), R4  // code pointer
-       JAL     (R4)
-       RET
+       MOVW    0(R29), R31     // restore LR
+       ADD     $4, R29
+       JMP     (R4)
 
 /*
  * support for morestack
index 2f2a4a7b0498a68d7410cf735eb92a9a8a85a003..487187f4d88674c25b68697eb477d0df13a78752 100644 (file)
@@ -265,6 +265,9 @@ switch:
 
 noswitch:
        // already on m stack, just call directly
+       // On other arches we do a tail call here, but it appears to be
+       // impossible to tail call a function pointer in shared mode on
+       // ppc64 because the caller is responsible for restoring the TOC.
        MOVD    0(R11), R12     // code pointer
        MOVD    R12, CTR
        BL      (CTR)
index 524b866b2135f0af2c1bfee1829fe391bd709e49..42b93266074b82cbc0c6bc98ecd6d81ce9801a22 100644 (file)
@@ -224,9 +224,12 @@ switch:
 
 noswitch:
        // already on m stack, just call directly
+       // Using a tail call here cleans up tracebacks since we won't stop
+       // at an intermediate systemstack.
        MOVD    0(R12), R3      // code pointer
-       BL      (R3)
-       RET
+       MOVD    0(R15), LR      // restore LR
+       ADD     $8, R15
+       BR      (R3)
 
 /*
  * support for morestack
index 9b269d965916b4d0abdb398a225b1fcb84959bb8..599ac2d84a3a0847c3daf20f5d3015481ac514f1 100644 (file)
@@ -395,3 +395,16 @@ func LockOSCounts() (external, internal uint32) {
        }
        return g.m.lockedExt, g.m.lockedInt
 }
+
+//go:noinline
+func TracebackSystemstack(stk []uintptr, i int) int {
+       if i == 0 {
+               pc, sp := getcallerpc(), getcallersp(unsafe.Pointer(&stk))
+               return gentraceback(pc, sp, 0, getg(), 0, &stk[0], len(stk), nil, nil, _TraceJumpStack)
+       }
+       n := 0
+       systemstack(func() {
+               n = TracebackSystemstack(stk, i-1)
+       })
+       return n
+}
index c9b84be0666612a552b4eb9aff05495261ce1896..8e7c7d47a8b0aade2bc160dc40eed7747bd2d3b8 100644 (file)
@@ -5,6 +5,7 @@
 package runtime_test
 
 import (
+       "bytes"
        "fmt"
        . "runtime"
        "strings"
@@ -685,3 +686,35 @@ func TestStackWrapperStack(t *testing.T) {
                t.Fatalf("<autogenerated> appears in stack trace:\n%s", stk)
        }
 }
+
+func TestTracebackSystemstack(t *testing.T) {
+       if GOARCH == "ppc64" || GOARCH == "ppc64le" {
+               t.Skip("systemstack tail call not implemented on ppc64x")
+       }
+
+       // Test that profiles correctly jump over systemstack,
+       // including nested systemstack calls.
+       pcs := make([]uintptr, 20)
+       pcs = pcs[:TracebackSystemstack(pcs, 5)]
+       // Check that runtime.TracebackSystemstack appears five times
+       // and that we see TestTracebackSystemstack.
+       countIn, countOut := 0, 0
+       frames := CallersFrames(pcs)
+       var tb bytes.Buffer
+       for {
+               frame, more := frames.Next()
+               fmt.Fprintf(&tb, "\n%s+0x%x %s:%d", frame.Function, frame.PC-frame.Entry, frame.File, frame.Line)
+               switch frame.Function {
+               case "runtime.TracebackSystemstack":
+                       countIn++
+               case "runtime_test.TestTracebackSystemstack":
+                       countOut++
+               }
+               if !more {
+                       break
+               }
+       }
+       if countIn != 5 || countOut != 1 {
+               t.Fatalf("expected 5 calls to TracebackSystemstack and 1 call to TestTracebackSystemstack, got:%s", tb.String())
+       }
+}