]> Cypherpunks.ru repositories - gostls13.git/commitdiff
runtime/trace: use regular unwinding for cgo callbacks
authorFelix Geisendörfer <felix.geisendoerfer@datadoghq.com>
Mon, 13 Mar 2023 09:18:36 +0000 (10:18 +0100)
committerMichael Pratt <mpratt@google.com>
Thu, 30 Mar 2023 19:18:12 +0000 (19:18 +0000)
Introduce a new m.incgocallback field that is true while C code calls
into Go code. Use it in the tracer in order to fallback to the default
unwinder instead of frame pointer unwinding for this scenario. The
existing fields (incgo, ncgo) were not sufficient to detect the case
where a thread created in C calls into Go code.

Motivation:

1. Take advantage of a cgo symbolizer, if registered, to unwind through
   C stacks without frame pointers.
2. Reduce the chance of crashes. It seems unsafe to follow frame
   pointers when there could be C code that was compiled without frame
   pointers.

Removing the curgp.m.incgocallback check in traceStackID shows the
following minor differences between frame pointer unwinding and the
default unwinder when there is no cgo symbolizer involved.

    trace_test.go:60: "goCalledFromCThread": got stack:
        main.goCalledFromCThread
         /src/runtime/testdata/testprogcgo/trace.go:58
        _cgoexp_45c15a3efb3a_goCalledFromCThread
         _cgo_gotypes.go:694
        runtime.cgocallbackg1
         /src/runtime/cgocall.go:318
        runtime.cgocallbackg
         /src/runtime/cgocall.go:236
        runtime.cgocallback
         /src/runtime/asm_amd64.s:998
        crosscall2
         /src/runtime/cgo/asm_amd64.s:30

        want stack:
        main.goCalledFromCThread
         /src/runtime/testdata/testprogcgo/trace.go:58
        _cgoexp_45c15a3efb3a_goCalledFromCThread
         _cgo_gotypes.go:694
        runtime.cgocallbackg1
         /src/runtime/cgocall.go:318
        runtime.cgocallbackg
         /src/runtime/cgocall.go:236
        runtime.cgocallback
         /src/runtime/asm_amd64.s:998

    trace_test.go:60: "goCalledFromC": got stack:
        main.goCalledFromC
         /src/runtime/testdata/testprogcgo/trace.go:51
        _cgoexp_45c15a3efb3a_goCalledFromC
         _cgo_gotypes.go:687
        runtime.cgocallbackg1
         /src/runtime/cgocall.go:318
        runtime.cgocallbackg
         /src/runtime/cgocall.go:236
        runtime.cgocallback
         /src/runtime/asm_amd64.s:998
        crosscall2
         /src/runtime/cgo/asm_amd64.s:30
        runtime.asmcgocall
         /src/runtime/asm_amd64.s:848
        main._Cfunc_cCalledFromGo
         _cgo_gotypes.go:263
        main.goCalledFromGo
         /src/runtime/testdata/testprogcgo/trace.go:46
        main.Trace
         /src/runtime/testdata/testprogcgo/trace.go:37
        main.main
         /src/runtime/testdata/testprogcgo/main.go:34

        want stack:
        main.goCalledFromC
         /src/runtime/testdata/testprogcgo/trace.go:51
        _cgoexp_45c15a3efb3a_goCalledFromC
         _cgo_gotypes.go:687
        runtime.cgocallbackg1
         /src/runtime/cgocall.go:318
        runtime.cgocallbackg
         /src/runtime/cgocall.go:236
        runtime.cgocallback
         /src/runtime/asm_amd64.s:998
        runtime.systemstack_switch
         /src/runtime/asm_amd64.s:463
        runtime.cgocall
         /src/runtime/cgocall.go:168
        main._Cfunc_cCalledFromGo
         _cgo_gotypes.go:263
        main.goCalledFromGo
         /src/runtime/testdata/testprogcgo/trace.go:46
        main.Trace
         /src/runtime/testdata/testprogcgo/trace.go:37
        main.main
         /src/runtime/testdata/testprogcgo/main.go:34

For #16638

Change-Id: I95fa27a3170c5abd923afc6eadab4eae777ced31
Reviewed-on: https://go-review.googlesource.com/c/go/+/474916
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>

src/runtime/proc.go
src/runtime/testdata/testprogcgo/trace.go [new file with mode: 0644]
src/runtime/testdata/testprogcgo/trace_unix.c [new file with mode: 0644]
src/runtime/testdata/testprogcgo/trace_windows.c [new file with mode: 0644]
src/runtime/trace.go
src/runtime/trace_cgo_test.go [new file with mode: 0644]

