From ea7d9e6a52ca64c200dcc75621e75f209ceceace Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 18 Jan 2017 15:12:18 -0500 Subject: [PATCH 01/19] runtime: check for nil g and m in msanread fixes #18707. Change-Id: Ibc4efef01197799f66d10bfead22faf8ac00473c Reviewed-on: https://go-review.googlesource.com/35452 Run-TryBot: Bryan Mills Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- misc/cgo/testsanitizers/msan_shared.go | 12 ++++++++++++ misc/cgo/testsanitizers/test.bash | 21 +++++++++++++++++++++ src/runtime/msan.go | 4 +++- 3 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 misc/cgo/testsanitizers/msan_shared.go diff --git a/misc/cgo/testsanitizers/msan_shared.go b/misc/cgo/testsanitizers/msan_shared.go new file mode 100644 index 0000000000..966947cac3 --- /dev/null +++ b/misc/cgo/testsanitizers/msan_shared.go @@ -0,0 +1,12 @@ +// Copyright 2017 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. + +// This program segfaulted during libpreinit when built with -msan: +// http://golang.org/issue/18707 + +package main + +import "C" + +func main() {} diff --git a/misc/cgo/testsanitizers/test.bash b/misc/cgo/testsanitizers/test.bash index dfc6d3819a..4da85020d8 100755 --- a/misc/cgo/testsanitizers/test.bash +++ b/misc/cgo/testsanitizers/test.bash @@ -68,6 +68,25 @@ fi status=0 +testmsanshared() { + goos=$(go env GOOS) + suffix="-installsuffix testsanitizers" + libext="so" + if [ "$goos" == "darwin" ]; then + libext="dylib" + fi + go build -msan -buildmode=c-shared $suffix -o ${TMPDIR}/libmsanshared.$libext msan_shared.go + + echo 'int main() { return 0; }' > ${TMPDIR}/testmsanshared.c + $CC $(go env GOGCCFLAGS) -fsanitize=memory -o ${TMPDIR}/testmsanshared ${TMPDIR}/testmsanshared.c ${TMPDIR}/libmsanshared.$libext + + if ! LD_LIBRARY_PATH=. ${TMPDIR}/testmsanshared; then + echo "FAIL: msan_shared" + status=1 + fi + rm -f ${TMPDIR}/{testmsanshared,testmsanshared.c,libmsanshared.$libext} +} + if test "$msan" = "yes"; then if ! go build -msan std; then echo "FAIL: build -msan std" @@ -108,6 +127,8 @@ if test "$msan" = "yes"; then echo "FAIL: msan_fail" status=1 fi + + testmsanshared fi if test "$tsan" = "yes"; then diff --git a/src/runtime/msan.go b/src/runtime/msan.go index 7177c8e611..c0f3957e28 100644 --- a/src/runtime/msan.go +++ b/src/runtime/msan.go @@ -28,9 +28,11 @@ const msanenabled = true // the runtime, but operations like a slice copy can call msanread // anyhow for values on the stack. Just ignore msanread when running // on the system stack. The other msan functions are fine. +// +//go:nosplit func msanread(addr unsafe.Pointer, sz uintptr) { g := getg() - if g == g.m.g0 || g == g.m.gsignal { + if g == nil || g.m == nil || g == g.m.g0 || g == g.m.gsignal { return } domsanread(addr, sz) From e8d5989ed1272bed3600193003ebc9980bcb9275 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Fri, 20 Jan 2017 08:11:34 -0800 Subject: [PATCH 02/19] cmd/compile: fix compilebench -alloc pprof.WriteHeapProfile is shorthand for pprof.Lookup("heap").WriteTo(f, 0). The second parameter is debug. If it is non-zero, pprof writes legacy-format pprof output, which compilebench can parse. Fixes #18641 Change-Id: Ica69adeb9809e9b5933aed943dcf4a07910e43fc Reviewed-on: https://go-review.googlesource.com/35484 Run-TryBot: Josh Bleecher Snyder TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick Reviewed-by: Matthew Dempsky --- src/cmd/compile/internal/gc/util.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/cmd/compile/internal/gc/util.go b/src/cmd/compile/internal/gc/util.go index bb5cede5a6..c62bd00808 100644 --- a/src/cmd/compile/internal/gc/util.go +++ b/src/cmd/compile/internal/gc/util.go @@ -57,8 +57,13 @@ func startProfile() { Fatalf("%v", err) } atExit(func() { - runtime.GC() // profile all outstanding allocations - if err := pprof.WriteHeapProfile(f); err != nil { + // Profile all outstanding allocations. + runtime.GC() + // compilebench parses the memory profile to extract memstats, + // which are only written in the legacy pprof format. + // See golang.org/issue/18641 and runtime/pprof/pprof.go:writeHeap. + const writeLegacyFormat = 1 + if err := pprof.Lookup("heap").WriteTo(f, writeLegacyFormat); err != nil { Fatalf("%v", err) } }) From 256a605faa10bc5507597c4669cc5bc400bf487a Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Fri, 20 Jan 2017 09:54:10 -0800 Subject: [PATCH 03/19] cmd/compile: don't use nilcheck information until the next block When nilcheck runs, the values in a block are not in any particular order. So any facts derived from examining the blocks shouldn't be used until we reach the next block. This is suboptimal as it won't eliminate nil checks within a block. But it's probably a better fix for now as it is a much smaller change than other strategies for fixing this bug. nilptr3.go changes are mostly because for this pattern: _ = *p _ = *p either nil check is fine to keep, and this CL changes which one the compiler tends to keep. There are a few regressions from code like this: _ = *p f() _ = *p For this pattern, after this CL we issue 2 nil checks instead of one. (For the curious, this happens because intra-block nil check elimination now falls to CSE, not nilcheck proper. The former pattern has two nil checks with the same store argument. The latter pattern has two nil checks with different store arguments.) Fixes #18725 Change-Id: I3721b494c8bc9ba1142dc5c4361ea55c66920ac8 Reviewed-on: https://go-review.googlesource.com/35485 Reviewed-by: Cherry Zhang --- src/cmd/compile/internal/ssa/nilcheck.go | 22 +++++++++++---- test/fixedbugs/issue18725.go | 24 ++++++++++++++++ test/nilptr3.go | 36 ++++++++++++------------ 3 files changed, 59 insertions(+), 23 deletions(-) create mode 100644 test/fixedbugs/issue18725.go diff --git a/src/cmd/compile/internal/ssa/nilcheck.go b/src/cmd/compile/internal/ssa/nilcheck.go index 9f58db664b..0a34cd1ae6 100644 --- a/src/cmd/compile/internal/ssa/nilcheck.go +++ b/src/cmd/compile/internal/ssa/nilcheck.go @@ -82,7 +82,7 @@ func nilcheckelim(f *Func) { } } - // Next, process values in the block. + // Next, eliminate any redundant nil checks in this block. i := 0 for _, v := range b.Values { b.Values[i] = v @@ -105,13 +105,10 @@ func nilcheckelim(f *Func) { f.Config.Warnl(v.Line, "removed nil check") } v.reset(OpUnknown) + // TODO: f.freeValue(v) i-- continue } - // Record the fact that we know ptr is non nil, and remember to - // undo that information when this dominator subtree is done. - nonNilValues[ptr.ID] = true - work = append(work, bp{op: ClearPtr, ptr: ptr}) } } for j := i; j < len(b.Values); j++ { @@ -119,6 +116,21 @@ func nilcheckelim(f *Func) { } b.Values = b.Values[:i] + // Finally, find redundant nil checks for subsequent blocks. + // Note that we can't add these until the loop above is done, as the + // values in the block are not ordered in any way when this pass runs. + // This was the cause of issue #18725. + for _, v := range b.Values { + if v.Op != OpNilCheck { + continue + } + ptr := v.Args[0] + // Record the fact that we know ptr is non nil, and remember to + // undo that information when this dominator subtree is done. + nonNilValues[ptr.ID] = true + work = append(work, bp{op: ClearPtr, ptr: ptr}) + } + // Add all dominated blocks to the work list. for w := sdom[node.block.ID].child; w != nil; w = sdom[w.ID].sibling { work = append(work, bp{op: Work, block: w}) diff --git a/test/fixedbugs/issue18725.go b/test/fixedbugs/issue18725.go new file mode 100644 index 0000000000..c632dbad63 --- /dev/null +++ b/test/fixedbugs/issue18725.go @@ -0,0 +1,24 @@ +// run + +// Copyright 2017 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 main + +import "os" + +func panicWhenNot(cond bool) { + if cond { + os.Exit(0) + } else { + panic("nilcheck elim failed") + } +} + +func main() { + e := (*string)(nil) + panicWhenNot(e == e) + // Should never reach this line. + panicWhenNot(*e == *e) +} diff --git a/test/nilptr3.go b/test/nilptr3.go index 8fdae8c075..c681cba50c 100644 --- a/test/nilptr3.go +++ b/test/nilptr3.go @@ -40,23 +40,23 @@ var ( ) func f1() { - _ = *intp // ERROR "generated nil check" + _ = *intp // ERROR "removed nil check" // This one should be removed but the block copy needs // to be turned into its own pseudo-op in order to see // the indirect. - _ = *arrayp // ERROR "generated nil check" + _ = *arrayp // ERROR "removed nil check" // 0-byte indirect doesn't suffice. // we don't registerize globals, so there are no removed.* nil checks. - _ = *array0p // ERROR "generated nil check" _ = *array0p // ERROR "removed nil check" + _ = *array0p // ERROR "generated nil check" - _ = *intp // ERROR "removed nil check" + _ = *intp // ERROR "generated nil check" _ = *arrayp // ERROR "removed nil check" _ = *structp // ERROR "generated nil check" _ = *emptyp // ERROR "generated nil check" - _ = *arrayp // ERROR "removed nil check" + _ = *arrayp // ERROR "generated nil check" } func f2() { @@ -71,15 +71,15 @@ func f2() { empty1p *Empty1 ) - _ = *intp // ERROR "generated nil check" - _ = *arrayp // ERROR "generated nil check" - _ = *array0p // ERROR "generated nil check" - _ = *array0p // ERROR "removed.* nil check" _ = *intp // ERROR "removed.* nil check" _ = *arrayp // ERROR "removed.* nil check" + _ = *array0p // ERROR "removed.* nil check" + _ = *array0p // ERROR "generated nil check" + _ = *intp // ERROR "generated nil check" + _ = *arrayp // ERROR "removed.* nil check" _ = *structp // ERROR "generated nil check" _ = *emptyp // ERROR "generated nil check" - _ = *arrayp // ERROR "removed.* nil check" + _ = *arrayp // ERROR "generated nil check" _ = *bigarrayp // ERROR "generated nil check" ARM removed nil check before indirect!! _ = *bigstructp // ERROR "generated nil check" _ = *empty1p // ERROR "generated nil check" @@ -122,16 +122,16 @@ func f3(x *[10000]int) { // x wasn't going to change across the function call. // But it's a little complex to do and in practice doesn't // matter enough. - _ = x[9999] // ERROR "removed nil check" + _ = x[9999] // ERROR "generated nil check" // TODO: fix } func f3a() { x := fx10k() y := fx10k() z := fx10k() - _ = &x[9] // ERROR "generated nil check" - y = z _ = &x[9] // ERROR "removed.* nil check" + y = z + _ = &x[9] // ERROR "generated nil check" x = y _ = &x[9] // ERROR "generated nil check" } @@ -139,11 +139,11 @@ func f3a() { func f3b() { x := fx10k() y := fx10k() - _ = &x[9] // ERROR "generated nil check" + _ = &x[9] // ERROR "removed.* nil check" y = x _ = &x[9] // ERROR "removed.* nil check" x = y - _ = &x[9] // ERROR "removed.* nil check" + _ = &x[9] // ERROR "generated nil check" } func fx10() *[10]int @@ -179,15 +179,15 @@ func f4(x *[10]int) { _ = x[9] // ERROR "generated nil check" // bug would like to remove before indirect fx10() - _ = x[9] // ERROR "removed nil check" + _ = x[9] // ERROR "generated nil check" // TODO: fix x = fx10() y := fx10() - _ = &x[9] // ERROR "generated nil check" + _ = &x[9] // ERROR "removed[a-z ]* nil check" y = x _ = &x[9] // ERROR "removed[a-z ]* nil check" x = y - _ = &x[9] // ERROR "removed[a-z ]* nil check" + _ = &x[9] // ERROR "generated nil check" } func f5(p *float32, q *float64, r *float32, s *float64) float64 { From ec654e2251f0104ee63eff57fba2749da2f177e5 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Thu, 19 Jan 2017 22:04:45 -0800 Subject: [PATCH 04/19] misc/cgo/test: fix test when using GCC 7 With GCC 7 (not yet released), cgo fails with errors like ./sigaltstack.go:65:8: call of non-function C.restoreSignalStack I do not know precisely why. Explicitly declaring that there are no arguments to the static function is a simple fix for the debug info. Change-Id: Id96e1cb1e55ee37a9f1f5ad243d7ee33e71584ac Reviewed-on: https://go-review.googlesource.com/35480 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- misc/cgo/test/sigaltstack.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/misc/cgo/test/sigaltstack.go b/misc/cgo/test/sigaltstack.go index b16adc7d88..2b7a1ec9ad 100644 --- a/misc/cgo/test/sigaltstack.go +++ b/misc/cgo/test/sigaltstack.go @@ -17,7 +17,7 @@ package cgotest static stack_t oss; static char signalStack[SIGSTKSZ]; -static void changeSignalStack() { +static void changeSignalStack(void) { stack_t ss; memset(&ss, 0, sizeof ss); ss.ss_sp = signalStack; @@ -29,7 +29,7 @@ static void changeSignalStack() { } } -static void restoreSignalStack() { +static void restoreSignalStack(void) { #if (defined(__x86_64__) || defined(__i386__)) && defined(__APPLE__) // The Darwin C library enforces a minimum that the kernel does not. // This is OK since we allocated this much space in mpreinit, @@ -42,7 +42,7 @@ static void restoreSignalStack() { } } -static int zero() { +static int zero(void) { return 0; } */ From 1be957d703832aa10952c4dc799dcc3a39f48aff Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Thu, 19 Jan 2017 22:07:20 -0800 Subject: [PATCH 05/19] misc/cgo/test: pass current environment to syscall.Exec This is needed for typical tests with gccgo, as it passes the LD_LIBRARY_PATH environment variable to the new program. Change-Id: I9bf4b0dbdff63f5449c7fcb8124eaeab10ed7f34 Reviewed-on: https://go-review.googlesource.com/35481 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- misc/cgo/test/issue18146.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misc/cgo/test/issue18146.go b/misc/cgo/test/issue18146.go index ffb04e9037..3c600463f0 100644 --- a/misc/cgo/test/issue18146.go +++ b/misc/cgo/test/issue18146.go @@ -73,7 +73,7 @@ func test18146(t *testing.T) { } runtime.GOMAXPROCS(threads) argv := append(os.Args, "-test.run=NoSuchTestExists") - if err := syscall.Exec(os.Args[0], argv, nil); err != nil { + if err := syscall.Exec(os.Args[0], argv, os.Environ()); err != nil { t.Fatal(err) } } From 4cce27a3fa0cc1f13afa6ffa358efa07144e00ec Mon Sep 17 00:00:00 2001 From: Michael Munday Date: Fri, 20 Jan 2017 22:03:13 -0500 Subject: [PATCH 06/19] cmd/compile: fix constant propagation through s390x MOVDNE instructions The constant propagation rules selected the wrong operand to propagate. So MOVDNE (move if not equal) propagated operands as if it were a MOVDEQ (move if equal). Fixes #18735. Change-Id: I87ac469172f9df7d5aabaf7106e2936ce54ae202 Reviewed-on: https://go-review.googlesource.com/35498 Run-TryBot: Michael Munday Reviewed-by: Cherry Zhang TryBot-Result: Gobot Gobot --- src/cmd/compile/internal/ssa/gen/S390X.rules | 6 +++--- src/cmd/compile/internal/ssa/rewriteS390X.go | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cmd/compile/internal/ssa/gen/S390X.rules b/src/cmd/compile/internal/ssa/gen/S390X.rules index 3e0533a951..be0d581fe0 100644 --- a/src/cmd/compile/internal/ssa/gen/S390X.rules +++ b/src/cmd/compile/internal/ssa/gen/S390X.rules @@ -885,9 +885,9 @@ (MOVDEQ y _ (FlagLT)) -> y (MOVDEQ y _ (FlagGT)) -> y -(MOVDNE _ y (FlagEQ)) -> y -(MOVDNE x _ (FlagLT)) -> x -(MOVDNE x _ (FlagGT)) -> x +(MOVDNE y _ (FlagEQ)) -> y +(MOVDNE _ x (FlagLT)) -> x +(MOVDNE _ x (FlagGT)) -> x (MOVDLT y _ (FlagEQ)) -> y (MOVDLT _ x (FlagLT)) -> x diff --git a/src/cmd/compile/internal/ssa/rewriteS390X.go b/src/cmd/compile/internal/ssa/rewriteS390X.go index 7d023bcf8b..5acaf2dbdc 100644 --- a/src/cmd/compile/internal/ssa/rewriteS390X.go +++ b/src/cmd/compile/internal/ssa/rewriteS390X.go @@ -9847,11 +9847,11 @@ func rewriteValueS390X_OpS390XMOVDNE(v *Value, config *Config) bool { v.AddArg(cmp) return true } - // match: (MOVDNE _ y (FlagEQ)) + // match: (MOVDNE y _ (FlagEQ)) // cond: // result: y for { - y := v.Args[1] + y := v.Args[0] v_2 := v.Args[2] if v_2.Op != OpS390XFlagEQ { break @@ -9861,11 +9861,11 @@ func rewriteValueS390X_OpS390XMOVDNE(v *Value, config *Config) bool { v.AddArg(y) return true } - // match: (MOVDNE x _ (FlagLT)) + // match: (MOVDNE _ x (FlagLT)) // cond: // result: x for { - x := v.Args[0] + x := v.Args[1] v_2 := v.Args[2] if v_2.Op != OpS390XFlagLT { break @@ -9875,11 +9875,11 @@ func rewriteValueS390X_OpS390XMOVDNE(v *Value, config *Config) bool { v.AddArg(x) return true } - // match: (MOVDNE x _ (FlagGT)) + // match: (MOVDNE _ x (FlagGT)) // cond: // result: x for { - x := v.Args[0] + x := v.Args[1] v_2 := v.Args[2] if v_2.Op != OpS390XFlagGT { break From a96e117a58dce1d55fd83a7b3391fa667dd66652 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Mon, 23 Jan 2017 08:22:10 -0800 Subject: [PATCH 07/19] runtime: amd64, use 4-byte ops for memmove of 4 bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit memmove used to use 2 2-byte load/store pairs to move 4 bytes. When the result is loaded with a single 4-byte load, it caused a store to load fowarding stall. To avoid the stall, special case memmove to use 4 byte ops for the 4 byte copy case. We already have a special case for 8-byte copies. 386 already specializes 4-byte copies. I'll do 2-byte copies also, but not for 1.8. benchmark old ns/op new ns/op delta BenchmarkIssue18740-8 7567 4799 -36.58% 3-byte copies get a bit slower. Other copies are unchanged. name old time/op new time/op delta Memmove/3-8 4.76ns ± 5% 5.26ns ± 3% +10.50% (p=0.000 n=10+10) Fixes #18740 Change-Id: Iec82cbac0ecfee80fa3c8fc83828f9a1819c3c74 Reviewed-on: https://go-review.googlesource.com/35567 Run-TryBot: Keith Randall TryBot-Result: Gobot Gobot Reviewed-by: David Chase --- src/runtime/memmove_amd64.s | 10 ++++++++-- src/runtime/memmove_test.go | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/runtime/memmove_amd64.s b/src/runtime/memmove_amd64.s index 464f5fdc1b..c2286d3edd 100644 --- a/src/runtime/memmove_amd64.s +++ b/src/runtime/memmove_amd64.s @@ -146,10 +146,16 @@ move_1or2: move_0: RET move_3or4: + CMPQ BX, $4 + JB move_3 + MOVL (SI), AX + MOVL AX, (DI) + RET +move_3: MOVW (SI), AX - MOVW -2(SI)(BX*1), CX + MOVB 2(SI), CX MOVW AX, (DI) - MOVW CX, -2(DI)(BX*1) + MOVB CX, 2(DI) RET move_5through7: MOVL (SI), AX diff --git a/src/runtime/memmove_test.go b/src/runtime/memmove_test.go index dbfa284c28..74b8753b5f 100644 --- a/src/runtime/memmove_test.go +++ b/src/runtime/memmove_test.go @@ -6,6 +6,7 @@ package runtime_test import ( "crypto/rand" + "encoding/binary" "fmt" "internal/race" . "runtime" @@ -447,3 +448,22 @@ func BenchmarkCopyFat1024(b *testing.B) { _ = y } } + +func BenchmarkIssue18740(b *testing.B) { + // This tests that memmove uses one 4-byte load/store to move 4 bytes. + // It used to do 2 2-byte load/stores, which leads to a pipeline stall + // when we try to read the result with one 4-byte load. + var buf [4]byte + for j := 0; j < b.N; j++ { + s := uint32(0) + for i := 0; i < 4096; i += 4 { + copy(buf[:], g[i:]) + s += binary.LittleEndian.Uint32(buf[:]) + } + sink = uint64(s) + } +} + +// TODO: 2 byte and 8 byte benchmarks also. + +var g [4096]byte From be9dcfec293854bfb4a13737ba09801769daccbf Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 23 Jan 2017 22:26:27 +0000 Subject: [PATCH 08/19] doc: mention testing.MainStart signature change Fixes #18766 Change-Id: Ic0f72f3b7bbccd0546692993c4ed414f8c88c1c6 Reviewed-on: https://go-review.googlesource.com/35573 Reviewed-by: Russ Cox --- doc/go1.8.html | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/go1.8.html b/doc/go1.8.html index 337f13d630..147221a44e 100644 --- a/doc/go1.8.html +++ b/doc/go1.8.html @@ -1645,6 +1645,17 @@ crypto/x509: return error for missing SerialNumber (CL 27238) and only the overall execution of the test binary would fail.

