From bc69ad3a77129cd4cf4d3bdaa592dfc95ff8c769 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Fri, 19 Aug 2022 13:42:38 +0000 Subject: [PATCH] Revert "misc/cgo/testcarchive: permit SIGQUIT for TestSignalForwardingExternal" This reverts CL 419014. Reason for revert: broke darwin-amd64 builders Change-Id: I77838e696a55c415d322ebb9846503de25b546d5 Reviewed-on: https://go-review.googlesource.com/c/go/+/424954 Run-TryBot: Cuong Manh Le TryBot-Result: Gopher Robot Reviewed-by: Joedian Reid Reviewed-by: Bryan Mills --- misc/cgo/testcarchive/carchive_test.go | 235 +++++++----------- .../testcarchive/testdata/libgo2/libgo2.go | 6 - misc/cgo/testcarchive/testdata/main5.c | 13 +- 3 files changed, 97 insertions(+), 157 deletions(-) diff --git a/misc/cgo/testcarchive/carchive_test.go b/misc/cgo/testcarchive/carchive_test.go index ed0e84d680..b959bc6cfa 100644 --- a/misc/cgo/testcarchive/carchive_test.go +++ b/misc/cgo/testcarchive/carchive_test.go @@ -19,7 +19,6 @@ import ( "runtime" "strconv" "strings" - "sync" "syscall" "testing" "time" @@ -527,13 +526,38 @@ func TestEarlySignalHandler(t *testing.T) { func TestSignalForwarding(t *testing.T) { checkSignalForwardingTest(t) - buildSignalForwardingTest(t) - cmd := exec.Command(bin[0], append(bin[1:], "1")...) + if !testWork { + defer func() { + os.Remove("libgo2.a") + os.Remove("libgo2.h") + os.Remove("testp" + exeSuffix) + os.RemoveAll(filepath.Join(GOPATH, "pkg")) + }() + } + + cmd := exec.Command("go", "build", "-buildmode=c-archive", "-o", "libgo2.a", "./libgo2") + if out, err := cmd.CombinedOutput(); err != nil { + t.Logf("%s", out) + t.Fatal(err) + } + checkLineComments(t, "libgo2.h") + checkArchive(t, "libgo2.a") + + ccArgs := append(cc, "-o", "testp"+exeSuffix, "main5.c", "libgo2.a") + if runtime.Compiler == "gccgo" { + ccArgs = append(ccArgs, "-lgo") + } + if out, err := exec.Command(ccArgs[0], ccArgs[1:]...).CombinedOutput(); err != nil { + t.Logf("%s", out) + t.Fatal(err) + } + + cmd = exec.Command(bin[0], append(bin[1:], "1")...) out, err := cmd.CombinedOutput() t.Logf("%v\n%s", cmd.Args, out) - expectSignal(t, err, syscall.SIGSEGV, 0) + expectSignal(t, err, syscall.SIGSEGV) // SIGPIPE is never forwarded on darwin. See golang.org/issue/33384. if runtime.GOOS != "darwin" && runtime.GOOS != "ios" { @@ -544,7 +568,7 @@ func TestSignalForwarding(t *testing.T) { if len(out) > 0 { t.Logf("%s", out) } - expectSignal(t, err, syscall.SIGPIPE, 0) + expectSignal(t, err, syscall.SIGPIPE) } } @@ -555,93 +579,21 @@ func TestSignalForwardingExternal(t *testing.T) { t.Skipf("skipping on %s/%s: runtime does not permit SI_USER SIGSEGV", GOOS, GOARCH) } checkSignalForwardingTest(t) - buildSignalForwardingTest(t) - - // We want to send the process a signal and see if it dies. - // Normally the signal goes to the C thread, the Go signal - // handler picks it up, sees that it is running in a C thread, - // and the program dies. Unfortunately, occasionally the - // signal is delivered to a Go thread, which winds up - // discarding it because it was sent by another program and - // there is no Go handler for it. To avoid this, run the - // program several times in the hopes that it will eventually - // fail. - const tries = 20 - for i := 0; i < tries; i++ { - err := runSignalForwardingTest(t, "2") - if err == nil { - continue - } - - // If the signal is delivered to a C thread, as expected, - // the Go signal handler will disable itself and re-raise - // the signal, causing the program to die with SIGSEGV. - // - // It is also possible that the signal will be - // delivered to a Go thread, such as a GC thread. - // Currently when the Go runtime sees that a SIGSEGV was - // sent from a different program, it first tries to send - // the signal to the os/signal API. If nothing is looking - // for (or explicitly ignoring) SIGSEGV, then it crashes. - // Because the Go runtime is invoked via a c-archive, - // it treats this as GOTRACEBACK=crash, meaning that it - // dumps a stack trace for all goroutines, which it does - // by raising SIGQUIT. The effect is that we will see the - // program die with SIGQUIT in that case, not SIGSEGV. - if expectSignal(t, err, syscall.SIGSEGV, syscall.SIGQUIT) { - return - } - } - - t.Errorf("program succeeded unexpectedly %d times", tries) -} - -func TestSignalForwardingGo(t *testing.T) { - checkSignalForwardingTest(t) - buildSignalForwardingTest(t) - err := runSignalForwardingTest(t, "4") - - // Occasionally the signal will be delivered to a C thread, - // and the program will crash with SIGSEGV. - expectSignal(t, err, syscall.SIGQUIT, syscall.SIGSEGV) -} -// checkSignalForwardingTest calls t.Skip if the SignalForwarding test -// doesn't work on this platform. -func checkSignalForwardingTest(t *testing.T) { - switch GOOS { - case "darwin", "ios": - switch GOARCH { - case "arm64": - t.Skipf("skipping on %s/%s; see https://golang.org/issue/13701", GOOS, GOARCH) - } - case "windows": - t.Skip("skipping signal test on Windows") - } -} - -// buildSignalForwardingTest builds the executable used by the various -// signal forwarding tests. -func buildSignalForwardingTest(t *testing.T) { if !testWork { - t.Cleanup(func() { + defer func() { os.Remove("libgo2.a") os.Remove("libgo2.h") os.Remove("testp" + exeSuffix) os.RemoveAll(filepath.Join(GOPATH, "pkg")) - }) + }() } - t.Log("go build -buildmode=c-archive -o libgo2.a ./libgo2") cmd := exec.Command("go", "build", "-buildmode=c-archive", "-o", "libgo2.a", "./libgo2") - out, err := cmd.CombinedOutput() - if len(out) > 0 { + if out, err := cmd.CombinedOutput(); err != nil { t.Logf("%s", out) - } - if err != nil { t.Fatal(err) } - checkLineComments(t, "libgo2.h") checkArchive(t, "libgo2.a") @@ -649,92 +601,91 @@ func buildSignalForwardingTest(t *testing.T) { if runtime.Compiler == "gccgo" { ccArgs = append(ccArgs, "-lgo") } - t.Log(ccArgs) - out, err = exec.Command(ccArgs[0], ccArgs[1:]...).CombinedOutput() - if len(out) > 0 { + if out, err := exec.Command(ccArgs[0], ccArgs[1:]...).CombinedOutput(); err != nil { t.Logf("%s", out) - } - if err != nil { t.Fatal(err) } -} -func runSignalForwardingTest(t *testing.T, arg string) error { - t.Logf("%v %s", bin, arg) - cmd := exec.Command(bin[0], append(bin[1:], arg)...) + // We want to send the process a signal and see if it dies. + // Normally the signal goes to the C thread, the Go signal + // handler picks it up, sees that it is running in a C thread, + // and the program dies. Unfortunately, occasionally the + // signal is delivered to a Go thread, which winds up + // discarding it because it was sent by another program and + // there is no Go handler for it. To avoid this, run the + // program several times in the hopes that it will eventually + // fail. + const tries = 20 + for i := 0; i < tries; i++ { + cmd = exec.Command(bin[0], append(bin[1:], "2")...) - var out strings.Builder - cmd.Stdout = &out + stderr, err := cmd.StderrPipe() + if err != nil { + t.Fatal(err) + } + defer stderr.Close() - stderr, err := cmd.StderrPipe() - if err != nil { - t.Fatal(err) - } - defer stderr.Close() + r := bufio.NewReader(stderr) - r := bufio.NewReader(stderr) + err = cmd.Start() - err = cmd.Start() - if err != nil { - t.Fatal(err) - } + if err != nil { + t.Fatal(err) + } - // Wait for trigger to ensure that process is started. - ok, err := r.ReadString('\n') + // Wait for trigger to ensure that the process is started. + ok, err := r.ReadString('\n') - // Verify trigger. - if err != nil || ok != "OK\n" { - t.Fatal("Did not receive OK signal") - } + // Verify trigger. + if err != nil || ok != "OK\n" { + t.Fatalf("Did not receive OK signal") + } - var wg sync.WaitGroup - wg.Add(1) - var errsb strings.Builder - go func() { - defer wg.Done() - io.Copy(&errsb, r) - }() + // Give the program a chance to enter the sleep function. + time.Sleep(time.Millisecond) - // Give the program a chance to enter the function. - // If the program doesn't get there the test will still - // pass, although it doesn't quite test what we intended. - // This is fine as long as the program normally makes it. - time.Sleep(time.Millisecond) + cmd.Process.Signal(syscall.SIGSEGV) - cmd.Process.Signal(syscall.SIGSEGV) + err = cmd.Wait() - err = cmd.Wait() + if err == nil { + continue + } - s := out.String() - if len(s) > 0 { - t.Log(s) - } - wg.Wait() - s = errsb.String() - if len(s) > 0 { - t.Log(s) + if expectSignal(t, err, syscall.SIGSEGV) { + return + } } - return err + t.Errorf("program succeeded unexpectedly %d times", tries) +} + +// checkSignalForwardingTest calls t.Skip if the SignalForwarding test +// doesn't work on this platform. +func checkSignalForwardingTest(t *testing.T) { + switch GOOS { + case "darwin", "ios": + switch GOARCH { + case "arm64": + t.Skipf("skipping on %s/%s; see https://golang.org/issue/13701", GOOS, GOARCH) + } + case "windows": + t.Skip("skipping signal test on Windows") + } } // expectSignal checks that err, the exit status of a test program, -// shows a failure due to a specific signal or two. Returns whether we -// found an expected signal. -func expectSignal(t *testing.T, err error, sig1, sig2 syscall.Signal) bool { - t.Helper() +// shows a failure due to a specific signal. Returns whether we found +// the expected signal. +func expectSignal(t *testing.T, err error, sig syscall.Signal) bool { if err == nil { t.Error("test program succeeded unexpectedly") } else if ee, ok := err.(*exec.ExitError); !ok { t.Errorf("error (%v) has type %T; expected exec.ExitError", err, err) } else if ws, ok := ee.Sys().(syscall.WaitStatus); !ok { t.Errorf("error.Sys (%v) has type %T; expected syscall.WaitStatus", ee.Sys(), ee.Sys()) - } else if !ws.Signaled() || (ws.Signal() != sig1 && ws.Signal() != sig2) { - if sig2 == 0 { - t.Errorf("got %q; expected signal %q", ee, sig1) - } else { - t.Errorf("got %q; expected signal %q or %q", ee, sig1, sig2) - } + } else if !ws.Signaled() || ws.Signal() != sig { + t.Errorf("got %v; expected signal %v", ee, sig) } else { return true } @@ -1071,14 +1022,14 @@ func TestCompileWithoutShared(t *testing.T) { binArgs := append(cmdToRun(exe), "1") out, err = exec.Command(binArgs[0], binArgs[1:]...).CombinedOutput() t.Logf("%v\n%s", binArgs, out) - expectSignal(t, err, syscall.SIGSEGV, 0) + expectSignal(t, err, syscall.SIGSEGV) // SIGPIPE is never forwarded on darwin. See golang.org/issue/33384. if runtime.GOOS != "darwin" && runtime.GOOS != "ios" { binArgs := append(cmdToRun(exe), "3") out, err = exec.Command(binArgs[0], binArgs[1:]...).CombinedOutput() t.Logf("%v\n%s", binArgs, out) - expectSignal(t, err, syscall.SIGPIPE, 0) + expectSignal(t, err, syscall.SIGPIPE) } } diff --git a/misc/cgo/testcarchive/testdata/libgo2/libgo2.go b/misc/cgo/testcarchive/testdata/libgo2/libgo2.go index 35c89ae92b..19c8e1a6dc 100644 --- a/misc/cgo/testcarchive/testdata/libgo2/libgo2.go +++ b/misc/cgo/testcarchive/testdata/libgo2/libgo2.go @@ -49,12 +49,6 @@ func RunGoroutines() { } } -// Block blocks the current thread while running Go code. -//export Block -func Block() { - select {} -} - var P *byte // TestSEGV makes sure that an invalid address turns into a run-time Go panic. diff --git a/misc/cgo/testcarchive/testdata/main5.c b/misc/cgo/testcarchive/testdata/main5.c index c64c246fde..d431ce01ce 100644 --- a/misc/cgo/testcarchive/testdata/main5.c +++ b/misc/cgo/testcarchive/testdata/main5.c @@ -29,6 +29,10 @@ int main(int argc, char** argv) { verbose = (argc > 2); + if (verbose) { + printf("calling RunGoroutines\n"); + } + Noop(); switch (test) { @@ -86,15 +90,6 @@ int main(int argc, char** argv) { printf("did not receive SIGPIPE\n"); return 0; } - case 4: { - fprintf(stderr, "OK\n"); - fflush(stderr); - - if (verbose) { - printf("calling Block\n"); - } - Block(); - } default: printf("Unknown test: %d\n", test); return 0; -- 2.44.0