From 6b974e2bd3197bf6ff2245f5b24c0fccef352903 Mon Sep 17 00:00:00 2001 From: Michael Munday Date: Tue, 14 Apr 2020 15:46:26 +0100 Subject: [PATCH 1/7] [release-branch.go1.13] cmd/compile: fix deallocation of live value copies in regalloc When deallocating the input register to a phi so that the phi itself could be allocated to that register the code was also deallocating all copies of that phi input value. Those copies of the value could still be live and if they were the register allocator could reuse them incorrectly to hold speculative copies of other phi inputs. This causes strange bugs. No test because this is a very obscure scenario that is hard to replicate but CL 228060 adds an assertion to the compiler that does trigger when running the std tests on linux/s390x without this CL applied. Hopefully that assertion will prevent future regressions. Fixes #38442. Change-Id: Id975dadedd731c7bb21933b9ea6b17daaa5c9e1d Reviewed-on: https://go-review.googlesource.com/c/go/+/228061 Run-TryBot: Michael Munday TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall (cherry picked from commit 382fe3e2498f2066400e7e7007aa9903440e339d) Reviewed-on: https://go-review.googlesource.com/c/go/+/230358 --- src/cmd/compile/internal/ssa/regalloc.go | 28 +++++++++++++----------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/cmd/compile/internal/ssa/regalloc.go b/src/cmd/compile/internal/ssa/regalloc.go index 8abbf61507..e92ab00e95 100644 --- a/src/cmd/compile/internal/ssa/regalloc.go +++ b/src/cmd/compile/internal/ssa/regalloc.go @@ -976,25 +976,22 @@ func (s *regAllocState) regalloc(f *Func) { } } - // Second pass - deallocate any phi inputs which are now dead. + // Second pass - deallocate all in-register phi inputs. for i, v := range phis { if !s.values[v.ID].needReg { continue } a := v.Args[idx] - if !regValLiveSet.contains(a.ID) { - // Input is dead beyond the phi, deallocate - // anywhere else it might live. - s.freeRegs(s.values[a.ID].regs) - } else { - // Input is still live. + r := phiRegs[i] + if r == noRegister { + continue + } + if regValLiveSet.contains(a.ID) { + // Input value is still live (it is used by something other than Phi). // Try to move it around before kicking out, if there is a free register. // We generate a Copy in the predecessor block and record it. It will be - // deleted if never used. - r := phiRegs[i] - if r == noRegister { - continue - } + // deleted later if never used. + // // Pick a free register. At this point some registers used in the predecessor // block may have been deallocated. Those are the ones used for Phis. Exclude // them (and they are not going to be helpful anyway). @@ -1010,8 +1007,8 @@ func (s *regAllocState) regalloc(f *Func) { s.assignReg(r2, a, c) s.endRegs[p.ID] = append(s.endRegs[p.ID], endReg{r2, a, c}) } - s.freeReg(r) } + s.freeReg(r) } // Copy phi ops into new schedule. @@ -1842,6 +1839,11 @@ func (s *regAllocState) shuffle(stacklive [][]ID) { e.process() } } + + if s.f.pass.debug > regDebug { + fmt.Printf("post shuffle %s\n", s.f.Name) + fmt.Println(s.f.String()) + } } type edgeState struct { From 237b6067c17ac3ef1e02632b77deefb5e9837cbb Mon Sep 17 00:00:00 2001 From: Andrew Bonventre Date: Thu, 14 May 2020 14:13:47 -0400 Subject: [PATCH 2/7] [release-branch.go1.13] go1.13.11 Change-Id: I6fb5fbb3caf64e7d33412be4893edf8564e3a4de Reviewed-on: https://go-review.googlesource.com/c/go/+/234001 Run-TryBot: Andrew Bonventre TryBot-Result: Gobot Gobot Reviewed-by: Dmitri Shuralyov --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 6d0e854fc6..9100b47ed9 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.13.10 \ No newline at end of file +go1.13.11 \ No newline at end of file From 1a1178122f31bfe67534ec637883636755adbfbc Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Thu, 7 May 2020 18:13:21 -0400 Subject: [PATCH 3/7] [release-branch.go1.13] runtime: disable preemption in startTemplateThread When a locked M wants to start a new M, it hands off to the template thread to actually call clone and start the thread. The template thread is lazily created the first time a thread is locked (or if cgo is in use). stoplockedm will release the P (_Pidle), then call handoffp to give the P to another M. In the case of a pending STW, one of two things can happen: 1. handoffp starts an M, which does acquirep followed by schedule, which will finally enter _Pgcstop. 2. handoffp immediately enters _Pgcstop. This only occurs if the P has no local work, GC work, and no spinning M is required. If handoffp starts an M, and must create a new M to do so, then newm will simply queue the M on newmHandoff for the template thread to do the clone. When a stop-the-world is required, stopTheWorldWithSema will start the stop and then wait for all Ps to enter _Pgcstop. If the template thread is not fully created because startTemplateThread gets stopped, then another stoplockedm may queue an M that will never get created, and the handoff P will never leave _Pidle. Thus stopTheWorldWithSema will wait forever. A sequence to trigger this hang when STW occurs can be visualized with two threads: T1 T2 ------------------------------- ----------------------------- LockOSThread LockOSThread haveTemplateThread == 0 startTemplateThread haveTemplateThread = 1 newm haveTemplateThread == 1 preempt -> schedule g.m.lockedExt++ gcstopm -> _Pgcstop g.m.lockedg = ... park g.lockedm = ... return ... (any code) preempt -> schedule stoplockedm releasep -> _Pidle handoffp startm (first 3 handoffp cases) newm g.m.lockedExt != 0 Add to newmHandoff, return park Note that the P in T2 is stuck sitting in _Pidle. Since the template thread isn't running, the new M will not be started complete the transition to _Pgcstop. To resolve this, we disable preemption around the assignment of haveTemplateThread and the creation of the template thread in order to guarantee that if handTemplateThread is set then the template thread will eventually exist, in the presence of stops. For #38931 Fixes #38932 Change-Id: I50535fbbe2f328f47b18e24d9030136719274191 Reviewed-on: https://go-review.googlesource.com/c/go/+/232978 Run-TryBot: Michael Pratt Reviewed-by: Austin Clements TryBot-Result: Gobot Gobot (cherry picked from commit 11b3730a02c93fd5745bfd977156541a9033759b) Reviewed-on: https://go-review.googlesource.com/c/go/+/234888 Reviewed-by: Cherry Zhang --- src/runtime/crash_test.go | 14 +++++- src/runtime/proc.go | 6 +++ src/runtime/proc_test.go | 24 +++++++++ src/runtime/testdata/testprog/lockosthread.go | 49 +++++++++++++++++++ 4 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go index c54bb57da2..ac0344e9a3 100644 --- a/src/runtime/crash_test.go +++ b/src/runtime/crash_test.go @@ -55,6 +55,16 @@ func runTestProg(t *testing.T, binary, name string, env ...string) string { t.Fatal(err) } + return runBuiltTestProg(t, exe, name, env...) +} + +func runBuiltTestProg(t *testing.T, exe, name string, env ...string) string { + if *flagQuick { + t.Skip("-quick") + } + + testenv.MustHaveGoBuild(t) + cmd := testenv.CleanCmdEnv(exec.Command(exe, name)) cmd.Env = append(cmd.Env, env...) if testing.Short() { @@ -64,7 +74,7 @@ func runTestProg(t *testing.T, binary, name string, env ...string) string { cmd.Stdout = &b cmd.Stderr = &b if err := cmd.Start(); err != nil { - t.Fatalf("starting %s %s: %v", binary, name, err) + t.Fatalf("starting %s %s: %v", exe, name, err) } // If the process doesn't complete within 1 minute, @@ -92,7 +102,7 @@ func runTestProg(t *testing.T, binary, name string, env ...string) string { }() if err := cmd.Wait(); err != nil { - t.Logf("%s %s exit status: %v", binary, name, err) + t.Logf("%s %s exit status: %v", exe, name, err) } close(done) diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 93d329d15e..3723f84a05 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -1862,10 +1862,16 @@ func startTemplateThread() { if GOARCH == "wasm" { // no threads on wasm yet return } + + // Disable preemption to guarantee that the template thread will be + // created before a park once haveTemplateThread is set. + mp := acquirem() if !atomic.Cas(&newmHandoff.haveTemplateThread, 0, 1) { + releasem(mp) return } newm(templateThread, nil) + releasem(mp) } // templateThread is a thread in a known-good state that exists solely diff --git a/src/runtime/proc_test.go b/src/runtime/proc_test.go index 6e6272e80a..108be6a881 100644 --- a/src/runtime/proc_test.go +++ b/src/runtime/proc_test.go @@ -6,6 +6,7 @@ package runtime_test import ( "fmt" + "internal/testenv" "math" "net" "runtime" @@ -912,6 +913,29 @@ func TestLockOSThreadAvoidsStatePropagation(t *testing.T) { } } +func TestLockOSThreadTemplateThreadRace(t *testing.T) { + testenv.MustHaveGoRun(t) + + exe, err := buildTestProg(t, "testprog") + if err != nil { + t.Fatal(err) + } + + iterations := 100 + if testing.Short() { + // Reduce run time to ~100ms, with much lower probability of + // catching issues. + iterations = 5 + } + for i := 0; i < iterations; i++ { + want := "OK\n" + output := runBuiltTestProg(t, exe, "LockOSThreadTemplateThreadRace") + if output != want { + t.Fatalf("run %d: want %q, got %q", i, want, output) + } + } +} + // fakeSyscall emulates a system call. //go:nosplit func fakeSyscall(duration time.Duration) { diff --git a/src/runtime/testdata/testprog/lockosthread.go b/src/runtime/testdata/testprog/lockosthread.go index fd3123e647..098cc4dd72 100644 --- a/src/runtime/testdata/testprog/lockosthread.go +++ b/src/runtime/testdata/testprog/lockosthread.go @@ -7,6 +7,7 @@ package main import ( "os" "runtime" + "sync" "time" ) @@ -30,6 +31,7 @@ func init() { runtime.LockOSThread() }) register("LockOSThreadAvoidsStatePropagation", LockOSThreadAvoidsStatePropagation) + register("LockOSThreadTemplateThreadRace", LockOSThreadTemplateThreadRace) } func LockOSThreadMain() { @@ -195,3 +197,50 @@ func LockOSThreadAvoidsStatePropagation() { runtime.UnlockOSThread() println("OK") } + +func LockOSThreadTemplateThreadRace() { + // This test attempts to reproduce the race described in + // golang.org/issue/38931. To do so, we must have a stop-the-world + // (achieved via ReadMemStats) racing with two LockOSThread calls. + // + // While this test attempts to line up the timing, it is only expected + // to fail (and thus hang) around 2% of the time if the race is + // present. + + // Ensure enough Ps to actually run everything in parallel. Though on + // <4 core machines, we are still at the whim of the kernel scheduler. + runtime.GOMAXPROCS(4) + + go func() { + // Stop the world; race with LockOSThread below. + var m runtime.MemStats + for { + runtime.ReadMemStats(&m) + } + }() + + // Try to synchronize both LockOSThreads. + start := time.Now().Add(10*time.Millisecond) + + var wg sync.WaitGroup + wg.Add(2) + + for i := 0; i < 2; i++ { + go func() { + for time.Now().Before(start) { + } + + // Add work to the local runq to trigger early startm + // in handoffp. + go func(){}() + + runtime.LockOSThread() + runtime.Gosched() // add a preemption point. + wg.Done() + }() + } + + wg.Wait() + // If both LockOSThreads completed then we did not hit the race. + println("OK") +} From 444ad952c4dd346955362165aa20d0b4938ffcca Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 15 Oct 2019 13:44:22 -0700 Subject: [PATCH 4/7] [release-branch.go1.13] math/big: make Rat accessors safe for concurrent use Do not modify the underlying Rat denominator when calling one of the accessors Float32, Float64; verify that we don't modify the Rat denominator when calling Inv, Sign, IsInt, Num. For #36689. For #34919. For #33792. Change-Id: Ife6d1252373f493a597398ee51e7b5695b708df5 Reviewed-on: https://go-review.googlesource.com/c/go/+/201205 Reviewed-by: Ian Lance Taylor Reviewed-on: https://go-review.googlesource.com/c/go/+/233321 Run-TryBot: Dmitri Shuralyov TryBot-Result: Gobot Gobot Reviewed-by: Emmanuel Odeke --- src/math/big/rat.go | 8 ++++---- src/math/big/rat_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/math/big/rat.go b/src/math/big/rat.go index c8bf698b18..9527b74949 100644 --- a/src/math/big/rat.go +++ b/src/math/big/rat.go @@ -271,7 +271,7 @@ func quotToFloat64(a, b nat) (f float64, exact bool) { func (x *Rat) Float32() (f float32, exact bool) { b := x.b.abs if len(b) == 0 { - b = b.set(natOne) // materialize denominator + b = natOne } f, exact = quotToFloat32(x.a.abs, b) if x.a.neg { @@ -287,7 +287,7 @@ func (x *Rat) Float32() (f float32, exact bool) { func (x *Rat) Float64() (f float64, exact bool) { b := x.b.abs if len(b) == 0 { - b = b.set(natOne) // materialize denominator + b = natOne } f, exact = quotToFloat64(x.a.abs, b) if x.a.neg { @@ -377,7 +377,7 @@ func (z *Rat) Inv(x *Rat) *Rat { z.Set(x) a := z.b.abs if len(a) == 0 { - a = a.set(natOne) // materialize numerator + a = a.set(natOne) // materialize numerator (a is part of z!) } b := z.a.abs if b.cmp(natOne) == 0 { @@ -416,7 +416,7 @@ func (x *Rat) Num() *Int { func (x *Rat) Denom() *Int { x.b.neg = false // the result is always >= 0 if len(x.b.abs) == 0 { - x.b.abs = x.b.abs.set(natOne) // materialize denominator + x.b.abs = x.b.abs.set(natOne) // materialize denominator (see issue #33792) } return &x.b } diff --git a/src/math/big/rat_test.go b/src/math/big/rat_test.go index 83c5d5cfea..35bc85c8cd 100644 --- a/src/math/big/rat_test.go +++ b/src/math/big/rat_test.go @@ -678,3 +678,29 @@ func BenchmarkRatCmp(b *testing.B) { x.Cmp(y) } } + +// TestIssue34919 verifies that a Rat's denominator is not modified +// when simply accessing the Rat value. +func TestIssue34919(t *testing.T) { + for _, acc := range []struct { + name string + f func(*Rat) + }{ + {"Float32", func(x *Rat) { x.Float32() }}, + {"Float64", func(x *Rat) { x.Float64() }}, + {"Inv", func(x *Rat) { new(Rat).Inv(x) }}, + {"Sign", func(x *Rat) { x.Sign() }}, + {"IsInt", func(x *Rat) { x.IsInt() }}, + {"Num", func(x *Rat) { x.Num() }}, + // {"Denom", func(x *Rat) { x.Denom() }}, TODO(gri) should we change the API? See issue #33792. + } { + // A denominator of length 0 is interpreted as 1. Make sure that + // "materialization" of the denominator doesn't lead to setting + // the underlying array element 0 to 1. + r := &Rat{Int{abs: nat{991}}, Int{abs: make(nat, 0, 1)}} + acc.f(r) + if d := r.b.abs[:1][0]; d != 0 { + t.Errorf("%s modified denominator: got %d, want 0", acc.name, d) + } + } +} From 7f1ef49347e6e1f5a51c9ca3e4f4f617c2317042 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 23 Oct 2019 14:22:32 -0700 Subject: [PATCH 5/7] [release-branch.go1.13] math/big: normalize unitialized denominators ASAP A Rat is represented via a quotient a/b where a and b are Int values. To make it possible to use an uninitialized Rat value (with a and b uninitialized and thus == 0), the implementation treats a 0 denominator as 1. For each operation we check if the denominator is 0, and then treat it as 1 (if necessary). Operations that create a new Rat result, normalize that value such that a result denominator 1 is represened as 0 again. This CL changes this behavior slightly: 0 denominators are still interpreted as 1, but whenever we (safely) can, we set an uninitialized 0 denominator to 1. This simplifies the code overall. Also: Improved some doc strings. Preparation for addressing issue #33792. For #36689. For #33792. Change-Id: I3040587c8d0dad2e840022f96ca027d8470878a0 Reviewed-on: https://go-review.googlesource.com/c/go/+/202997 Run-TryBot: Robert Griesemer TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick Reviewed-on: https://go-review.googlesource.com/c/go/+/233322 Run-TryBot: Dmitri Shuralyov Reviewed-by: Emmanuel Odeke --- src/math/big/rat.go | 55 +++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/src/math/big/rat.go b/src/math/big/rat.go index 9527b74949..ee8af1456d 100644 --- a/src/math/big/rat.go +++ b/src/math/big/rat.go @@ -22,7 +22,9 @@ import ( // of Rats are not supported and may lead to errors. type Rat struct { // To make zero values for Rat work w/o initialization, - // a zero value of b (len(b) == 0) acts like b == 1. + // a zero value of b (len(b) == 0) acts like b == 1. At + // the earliest opportunity (when an assignment to the Rat + // is made), such uninitialized denominators are set to 1. // a.neg determines the sign of the Rat, b.neg is ignored. a, b Int } @@ -297,6 +299,7 @@ func (x *Rat) Float64() (f float64, exact bool) { } // SetFrac sets z to a/b and returns z. +// If b == 0, SetFrac panics. func (z *Rat) SetFrac(a, b *Int) *Rat { z.a.neg = a.neg != b.neg babs := b.abs @@ -312,11 +315,12 @@ func (z *Rat) SetFrac(a, b *Int) *Rat { } // SetFrac64 sets z to a/b and returns z. +// If b == 0, SetFrac64 panics. func (z *Rat) SetFrac64(a, b int64) *Rat { - z.a.SetInt64(a) if b == 0 { panic("division by zero") } + z.a.SetInt64(a) if b < 0 { b = -b z.a.neg = !z.a.neg @@ -328,21 +332,21 @@ func (z *Rat) SetFrac64(a, b int64) *Rat { // SetInt sets z to x (by making a copy of x) and returns z. func (z *Rat) SetInt(x *Int) *Rat { z.a.Set(x) - z.b.abs = z.b.abs[:0] + z.b.abs = z.b.abs.setWord(1) return z } // SetInt64 sets z to x and returns z. func (z *Rat) SetInt64(x int64) *Rat { z.a.SetInt64(x) - z.b.abs = z.b.abs[:0] + z.b.abs = z.b.abs.setWord(1) return z } // SetUint64 sets z to x and returns z. func (z *Rat) SetUint64(x uint64) *Rat { z.a.SetUint64(x) - z.b.abs = z.b.abs[:0] + z.b.abs = z.b.abs.setWord(1) return z } @@ -352,6 +356,9 @@ func (z *Rat) Set(x *Rat) *Rat { z.a.Set(&x.a) z.b.Set(&x.b) } + if len(z.b.abs) == 0 { + z.b.abs = z.b.abs.setWord(1) + } return z } @@ -370,20 +377,13 @@ func (z *Rat) Neg(x *Rat) *Rat { } // Inv sets z to 1/x and returns z. +// If x == 0, Inv panics. func (z *Rat) Inv(x *Rat) *Rat { if len(x.a.abs) == 0 { panic("division by zero") } z.Set(x) - a := z.b.abs - if len(a) == 0 { - a = a.set(natOne) // materialize numerator (a is part of z!) - } - b := z.a.abs - if b.cmp(natOne) == 0 { - b = b[:0] // normalize denominator - } - z.a.abs, z.b.abs = a, b // sign doesn't change + z.a.abs, z.b.abs = z.b.abs, z.a.abs return z } @@ -424,25 +424,20 @@ func (x *Rat) Denom() *Int { func (z *Rat) norm() *Rat { switch { case len(z.a.abs) == 0: - // z == 0 - normalize sign and denominator + // z == 0; normalize sign and denominator z.a.neg = false - z.b.abs = z.b.abs[:0] + fallthrough case len(z.b.abs) == 0: - // z is normalized int - nothing to do - case z.b.abs.cmp(natOne) == 0: - // z is int - normalize denominator - z.b.abs = z.b.abs[:0] + // z is integer; normalize denominator + z.b.abs = z.b.abs.setWord(1) default: + // z is fraction; normalize numerator and denominator neg := z.a.neg z.a.neg = false z.b.neg = false if f := NewInt(0).lehmerGCD(nil, nil, &z.a, &z.b); f.Cmp(intOne) != 0 { z.a.abs, _ = z.a.abs.div(nil, z.a.abs, f.abs) z.b.abs, _ = z.b.abs.div(nil, z.b.abs, f.abs) - if z.b.abs.cmp(natOne) == 0 { - // z is int - normalize denominator - z.b.abs = z.b.abs[:0] - } } z.a.neg = neg } @@ -454,6 +449,8 @@ func (z *Rat) norm() *Rat { // returns z. func mulDenom(z, x, y nat) nat { switch { + case len(x) == 0 && len(y) == 0: + return z.setWord(1) case len(x) == 0: return z.set(y) case len(y) == 0: @@ -509,10 +506,14 @@ func (z *Rat) Sub(x, y *Rat) *Rat { // Mul sets z to the product x*y and returns z. func (z *Rat) Mul(x, y *Rat) *Rat { if x == y { - // a squared Rat is positive and can't be reduced + // a squared Rat is positive and can't be reduced (no need to call norm()) z.a.neg = false z.a.abs = z.a.abs.sqr(x.a.abs) - z.b.abs = z.b.abs.sqr(x.b.abs) + if len(x.b.abs) == 0 { + z.b.abs = z.b.abs.setWord(1) + } else { + z.b.abs = z.b.abs.sqr(x.b.abs) + } return z } z.a.Mul(&x.a, &y.a) @@ -521,7 +522,7 @@ func (z *Rat) Mul(x, y *Rat) *Rat { } // Quo sets z to the quotient x/y and returns z. -// If y == 0, a division-by-zero run-time panic occurs. +// If y == 0, Quo panics. func (z *Rat) Quo(x, y *Rat) *Rat { if len(y.a.abs) == 0 { panic("division by zero") From 90fe4a73fff10394097e21085a9c0c369c61b09a Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 23 Oct 2019 16:44:51 -0700 Subject: [PATCH 6/7] [release-branch.go1.13] math/big: make Rat.Denom side-effect free A Rat is represented via a quotient a/b where a and b are Int values. To make it possible to use an uninitialized Rat value (with a and b uninitialized and thus == 0), the implementation treats a 0 denominator as 1. Rat.Num and Rat.Denom return pointers to these values a and b. Because b may be 0, Rat.Denom used to first initialize it to 1 and thus produce an undesirable side-effect (by changing the Rat's denominator). This CL changes Denom to return a new (not shared) *Int with value 1 in the rare case where the Rat was not initialized. This eliminates the side effect and returns the correct denominator value. While this is changing behavior of the API, the impact should now be minor because together with (prior) CL https://golang.org/cl/202997, which initializes Rats ASAP, Denom is unlikely used to access the denominator of an uninitialized (and thus 0) Rat. Any operation that will somehow set a Rat value will ensure that the denominator is not 0. Fixes #36689. For #33792. For #3521. Change-Id: I0bf15ac60513cf52162bfb62440817ba36f0c3fc Reviewed-on: https://go-review.googlesource.com/c/go/+/203059 Run-TryBot: Robert Griesemer TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick Reviewed-on: https://go-review.googlesource.com/c/go/+/233323 Reviewed-by: Emmanuel Odeke Run-TryBot: Dmitri Shuralyov --- src/math/big/rat.go | 11 +++++++++-- src/math/big/rat_test.go | 36 +++++++++++++++++++++++++++++------- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/math/big/rat.go b/src/math/big/rat.go index ee8af1456d..d35cd4cbd1 100644 --- a/src/math/big/rat.go +++ b/src/math/big/rat.go @@ -411,12 +411,19 @@ func (x *Rat) Num() *Int { } // Denom returns the denominator of x; it is always > 0. -// The result is a reference to x's denominator; it +// The result is a reference to x's denominator, unless +// x is an uninitialized (zero value) Rat, in which case +// the result is a new Int of value 1. (To initialize x, +// any operation that sets x will do, including x.Set(x).) +// If the result is a reference to x's denominator it // may change if a new value is assigned to x, and vice versa. func (x *Rat) Denom() *Int { x.b.neg = false // the result is always >= 0 if len(x.b.abs) == 0 { - x.b.abs = x.b.abs.set(natOne) // materialize denominator (see issue #33792) + // Note: If this proves problematic, we could + // panic instead and require the Rat to + // be explicitly initialized. + return &Int{abs: nat{1}} } return &x.b } diff --git a/src/math/big/rat_test.go b/src/math/big/rat_test.go index 35bc85c8cd..02569c1b16 100644 --- a/src/math/big/rat_test.go +++ b/src/math/big/rat_test.go @@ -329,18 +329,40 @@ func TestIssue3521(t *testing.T) { t.Errorf("0) got %s want %s", zero.Denom(), one) } - // 1a) a zero value remains zero independent of denominator - x := new(Rat) - x.Denom().Set(new(Int).Neg(b)) - if x.Cmp(zero) != 0 { - t.Errorf("1a) got %s want %s", x, zero) + // 1a) the denominator of an (uninitialized) zero value is not shared with the value + s := &zero.b + d := zero.Denom() + if d == s { + t.Errorf("1a) got %s (%p) == %s (%p) want different *Int values", d, d, s, s) } - // 1b) a zero value may have a denominator != 0 and != 1 + // 1b) the denominator of an (uninitialized) value is a new 1 each time + d1 := zero.Denom() + d2 := zero.Denom() + if d1 == d2 { + t.Errorf("1b) got %s (%p) == %s (%p) want different *Int values", d1, d1, d2, d2) + } + + // 1c) the denominator of an initialized zero value is shared with the value + x := new(Rat) + x.Set(x) // initialize x (any operation that sets x explicitly will do) + s = &x.b + d = x.Denom() + if d != s { + t.Errorf("1c) got %s (%p) != %s (%p) want identical *Int values", d, d, s, s) + } + + // 1d) a zero value remains zero independent of denominator + x.Denom().Set(new(Int).Neg(b)) + if x.Cmp(zero) != 0 { + t.Errorf("1d) got %s want %s", x, zero) + } + + // 1e) a zero value may have a denominator != 0 and != 1 x.Num().Set(a) qab := new(Rat).SetFrac(a, b) if x.Cmp(qab) != 0 { - t.Errorf("1b) got %s want %s", x, qab) + t.Errorf("1e) got %s want %s", x, qab) } // 2a) an integral value becomes a fraction depending on denominator From 6be4a5eb4898c7b5e7557dda061cc09ba310698b Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Mon, 1 Jun 2020 13:15:51 -0400 Subject: [PATCH 7/7] [release-branch.go1.13] go1.13.12 Change-Id: I1989d7cab0bf75c4e42d1c48146be9131d2c105c Reviewed-on: https://go-review.googlesource.com/c/go/+/235918 Run-TryBot: Dmitri Shuralyov TryBot-Result: Gobot Gobot Reviewed-by: Andrew Bonventre --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 9100b47ed9..0b3e27fd11 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.13.11 \ No newline at end of file +go1.13.12 \ No newline at end of file