+

+ The signature of the + MainStart + function has changed, as allowed by the documentation. It is an + internal detail and not part of the Go 1 compatibility promise. + If you're not calling MainStart directly but see + errors, that likely means you set the + normally-empty GOROOT environment variable and it + doesn't match the version of your go command's binary. +

+ From aad06da2b9b293fd245626fc8e116e3b56654dae Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Mon, 23 Jan 2017 17:30:41 -0800 Subject: [PATCH 09/19] cmd/link: mark DWARF function symbols as reachable Otherwise we don't emit any required ELF relocations when doing an external link, because elfrelocsect skips unreachable symbols. Fixes #18745. Change-Id: Ia3583c41bb6c5ebb7579abd26ed8689370311cd6 Reviewed-on: https://go-review.googlesource.com/35590 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: David Crawshaw --- src/cmd/link/internal/ld/dwarf.go | 2 +- src/runtime/runtime-gdb_test.go | 22 ++++++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/cmd/link/internal/ld/dwarf.go b/src/cmd/link/internal/ld/dwarf.go index 61d3e4fb72..22d2c548c3 100644 --- a/src/cmd/link/internal/ld/dwarf.go +++ b/src/cmd/link/internal/ld/dwarf.go @@ -1080,7 +1080,7 @@ func writelines(ctxt *Link, syms []*Symbol) ([]*Symbol, []*Symbol) { epcs = s dsym := ctxt.Syms.Lookup(dwarf.InfoPrefix+s.Name, int(s.Version)) - dsym.Attr |= AttrHidden + dsym.Attr |= AttrHidden | AttrReachable dsym.Type = obj.SDWARFINFO for _, r := range dsym.R { if r.Type == obj.R_DWARFREF && r.Sym.Size == 0 { diff --git a/src/runtime/runtime-gdb_test.go b/src/runtime/runtime-gdb_test.go index c2844375f7..f886961d6a 100644 --- a/src/runtime/runtime-gdb_test.go +++ b/src/runtime/runtime-gdb_test.go @@ -7,6 +7,7 @@ package runtime_test import ( "bytes" "fmt" + "go/build" "internal/testenv" "io/ioutil" "os" @@ -67,7 +68,6 @@ func checkGdbPython(t *testing.T) { } const helloSource = ` -package main import "fmt" var gslice []string func main() { @@ -85,9 +85,20 @@ func main() { ` func TestGdbPython(t *testing.T) { + testGdbPython(t, false) +} + +func TestGdbPythonCgo(t *testing.T) { + testGdbPython(t, true) +} + +func testGdbPython(t *testing.T, cgo bool) { if runtime.GOARCH == "mips64" { testenv.SkipFlaky(t, 18173) } + if cgo && !build.Default.CgoEnabled { + t.Skip("skipping because cgo is not enabled") + } t.Parallel() checkGdbEnvironment(t) @@ -100,8 +111,15 @@ func TestGdbPython(t *testing.T) { } defer os.RemoveAll(dir) + var buf bytes.Buffer + buf.WriteString("package main\n") + if cgo { + buf.WriteString(`import "C"` + "\n") + } + buf.WriteString(helloSource) + src := filepath.Join(dir, "main.go") - err = ioutil.WriteFile(src, []byte(helloSource), 0644) + err = ioutil.WriteFile(src, buf.Bytes(), 0644) if err != nil { t.Fatalf("failed to create file: %v", err) } From 314180e7f66b6768b0db026138a6fedc52b0c08b Mon Sep 17 00:00:00 2001 From: Mikio Hara Date: Tue, 24 Jan 2017 19:46:27 +0900 Subject: [PATCH 10/19] net/http: fix a nit Change-Id: I31fa5f906ad2e8dc475dbbeb91f568f91e16861b Reviewed-on: https://go-review.googlesource.com/35514 Run-TryBot: Mikio Hara TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- src/net/http/serve_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 681dff193a..73dd56e8c4 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -5277,7 +5277,7 @@ func TestServerHijackGetsBackgroundByte_big(t *testing.T) { defer conn.Close() slurp, err := ioutil.ReadAll(buf.Reader) if err != nil { - t.Error("Copy: %v", err) + t.Errorf("Copy: %v", err) } allX := true for _, v := range slurp { From 98842cabb6133ab7b8f2b323754a48085eed82f3 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 24 Jan 2017 17:52:54 +0000 Subject: [PATCH 11/19] net/http: don't send body on redirects for 301, 302, 303 when GetBody is set The presence of Request.GetBody being set on a request was causing all redirected requests to have a body, even if the redirect status didn't warrant one. This bug came from 307/308 support (https://golang.org/cl/29852) which removed the line that set req.Body to nil after POST/PUT redirects. Change-Id: I2a4dd5320f810ae25cfd8ea8ca7c9700e5dbd369 Reviewed-on: https://go-review.googlesource.com/35633 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Joe Tsai --- src/net/http/client.go | 21 ++++++++----- src/net/http/client_test.go | 61 +++++++++++++++++++++++++------------ 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/src/net/http/client.go b/src/net/http/client.go index d368bae861..0005538e70 100644 --- a/src/net/http/client.go +++ b/src/net/http/client.go @@ -413,11 +413,12 @@ func (c *Client) checkRedirect(req *Request, via []*Request) error { // redirectBehavior describes what should happen when the // client encounters a 3xx status code from the server -func redirectBehavior(reqMethod string, resp *Response, ireq *Request) (redirectMethod string, shouldRedirect bool) { +func redirectBehavior(reqMethod string, resp *Response, ireq *Request) (redirectMethod string, shouldRedirect, includeBody bool) { switch resp.StatusCode { case 301, 302, 303: redirectMethod = reqMethod shouldRedirect = true + includeBody = false // RFC 2616 allowed automatic redirection only with GET and // HEAD requests. RFC 7231 lifts this restriction, but we still @@ -429,6 +430,7 @@ func redirectBehavior(reqMethod string, resp *Response, ireq *Request) (redirect case 307, 308: redirectMethod = reqMethod shouldRedirect = true + includeBody = true // Treat 307 and 308 specially, since they're new in // Go 1.8, and they also require re-sending the request body. @@ -449,7 +451,7 @@ func redirectBehavior(reqMethod string, resp *Response, ireq *Request) (redirect shouldRedirect = false } } - return redirectMethod, shouldRedirect + return redirectMethod, shouldRedirect, includeBody } // Do sends an HTTP request and returns an HTTP response, following @@ -492,11 +494,14 @@ func (c *Client) Do(req *Request) (*Response, error) { } var ( - deadline = c.deadline() - reqs []*Request - resp *Response - copyHeaders = c.makeHeadersCopier(req) + deadline = c.deadline() + reqs []*Request + resp *Response + copyHeaders = c.makeHeadersCopier(req) + + // Redirect behavior: redirectMethod string + includeBody bool ) uerr := func(err error) error { req.closeBody() @@ -534,7 +539,7 @@ func (c *Client) Do(req *Request) (*Response, error) { Cancel: ireq.Cancel, ctx: ireq.ctx, } - if ireq.GetBody != nil { + if includeBody && ireq.GetBody != nil { req.Body, err = ireq.GetBody() if err != nil { return nil, uerr(err) @@ -598,7 +603,7 @@ func (c *Client) Do(req *Request) (*Response, error) { } var shouldRedirect bool - redirectMethod, shouldRedirect = redirectBehavior(req.Method, resp, reqs[0]) + redirectMethod, shouldRedirect, includeBody = redirectBehavior(req.Method, resp, reqs[0]) if !shouldRedirect { return resp, nil } diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go index eaf2cdca8e..4f674dd8d6 100644 --- a/src/net/http/client_test.go +++ b/src/net/http/client_test.go @@ -360,25 +360,25 @@ func TestPostRedirects(t *testing.T) { wantSegments := []string{ `POST / "first"`, `POST /?code=301&next=302 "c301"`, - `GET /?code=302 "c301"`, - `GET / "c301"`, + `GET /?code=302 ""`, + `GET / ""`, `POST /?code=302&next=302 "c302"`, - `GET /?code=302 "c302"`, - `GET / "c302"`, + `GET /?code=302 ""`, + `GET / ""`, `POST /?code=303&next=301 "c303wc301"`, - `GET /?code=301 "c303wc301"`, - `GET / "c303wc301"`, + `GET /?code=301 ""`, + `GET / ""`, `POST /?code=304 "c304"`, `POST /?code=305 "c305"`, `POST /?code=307&next=303,308,302 "c307"`, `POST /?code=303&next=308,302 "c307"`, - `GET /?code=308&next=302 "c307"`, + `GET /?code=308&next=302 ""`, `GET /?code=302 "c307"`, - `GET / "c307"`, + `GET / ""`, `POST /?code=308&next=302,301 "c308"`, `POST /?code=302&next=301 "c308"`, - `GET /?code=301 "c308"`, - `GET / "c308"`, + `GET /?code=301 ""`, + `GET / ""`, `POST /?code=404 "c404"`, } want := strings.Join(wantSegments, "\n") @@ -399,20 +399,20 @@ func TestDeleteRedirects(t *testing.T) { wantSegments := []string{ `DELETE / "first"`, `DELETE /?code=301&next=302,308 "c301"`, - `GET /?code=302&next=308 "c301"`, - `GET /?code=308 "c301"`, + `GET /?code=302&next=308 ""`, + `GET /?code=308 ""`, `GET / "c301"`, `DELETE /?code=302&next=302 "c302"`, - `GET /?code=302 "c302"`, - `GET / "c302"`, + `GET /?code=302 ""`, + `GET / ""`, `DELETE /?code=303 "c303"`, - `GET / "c303"`, + `GET / ""`, `DELETE /?code=307&next=301,308,303,302,304 "c307"`, `DELETE /?code=301&next=308,303,302,304 "c307"`, - `GET /?code=308&next=303,302,304 "c307"`, + `GET /?code=308&next=303,302,304 ""`, `GET /?code=303&next=302,304 "c307"`, - `GET /?code=302&next=304 "c307"`, - `GET /?code=304 "c307"`, + `GET /?code=302&next=304 ""`, + `GET /?code=304 ""`, `DELETE /?code=308&next=307 "c308"`, `DELETE /?code=307 "c308"`, `DELETE / "c308"`, @@ -432,7 +432,11 @@ func testRedirectsByMethod(t *testing.T, method string, table []redirectTest, wa ts = httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { log.Lock() slurp, _ := ioutil.ReadAll(r.Body) - fmt.Fprintf(&log.Buffer, "%s %s %q\n", r.Method, r.RequestURI, slurp) + fmt.Fprintf(&log.Buffer, "%s %s %q", r.Method, r.RequestURI, slurp) + if cl := r.Header.Get("Content-Length"); r.Method == "GET" && len(slurp) == 0 && (r.ContentLength != 0 || cl != "") { + fmt.Fprintf(&log.Buffer, " (but with body=%T, content-length = %v, %q)", r.Body, r.ContentLength, cl) + } + log.WriteByte('\n') log.Unlock() urlQuery := r.URL.Query() if v := urlQuery.Get("code"); v != "" { @@ -475,7 +479,24 @@ func testRedirectsByMethod(t *testing.T, method string, table []redirectTest, wa want = strings.TrimSpace(want) if got != want { - t.Errorf("Log differs.\n Got:\n%s\nWant:\n%s\n", got, want) + got, want, lines := removeCommonLines(got, want) + t.Errorf("Log differs after %d common lines.\n\nGot:\n%s\n\nWant:\n%s\n", lines, got, want) + } +} + +func removeCommonLines(a, b string) (asuffix, bsuffix string, commonLines int) { + for { + nl := strings.IndexByte(a, '\n') + if nl < 0 { + return a, b, commonLines + } + line := a[:nl+1] + if !strings.HasPrefix(b, line) { + return a, b, commonLines + } + commonLines++ + a = a[len(line):] + b = b[len(line):] } } From 3717b429f25b042b98fbdf2c0d4e3dc5307e91ed Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 23 Jan 2017 20:55:17 +0000 Subject: [PATCH 12/19] doc: note that plugins are not fully baked Change-Id: I6341b8cce0b4a9922928f73f8b459cbb9ec25e79 Reviewed-on: https://go-review.googlesource.com/35571 Reviewed-by: David Crawshaw Reviewed-by: Joe Tsai --- doc/go1.8.html | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/go1.8.html b/doc/go1.8.html index 147221a44e..bc40378a6a 100644 --- a/doc/go1.8.html +++ b/doc/go1.8.html @@ -435,11 +435,11 @@ version of gccgo.

