From 41e8a9f1cf78933ebdaf9fa29694df5129f1862c Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Wed, 31 Mar 2021 12:13:58 -0400 Subject: [PATCH] runtime: fix TestGCTestMoveStackOnNextCall flakes gcTestMoveStackOnNextCall can fail to move the stack in very rare cases if there's an unfortunately timed preemption that clobbers the stack guard. This won't happen multiple times in quick succession, so make the test just retry a few times. Change-Id: I247dc0551514e269e7132cee7945291429b0e865 Reviewed-on: https://go-review.googlesource.com/c/go/+/306671 Trust: Austin Clements Run-TryBot: Austin Clements Reviewed-by: Than McIntosh Reviewed-by: Michael Knyszek TryBot-Result: Go Bot --- src/runtime/gc_test.go | 18 ++++++++++++++---- src/runtime/mgc.go | 3 +++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/runtime/gc_test.go b/src/runtime/gc_test.go index 1ea1c2c745..273f399864 100644 --- a/src/runtime/gc_test.go +++ b/src/runtime/gc_test.go @@ -205,15 +205,24 @@ func TestGcZombieReporting(t *testing.T) { func TestGCTestMoveStackOnNextCall(t *testing.T) { t.Parallel() var onStack int - runtime.GCTestMoveStackOnNextCall() - moveStackCheck(t, &onStack, uintptr(unsafe.Pointer(&onStack))) + // GCTestMoveStackOnNextCall can fail in rare cases if there's + // a preemption. This won't happen many times in quick + // succession, so just retry a few times. + for retry := 0; retry < 5; retry++ { + runtime.GCTestMoveStackOnNextCall() + if moveStackCheck(t, &onStack, uintptr(unsafe.Pointer(&onStack))) { + // Passed. + return + } + } + t.Fatal("stack did not move") } // This must not be inlined because the point is to force a stack // growth check and move the stack. // //go:noinline -func moveStackCheck(t *testing.T, new *int, old uintptr) { +func moveStackCheck(t *testing.T, new *int, old uintptr) bool { // new should have been updated by the stack move; // old should not have. @@ -228,8 +237,9 @@ func moveStackCheck(t *testing.T, new *int, old uintptr) { t.Fatalf("test bug: new (%#x) should be a stack pointer, not %s", new2, cls) } // This was a real failure. - t.Fatal("stack did not move") + return false } + return true } func TestGCTestIsReachable(t *testing.T) { diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go index ecac354d83..f0c03b5102 100644 --- a/src/runtime/mgc.go +++ b/src/runtime/mgc.go @@ -2347,6 +2347,9 @@ func fmtNSAsMS(buf []byte, ns uint64) []byte { // if any other work appears after this call (such as returning). // Typically the following call should be marked go:noinline so it // performs a stack check. +// +// In rare cases this may not cause the stack to move, specifically if +// there's a preemption between this call and the next. func gcTestMoveStackOnNextCall() { gp := getg() gp.stackguard0 = getcallersp() -- 2.50.0