index f4bf650e3b10e1fe0bd49f53e191a36b6a28c129..fd7760a571fd2f23057a05d1e104d79c03576428 100644 (file)
@@ -862,6 +862,10 @@ func (mp *m) becomeSpinning() {
        sched.needspinning.Store(0)
 }
 
+func (mp *m) incgocallback() bool {
+       return (!mp.incgo && mp.ncgo > 0) || mp.isextra
+}
+
 var fastrandseed uintptr
 
 func fastrandinit() {
diff --git a/src/runtime/testdata/testprogcgo/trace.go b/src/runtime/testdata/testprogcgo/trace.go
new file mode 100644 (file)
index 0000000..875434b
--- /dev/null
@@ -0,0 +1,60 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+/*
+// Defined in trace_*.c.
+void cCalledFromGo(void);
+*/
+import "C"
+import (
+       "context"
+       "fmt"
+       "log"
+       "os"
+       "runtime/trace"
+)
+
+func init() {
+       register("Trace", Trace)
+}
+
+// Trace is used by TestTraceUnwindCGO.
+func Trace() {
+       file, err := os.CreateTemp("", "testprogcgo_trace")
+       if err != nil {
+               log.Fatalf("failed to create temp file: %s", err)
+       }
+       defer file.Close()
+
+       if err := trace.Start(file); err != nil {
+               log.Fatal(err)
+       }
+       defer trace.Stop()
+
+       goCalledFromGo()
+       <-goCalledFromCThreadChan
+
+       fmt.Printf("trace path:%s", file.Name())
+}
+
+// goCalledFromGo calls cCalledFromGo which calls back into goCalledFromC and
+// goCalledFromCThread.
+func goCalledFromGo() {
+       C.cCalledFromGo()
+}
+
+//export goCalledFromC
+func goCalledFromC() {
+       trace.Log(context.Background(), "goCalledFromC", "")
+}
+
+var goCalledFromCThreadChan = make(chan struct{})
+
+//export goCalledFromCThread
+func goCalledFromCThread() {
+       trace.Log(context.Background(), "goCalledFromCThread", "")
+       close(goCalledFromCThreadChan)
+}
diff --git a/src/runtime/testdata/testprogcgo/trace_unix.c b/src/runtime/testdata/testprogcgo/trace_unix.c
new file mode 100644 (file)
index 0000000..0fa55c7
--- /dev/null
@@ -0,0 +1,27 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build unix
+
+// The unix C definitions for trace.go. That file uses //export so
+// it can't put function definitions in the "C" import comment.
+
+#include <pthread.h>
+#include <assert.h>
+
+extern void goCalledFromC(void);
+extern void goCalledFromCThread(void);
+
+static void* cCalledFromCThread(void *p) {
+       goCalledFromCThread();
+       return NULL;
+}
+
+void cCalledFromGo(void) {
+       goCalledFromC();
+
+       pthread_t thread;
+       assert(pthread_create(&thread, NULL, cCalledFromCThread, NULL) == 0);
+       assert(pthread_join(thread, NULL) == 0);
+}
diff --git a/src/runtime/testdata/testprogcgo/trace_windows.c b/src/runtime/testdata/testprogcgo/trace_windows.c
new file mode 100644 (file)
index 0000000..7758054
--- /dev/null
@@ -0,0 +1,29 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// The windows C definitions for trace.go. That file uses //export so
+// it can't put function definitions in the "C" import comment.
+
+#define WIN32_LEAN_AND_MEAN
+#include <windows.h>
+#include <process.h>
+#include "_cgo_export.h"
+
+extern void goCalledFromC(void);
+extern void goCalledFromCThread(void);
+
+__stdcall
+static unsigned int cCalledFromCThread(void *p) {
+       goCalledFromCThread();
+       return 0;
+}
+
+void cCalledFromGo(void) {
+       goCalledFromC();
+
+       uintptr_t thread;
+       thread = _beginthreadex(NULL, 0, cCalledFromCThread, NULL, 0, NULL);
+       WaitForSingleObject((HANDLE)thread, INFINITE);
+       CloseHandle((HANDLE)thread);
+}
index c152b103368f3e6b37ed836b5a301da83af2f45d..81093cc9b9c9df48436c444bd15e0814f831204e 100644 (file)
@@ -888,9 +888,13 @@ func traceStackID(mp *m, pcBuf []uintptr, skip int) uint64 {
        gp := getg()
        curgp := mp.curg
        nstk := 1
-       if tracefpunwindoff() {
+       if tracefpunwindoff() || mp.incgocallback() {
                // Slow path: Unwind using default unwinder. Used when frame pointer
-               // unwinding is unavailable or disabled.
+               // unwinding is unavailable or disabled (tracefpunwindoff), or might
+               // produce incomplete results or crashes (incgocallback). Note that no
+               // cgo callback related crashes have been observed yet. The main
+               // motivation is to take advantage of a potentially registered cgo
+               // symbolizer.
                pcBuf[0] = logicalStackSentinel
                if curgp == gp {
                        nstk += callers(skip+1, pcBuf[1:])
diff --git a/src/runtime/trace_cgo_test.go b/src/runtime/trace_cgo_test.go
new file mode 100644 (file)
index 0000000..3f207aa
--- /dev/null
@@ -0,0 +1,105 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build cgo
+
+package runtime_test
+
+import (
+       "bytes"
+       "fmt"
+       "internal/testenv"
+       "internal/trace"
+       "io"
+       "os"
+       "runtime"
+       "strings"
+       "testing"
+)
+
+// TestTraceUnwindCGO verifies that trace events emitted in cgo callbacks
+// produce the same stack traces and don't cause any crashes regardless of
+// tracefpunwindoff being set to 0 or 1.
+func TestTraceUnwindCGO(t *testing.T) {
+       if *flagQuick {
+               t.Skip("-quick")
+       }
+       testenv.MustHaveGoBuild(t)
+       t.Parallel()
+
+       exe, err := buildTestProg(t, "testprogcgo")
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       logs := map[string]*trace.Event{
+               "goCalledFromC":       nil,
+               "goCalledFromCThread": nil,
+       }
+       for _, tracefpunwindoff := range []int{1, 0} {
+               env := fmt.Sprintf("GODEBUG=tracefpunwindoff=%d", tracefpunwindoff)
+               got := runBuiltTestProg(t, exe, "Trace", env)
+               prefix, tracePath, found := strings.Cut(got, ":")
+               if !found || prefix != "trace path" {
+                       t.Fatalf("unexpected output:\n%s\n", got)
+               }
+               defer os.Remove(tracePath)
+
+               traceData, err := os.ReadFile(tracePath)
+               if err != nil {
+                       t.Fatalf("failed to read trace: %s", err)
+               }
+               events := parseTrace(t, bytes.NewReader(traceData))
+
+               for category := range logs {
+                       event := mustFindLog(t, events, category)
+                       if wantEvent := logs[category]; wantEvent == nil {
+                               logs[category] = event
+                       } else if got, want := dumpStack(event), dumpStack(wantEvent); got != want {
+                               t.Errorf("%q: got stack:\n%s\nwant stack:\n%s\n", category, got, want)
+                       }
+               }
+       }
+}
+
+// mustFindLog returns the EvUserLog event with the given category in events. It
+// fails if no event or multiple events match the category.
+func mustFindLog(t *testing.T, events []*trace.Event, category string) *trace.Event {
+       t.Helper()
+       var candidates []*trace.Event
+       for _, e := range events {
+               if e.Type == trace.EvUserLog && len(e.SArgs) >= 1 && e.SArgs[0] == category {
+                       candidates = append(candidates, e)
+               }
+       }
+       if len(candidates) == 0 {
+               t.Errorf("could not find log with category: %q", category)
+       } else if len(candidates) > 1 {
+               t.Errorf("found more than one log with category: %q", category)
+       }
+       return candidates[0]
+}
+
+// dumpStack returns e.Stk as a string.
+func dumpStack(e *trace.Event) string {
+       var buf bytes.Buffer
+       for _, f := range e.Stk {
+               file := strings.TrimPrefix(f.File, runtime.GOROOT())
+               fmt.Fprintf(&buf, "%s\n\t%s:%d\n", f.Fn, file, f.Line)
+       }
+       return buf.String()
+}
+
+// parseTrace parses the given trace or skips the test if the trace is broken
+// due to known issues. Partially copied from runtime/trace/trace_test.go.
+func parseTrace(t *testing.T, r io.Reader) []*trace.Event {
+       res, err := trace.Parse(r, "")
+       if err == trace.ErrTimeOrder {
+               t.Skipf("skipping trace: %v", err)
+       }
+       if err != nil {
+               t.Fatalf("failed to parse trace: %v", err)
+       }
+       return res.Events
+}