Michael Munday [Fri, 17 Feb 2017 00:53:08 +0000 (19:53 -0500)]
[release-branch.go1.8] cmd/compile: zero extend when replacing load-hit-store on s390x
Keith pointed out that these rules should zero extend during the review
of CL 36845. In practice the generic rules are responsible for eliminating
most load-hit-stores and they do not have this problem. When the s390x
rules are triggered any cast following the elided load-hit-store is
kept because of the sequence the rules are applied in (i.e. the load is
removed before the zero extension gets a chance to be merged into the load).
It is therefore not clear that this issue results in any functional bugs.
This CL includes a test, but it only tests the generic rules currently.
Change-Id: Idbc43c782097a3fb159be293ec3138c5b36858ad
Reviewed-on: https://go-review.googlesource.com/37154
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-on: https://go-review.googlesource.com/70831
Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Michael Munday <mike.munday@ibm.com>
Russ Cox [Wed, 4 Oct 2017 17:24:49 +0000 (13:24 -0400)]
[release-branch.go1.8] net/smtp: fix PlainAuth to refuse to send passwords to non-TLS servers
PlainAuth originally refused to send passwords to non-TLS servers
and was documented as such.
In 2013, issue #5184 was filed objecting to the TLS requirement,
despite the fact that it is spelled out clearly in RFC 4954.
The only possibly legitimate use case raised was using PLAIN auth
for connections to localhost, and the suggested fix was to let the
server decide: if it advertises that PLAIN auth is OK, believe it.
That approach was adopted in CL 8279043 and released in Go 1.1.
Unfortunately, this is exactly wrong. The whole point of the TLS
requirement is to make sure not to send the password to the wrong
server or to a man-in-the-middle. Instead of implementing this rule,
CL 8279043 blindly trusts the server, so that if a man-in-the-middle
says "it's OK, you can send me your password," PlainAuth does.
And the documentation was not updated to reflect any of this.
This CL restores the original TLS check, as required by RFC 4954
and as promised in the documentation for PlainAuth.
It then carves out a documented exception for connections made
to localhost (defined as "localhost", "127.0.0.1", or "::1").
This includes the patch for systems that build PIE executables by
defaul
Updates #20276.
Change-Id: Iecf8dfcf11bc18d397b8075559c37e3610f825cb
Reviewed-on: https://go-review.googlesource.com/44470 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/44491 Reviewed-by: Chris Broadfoot <cbro@golang.org>
Keith Randall [Wed, 24 May 2017 05:55:59 +0000 (22:55 -0700)]
[release-branch.go1.8] cmd/compile: don't move spills to loop exits where the spill is dead
We shouldn't move a spill to a loop exit where the spill itself
is dead. The stack location assigned to the spill might already
be reused by another spill at this point.
The case we previously handled incorrectly is the one where the value
being spilled is still live, but the spill itself is dead.
Fixes #20472
Patching directly on the release branch because the spill moving code has
already been rewritten for 1.9. (And it doesn't have this bug.)
Keith Randall [Tue, 23 May 2017 22:19:27 +0000 (15:19 -0700)]
[release-branch.go1.8] cmd/compile: zero ambiguously live variables at VARKILLs
This is a redo of CL 41076 backported to the 1.8 release branch.
There were major conflicts, so I had to basically rewrite it again
from scratch. The way Progs are allocated changed. Liveness analysis
and Prog generation got reordered. Liveness analysis changed from
running on gc.BasicBlock to ssa.Block. All that makes the logic quite
a bit different.
Please review carefully.
From CL 41076:
At VARKILLs, zero a variable if it is ambiguously live.
After the VARKILL anything this variable references
might be collected. If it were to become live again later,
the GC will see references to already-collected objects.
We don't know a variable is ambiguously live until very
late in compilation (after lowering, register allocation, ...),
so it is hard to generate the code in an arch-independent way.
We also have to be careful not to clobber any registers.
Fortunately, this almost never happens so performance is ~irrelevant.
There are only 2 instances where this triggers in the stdlib.
Fixes #20029
Change-Id: Ibb757eec58ee07f40df5e561b19d315684dc4bda
Reviewed-on: https://go-review.googlesource.com/43998
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Austin Clements [Tue, 23 May 2017 21:54:24 +0000 (17:54 -0400)]
[release-branch.go1.8] runtime: use pselect6 for usleep on linux/386
Commit 4dcba023c6 replaced select with pselect6 on linux/amd64 and
linux/arm, but it turns out the Android emulator uses linux/386. This
makes the equivalent change there, too.
Fixes #20409 more.
Change-Id: If542d6ade06309aab8758d5f5f6edec201ca7670
Reviewed-on: https://go-review.googlesource.com/44011
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit ecad34a40ea390ddf5ba2da8f3c3f2c5f15297c8)
Reviewed-on: https://go-review.googlesource.com/44002
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Chris Broadfoot <cbro@golang.org>
Austin Clements [Thu, 18 May 2017 20:56:48 +0000 (16:56 -0400)]
[release-branch.go1.8] runtime: use pselect6 for usleep on linux/amd64 and linux/arm
Android O black-lists the select system call because its libc, Bionic,
does not use this system call. Replace our use of select with pselect6
(which is allowed) on the platforms that support targeting Android.
linux/arm64 already uses pselect6 because there is no select on arm64,
so only linux/amd64 and linux/arm need changing. pselect6 has been
available since Linux 2.6.16, which is before Go's minimum
requirement.
Fixes #20409.
Change-Id: Ic526b5b259a9e01d2f145a1f4d2e76e8c49ce809
Reviewed-on: https://go-review.googlesource.com/43641
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 4dcba023c62d7f7968abc54fa5d38d2bf11412ba)
Reviewed-on: https://go-review.googlesource.com/44001
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Chris Broadfoot <cbro@golang.org>
On s390x unsigned integer comparisons with immediates require the immediate
to be an unsigned 32-bit integer. The rule was checking that the immediate
was a signed 32-bit integer.
This CL also adds a test for comparisons that could be turned into compare
with immediate or equivalent instructions (depending on architecture and
optimizations applied).
Austin Clements [Mon, 22 May 2017 19:53:49 +0000 (15:53 -0400)]
[release-branch.go1.8] runtime: don't corrupt arena bounds on low mmap
Cherry-pick of CL 43870.
If mheap.sysAlloc doesn't have room in the heap arena for an
allocation, it will attempt to map more address space with sysReserve.
sysReserve is given a hint, but can return any unused address range.
Currently, mheap.sysAlloc incorrectly assumes the returned region will
never fall between arena_start and arena_used. If it does,
mheap.sysAlloc will blindly accept the new region as the new
arena_used and arena_end, causing these to decrease and make it so any
Go heap above the new arena_used is no longer considered part of the
Go heap. This assumption *used to be* safe because we had all memory
between arena_start and arena_used mapped, but when we switched to an
arena_start of 0 on 32-bit, it became no longer safe.
Most likely, we've only recently seen this bug occur because we
usually start arena_used just above the binary, which is low in the
address space. Hence, the kernel is very unlikely to give us a region
before arena_used.
Since mheap.sysAlloc is a linear allocator, there's not much we can do
to handle this well. Hence, we fix this problem by simply rejecting
the new region if it isn't after arena_end. In this case, we'll take
the fall-back path and mmap a small region at any address just for the
requested memory.
Keith Randall [Thu, 11 May 2017 21:46:49 +0000 (14:46 -0700)]
[release-branch.go1.8] cmd/compile: fix store chain in schedule pass
Cherry-pick of CL 43294.
Tuple ops are weird. They are essentially a pair of ops,
one which consumes a mem and one which generates a mem (the Select1).
The schedule pass didn't handle these quite right.
Fix the scheduler to include both parts of the paired op in
the store chain. That makes sure that loads are correctly ordered
with respect to the first of the pair.
Add a check for the ssacheck builder, that there is only one
live store at a time. I thought we already had such a check, but
apparently not...
runtime.gchelper depends on the non-atomic load of work.ndone
happening strictly before the atomic add of work.nwait. Until very
recently (commit 978af9c2db, fixing #20334), the compiler reordered
these operations. This created a race since work.ndone can change as
soon as work.nwait is equal to work.ndone. If that happened, more than
one gchelper could attempt to wake up the work.alldone note, causing a
"double wakeup" panic.
This was fixed in the compiler, but to make this code less subtle,
make the load of work.ndone atomic. This clearly forces the order of
these operations, ensuring the race doesn't happen.
Brad Fitzpatrick [Fri, 19 May 2017 19:39:20 +0000 (19:39 +0000)]
[release-branch.go1.8] doc: remove mentions of yacc tool
It was removed in CL 27325.
Fixes #20431
Change-Id: I6842851444186e19029d040f61fdf4f87a3103a6
Reviewed-on: https://go-review.googlesource.com/43771 Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit deebd8fe273df2de2d590ee41ae1155c521219e9)
Reviewed-on: https://go-review.googlesource.com/43772 Reviewed-by: Chris Broadfoot <cbro@golang.org>
David Chase [Tue, 28 Mar 2017 21:55:26 +0000 (17:55 -0400)]
[release-branch.go1.8] cmd/compile: added special case for reflect header fields to esc
The uintptr-typed Data field in reflect.SliceHeader and
reflect.StringHeader needs special treatment because it is
really a pointer. Add the special treatment in walk for
bug #19168 to escape analysis.
Matthew Dempsky [Thu, 2 Mar 2017 05:46:08 +0000 (21:46 -0800)]
[release-branch.go1.8] cmd/compile: add missing WBs for reflect.{Slice,String}Header.Data
Fixes #19168.
(*state).insertWBstore needed to be tweaked for backporting so that
store reflect.{Slice,String}Header.Data stores still fallthrough and
end the SSA block. This wasn't necessary at master because of CL
36834.
but this broke XML code that had empty attributes (#19333).
In Go 1.9 we plan to start allowing empty children (#13417).
The fix for that will also make empty attributes work again:
$ go run /tmp/x.go # Go 1.9 development
<X></X> ok
<X A=""></X> ok
<X A="bad"></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax
<X></X> ok
<X><C></C></X> ok
<X><C/></X> ok
<X><C>bad</C></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax
$
For Go 1.8.1, we want to restore the empty attribute behavior
to match Go 1.7 but not yet change the child behavior as planned for Go 1.9,
since that change hasn't been through release testing.
Instead, restore the more lax Go 1.7 behavior, so that XML files
with empty attributes will not be broken until Go 1.9:
$ go run /tmp/x.go # after this CL
<X></X> ok
<X A=""></X> ok
<X A="bad"></X> ok
<X></X> ok
<X><C></C></X> ERROR strconv.ParseInt: parsing "": invalid syntax
<X><C/></X> ERROR strconv.ParseInt: parsing "": invalid syntax
<X><C>bad</C></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax
$
reflect.callReflect heap-allocates a stack frame and then constructs
pointers to the arguments and result areas of that frame. However, if
there are no results, the results pointer will point past the end of
the frame allocation. If there are also no arguments, the arguments
pointer will also point past the end of the frame allocation. If the
GC observes either these pointers, it may panic.
Fix this by not constructing these pointers if these areas of the
frame are empty.
This adds a test of calling no-argument/no-result methods via reflect,
since nothing in std did this before. However, it's quite difficult to
demonstrate the actual failure because it depends on both exact
allocation patterns and on GC scanning the goroutine's stack while
inside one of the typedmemmovepartial calls.
I also audited other uses of typedmemmovepartial and
memclrNoHeapPointers in reflect, since these are the most susceptible
to this. These appear to be the only two cases that can construct
out-of-bounds arguments to these functions.
Russ Cox [Thu, 30 Mar 2017 00:50:34 +0000 (20:50 -0400)]
[release-branch.go1.8] cmd/link: emit a mach-o dwarf segment that dsymutil will accept
Right now, at least with Xcode 8.3, we invoke dsymutil and dutifully
copy what it produces back into the binary, but it has actually dropped
all the DWARF information that we wanted, because it didn't like
the look of go.o.
Make it like the look of go.o.
DWARF is tested in other ways, but typically indirectly and not for cgo programs.
Add a direct test, and one that exercises cgo.
This detects missing dwarf information in cgo-using binaries on macOS,
at least with Xcode 8.3, and possibly earlier versions as well.
Fixes #19772.
The backport to Go 1.8 disables TestDWARF on Windows because Windows
DWARF support is new in Go 1.9.
Russ Cox [Thu, 30 Mar 2017 00:53:32 +0000 (20:53 -0400)]
[release-branch.go1.8] cmd/link: make mach-o dwarf segment properly aligned
Without this, the load fails during kernel exec, which results in the
mysterious and completely uninformative "Killed: 9" error.
It appears that the stars (or at least the inputs) were properly aligned
with earlier versions of Xcode so that this happened accidentally.
Make it happen on purpose.
Gregory Man bisected the breakage to this change in LLVM,
which fits the theory nicely:
https://github.com/llvm-mirror/llvm/commit/9a41e59c
Austin Clements [Fri, 10 Mar 2017 15:59:39 +0000 (10:59 -0500)]
[release-branch.go1.8] runtime: print user stack on other threads during GOTRACBEACK=crash
Currently, when printing tracebacks of other threads during
GOTRACEBACK=crash, if the thread is on the system stack we print only
the header for the user goroutine and fail to print its stack. This
happens because we passed the g0 to traceback instead of curg. The g0
never has anything set in its gobuf, so traceback doesn't print
anything.
Fix this by passing _g_.m.curg to traceback instead of the g0.
Vladimir Stefanovic [Tue, 21 Mar 2017 13:57:58 +0000 (14:57 +0100)]
[release-branch.go1.8] cmd/compile,runtime: fix atomic And8 for mipsle
Removing stray xori that came from big endian copy/paste.
Adding atomicand8 check to runtime.check() that would have revealed
this error.
Might fix #19396.
David Chase [Thu, 23 Feb 2017 18:49:25 +0000 (13:49 -0500)]
[release-branch.go1.8] cmd/compile: repaired loop-finder to handle trickier nesting
The loop-A-encloses-loop-C code did not properly handle the
case where really C was already known to be enclosed by B,
and A was nearest-outer to B, not C.
David Chase [Tue, 21 Feb 2017 20:22:52 +0000 (15:22 -0500)]
[release-branch.go1.8] cmd/compile: add opcode flag hasSideEffects for do-not-remove
Added a flag to generic and various architectures' atomic
operations that are judged to have observable side effects
and thus cannot be dead-code-eliminated.
Test requires GOMAXPROCS > 1 without preemption in loop.
Brad Fitzpatrick [Wed, 29 Mar 2017 16:55:58 +0000 (16:55 +0000)]
[release-branch.go1.8] net, net/http: adjust time-in-past constant even earlier
The aLongTimeAgo time value in net and net/http is used to cancel
in-flight read and writes. It was set to time.Unix(233431200, 0)
which seemed like far enough in the past.
But Raspberry Pis, lacking a real time clock, had to spoil the fun and
boot in 1970 at the Unix epoch time, breaking assumptions in net and
net/http.
So change aLongTimeAgo to time.Unix(1, 0), which seems like the
earliest safe value. I don't trust subsecond values on all operating
systems, and I don't trust the Unix zero time. The Raspberry Pis do
advance their clock at least. And the reported problem was that Hijack
on a ResponseWriter hung forever, waiting for the connection read
operation to finish. So now, even if kernel + userspace boots in under
a second (unlikely), the Hijack will just have to wait for up to a
second.
Updates #19747
Fixes #19771 (backport to Go 1.8.x)
Ilya Tocar [Tue, 21 Feb 2017 22:57:37 +0000 (16:57 -0600)]
[release-branch.go1.8] cmd/compile/internal/ssa: don't schedule values after select
Scheduling values after calls to selectrecv,
will cause them to be executed multiple times, due to runtime.selectgo
jumping to the next instruction in the selectrecv basic block.
Prevent this by scheduling calls to selectrecv as late as possible
Lynn Boger [Thu, 23 Mar 2017 14:58:40 +0000 (10:58 -0400)]
[release-branch.go1.8] cmd/link: put plt stubs first in Textp on ppc64x
Previously call stubs were generated and inserted in
Textp after much of the text, resulting in calls too
far in some cases. This puts the call stubs first, which
in many cases makes some calls not so far, but also
enables trampolines to be generated when necessary.
Steve Francia [Thu, 9 Feb 2017 20:33:13 +0000 (15:33 -0500)]
[release-branch.go1.8] doc: reorganize the contribution guidelines into a guide
Updates #17802
Change-Id: I65ea0f4cde973604c04051e7eb25d12e4facecd3
Reviewed-on: https://go-review.googlesource.com/36626 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-on: https://go-review.googlesource.com/38312
Alberto Donizetti [Thu, 9 Mar 2017 12:20:54 +0000 (13:20 +0100)]
[release-branch.go1.8] time: make the ParseInLocation test more robust
The tzdata 2017a update (2017-02-28) changed the abbreviation of the
Asia/Baghdad time zone (used in TestParseInLocation) from 'AST' to the
numeric '+03'.
Update the test so that it skips the checks if we're using a recent
tzdata release.
Fixes #19457
Change-Id: I45d705a5520743a611bdd194dc8f8d618679980c
Reviewed-on: https://go-review.googlesource.com/37964 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 91563ced5897faf729a34be7081568efcfedda31)
Reviewed-on: https://go-review.googlesource.com/37991
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Mike Danese [Wed, 1 Mar 2017 18:43:57 +0000 (10:43 -0800)]
[release-branch.go1.8] crypto/tls: make Config.Clone also clone the GetClientCertificate field
Using GetClientCertificate with the http client is currently completely
broken because inside the transport we clone the tls.Config and pass it
off to the tls.Client. Since tls.Config.Clone() does not pass forward
the GetClientCertificate field, GetClientCertificate is ignored in this
context.
Fixes #19264
Change-Id: Ie214f9f0039ac7c3a2dab8ffd14d30668bdb4c71 Signed-off-by: Mike Danese <mikedanese@google.com>
Reviewed-on: https://go-review.googlesource.com/37541 Reviewed-by: Filippo Valsorda <hi@filippo.io> Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 87649d32ad16a9a0b7bd5dbd1c124b2032a270f1)
Reviewed-on: https://go-review.googlesource.com/37946
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Tom Bergan <tombergan@google.com>
Cherry Zhang [Sun, 19 Feb 2017 02:03:15 +0000 (21:03 -0500)]
[release-branch.go1.8] cmd/compile: do not fold offset into load/store for args on ARM64
Args may be not at 8-byte aligned offset to SP. When the stack
frame is large, folding the offset of args may cause large
unaligned offsets that does not fit in a machine instruction on
ARM64. Therefore disable folding offsets for args.
This has small performance impact (see below). A better fix would
be letting the assembler backend fix up the offset by loading it
into a register if it doesn't fit into an instruction. And the
compiler can simply generate large load/stores with offset. Since
in most of the cases the offset is aligned or the stack frame is
small, it can fit in an instruction and no fixup is needed. But
this is too complicated for Go 1.8.
Cherry Zhang [Fri, 17 Feb 2017 15:27:43 +0000 (10:27 -0500)]
[release-branch.go1.8] cmd/compile: check both syms when folding address into load/store on ARM64
The rules for folding addresses into load/stores checks sym1 is
not on stack (because the stack offset is not known at that point).
But sym1 could be nil, which invalidates the check. Check merged
sym instead.
Michael Munday [Tue, 21 Feb 2017 20:20:38 +0000 (15:20 -0500)]
[release-branch.go1.8] cmd/compile: fix merging of s390x conditional moves into branch conditions
A type conversion inserted between MOVD{LT,LE,GT,GE,EQ,NE} and CMPWconst
by CL 36256 broke the rewrite rule designed to merge the two.
This results in simple for loops (e.g. for i := 0; i < N; i++ {})
emitting two comparisons instead of one, plus a conditional move.
This CL explicitly types the input to CMPWconst so that the type conversion
can be omitted. It also adds a test to check that conditional moves aren't
emitted for loops with 'less than' conditions (i.e. i < N) on s390x.
Fixes #19227.
Change-Id: I44958eebf6c74c5819b2a9511caf3c47c20fbf45
Reviewed-on: https://go-review.googlesource.com/37536
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bill O'Farrell <billotosyr@gmail.com> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Michael Munday [Tue, 28 Feb 2017 19:28:28 +0000 (14:28 -0500)]
[release-branch.go1.8] cmd/compile: remove unnecessary type conversions on s390x
Some rules insert MOVDreg ops to ensure that type changes are kept.
If there is no type change (or the input is constant) then the MOVDreg
can be omitted, allowing further optimization.
Reduces the size of the .text section in the asm tool by ~33KB.
Russ Cox [Thu, 23 Feb 2017 20:13:25 +0000 (15:13 -0500)]
[release-branch.go1.8] runtime: avoid O(n) semaphore list walk in contention profiling
Contention profiling is off by default.
If you turn it on, it has the unfortunate effect of making
the wakeup on a contention mutex go from O(1) to O(n).
Change it back to O(1).
This is already fixed in essentially the same way on master;
master also contains some fixes for the non-profiling code
paths.
Possible for Go 1.8.1.
Change-Id: Iaa644c06e20ca28da4dfa348b7211eedb657e0ba
Reviewed-on: https://go-review.googlesource.com/37341
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Wed, 15 Feb 2017 20:41:50 +0000 (15:41 -0500)]
[release-branch.go1.8] runtime: do not call wakep from enlistWorker, to avoid possible deadlock
We have seen one instance of a production job suddenly spinning to
100% CPU and becoming unresponsive. In that one instance, a SIGQUIT
was sent after 328 minutes of spinning, and the stacks showed a single
goroutine in "IO wait (scan)" state.
Looking for things that might get stuck if a goroutine got stuck in
scanning a stack, we found that injectglist does:
lock(&sched.lock)
var n int
for n = 0; glist != nil; n++ {
gp := glist
glist = gp.schedlink.ptr()
casgstatus(gp, _Gwaiting, _Grunnable)
globrunqput(gp)
}
unlock(&sched.lock)
and that casgstatus spins on gp.atomicstatus until the _Gscan bit goes
away. Essentially, this code locks sched.lock and then while holding
sched.lock, waits to lock gp.atomicstatus.
The code that is doing the scan is:
if castogscanstatus(gp, s, s|_Gscan) {
if !gp.gcscandone {
scanstack(gp, gcw)
gp.gcscandone = true
}
restartg(gp)
break loop
}
More analysis showed that scanstack can, in a rare case, end up
calling back into code that acquires sched.lock. For example:
runtime.scanstack at proc.go:866
calls runtime.gentraceback at mgcmark.go:842
calls runtime.scanstack$1 at traceback.go:378
calls runtime.scanframeworker at mgcmark.go:819
calls runtime.scanblock at mgcmark.go:904
calls runtime.greyobject at mgcmark.go:1221
calls (*runtime.gcWork).put at mgcmark.go:1412
calls (*runtime.gcControllerState).enlistWorker at mgcwork.go:127
calls runtime.wakep at mgc.go:632
calls runtime.startm at proc.go:1779
acquires runtime.sched.lock at proc.go:1675
This path was found with an automated deadlock-detecting tool.
There are many such paths but they all go through enlistWorker -> wakep.
The evidence strongly suggests that one of these paths is what caused
the deadlock we observed. We're running those jobs with
GOTRACEBACK=crash now to try to get more information if it happens
again.
Further refinement and analysis shows that if we drop the wakep call
from enlistWorker, the remaining few deadlock cycles found by the tool
are all false positives caused by not understanding the effect of calls
to func variables.
The enlistWorker -> wakep call was intended only as a performance
optimization, it rarely executes, and if it does execute at just the
wrong time it can (and plausibly did) cause the deadlock we saw.
Sarah Adams [Tue, 14 Feb 2017 23:34:46 +0000 (15:34 -0800)]
[release-branch.go1.8] doc: update Code of Conduct wording and scope
This change removes the punitive language and anonymous reporting mechanism
from the Code of Conduct document. Read on for the rationale.
More than a year has passed since the Go Code of Conduct was introduced.
In that time, there have been a small number (<30) of reports to the Working Group.
Some reports we handled well, with positive outcomes for all involved.
A few reports we handled badly, resulting in hurt feelings and a bad
experience for all involved.
On reflection, the reports that had positive outcomes were ones where the
Working Group took the role of advisor/facilitator, listening to complaints and
providing suggestions and advice to the parties involved.
The reports that had negative outcomes were ones where the subject of the
report felt threatened by the Working Group and Code of Conduct.
After some discussion among the Working Group, we saw that we are most
effective as facilitators, rather than disciplinarians. The various Go spaces
already have moderators; this change to the CoC acknowledges their authority
and places the group in a purely advisory role. If an incident is
reported to the group we may provide information to or make a
suggestion the moderators, but the Working Group need not (and should not) have
any authority to take disciplinary action.
In short, we want it to be clear that the Working Group are here to help
resolve conflict, period.
The second change made here is the removal of the anonymous reporting mechanism.
To date, the quality of anonymous reports has been low, and with no way to
reach out to the reporter for more information there is often very little we
can do in response. Removing this one-way reporting mechanism strengthens the
message that the Working Group are here to facilitate a constructive dialogue.
Change-Id: Iee52aff5446accd0dae0c937bb3aa89709ad5fb4
Reviewed-on: https://go-review.googlesource.com/37014 Reviewed-by: Andrew Gerrand <adg@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/37040 Reviewed-by: Chris Broadfoot <cbro@golang.org>
The new tests in this CL have been checked against Go 1.7 as well
and all pass in Go 1.7, with the one exception noted in a comment
(an intentional change to omitempty already present before this CL).
CL 15684 made the intentional change to omitempty.
This CL fixes bugs introduced along the way.
Most of these are corner cases that are arguably not that important,
but they've always worked all the way back to Go 1, and someone
cared enough to file #19063. The most significant problem found
while adding tests is that in the case of a nil *string field with
`xml:",chardata"`, the existing code silently stops processing not just
that field but the entire remainder of the struct.
Even if #19063 were not worth fixing, this chardata bug would be.
Fixes #19063.
Change-Id: I318cf8f9945e1a4615982d9904e109fde577ebf9
Reviewed-on: https://go-review.googlesource.com/36954
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 72aa757dddad7e915f4faad87aacf8010d91561b)
Reviewed-on: https://go-review.googlesource.com/37016
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Daniel Theophanes [Sun, 12 Feb 2017 23:12:52 +0000 (15:12 -0800)]
[release-branch.go1.8] database/sql: convert test timeouts to explicit waits with checks
When testing context cancelation behavior do not rely on context
timeouts. Use explicit checks in all such tests. In closeDB
convert the simple check for zero open conns with a wait loop
for zero open conns.
Daniel Theophanes [Thu, 9 Feb 2017 05:38:51 +0000 (21:38 -0800)]
[release-branch.go1.8] database/sql: ensure driverConns are closed if not returned to pool
Previously if a connection was requested but timed out during the
request and when acquiring the db.Lock the connection request
is fulfilled and the request is unable to be returned to the
connection pool, then then driver connection would not be closed.
No tests were added or modified because I was unable to determine
how to trigger this situation without something invasive.
Daniel Theophanes [Wed, 8 Feb 2017 18:32:22 +0000 (10:32 -0800)]
[release-branch.go1.8] database/sql: do not exhaust connection pool on conn request timeout
Previously if a context was canceled while it was waiting for a
connection request, that connection request would leak.
To prevent this remove the pending connection request if the
context is canceled and ensure no connection has been sent on the channel.
This requires a change to how the connection requests are represented in the DB.
Daniel Theophanes [Tue, 7 Feb 2017 18:19:02 +0000 (10:19 -0800)]
[release-branch.go1.8] database/sql: record the context error in Rows if canceled
Previously it was intended that Rows.Scan would return
an error and Rows.Err would return nil. This was problematic
because drivers could not differentiate between a normal
Rows.Close or a context cancel close.
The alternative is to require drivers to return a Scan to return
an error if the driver is closed while there are still rows to be read.
This is currently not how several drivers currently work and may be
difficult to detect when there are additional rows.
At the same time guard the the Rows.lasterr and prevent a close
while a Rows operation is active.
For the drivers that do not have Context methods, do not check for
context cancelation after the operation, but before for any operation
that may modify the database state.
Russ Cox [Tue, 7 Feb 2017 16:59:38 +0000 (11:59 -0500)]
[release-branch.go1.8] crypto/x509: check for new tls-ca-bundle.pem last
We added CentOS 7's /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
to the list in response to #17549 - not being able to find any certs otherwise.
Now we have #18813, where CentOS 6 apparently has both that file
and /etc/pki/tls/certs/ca-bundle.crt, and the latter is complete while
the former is not.
Moving the new CentOS 7 file to the bottom of the list should fix both
problems: the CentOS 7 system that didn't have any of the other files
in the list will still find the new one, and existing systems will still
keep using what they were using instead of preferring the new path
that may or may not be complete on some systems.
Cherry Zhang [Mon, 6 Feb 2017 19:24:16 +0000 (14:24 -0500)]
[release-branch.go1.8] cmd/compile: do not use "oaslit" for global
The compiler did not emit write barrier for assigning global with
struct literal, like global = T{} where T contains pointer.
The relevant code path is:
walkexpr OAS var_ OSTRUCTLIT
oaslit
anylit OSTRUCTLIT
walkexpr OAS var_ nil
return without adding write barrier
return true
break (without adding write barrier)
This CL makes oaslit not apply to globals. See also CL
https://go-review.googlesource.com/c/36355/ for an alternative
fix.
The downside of this is that it generates static data for zeroing
struct now. Also this only covers global. If there is any lurking
bug with implicit zeroing other than globals, this doesn't fix.
Robert Griesemer [Tue, 7 Feb 2017 06:01:07 +0000 (22:01 -0800)]
[release-branch.go1.8] cmd/compile/internal/syntax: avoid follow-up error for incorrect if statement
This is a follow-up on https://go-review.googlesource.com/36470
and leads to a more stable fix. The above CL relied on filtering
of multiple errors on the same line to avoid more than one error
for an `if` statement of the form `if a := 10 {}`. This CL avoids
the secondary error ("missing condition in if statement") in the
first place.
Michael Munday [Fri, 3 Feb 2017 21:47:32 +0000 (16:47 -0500)]
[release-branch.go1.8] cmd/compile: fix type propagation through s390x SSA rules
This CL fixes two issues:
1. Load ops were initially always lowered to unsigned loads, even
for signed types. This was fine by itself however LoadReg ops
(used to re-load spilled values) were lowered to signed loads
for signed types. This meant that spills could invalidate
optimizations that assumed the original unsigned load.
2. Types were not always being maintained correctly through rules
designed to eliminate unnecessary zero and sign extensions.
Updates #18906 and fixes #18958 (backport of CL 36256 to 1.8).
Russ Cox [Mon, 30 Jan 2017 21:55:52 +0000 (16:55 -0500)]
[release-branch.go1.8] all: final merge of master into Go 1.8 release branch
After this, we will merge some of the dev work like
type aliases and inlining into master, so any additional
changes for the Go 1.8 release will need to be cherry-picked,
not merged.
3e55059f cmd/dist: really skip the testsanitizers tests on Android 09496599 runtime: add explicit (void) in C to avoid GCC 7 problem 4cffe2b6 cmd/dist: use the target GOOS to skip the test for issue 18153 6bdb0c11 doc: update go1.8 release notes after TxOptions change 09096bd3 cmd/go: update alldocs after CL 35150 96ea0918 cmd/compile: use CMPWU for 32-bit or smaller unsigned Geq on ppc64{,le} 21a8db1c doc: document go1.7.5
Ian Lance Taylor [Mon, 30 Jan 2017 17:43:25 +0000 (09:43 -0800)]
runtime: add explicit (void) in C to avoid GCC 7 problem
This avoids errors like
./traceback.go:80:2: call of non-function C.f1
I filed https://gcc.gnu.org/PR79289 for the GCC problem. I think this
is a bug in GCC, and it may be fixed before the final GCC 7 release.
This CL is correct either way.
Fixes #18855.
Change-Id: I0785a7b7c5b1d0ca87b454b5eca9079f390fcbd4
Reviewed-on: https://go-review.googlesource.com/35919
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: David Crawshaw <crawshaw@golang.org>
Chris Broadfoot [Thu, 26 Jan 2017 17:24:20 +0000 (09:24 -0800)]
[release-branch.go1.8] all: merge master into release-branch.go1.8
78860b2ad2 cmd/go: don't reject ./... matching top-level file outside GOPATH 2b283cedef database/sql: fix race when canceling queries immediately 1cf08182f9 go/printer: fix format with leading comments in composite literal b531eb3062 runtime: reorder modules so main.main comes first 165cfbc409 database/sql: let tests wait for db pool to come to expected state ea73649343 doc: update gccgo docs 1db16711f5 doc: clarify what to do with Go 1.4 when installing from source 3717b429f2 doc: note that plugins are not fully baked 98842cabb6 net/http: don't send body on redirects for 301, 302, 303 when GetBody is set 314180e7f6 net/http: fix a nit aad06da2b9 cmd/link: mark DWARF function symbols as reachable be9dcfec29 doc: mention testing.MainStart signature change a96e117a58 runtime: amd64, use 4-byte ops for memmove of 4 bytes 4cce27a3fa cmd/compile: fix constant propagation through s390x MOVDNE instructions 1be957d703 misc/cgo/test: pass current environment to syscall.Exec ec654e2251 misc/cgo/test: fix test when using GCC 7 256a605faa cmd/compile: don't use nilcheck information until the next block e8d5989ed1 cmd/compile: fix compilebench -alloc ea7d9e6a52 runtime: check for nil g and m in msanread
Robert Griesemer [Wed, 25 Jan 2017 23:05:39 +0000 (15:05 -0800)]
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
David Crawshaw [Wed, 25 Jan 2017 04:19:36 +0000 (20:19 -0800)]
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 <crawshaw@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Michael Hudson-Doyle <michael.hudson@canonical.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>