Plugins

- Go now supports a “plugin” build mode for generating - plugins written in Go, and a + Go now provides early support for plugins with a “plugin” + build mode for generating plugins written in Go, and a new plugin package for - loading such plugins at run time. Plugin support is only currently - available on Linux. + loading such plugins at run time. Plugin support is currently only + available on Linux. Please report any issues.

Runtime

From 1db16711f595d291bdd22f7ca70f4e0df50ac0e7 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Tue, 24 Jan 2017 09:56:57 -0800 Subject: [PATCH 13/19] doc: clarify what to do with Go 1.4 when installing from source You have to actually run make.bash (or make.bat). Update #18771. Change-Id: Ie6672a4e4abde0150c1ae57cabb1222de2c78716 Reviewed-on: https://go-review.googlesource.com/35632 Reviewed-by: Brad Fitzpatrick --- doc/install-source.html | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/install-source.html b/doc/install-source.html index 4bf0ba35fb..efe864cb1a 100644 --- a/doc/install-source.html +++ b/doc/install-source.html @@ -147,6 +147,9 @@ either the git branch release-branch.go1.4 or which contains the Go 1.4 source code plus accumulated fixes to keep the tools running on newer operating systems. (Go 1.4 was the last distribution in which the tool chain was written in C.) +After unpacking the Go 1.4 source, cd to +the src subdirectory and run make.bash (or, +on Windows, make.bat).

