From d568e6e075e6434635268d3caf55963f4e564579 Mon Sep 17 00:00:00 2001 From: Benny Siegert Date: Tue, 20 Jul 2021 09:36:08 -0400 Subject: [PATCH 1/6] runtime/debug: skip TestPanicOnFault on netbsd/arm This test has been failing since the builder was updated to NetBSD 9. While the issue is under investigation, skip the test so that we do not miss other breakage. Update issue #45026 Change-Id: Id083901c517f3f88e6b4bc2b51208f65170d47a6 Reviewed-on: https://go-review.googlesource.com/c/go/+/335909 Reviewed-by: Than McIntosh Reviewed-by: Keith Randall Trust: Benny Siegert Run-TryBot: Benny Siegert TryBot-Result: Go Bot --- src/runtime/debug/panic_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/runtime/debug/panic_test.go b/src/runtime/debug/panic_test.go index b93631e1d8..65f9555f37 100644 --- a/src/runtime/debug/panic_test.go +++ b/src/runtime/debug/panic_test.go @@ -24,6 +24,9 @@ func TestPanicOnFault(t *testing.T) { if runtime.GOOS == "ios" { t.Skip("iOS doesn't provide fault addresses") } + if runtime.GOOS == "netbsd" && runtime.GOARCH == "arm" { + t.Skip("netbsd-arm doesn't provide fault address (golang.org/issue/45026)") + } m, err := syscall.Mmap(-1, 0, 0x1000, syscall.PROT_READ /* Note: no PROT_WRITE */, syscall.MAP_SHARED|syscall.MAP_ANON) if err != nil { t.Fatalf("can't map anonymous memory: %s", err) From 9e26569293c13974d210fd588ebfd29b857d8525 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Sun, 18 Jul 2021 13:58:13 -0700 Subject: [PATCH 2/6] cmd/go: don't add C compiler ID to hash for standard library No test because a real test requires installing two different compilers. For #40042 For #47251 Change-Id: Iefddd67830d242a119378b7ce20be481904806e4 Reviewed-on: https://go-review.googlesource.com/c/go/+/335409 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Bryan C. Mills Reviewed-by: Jay Conrod --- src/cmd/go/go_test.go | 32 ++++++++++++++++++++++++++++++++ src/cmd/go/internal/work/exec.go | 11 +++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index c0c86ab9f5..6ce276537b 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -2848,3 +2848,35 @@ func TestExecInDeletedDir(t *testing.T) { // `go version` should not fail tg.run("version") } + +// A missing C compiler should not force the net package to be stale. +// Issue 47215. +func TestMissingCC(t *testing.T) { + if !canCgo { + t.Skip("test is only meaningful on systems with cgo") + } + cc := os.Getenv("CC") + if cc == "" { + cc = "gcc" + } + if filepath.IsAbs(cc) { + t.Skipf(`"CC" (%s) is an absolute path`, cc) + } + _, err := exec.LookPath(cc) + if err != nil { + t.Skipf(`"CC" (%s) not on PATH`, cc) + } + + tg := testgo(t) + defer tg.cleanup() + netStale, _ := tg.isStale("net") + if netStale { + t.Skip(`skipping test because "net" package is currently stale`) + } + + tg.setenv("PATH", "") // No C compiler on PATH. + netStale, _ = tg.isStale("net") + if netStale { + t.Error(`clearing "PATH" causes "net" to be stale`) + } +} diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index b506b83656..5a225fb9f1 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -252,8 +252,15 @@ func (b *Builder) buildActionID(a *Action) cache.ActionID { ccExe := b.ccExe() fmt.Fprintf(h, "CC=%q %q %q %q\n", ccExe, cppflags, cflags, ldflags) - if ccID, err := b.gccToolID(ccExe[0], "c"); err == nil { - fmt.Fprintf(h, "CC ID=%q\n", ccID) + // Include the C compiler tool ID so that if the C + // compiler changes we rebuild the package. + // But don't do that for standard library packages like net, + // so that the prebuilt .a files from a Go binary install + // don't need to be rebuilt with the local compiler. + if !p.Standard { + if ccID, err := b.gccToolID(ccExe[0], "c"); err == nil { + fmt.Fprintf(h, "CC ID=%q\n", ccID) + } } if len(p.CXXFiles)+len(p.SwigCXXFiles) > 0 { cxxExe := b.cxxExe() From 48c88f1b1bac1ef4fc81246a7f31933f8f922706 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 14 Jul 2021 14:46:09 -0700 Subject: [PATCH 3/6] reflect: add Value.CanConvert For #395 For #46746 Change-Id: I4bfc094cf1cecd27ce48e31f92384cf470f371a6 Reviewed-on: https://go-review.googlesource.com/c/go/+/334669 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Keith Randall Reviewed-by: Joe Tsai --- api/go1.17.txt | 1 + doc/go1.17.html | 12 ++++++++++++ src/reflect/all_test.go | 9 +++++++++ src/reflect/value.go | 20 ++++++++++++++++++++ 4 files changed, 42 insertions(+) diff --git a/api/go1.17.txt b/api/go1.17.txt index 3d0a464fec..48505381f1 100644 --- a/api/go1.17.txt +++ b/api/go1.17.txt @@ -80,6 +80,7 @@ pkg net/url, method (Values) Has(string) bool pkg reflect, func VisibleFields(Type) []StructField pkg reflect, method (Method) IsExported() bool pkg reflect, method (StructField) IsExported() bool +pkg reflect, method (Value) CanConvert(Type) bool pkg runtime/cgo (darwin-amd64-cgo), func NewHandle(interface{}) Handle pkg runtime/cgo (darwin-amd64-cgo), method (Handle) Delete() pkg runtime/cgo (darwin-amd64-cgo), method (Handle) Value() interface{} diff --git a/doc/go1.17.html b/doc/go1.17.html index b31006fe65..7739d1c62e 100644 --- a/doc/go1.17.html +++ b/doc/go1.17.html @@ -989,6 +989,18 @@ func Foo() bool {
reflect
+

