[release-branch.go1.21] runtime: add race annotations in IncNonDefault

Also use CompareAndSwap to make the code actually less racy.

Added a test which will be meaningful when run under the race
detector (tested it -race with broken fix in runtime, it failed).

This backport incorporates the correction in CL 551856,
using racereleasemerge instead of racerelease.

Fixes #64757

Change-Id: I5972e08901d1adc8ba74858edad7eba91be1b0ce
Reviewed-on: https://go-review.googlesource.com/c/go/+/549796
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 3313bbb4055f38f53cd43c6c5782a229f445f230)
Reviewed-on: https://go-review.googlesource.com/c/go/+/550236
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Bypass: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

diff --git a/src/internal/godebug/godebug_test.go b/src/internal/godebug/godebug_test.go
index 8e46283adab3267a9f7bf671791748c03eecaf00..65dd256e8e7d29890b6375313a7a71bebdaed76c 100644 (file)
--- a/src/internal/godebug/godebug_test.go
+++ b/src/internal/godebug/godebug_test.go
@@ -7,6 +7,7 @@ package godebug_test
 import (
        "fmt"
        . "internal/godebug"
+       "internal/race"
        "internal/testenv"
        "os"
        "os/exec"
@@ -70,6 +71,36 @@ func TestMetrics(t *testing.T) {
        }
 }
 
+// TestPanicNilRace checks for a race in the runtime caused by use of runtime
+// atomics (not visible to usual race detection) to install the counter for
+// non-default panic(nil) semantics.  For #64649.
+func TestPanicNilRace(t *testing.T) {
+       if !race.Enabled {
+               t.Skip("Skipping test intended for use with -race.")
+       }
+       if os.Getenv("GODEBUG") != "panicnil=1" {
+               cmd := testenv.CleanCmdEnv(testenv.Command(t, os.Args[0], "-test.run=^TestPanicNilRace$", "-test.v", "-test.parallel=2", "-test.count=1"))
+               cmd.Env = append(cmd.Env, "GODEBUG=panicnil=1")
+               out, err := cmd.CombinedOutput()
+               t.Logf("output:\n%s", out)
+
+               if err != nil {
+                       t.Errorf("Was not expecting a crash")
+               }
+               return
+       }
+
+       test := func(t *testing.T) {
+               t.Parallel()
+               defer func() {
+                       recover()
+               }()
+               panic(nil)
+       }
+       t.Run("One", test)
+       t.Run("Two", test)
+}
+
 func TestCmdBisect(t *testing.T) {
        testenv.MustHaveGoBuild(t)
        out, err := exec.Command("go", "run", "cmd/vendor/golang.org/x/tools/cmd/bisect", "GODEBUG=buggy=1#PATTERN", os.Args[0], "-test.run=BisectTestCase").CombinedOutput()

diff --git a/src/runtime/runtime.go b/src/runtime/runtime.go
index 0822d0e8054e7b53f6347f8bcdbb1228dd82d80a..15119cf5dfc730d24757650e45c0ac16578e7b9b 100644 (file)
--- a/src/runtime/runtime.go
+++ b/src/runtime/runtime.go
@@ -101,12 +101,17 @@ func (g *godebugInc) IncNonDefault() {
                if newInc == nil {
                        return
                }
-               // If other goroutines are racing here, no big deal. One will win,
-               // and all the inc functions will be using the same underlying
-               // *godebug.Setting.
                inc = new(func())
                *inc = (*newInc)(g.name)
-               g.inc.Store(inc)
+               if raceenabled {
+                       racereleasemerge(unsafe.Pointer(&g.inc))
+               }
+               if !g.inc.CompareAndSwap(nil, inc) {
+                       inc = g.inc.Load()
+               }
+       }
+       if raceenabled {
+               raceacquire(unsafe.Pointer(&g.inc))
        }
        (*inc)()
 }