From ea73649343b5d199d7f3d8525399e7a07a608543 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Tue, 24 Jan 2017 09:32:29 -0800 Subject: [PATCH 14/19] doc: update gccgo docs Update docs on correspondence between Go releases and GCC releases. Update C type that corresponds to Go type `int`. Drop out of date comments about Ubuntu and RTEMS. Change-Id: Ic1b5ce9f242789af23ec3b7e7a64c9d257d6913e Reviewed-on: https://go-review.googlesource.com/35631 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- doc/gccgo_install.html | 63 +++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 38 deletions(-) diff --git a/doc/gccgo_install.html b/doc/gccgo_install.html index ef27fd1818..4f6a911541 100644 --- a/doc/gccgo_install.html +++ b/doc/gccgo_install.html @@ -52,6 +52,19 @@ user libraries. The Go 1.4 runtime is not fully merged, but that should not be visible to Go programs.

+

+The GCC 6 releases include a complete implementation of the Go 1.6.1 +user libraries. The Go 1.6 runtime is not fully merged, but that +should not be visible to Go programs. +

+ +

+The GCC 7 releases are expected to include a complete implementation +of the Go 1.8 user libraries. As with earlier releases, the Go 1.8 +runtime is not fully merged, but that should not be visible to Go +programs. +

+