+ The new + Value.CanConvert + method reports whether a value can be converted to a type. + This may be used to avoid a panic when converting a slice to an + array pointer type if the slice is too short. + Previously it was sufficient to use + Type.ConvertibleTo + for this, but the newly permitted conversion from slice to array + pointer type can panic even if the types are convertible. +

+

The new StructField.IsExported diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go index 0db5e13217..eac27e886f 100644 --- a/src/reflect/all_test.go +++ b/src/reflect/all_test.go @@ -4304,6 +4304,9 @@ func TestConvert(t *testing.T) { // vout1 represents the in value converted to the in type. v1 := tt.in + if !v1.CanConvert(t1) { + t.Errorf("ValueOf(%T(%[1]v)).CanConvert(%s) = false, want true", tt.in.Interface(), t1) + } vout1 := v1.Convert(t1) out1 := vout1.Interface() if vout1.Type() != tt.in.Type() || !DeepEqual(out1, tt.in.Interface()) { @@ -4311,6 +4314,9 @@ func TestConvert(t *testing.T) { } // vout2 represents the in value converted to the out type. + if !v1.CanConvert(t2) { + t.Errorf("ValueOf(%T(%[1]v)).CanConvert(%s) = false, want true", tt.in.Interface(), t2) + } vout2 := v1.Convert(t2) out2 := vout2.Interface() if vout2.Type() != tt.out.Type() || !DeepEqual(out2, tt.out.Interface()) { @@ -4371,6 +4377,9 @@ func TestConvertPanic(t *testing.T) { if !v.Type().ConvertibleTo(pt) { t.Errorf("[]byte should be convertible to *[8]byte") } + if v.CanConvert(pt) { + t.Errorf("slice with length 4 should not be convertible to *[8]byte") + } shouldPanic("reflect: cannot convert slice with length 4 to pointer to array with length 8", func() { _ = v.Convert(pt) }) diff --git a/src/reflect/value.go b/src/reflect/value.go index 9dce251ac5..6f878eba5b 100644 --- a/src/reflect/value.go +++ b/src/reflect/value.go @@ -2811,6 +2811,26 @@ func (v Value) Convert(t Type) Value { return op(v, t) } +// CanConvert reports whether the value v can be converted to type t. +// If v.CanConvert(t) returns true then v.Convert(t) will not panic. +func (v Value) CanConvert(t Type) bool { + vt := v.Type() + if !vt.ConvertibleTo(t) { + return false + } + // Currently the only conversion that is OK in terms of type + // but that can panic depending on the value is converting + // from slice to pointer-to-array. + if vt.Kind() == Slice && t.Kind() == Ptr && t.Elem().Kind() == Array { + n := t.Elem().Len() + h := (*unsafeheader.Slice)(v.ptr) + if n > h.Len { + return false + } + } + return true +} + // convertOp returns the function to convert a value of type src // to a value of type dst. If the conversion is illegal, convertOp returns nil. func convertOp(dst, src *rtype) func(Value, Type) Value { From 3e48c0381fd1beb78e993e940c3b46ca9898ce6d Mon Sep 17 00:00:00 2001 From: wdvxdr Date: Wed, 21 Jul 2021 11:59:45 +0800 Subject: [PATCH 4/6] reflect: add missing copyright header Change-Id: I5a2f7203f83be02b03aa7be5fe386e485bf68ca3 Reviewed-on: https://go-review.googlesource.com/c/go/+/336189 Reviewed-by: Ian Lance Taylor Trust: Robert Findley --- src/reflect/visiblefields.go | 4 ++++ src/reflect/visiblefields_test.go | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/reflect/visiblefields.go b/src/reflect/visiblefields.go index c068979dcc..1a2b53570b 100644 --- a/src/reflect/visiblefields.go +++ b/src/reflect/visiblefields.go @@ -1,3 +1,7 @@ +// Copyright 2021 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 reflect // VisibleFields returns all the visible fields in t, which must be a diff --git a/src/reflect/visiblefields_test.go b/src/reflect/visiblefields_test.go index 2688b63091..915bbee867 100644 --- a/src/reflect/visiblefields_test.go +++ b/src/reflect/visiblefields_test.go @@ -1,3 +1,7 @@ +// Copyright 2021 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 reflect_test import ( From fdb45acd1f062884c77ea6961fb638e004af1b8e Mon Sep 17 00:00:00 2001 From: David Chase Date: Wed, 21 Jul 2021 18:38:05 -0400 Subject: [PATCH 5/6] runtime: move mem profile sampling into m-acquired section It was not safe to do mcache profiling updates outside the critical section, but we got lucky because the runtime was not preemptible. Adding chunked memory clearing (CL 270943) created preemption opportunities, which led to corruption of runtime data structures. Fixes #47304. Fixes #47302. Change-Id: I461615470d62328a83ccbac537fbdc6dcde81c85 Reviewed-on: https://go-review.googlesource.com/c/go/+/336449 Trust: David Chase Run-TryBot: David Chase TryBot-Result: Go Bot Reviewed-by: Austin Clements Reviewed-by: Keith Randall --- src/runtime/malloc.go | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/runtime/malloc.go b/src/runtime/malloc.go index 2759bbdaf9..cc22b0f276 100644 --- a/src/runtime/malloc.go +++ b/src/runtime/malloc.go @@ -1135,13 +1135,21 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { msanmalloc(x, size) } + if rate := MemProfileRate; rate > 0 { + // Note cache c only valid while m acquired; see #47302 + if rate != 1 && size < c.nextSample { + c.nextSample -= size + } else { + profilealloc(mp, x, size) + } + } mp.mallocing = 0 releasem(mp) // Pointerfree data can be zeroed late in a context where preemption can occur. // x will keep the memory alive. if !isZeroed && needzero { - memclrNoHeapPointersChunked(size, x) + memclrNoHeapPointersChunked(size, x) // This is a possible preemption point: see #47302 } if debug.malloc { @@ -1155,16 +1163,6 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { } } - if rate := MemProfileRate; rate > 0 { - if rate != 1 && size < c.nextSample { - c.nextSample -= size - } else { - mp := acquirem() - profilealloc(mp, x, size) - releasem(mp) - } - } - if assistG != nil { // Account for internal fragmentation in the assist // debt now that we know it. From 798ec73519a7226d6d436e42498a54aed23b8468 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 21 Jul 2021 19:57:56 -0700 Subject: [PATCH 6/6] runtime: don't clear timerModifiedEarliest if adjustTimers is 0 This avoids a race when a new timerModifiedEarlier timer is created by a different goroutine. Fixes #47329 Change-Id: I6f6c87b4a9b5491b201c725c10bc98e23e0ed9d1 Reviewed-on: https://go-review.googlesource.com/c/go/+/336432 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Michael Knyszek --- src/runtime/runtime2.go | 2 +- src/runtime/time.go | 5 ----- src/time/sleep_test.go | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index 2a66826f34..8a15787382 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -681,7 +681,7 @@ type p struct { // timerModifiedEarlier status. Because the timer may have been // modified again, there need not be any timer with this value. // This is updated using atomic functions. - // This is 0 if the value is unknown. + // This is 0 if there are no timerModifiedEarlier timers. timerModifiedEarliest uint64 // Per-P GC state diff --git a/src/runtime/time.go b/src/runtime/time.go index dee6a674e4..7b84d2af57 100644 --- a/src/runtime/time.go +++ b/src/runtime/time.go @@ -668,11 +668,6 @@ func adjusttimers(pp *p, now int64) { if verifyTimers { verifyTimerHeap(pp) } - // There are no timers to adjust, so it is safe to clear - // timerModifiedEarliest. Do so in case it is stale. - // Everything will work if we don't do this, - // but clearing here may save future calls to adjusttimers. - atomic.Store64(&pp.timerModifiedEarliest, 0) return } diff --git a/src/time/sleep_test.go b/src/time/sleep_test.go index 6ee0631a85..e0172bf5e0 100644 --- a/src/time/sleep_test.go +++ b/src/time/sleep_test.go @@ -527,6 +527,40 @@ func TestZeroTimer(t *testing.T) { } } +// Test that rapidly moving a timer earlier doesn't cause it to get dropped. +// Issue 47329. +func TestTimerModifiedEarlier(t *testing.T) { + past := Until(Unix(0, 0)) + count := 1000 + fail := 0 + for i := 0; i < count; i++ { + timer := NewTimer(Hour) + for j := 0; j < 10; j++ { + if !timer.Stop() { + <-timer.C + } + timer.Reset(past) + } + + deadline := NewTimer(10 * Second) + defer deadline.Stop() + now := Now() + select { + case <-timer.C: + if since := Since(now); since > 8*Second { + t.Errorf("timer took too long (%v)", since) + fail++ + } + case <-deadline.C: + t.Error("deadline expired") + } + } + + if fail > 0 { + t.Errorf("%d failures", fail) + } +} + // Benchmark timer latency when the thread that creates the timer is busy with // other work and the timers must be serviced by other threads. // https://golang.org/issue/38860