Source code

@@ -160,23 +173,6 @@ make make install -

A note on Ubuntu

- -

-Current versions of Ubuntu and versions of GCC before 4.8 disagree on -where system libraries and header files are found. This is not a -gccgo issue. When building older versions of GCC, setting these -environment variables while configuring and building gccgo may fix the -problem. -

- -
-LIBRARY_PATH=/usr/lib/x86_64-linux-gnu
-C_INCLUDE_PATH=/usr/include/x86_64-linux-gnu
-CPLUS_INCLUDE_PATH=/usr/include/x86_64-linux-gnu
-export LIBRARY_PATH C_INCLUDE_PATH CPLUS_INCLUDE_PATH
-
-

Using gccgo

@@ -364,12 +360,15 @@ or with C++ code compiled using extern "C".

Types

-Basic types map directly: an int in Go is an int -in C, an int32 is an int32_t, -etc. Go byte is equivalent to C unsigned -char. -Pointers in Go are pointers in C. A Go struct is the same as C -struct with the same fields and types. +Basic types map directly: an int32 in Go is +an int32_t in C, an int64 is +an int64_t, etc. +The Go type int is an integer that is the same size as a +pointer, and as such corresponds to the C type intptr_t. +Go byte is equivalent to C unsigned char. +Pointers in Go are pointers in C. +A Go struct is the same as C struct with the +same fields and types.

@@ -380,7 +379,7 @@ structure (this is subject to change): 

 struct __go_string {
   const unsigned char *__data;
-  int __length;
+  intptr_t __length;
 };
@@ -400,8 +399,8 @@ A slice in Go is a structure. The current definition is 
 struct __go_slice {
   void *__values;
-  int __count;
-  int __capacity;
+  intptr_t __count;
+  intptr_t __capacity;
 };
@@ -526,15 +525,3 @@ This procedure is full of unstated caveats and restrictions and we make no guarantee that it will not change in the future. It is more useful as a starting point for real Go code than as a regular procedure.

- -

RTEMS Port

-

-The gccgo compiler has been ported to -RTEMS. RTEMS is a real-time executive -that provides a high performance environment for embedded applications -on a range of processors and embedded hardware. The current gccgo -port is for x86. The goal is to extend the port to most of the - -architectures supported by RTEMS. For more information on the port, -as well as instructions on how to install it, please see this -RTEMS Wiki page. From 165cfbc409d54154263c26fb0cc2b2acd75d8b53 Mon Sep 17 00:00:00 2001 From: Daniel Theophanes Date: Wed, 25 Jan 2017 08:27:45 -0800 Subject: [PATCH 15/19] database/sql: let tests wait for db pool to come to expected state Slower builders were failing TestQueryContext because the cancel and return to conn pool happens async. TestQueryContext already uses a wait method for this reason. Use the same method for other context tests. Fixes #18759 Change-Id: I84cce697392b867e4ebdfadd38027a06ca14655f Reviewed-on: https://go-review.googlesource.com/35750 Reviewed-by: Brad Fitzpatrick Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- src/database/sql/sql_test.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 63e1292cb1..3f8e03ce13 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -326,9 +326,7 @@ func TestQueryContext(t *testing.T) { // And verify that the final rows.Next() call, which hit EOF, // also closed the rows connection. - if n := db.numFreeConns(); n != 1 { - t.Fatalf("free conns after query hitting EOF = %d; want 1", n) - } + waitForFree(t, db, 5*time.Second, 1) if prepares := numPrepares(t, db) - prepares0; prepares != 1 { t.Errorf("executed %d Prepare statements; want 1", prepares) } @@ -345,6 +343,18 @@ func waitCondition(waitFor, checkEvery time.Duration, fn func() bool) bool { return false } +// waitForFree checks db.numFreeConns until either it equals want or +// the maxWait time elapses. +func waitForFree(t *testing.T, db *DB, maxWait time.Duration, want int) { + var numFree int + if !waitCondition(maxWait, 5*time.Millisecond, func() bool { + numFree = db.numFreeConns() + return numFree == want + }) { + t.Fatalf("free conns after hitting EOF = %d; want %d", numFree, want) + } +} + func TestQueryContextWait(t *testing.T) { db := newTestDB(t, "people") defer closeDB(t, db) @@ -361,9 +371,7 @@ func TestQueryContextWait(t *testing.T) { } // Verify closed rows connection after error condition. - if n := db.numFreeConns(); n != 1 { - t.Fatalf("free conns after query hitting EOF = %d; want 1", n) - } + waitForFree(t, db, 5*time.Second, 1) if prepares := numPrepares(t, db) - prepares0; prepares != 1 { t.Errorf("executed %d Prepare statements; want 1", prepares) } @@ -388,13 +396,7 @@ func TestTxContextWait(t *testing.T) { t.Fatalf("expected QueryContext to error with context deadline exceeded but returned %v", err) } - var numFree int - if !waitCondition(5*time.Second, 5*time.Millisecond, func() bool { - numFree = db.numFreeConns() - return numFree == 0 - }) { - t.Fatalf("free conns after hitting EOF = %d; want 0", numFree) - } + waitForFree(t, db, 5*time.Second, 0) // Ensure the dropped connection allows more connections to be made. // Checked on DB Close. @@ -471,9 +473,7 @@ func TestMultiResultSetQuery(t *testing.T) { // And verify that the final rows.Next() call, which hit EOF, // also closed the rows connection. - if n := db.numFreeConns(); n != 1 { - t.Fatalf("free conns after query hitting EOF = %d; want 1", n) - } + waitForFree(t, db, 5*time.Second, 1) if prepares := numPrepares(t, db) - prepares0; prepares != 1 { t.Errorf("executed %d Prepare statements; want 1", prepares) } From b531eb30625a28eb99f9b0137ea5a409a733a1bb Mon Sep 17 00:00:00 2001 From: David Crawshaw Date: Tue, 24 Jan 2017 20:19:36 -0800 Subject: [PATCH 16/19] runtime: reorder modules so main.main comes first Modules appear in the moduledata linked list in the order they are loaded by the dynamic loader, with one exception: the firstmoduledata itself the module that contains the runtime. This is not always the first module (when using -buildmode=shared, it is typically libstd.so, the second module). The order matters for typelinksinit, so we swap the first module with whatever module contains the main function. Updates #18729 This fixes the test case extracted with -linkshared, and now go test -linkshared encoding/... passes. However the original issue about a plugin failure is not yet fixed. Change-Id: I9f399ecc3518e22e6b0a350358e90b0baa44ac96 Reviewed-on: https://go-review.googlesource.com/35644 Run-TryBot: David Crawshaw TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick Reviewed-by: Michael Hudson-Doyle Reviewed-by: Ian Lance Taylor --- misc/cgo/testshared/src/depBase/dep.go | 2 ++ misc/cgo/testshared/src/exe/exe.go | 9 +++++++++ src/runtime/symtab.go | 19 +++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/misc/cgo/testshared/src/depBase/dep.go b/misc/cgo/testshared/src/depBase/dep.go index a518b4efe2..9f86710db0 100644 --- a/misc/cgo/testshared/src/depBase/dep.go +++ b/misc/cgo/testshared/src/depBase/dep.go @@ -5,6 +5,8 @@ import ( "reflect" ) +var SlicePtr interface{} = &[]int{} + var V int = 1 var HasMask []string = []string{"hi"} diff --git a/misc/cgo/testshared/src/exe/exe.go b/misc/cgo/testshared/src/exe/exe.go index 433727112b..84302a811f 100644 --- a/misc/cgo/testshared/src/exe/exe.go +++ b/misc/cgo/testshared/src/exe/exe.go @@ -19,6 +19,8 @@ func F() *C { return nil } +var slicePtr interface{} = &[]int{} + func main() { defer depBase.ImplementedInAsm() // This code below causes various go.itab.* symbols to be generated in @@ -32,4 +34,11 @@ func main() { if reflect.TypeOf(F).Out(0) != reflect.TypeOf(c) { panic("bad reflection results, see golang.org/issue/18252") } + + sp := reflect.New(reflect.TypeOf(slicePtr).Elem()) + s := sp.Interface() + + if reflect.TypeOf(s) != reflect.TypeOf(slicePtr) { + panic("bad reflection results, see golang.org/issue/18729") + } } diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index f52190661c..ed82783ca9 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -285,6 +285,25 @@ func modulesinit() { md.gcbssmask = progToPointerMask((*byte)(unsafe.Pointer(md.gcbss)), md.ebss-md.bss) } } + + // Modules appear in the moduledata linked list in the order they are + // loaded by the dynamic loader, with one exception: the + // firstmoduledata itself the module that contains the runtime. This + // is not always the first module (when using -buildmode=shared, it + // is typically libstd.so, the second module). The order matters for + // typelinksinit, so we swap the first module with whatever module + // contains the main function. + // + // See Issue #18729. + mainText := funcPC(main_main) + for i, md := range *modules { + if md.text <= mainText && mainText <= md.etext { + (*modules)[0] = md + (*modules)[i] = &firstmoduledata + break + } + } + atomicstorep(unsafe.Pointer(&modulesSlice), unsafe.Pointer(modules)) } From 1cf08182f92698aeba8be040dafb5296707cdbf2 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 25 Jan 2017 15:05:39 -0800 Subject: [PATCH 17/19] go/printer: fix format with leading comments in composite literal This fix is less pervasive than it seems. The only change affecting formatting is on printer.go:760. The remaining changes have no effect on formatting since the value of p.level is ignored except on this specific line. The remaining changes are: - renamed adjBlock to funcBody since that's how it is used - introduced new printer field 'level' tracking the composite literal nesting level - update/restore the composite literal nesting level as needed Fixes #18782. Change-Id: Ie833a9b5a559c4ec0f2eef2c5dc97aa263dca53a Reviewed-on: https://go-review.googlesource.com/35811 Reviewed-by: Brad Fitzpatrick --- src/go/printer/nodes.go | 17 +++++-- src/go/printer/printer.go | 11 +++-- src/go/printer/testdata/comments2.golden | 59 ++++++++++++++++++++++ src/go/printer/testdata/comments2.input | 63 ++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 8 deletions(-) diff --git a/src/go/printer/nodes.go b/src/go/printer/nodes.go index 11f26d45ea..ea432860a8 100644 --- a/src/go/printer/nodes.go +++ b/src/go/printer/nodes.go @@ -733,7 +733,7 @@ func (p *printer) expr1(expr ast.Expr, prec1, depth int) { case *ast.FuncLit: p.expr(x.Type) - p.adjBlock(p.distanceFrom(x.Type.Pos()), blank, x.Body) + p.funcBody(p.distanceFrom(x.Type.Pos()), blank, x.Body) case *ast.ParenExpr: if _, hasParens := x.X.(*ast.ParenExpr); hasParens { @@ -825,6 +825,7 @@ func (p *printer) expr1(expr ast.Expr, prec1, depth int) { if x.Type != nil { p.expr1(x.Type, token.HighestPrec, depth) } + p.level++ p.print(x.Lbrace, token.LBRACE) p.exprList(x.Lbrace, x.Elts, 1, commaTerm, x.Rbrace) // do not insert extra line break following a /*-style comment @@ -837,6 +838,7 @@ func (p *printer) expr1(expr ast.Expr, prec1, depth int) { mode |= noExtraBlank } p.print(mode, x.Rbrace, token.RBRACE, mode) + p.level-- case *ast.Ellipsis: p.print(token.ELLIPSIS) @@ -1557,18 +1559,23 @@ func (p *printer) bodySize(b *ast.BlockStmt, maxSize int) int { return bodySize } -// adjBlock prints an "adjacent" block (e.g., a for-loop or function body) following -// a header (e.g., a for-loop control clause or function signature) of given headerSize. +// funcBody prints a function body following a function header of given headerSize. // If the header's and block's size are "small enough" and the block is "simple enough", // the block is printed on the current line, without line breaks, spaced from the header // by sep. Otherwise the block's opening "{" is printed on the current line, followed by // lines for the block's statements and its closing "}". // -func (p *printer) adjBlock(headerSize int, sep whiteSpace, b *ast.BlockStmt) { +func (p *printer) funcBody(headerSize int, sep whiteSpace, b *ast.BlockStmt) { if b == nil { return } + // save/restore composite literal nesting level + defer func(level int) { + p.level = level + }(p.level) + p.level = 0 + const maxSize = 100 if headerSize+p.bodySize(b, maxSize) <= maxSize { p.print(sep, b.Lbrace, token.LBRACE) @@ -1613,7 +1620,7 @@ func (p *printer) funcDecl(d *ast.FuncDecl) { } p.expr(d.Name) p.signature(d.Type.Params, d.Type.Results) - p.adjBlock(p.distanceFrom(d.Pos()), vtab, d.Body) + p.funcBody(p.distanceFrom(d.Pos()), vtab, d.Body) } func (p *printer) decl(decl ast.Decl) { diff --git a/src/go/printer/printer.go b/src/go/printer/printer.go index eabf23e8b2..be61dad590 100644 --- a/src/go/printer/printer.go +++ b/src/go/printer/printer.go @@ -58,6 +58,7 @@ type printer struct { // Current state output []byte // raw printer result indent int // current indentation + level int // level == 0: outside composite literal; level > 0: inside composite literal mode pmode // current printer mode impliedSemi bool // if set, a linebreak implies a semicolon lastTok token.Token // last token printed (token.ILLEGAL if it's whitespace) @@ -744,15 +745,19 @@ func (p *printer) intersperseComments(next token.Position, tok token.Token) (wro // follows on the same line but is not a comma, and not a "closing" // token immediately following its corresponding "opening" token, // add an extra separator unless explicitly disabled. Use a blank - // as separator unless we have pending linebreaks and they are not - // disabled, in which case we want a linebreak (issue 15137). + // as separator unless we have pending linebreaks, they are not + // disabled, and we are outside a composite literal, in which case + // we want a linebreak (issue 15137). + // TODO(gri) This has become overly complicated. We should be able + // to track whether we're inside an expression or statement and + // use that information to decide more directly. needsLinebreak := false if p.mode&noExtraBlank == 0 && last.Text[1] == '*' && p.lineFor(last.Pos()) == next.Line && tok != token.COMMA && (tok != token.RPAREN || p.prevOpen == token.LPAREN) && (tok != token.RBRACK || p.prevOpen == token.LBRACK) { - if p.containsLinebreak() && p.mode&noExtraLinebreak == 0 { + if p.containsLinebreak() && p.mode&noExtraLinebreak == 0 && p.level == 0 { needsLinebreak = true } else { p.writeByte(' ', 1) diff --git a/src/go/printer/testdata/comments2.golden b/src/go/printer/testdata/comments2.golden index 7676a26c12..8b3a94ddcd 100644 --- a/src/go/printer/testdata/comments2.golden +++ b/src/go/printer/testdata/comments2.golden @@ -103,3 +103,62 @@ label: mask := uint64(1)< Date: Fri, 20 Jan 2017 17:12:50 -0800 Subject: [PATCH 18/19] database/sql: fix race when canceling queries immediately Previously the following could happen, though in practice it would be rare. Goroutine 1: (*Tx).QueryContext begins a query, passing in userContext Goroutine 2: (*Tx).awaitDone starts to wait on the context derived from the passed in context Goroutine 1: (*Tx).grabConn returns a valid (*driverConn) The (*driverConn) passes to (*DB).queryConn Goroutine 3: userContext is canceled Goroutine 2: (*Tx).awaitDone unblocks and calls (*Tx).rollback (*driverConn).finalClose obtains dc.Mutex (*driverConn).finalClose sets dc.ci = nil Goroutine 1: (*DB).queryConn obtains dc.Mutex in withLock ctxDriverPrepare accepts dc.ci which is now nil ctxCriverPrepare panics on the nil ci The fix for this is to guard the Tx methods with a RWLock holding it exclusivly when closing the Tx and holding a read lock when executing a query. Fixes #18719 Change-Id: I37aa02c37083c9793dabd28f7f934a1c5cbc05ea Reviewed-on: https://go-review.googlesource.com/35550 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- src/database/sql/sql.go | 91 +++++++++++++++++++++++++----------- src/database/sql/sql_test.go | 91 ++++++++++++++++++++++++++++++++---- 2 files changed, 147 insertions(+), 35 deletions(-) diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index 0fa7c34a13..feb91223a9 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -1357,16 +1357,7 @@ func (db *DB) begin(ctx context.Context, opts *TxOptions, strategy connReuseStra cancel: cancel, ctx: ctx, } - go func(tx *Tx) { - select { - case <-tx.ctx.Done(): - if !tx.isDone() { - // Discard and close the connection used to ensure the transaction - // is closed and the resources are released. - tx.rollback(true) - } - } - }(tx) + go tx.awaitDone() return tx, nil } @@ -1388,6 +1379,11 @@ func (db *DB) Driver() driver.Driver { type Tx struct { db *DB + // closemu prevents the transaction from closing while there + // is an active query. It is held for read during queries + // and exclusively during close. + closemu sync.RWMutex + // dc is owned exclusively until Commit or Rollback, at which point // it's returned with putConn. dc *driverConn @@ -1413,6 +1409,20 @@ type Tx struct { ctx context.Context } +// awaitDone blocks until the context in Tx is canceled and rolls back +// the transaction if it's not already done. +func (tx *Tx) awaitDone() { + // Wait for either the transaction to be committed or rolled + // back, or for the associated context to be closed. + <-tx.ctx.Done() + + // Discard and close the connection used to ensure the + // transaction is closed and the resources are released. This + // rollback does nothing if the transaction has already been + // committed or rolled back. + tx.rollback(true) +} + func (tx *Tx) isDone() bool { return atomic.LoadInt32(&tx.done) != 0 } @@ -1424,16 +1434,31 @@ var ErrTxDone = errors.New("sql: Transaction has already been committed or rolle // close returns the connection to the pool and // must only be called by Tx.rollback or Tx.Commit. func (tx *Tx) close(err error) { + tx.closemu.Lock() + defer tx.closemu.Unlock() + tx.db.putConn(tx.dc, err) tx.cancel() tx.dc = nil tx.txi = nil } +// hookTxGrabConn specifies an optional hook to be called on +// a successful call to (*Tx).grabConn. For tests. +var hookTxGrabConn func() + func (tx *Tx) grabConn(ctx context.Context) (*driverConn, error) { + select { + default: + case <-ctx.Done(): + return nil, ctx.Err() + } if tx.isDone() { return nil, ErrTxDone } + if hookTxGrabConn != nil { // test hook + hookTxGrabConn() + } return tx.dc, nil } @@ -1503,6 +1528,9 @@ func (tx *Tx) Rollback() error { // for the execution of the returned statement. The returned statement // will run in the transaction context. func (tx *Tx) PrepareContext(ctx context.Context, query string) (*Stmt, error) { + tx.closemu.RLock() + defer tx.closemu.RUnlock() + // TODO(bradfitz): We could be more efficient here and either // provide a method to take an existing Stmt (created on // perhaps a different Conn), and re-create it on this Conn if @@ -1567,6 +1595,9 @@ func (tx *Tx) Prepare(query string) (*Stmt, error) { // The returned statement operates within the transaction and will be closed // when the transaction has been committed or rolled back. func (tx *Tx) StmtContext(ctx context.Context, stmt *Stmt) *Stmt { + tx.closemu.RLock() + defer tx.closemu.RUnlock() + // TODO(bradfitz): optimize this. Currently this re-prepares // each time. This is fine for now to illustrate the API but // we should really cache already-prepared statements @@ -1618,6 +1649,9 @@ func (tx *Tx) Stmt(stmt *Stmt) *Stmt { // ExecContext executes a query that doesn't return rows. // For example: an INSERT and UPDATE. func (tx *Tx) ExecContext(ctx context.Context, query string, args ...interface{}) (Result, error) { + tx.closemu.RLock() + defer tx.closemu.RUnlock() + dc, err := tx.grabConn(ctx) if err != nil { return nil, err @@ -1661,6 +1695,9 @@ func (tx *Tx) Exec(query string, args ...interface{}) (Result, error) { // QueryContext executes a query that returns rows, typically a SELECT. func (tx *Tx) QueryContext(ctx context.Context, query string, args ...interface{}) (*Rows, error) { + tx.closemu.RLock() + defer tx.closemu.RUnlock() + dc, err := tx.grabConn(ctx) if err != nil { return nil, err @@ -2038,25 +2075,21 @@ type Rows struct { // closed value is 1 when the Rows is closed. // Use atomic operations on value when checking value. closed int32 - ctxClose chan struct{} // closed when Rows is closed, may be null. + cancel func() // called when Rows is closed, may be nil. lastcols []driver.Value lasterr error // non-nil only if closed is true closeStmt *driverStmt // if non-nil, statement to Close on close } func (rs *Rows) initContextClose(ctx context.Context) { - if ctx.Done() == context.Background().Done() { - return - } + ctx, rs.cancel = context.WithCancel(ctx) + go rs.awaitDone(ctx) +} - rs.ctxClose = make(chan struct{}) - go func() { - select { - case <-ctx.Done(): - rs.Close() - case <-rs.ctxClose: - } - }() +// awaitDone blocks until the rows are closed or the context canceled. +func (rs *Rows) awaitDone(ctx context.Context) { + <-ctx.Done() + rs.Close() } // Next prepares the next result row for reading with the Scan method. It @@ -2314,7 +2347,9 @@ func (rs *Rows) Scan(dest ...interface{}) error { return nil } -var rowsCloseHook func(*Rows, *error) +// rowsCloseHook returns a function so tests may install the +// hook throug a test only mutex. +var rowsCloseHook = func() func(*Rows, *error) { return nil } func (rs *Rows) isClosed() bool { return atomic.LoadInt32(&rs.closed) != 0 @@ -2328,13 +2363,15 @@ func (rs *Rows) Close() error { if !atomic.CompareAndSwapInt32(&rs.closed, 0, 1) { return nil } - if rs.ctxClose != nil { - close(rs.ctxClose) - } + err := rs.rowsi.Close() - if fn := rowsCloseHook; fn != nil { + if fn := rowsCloseHook(); fn != nil { fn(rs, &err) } + if rs.cancel != nil { + rs.cancel() + } + if rs.closeStmt != nil { rs.closeStmt.Close() } diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 3f8e03ce13..898df3b455 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -14,6 +14,7 @@ import ( "runtime" "strings" "sync" + "sync/atomic" "testing" "time" ) @@ -1135,6 +1136,24 @@ func TestQueryRowClosingStmt(t *testing.T) { } } +var atomicRowsCloseHook atomic.Value // of func(*Rows, *error) + +func init() { + rowsCloseHook = func() func(*Rows, *error) { + fn, _ := atomicRowsCloseHook.Load().(func(*Rows, *error)) + return fn + } +} + +func setRowsCloseHook(fn func(*Rows, *error)) { + if fn == nil { + // Can't change an atomic.Value back to nil, so set it to this + // no-op func instead. + fn = func(*Rows, *error) {} + } + atomicRowsCloseHook.Store(fn) +} + // Test issue 6651 func TestIssue6651(t *testing.T) { db := newTestDB(t, "people") @@ -1147,6 +1166,7 @@ func TestIssue6651(t *testing.T) { return fmt.Errorf(want) } defer func() { rowsCursorNextHook = nil }() + err := db.QueryRow("SELECT|people|name|").Scan(&v) if err == nil || err.Error() != want { t.Errorf("error = %q; want %q", err, want) @@ -1154,10 +1174,10 @@ func TestIssue6651(t *testing.T) { rowsCursorNextHook = nil want = "error in rows.Close" - rowsCloseHook = func(rows *Rows, err *error) { + setRowsCloseHook(func(rows *Rows, err *error) { *err = fmt.Errorf(want) - } - defer func() { rowsCloseHook = nil }() + }) + defer setRowsCloseHook(nil) err = db.QueryRow("SELECT|people|name|").Scan(&v) if err == nil || err.Error() != want { t.Errorf("error = %q; want %q", err, want) @@ -1830,7 +1850,9 @@ func TestStmtCloseDeps(t *testing.T) { db.dumpDeps(t) } - if len(stmt.css) > nquery { + if !waitCondition(5*time.Second, 5*time.Millisecond, func() bool { + return len(stmt.css) <= nquery + }) { t.Errorf("len(stmt.css) = %d; want <= %d", len(stmt.css), nquery) } @@ -2576,10 +2598,10 @@ func TestIssue6081(t *testing.T) { if err != nil { t.Fatal(err) } - rowsCloseHook = func(rows *Rows, err *error) { + setRowsCloseHook(func(rows *Rows, err *error) { *err = driver.ErrBadConn - } - defer func() { rowsCloseHook = nil }() + }) + defer setRowsCloseHook(nil) for i := 0; i < 10; i++ { rows, err := stmt.Query() if err != nil { @@ -2642,7 +2664,10 @@ func TestIssue18429(t *testing.T) { if err != nil { return } - rows, err := tx.QueryContext(ctx, "WAIT|"+qwait+"|SELECT|people|name|") + // This is expected to give a cancel error many, but not all the time. + // Test failure will happen with a panic or other race condition being + // reported. + rows, _ := tx.QueryContext(ctx, "WAIT|"+qwait+"|SELECT|people|name|") if rows != nil { rows.Close() } @@ -2655,6 +2680,56 @@ func TestIssue18429(t *testing.T) { time.Sleep(milliWait * 3 * time.Millisecond) } +// TestIssue18719 closes the context right before use. The sql.driverConn +// will nil out the ci on close in a lock, but if another process uses it right after +// it will panic with on the nil ref. +// +// See https://golang.org/cl/35550 . +func TestIssue18719(t *testing.T) { + db := newTestDB(t, "people") + defer closeDB(t, db) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + tx, err := db.BeginTx(ctx, nil) + if err != nil { + t.Fatal(err) + } + + hookTxGrabConn = func() { + cancel() + + // Wait for the context to cancel and tx to rollback. + for tx.isDone() == false { + time.Sleep(time.Millisecond * 3) + } + } + defer func() { hookTxGrabConn = nil }() + + // This call will grab the connection and cancel the context + // after it has done so. Code after must deal with the canceled state. + rows, err := tx.QueryContext(ctx, "SELECT|people|name|") + if err != nil { + rows.Close() + t.Fatalf("expected error %v but got %v", nil, err) + } + + // Rows may be ignored because it will be closed when the context is canceled. + + // Do not explicitly rollback. The rollback will happen from the + // canceled context. + + // Wait for connections to return to pool. + var numOpen int + if !waitCondition(5*time.Second, 5*time.Millisecond, func() bool { + numOpen = db.numOpenConns() + return numOpen == 0 + }) { + t.Fatalf("open conns after hitting EOF = %d; want 0", numOpen) + } +} + func TestConcurrency(t *testing.T) { doConcurrentTest(t, new(concurrentDBQueryTest)) doConcurrentTest(t, new(concurrentDBExecTest)) From 78860b2ad2261b6e3dc29006d2ffbdc4da14cb2e Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 25 Jan 2017 06:25:17 -0800 Subject: [PATCH 19/19] cmd/go: don't reject ./... matching top-level file outside GOPATH This unwinds a small part of CL 31668: we now accept "./." in cleanImport. Fixes #18778. Change-Id: Ia7f1fde1cafcea3cc9e0b597a95a0e0bb410a3ed Reviewed-on: https://go-review.googlesource.com/35646 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Russ Cox --- src/cmd/go/go_test.go | 23 +++++++++++++++++++++++ src/cmd/go/pkg.go | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 5727eb094e..f26c3660e4 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -3787,3 +3787,26 @@ GLOBL ·constants<>(SB),8,$8 tg.setenv("GOPATH", tg.path("go")) tg.run("build", "p") } + +// Issue 18778. +func TestDotDotDotOutsideGOPATH(t *testing.T) { + tg := testgo(t) + defer tg.cleanup() + + tg.tempFile("pkgs/a.go", `package x`) + tg.tempFile("pkgs/a_test.go", `package x_test +import "testing" +func TestX(t *testing.T) {}`) + + tg.tempFile("pkgs/a/a.go", `package a`) + tg.tempFile("pkgs/a/a_test.go", `package a_test +import "testing" +func TestA(t *testing.T) {}`) + + tg.cd(tg.path("pkgs")) + tg.run("build", "./...") + tg.run("test", "./...") + tg.run("list", "./...") + tg.grepStdout("pkgs$", "expected package not listed") + tg.grepStdout("pkgs/a", "expected package not listed") +} diff --git a/src/cmd/go/pkg.go b/src/cmd/go/pkg.go index d69fa5118f..e40f9420c7 100644 --- a/src/cmd/go/pkg.go +++ b/src/cmd/go/pkg.go @@ -429,7 +429,7 @@ func setErrorPos(p *Package, importPos []token.Position) *Package { func cleanImport(path string) string { orig := path path = pathpkg.Clean(path) - if strings.HasPrefix(orig, "./") && path != ".." && path != "." && !strings.HasPrefix(path, "../") { + if strings.HasPrefix(orig, "./") && path != ".." && !strings.HasPrefix(path, "../") { path = "./" + path } return path