Austin Clements [Sun, 16 Oct 2016 22:23:39 +0000 (18:23 -0400)]
[release-branch.go1.7] runtime: fix getArgInfo for deferred reflection calls
Fixes #18333 (backport)
getArgInfo for reflect.makeFuncStub and reflect.methodValueCall is
necessarily special. These have dynamically determined argument maps
that are stored in their context (that is, their *funcval). These
functions are written to store this context at 0(SP) when called, and
getArgInfo retrieves it from there.
This technique works if getArgInfo is passed an active call frame for
one of these functions. However, getArgInfo is also used in
tracebackdefers, where the "call" is not a true call with an active
stack frame, but a deferred call. In this situation, getArgInfo
currently crashes because tracebackdefers passes a frame with sp set
to 0. However, the entire approach used by getArgInfo is flawed in
this situation because the wrapper has not actually executed, and
hence hasn't saved this metadata to any stack frame.
In the defer case, we know the *funcval from the _defer itself, so we
can fix this by teaching getArgInfo to use the *funcval context
directly when its available, and otherwise get it from the active call
frame.
While we're here, this commit simplifies getArgInfo a bit by making it
play more nicely with the type system. Rather than decoding the
*reflect.methodValue that is the wrapper's context as a *[2]uintptr,
just write out a copy of the reflect.methodValue type in the runtime.
Austin Clements [Wed, 18 Jan 2017 02:58:10 +0000 (21:58 -0500)]
[release-branch.go1.7] runtime: force workers out before checking mark roots
Fixes #18700 (backport)
Currently we check that all roots are marked as soon as gcMarkDone
decides to transition from mark 1 to mark 2. However, issue #16083
indicates that there may be a race where we try to complete mark 1
while a worker is still scanning a stack, causing the root mark check
to fail.
We don't yet understand this race, but as a simple mitigation, move
the root check to after gcMarkDone performs a ragged barrier, which
will force any remaining workers to finish their current job.
Updates #16083. This may "fix" it, but it would be better to
understand and fix the underlying race.
Russ Cox [Fri, 6 Jan 2017 05:54:24 +0000 (00:54 -0500)]
[release-branch.go1.7] runtime: fix corruption crash/race between select and stack growth
Fixes #18708 (backport)
To implement the blocking of a select, a goroutine builds a list of
offers to communicate (pseudo-g's, aka sudog), one for each case,
queues them on the corresponding channels, and waits for another
goroutine to complete one of those cases and wake it up. Obviously it
is not OK for two other goroutines to complete multiple cases and both
wake the goroutine blocked in select. To make sure that only one
branch of the select is chosen, all the sudogs contain a pointer to a
shared (single) 'done uint32', which is atomically cas'ed by any
interested goroutines. The goroutine that wins the cas race gets to
wake up the select. A complication is that 'done uint32' is stored on
the stack of the goroutine running the select, and that stack can move
during the select due to stack growth or stack shrinking.
The relevant ordering to block and unblock in select is:
1. Lock all channels.
2. Create list of sudogs and queue sudogs on all channels.
3. Switch to system stack, mark goroutine as asleep,
unlock all channels.
4. Sleep until woken.
5. Wake up on goroutine stack.
6. Lock all channels.
7. Dequeue sudogs from all channels.
8. Free list of sudogs.
9. Unlock all channels.
There are two kinds of stack moves: stack growth and stack shrinking.
Stack growth happens while the original goroutine is running.
Stack shrinking happens asynchronously, during garbage collection.
While a channel listing a sudog is locked by select in this process,
no other goroutine can attempt to complete communication on that
channel, because that other goroutine doesn't hold the lock and can't
find the sudog. If the stack moves while all the channel locks are
held or when the sudogs are not yet or no longer queued in the
channels, no problem, because no goroutine can get to the sudogs and
therefore to selectdone. We only need to worry about the stack (and
'done uint32') moving with the sudogs queued in unlocked channels.
Stack shrinking can happen any time the goroutine is stopped.
That code already acquires all the channel locks before doing the
stack move, so it avoids this problem.
Stack growth can happen essentially any time the original goroutine is
running on its own stack (not the system stack). In the first half of
the select, all the channels are locked before any sudogs are queued,
and the channels are not unlocked until the goroutine has stopped
executing on its own stack and is asleep, so that part is OK. In the
second half of the select, the goroutine wakes up on its own goroutine
stack and immediately locks all channels. But the actual call to lock
might grow the stack, before acquiring any locks. In that case, the
stack is moving with the sudogs queued in unlocked channels. Not good.
One goroutine has already won a cas on the old stack (that goroutine
woke up the selecting goroutine, moving it out of step 4), and the
fact that done = 1 now should prevent any other goroutines from
completing any other select cases. During the stack move, however,
sudog.selectdone is moved from pointing to the old done variable on
the old stack to a new memory location on the new stack. Another
goroutine might observe the moved pointer before the new memory
location has been initialized. If the new memory word happens to be
zero, that goroutine might win a cas on the new location, thinking it
can now complete the select (again). It will then complete a second
communication (reading from or writing to the goroutine stack
incorrectly) and then attempt to wake up the selecting goroutine,
which is already awake.
The scribbling over the goroutine stack unexpectedly is already bad,
but likely to go unnoticed, at least immediately. As for the second
wakeup, there are a variety of ways it might play out.
* The goroutine might not be asleep.
That will produce a runtime crash (throw) like in #17007:
runtime: gp: gp=0xc0422dcb60, goid=2299, gp->atomicstatus=8
runtime: g: g=0xa5cfe0, goid=0, g->atomicstatus=0
fatal error: bad g->status in ready
Here, atomicstatus=8 is copystack; the second, incorrect wakeup is
observing that the selecting goroutine is in state "Gcopystack"
instead of "Gwaiting".
* The goroutine might be sleeping in a send on a nil chan.
If it wakes up, it will crash with 'fatal error: unreachable'.
* The goroutine might be sleeping in a send on a non-nil chan.
If it wakes up, it will crash with 'fatal error: chansend:
spurious wakeup'.
* The goroutine might be sleeping in a receive on a nil chan.
If it wakes up, it will crash with 'fatal error: unreachable'.
* The goroutine might be sleeping in a receive on a non-nil chan.
If it wakes up, it will silently (incorrectly!) continue as if it
received a zero value from a closed channel, leaving a sudog queued on
the channel pointing at that zero vaue on the goroutine's stack; that
space will be reused as the goroutine executes, and when some other
goroutine finally completes the receive, it will do a stray write into
the goroutine's stack memory, which may cause problems. Then it will
attempt the real wakeup of the goroutine, leading recursively to any
of the cases in this list.
* The goroutine might have been running a select in a finalizer
(I hope not!) and might now be sleeping waiting for more things to
finalize. If it wakes up, as long as it goes back to sleep quickly
(before the real GC code tries to wake it), the spurious wakeup does
no harm (but the stack was still scribbled on).
* The goroutine might be sleeping in gcParkAssist.
If it wakes up, that will let the goroutine continue executing a bit
earlier than we would have liked. Eventually the GC will attempt the
real wakeup of the goroutine, leading recursively to any of the cases
in this list.
* The goroutine cannot be sleeping in bgsweep, because the background
sweepers never use select.
* The goroutine might be sleeping in netpollblock.
If it wakes up, it will crash with 'fatal error: netpollblock:
corrupted state'.
* The goroutine might be sleeping in main as another thread crashes.
If it wakes up, it will exit(0) instead of letting the other thread
crash with a non-zero exit status.
* The goroutine cannot be sleeping in forcegchelper,
because forcegchelper never uses select.
* The goroutine might be sleeping in an empty select - select {}.
If it wakes up, it will return to the next line in the program!
* The goroutine might be sleeping in a non-empty select (again).
In this case, it will wake up spuriously, with gp.param == nil (no
reason for wakeup), but that was fortuitously overloaded for handling
wakeup due to a closing channel and the way it is handled is to rerun
the select, which (accidentally) handles the spurious wakeup
correctly:
if cas == nil {
// This can happen if we were woken up by a close().
// TODO: figure that out explicitly so we don't need this loop.
goto loop
}
Before looping, it will dequeue all the sudogs on all the channels
involved, so that no other goroutine will attempt to wake it.
Since the goroutine was blocked in select before, being blocked in
select again when the spurious wakeup arrives may be quite likely.
In this case, the spurious wakeup does no harm (but the stack was
still scribbled on).
* The goroutine might be sleeping in semacquire (mutex slow path).
If it wakes up, that is taken as a signal to try for the semaphore
again, not a signal that the semaphore is now held, but the next
iteration around the loop will queue the sudog a second time, causing
a cycle in the wakeup list for the given address. If that sudog is the
only one in the list, when it is eventually dequeued, it will
(due to the precise way the code is written) leave the sudog on the
queue inactive with the sudog broken. But the sudog will also be in
the free list, and that will eventually cause confusion.
* The goroutine might be sleeping in notifyListWait, for sync.Cond.
If it wakes up, (*Cond).Wait returns. The docs say "Unlike in other
systems, Wait cannot return unless awoken by Broadcast or Signal,"
so the spurious wakeup is incorrect behavior, but most callers do not
depend on that fact. Eventually the condition will happen, attempting
the real wakeup of the goroutine and leading recursively to any of the
cases in this list.
* The goroutine might be sleeping in timeSleep aka time.Sleep.
If it wakes up, it will continue running, leaving a timer ticking.
When that time bomb goes off, it will try to ready the goroutine
again, leading to any one of the cases in this list.
* The goroutine cannot be sleeping in timerproc,
because timerproc never uses select.
* The goroutine might be sleeping in ReadTrace.
If it wakes up, it will print 'runtime: spurious wakeup of trace
reader' and return nil. All future calls to ReadTrace will print
'runtime: ReadTrace called from multiple goroutines simultaneously'.
Eventually, when trace data is available, a true wakeup will be
attempted, leading to any one of the cases in this list.
None of these fatal errors appear in any of the trybot or dashboard
logs. The 'bad g->status in ready' that happens if the goroutine is
running (the most likely scenario anyway) has happened once on the
dashboard and eight times in trybot logs. Of the eight, five were
atomicstatus=8 during net/http tests, so almost certainly this bug.
The other three were atomicstatus=2, all near code in select,
but in a draft CL by Dmitry that was rewriting select and may or may
not have had its own bugs.
This bug has existed since Go 1.4. Until then the select code was
implemented in C, 'done uint32' was a C stack variable 'uint32 done',
and C stacks never moved. I believe it has become more common recently
because of Brad's work to run more and more tests in net/http in
parallel, which lengthens race windows.
The fix is to run step 6 on the system stack,
avoiding possibility of stack growth.
Fixes #17007 and possibly other mysterious failures.
Quentin Smith [Wed, 30 Nov 2016 20:16:37 +0000 (15:16 -0500)]
[release-branch.go1.7] crypto/x509: read Darwin trust settings for root CAs
Darwin separately stores bits indicating whether a root certificate
should be trusted; this changes Go to read and use those when
initializing SystemCertPool.
Unfortunately, the trust API is very slow. To avoid a delay of up to
0.5s in initializing the system cert pool, we assume that
the trust settings found in kSecTrustSettingsDomainSystem will always
indicate trust. (That is, all root certs Apple distributes are trusted.)
This is not guaranteed by the API but is true in practice.
In the non-cgo codepath, we do not have that benefit, so we must check
the trust status of every certificate. This causes about 0.5s of delay
in initializing the SystemCertPool.
On OS X 10.11 and older, the "security" command requires a certificate
to be provided in a file and not on stdin, so the non-cgo codepath
creates temporary files for each certificate, further slowing initialization.
David Chase [Sat, 8 Oct 2016 20:45:58 +0000 (16:45 -0400)]
[release-branch.go1.7] cmd/compile: escape analysis needs to run "flood" to fixed point
In some cases the members of the root set from which flood
runs themselves escape, without their referents being also
tagged as escaping. Fix this by reflooding from those roots
whose escape increases, and also enhance the "leak" test to
include reachability from a heap-escaped root.
Mike Appleby [Wed, 28 Sep 2016 21:01:27 +0000 (16:01 -0500)]
[release-branch.go1.7] runtime: sleep on CLOCK_MONOTONIC in futexsleep1 on freebsd
In FreeBSD 10.0, the _umtx_op syscall was changed to allow sleeping on
any supported clock, but the default clock was switched from a monotonic
clock to CLOCK_REALTIME.
Prior to 10.0, the __umtx_op_wait* functions ignored the fourth argument
to _umtx_op (uaddr1), expected the fifth argument (uaddr2) to be a
struct timespec pointer, and used a monotonic clock (nanouptime(9)) for
timeout calculations.
Since 10.0, if callers want a clock other than CLOCK_REALTIME, they must
call _umtx_op with uaddr1 set to a value greater than sizeof(struct
timespec), and with uaddr2 as pointer to a struct _umtx_time, rather
than a timespec. Callers can set the _clockid field of the struct
_umtx_time to request the clock they want.
The relevant FreeBSD commit:
https://svnweb.freebsd.org/base?view=revision&revision=232144
Fixes #17168
Change-Id: I3dd7b32b683622b8d7b4a6a8f9eb56401bed6bdf
Reviewed-on: https://go-review.googlesource.com/30154 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/31269
Adam Langley [Wed, 14 Sep 2016 18:50:36 +0000 (11:50 -0700)]
[release-branch.go1.7] crypto/tls: fix deadlock when racing to complete handshake.
After renegotiation support was added (af125a5193c) it's possible for a
Write to block on a Read when racing to complete the handshake:
1. The Write determines that a handshake is needed and tries to
take the neccesary locks in the correct order.
2. The Read also determines that a handshake is needed and wins
the race to take the locks.
3. The Read goroutine completes the handshake and wins a race
to unlock and relock c.in, which it'll hold when waiting for
more network data.
If the application-level protocol requires the Write to complete before
data can be read then the system as a whole will deadlock.
Unfortunately it doesn't appear possible to reverse the locking order of
c.in and handshakeMutex because we might read a renegotiation request at
any point and need to be able to do a handshake without unlocking.
So this change adds a sync.Cond that indicates that a goroutine has
committed to doing a handshake. Other interested goroutines can wait on
that Cond when needed.
The test for this isn't great. I was able to reproduce the deadlock with
it only when building with -race. (Because -race happened to alter the
timing just enough.)
Fixes #17101.
Change-Id: I4e8757f7b82a84e46c9963a977d089f0fb675495
Reviewed-on: https://go-review.googlesource.com/29164 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/31268
Michael Munday [Thu, 8 Sep 2016 23:27:24 +0000 (19:27 -0400)]
[release-branch.go1.7] runtime: fix SIGILL in checkvectorfacility on s390x
STFLE does not necessarily write to all the double-words that are
requested. It is therefore necessary to clear the target memory
before calling STFLE in order to ensure that the facility list does
not contain false positives.
Austin Clements [Fri, 19 Aug 2016 20:03:14 +0000 (16:03 -0400)]
[release-branch.go1.7] runtime: fix check for vacuous page boundary rounding again
The previous fix for this, commit 336dad2a, had everything right in
the commit message, but reversed the test in the code. Fix the test in
the code.
This reversal effectively disabled the scavenger on large page systems
*except* in the rare cases where this code was originally wrong, which
is why it didn't obviously show up in testing.
Keith Randall [Wed, 7 Sep 2016 17:57:26 +0000 (10:57 -0700)]
[release-branch.go1.7] cmd/compile: compare size in dead store elimination
This CL is a manual backpatch of CL 27320 into the 1.7.1 release branch.
The manual backpatch is required because OpZero changed from having a
size as its AuxInt to having a size+align as its AuxInt (that was to support
the ARM SSA backend). Otherwise the CLs should be identical.
Please review carefully!
Change-Id: I569b759c06d1971c9c62dc5dd589abc7ef7c844a
Reviewed-on: https://go-review.googlesource.com/28670 Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Brad Fitzpatrick [Fri, 19 Aug 2016 23:13:29 +0000 (23:13 +0000)]
[release-branch.go1.7] net/http: fix unwanted HTTP/2 conn Transport crash after IdleConnTimeout
Go 1.7 crashed after Transport.IdleConnTimeout if an HTTP/2 connection
was established but but its caller no longer wanted it. (Assuming the
connection cache was enabled, which it is by default)
Hiroshi Ioka [Fri, 19 Aug 2016 00:37:19 +0000 (09:37 +0900)]
[release-branch.go1.7] path/filepath: handle ".." in normalizing a path on Windows
Current code assumes there are not ".." in the Clean(path).
That's not true. Clean doesn't handle leading "..", so we need to stop
normalization if we see "..".
Fixes #16793
Change-Id: I0a7901bedac17f1210b134d593ebd9f5e8483775
Reviewed-on: https://go-review.googlesource.com/27410 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/28641 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The code wasn't checking to see if the data was still >= 64 bytes
long after aligning it.
Aligning the data is an optimization and we don't actually need
to do it. In fact for smaller sizes it slows things down due to
the overhead of calling the generic function. Therefore for now
I have simply removed the alignment stage. I have also added a
check into the assembly to deliberately trigger a segmentation
fault if the data is too short.
Brad Fitzpatrick [Fri, 19 Aug 2016 02:21:26 +0000 (19:21 -0700)]
[release-branch.go1.7] io: fix infinite loop bug in MultiReader
If an io.Reader returned (non-zero, EOF), MultiReader would yield
bytes forever.
This bug has existed before Go 1 (!!), introduced in the original
MultiReader implementation in https://golang.org/cl/1764043 and also
survived basically the only update to this code since then
(https://golang.org/cl/17873, git rev ccdca832c), which was added in
Go 1.7.
This just bit me when writing a test for some unrelated code.
Joe Tsai [Tue, 16 Aug 2016 23:03:00 +0000 (16:03 -0700)]
[release-branch.go1.7] compress/flate: make huffmanBitWriter errors persistent
For persistent error handling, the methods of huffmanBitWriter have to be
consistent about how they check errors. It must either consistently
check error *before* every operation OR immediately *after* every
operation. Since most of the current logic uses the previous approach,
we apply the same style of error checking to writeBits and all calls
to Write such that they only operate if w.err is already nil going
into them.
The error handling approach is brittle and easily broken by future commits to
the code. In the near future, we should switch the logic to use panic at the
lowest levels and a recover at the edge of the public API to ensure
that errors are always persistent.
Chris Broadfoot [Mon, 8 Aug 2016 18:28:55 +0000 (11:28 -0700)]
all: merge master into release-branch.go1.7
7a62274 net/http: make Transport use new connection if over HTTP/2 concurrency limit 219ca60 doc: fix required OS X version inconsistency for binary downloads 26015b9 runtime: make stack 16-byte aligned for external code in _rt0_amd64_linux_lib 9fde86b runtime, syscall: fix kernel gettimeofday ABI change on iOS 10 3a03e87 os: check for waitid returning ENOSYS 1031675 net/http: update bundled http2 for flow control window adjustment fix da070be syscall: fix Gettimeofday on macOS Sierra f135c32 runtime: initialize hash algs before typemap
Brad Fitzpatrick [Mon, 8 Aug 2016 17:14:01 +0000 (17:14 +0000)]
net/http: make Transport use new connection if over HTTP/2 concurrency limit
The Go HTTP/1 client will make as many new TCP connections as the user requests.
The HTTP/2 client tried to have that behavior, but the policy of
whether a connection is re-usable didn't take into account the extra 1
stream counting against SETTINGS_MAX_CONCURRENT_STREAMS so in practice
users were getting errors.
For example, if the server's advertised max concurrent streams is 100
and 200 concurrrent Go HTTP requests ask for a connection at once, all
200 will think they can reuse that TCP connection, but then 100 will
fail later when the number of concurrent streams exceeds 100.
Instead, recognize the "no cached connections" error value in the
shouldRetryRequest method, so those 100 will retry a new connection.
This is the conservative fix for Go 1.7 so users don't get errors, and
to match the HTTP/1 behavior. Issues #13957 and #13774 are the more
involved bugs for Go 1.8.
Updates #16582
Updates #13957
Change-Id: I1f15a7ce60c07a4baebca87675836d6fe03993e8
Reviewed-on: https://go-review.googlesource.com/25580
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Brad Fitzpatrick [Sat, 6 Aug 2016 17:12:03 +0000 (10:12 -0700)]
doc: fix required OS X version inconsistency for binary downloads
Updates #16625
Change-Id: Icac6705828bd9b29379596ba64b34d922b9002c3
Reviewed-on: https://go-review.googlesource.com/25548 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Shenghou Ma [Fri, 5 Aug 2016 23:16:07 +0000 (19:16 -0400)]
runtime: make stack 16-byte aligned for external code in _rt0_amd64_linux_lib
Fixes #16618.
Change-Id: Iffada12e8672bbdbcf2e787782c497e2c45701b1
Reviewed-on: https://go-review.googlesource.com/25550
Run-TryBot: Minux Ma <minux@golang.org> Reviewed-by: Arjan Van De Ven <arjan.van.de.ven@intel.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
David Crawshaw [Thu, 4 Aug 2016 17:09:29 +0000 (13:09 -0400)]
runtime: initialize hash algs before typemap
When compiling with -buildmode=shared, a map[int32]*_type is created for
each extra module mapping duplicate types back to a canonical object.
This is done in the function typelinksinit, which is called before the
init function that sets up the hash functions for the map
implementation. The result is typemap becomes unusable after
runtime initialization.
The fix in this CL is to move algorithm init before typelinksinit in
the runtime setup process. (For 1.8, we may want to turn typemap into
a sorted slice of types and use binary search.)
Manually tested on GOOS=linux with:
GOHOSTARCH=386 GOARCH=386 ./make.bash && \
go install -buildmode=shared std && \
cd ../test && \
go run run.go -linkshared
Fixes #16590
Change-Id: Idc08c50cc70d20028276fbf564509d2cd5405210
Reviewed-on: https://go-review.googlesource.com/25469
Run-TryBot: David Crawshaw <crawshaw@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Brad Fitzpatrick [Tue, 2 Aug 2016 04:54:40 +0000 (21:54 -0700)]
runtime: fix nanotime for macOS Sierra, again.
macOS Sierra beta4 changed the kernel interface for getting time.
DX now optionally points to an address for additional info.
Set it to zero to avoid corrupting memory.
Fixes #16570
Change-Id: I9f537e552682045325cdbb68b7d0b4ddafade14a
Reviewed-on: https://go-review.googlesource.com/25400 Reviewed-by: David Crawshaw <crawshaw@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Quentin Smith <quentin@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Joe Tsai [Tue, 2 Aug 2016 03:04:25 +0000 (20:04 -0700)]
cmd/doc: ensure functions with unexported return values are shown
The commit in golang.org/cl/22354 groups constructors functions under
the type that they construct to. However, this caused a minor regression
where functions that had unexported return values were not being printed
at all. Thus, we forgo the grouping logic if the type the constructor falls
under is not going to be printed.
Fixes #16568
Change-Id: Idc14f5d03770282a519dc22187646bda676af612
Reviewed-on: https://go-review.googlesource.com/25369
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com> Reviewed-by: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Joe Tsai [Mon, 1 Aug 2016 21:33:19 +0000 (14:33 -0700)]
cmd/doc: handle embedded interfaces properly
Changes made:
* Disallow star expression on interfaces as this is not possible.
* Show an embedded "error" in an interface as public similar to
how godoc does it.
* Properly handle selector expressions in both structs and interfaces.
This is possible since a type may refer to something defined in
another package (e.g. io.Reader).
Before:
<<<
$ go doc runtime.Error
type Error interface {
// RuntimeError is a no-op function but
// serves to distinguish types that are run time
// errors from ordinary errors: a type is a
// run time error if it has a RuntimeError method.
RuntimeError()
// Has unexported methods.
}
$ go doc compress/flate Reader
doc: invalid program: unexpected type for embedded field
doc: invalid program: unexpected type for embedded field
type Reader interface {
io.Reader
io.ByteReader
}
>>>
After:
<<<
$ go doc runtime.Error
type Error interface {
error
// RuntimeError is a no-op function but
// serves to distinguish types that are run time
// errors from ordinary errors: a type is a
// run time error if it has a RuntimeError method.
RuntimeError()
}
$ go doc compress/flate Reader
type Reader interface {
io.Reader
io.ByteReader
}
>>>
Fixes #16567
Change-Id: I272dede971eee9f43173966233eb8810e4a8c907
Reviewed-on: https://go-review.googlesource.com/25365 Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
cmd/compile: fix possible spill of invalid pointer with DUFFZERO on AMD64
SSA compiler on AMD64 may spill Duff-adjusted address as scalar. If
the object is on stack and the stack moves, the spilled address become
invalid.
Making the spill pointer-typed does not work. The Duff-adjusted address
points to the memory before the area to be zeroed and may be invalid.
This may cause stack scanning code panic.
Fix it by doing Duff-adjustment in genValue, so the intermediate value
is not seen by the reg allocator, and will not be spilled.
Add a test to cover both cases. As it depends on allocation, it may
be not always triggered.
doc/go1.7.html: add known issues section for FreeBSD crashes
Updates #16396
Change-Id: I7b4f85610e66f2c77c17cf8898cc41d81b2efc8c
Reviewed-on: https://go-review.googlesource.com/25283 Reviewed-by: Chris Broadfoot <cbro@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Andrew Gerrand <adg@golang.org>
Rhys Hiltner [Fri, 22 Jul 2016 23:36:30 +0000 (16:36 -0700)]
runtime: reduce GC assist extra credit
Mutator goroutines that allocate memory during the concurrent mark
phase are required to spend some time assisting the garbage
collector. The magnitude of this mandatory assistance is proportional
to the goroutine's allocation debt and subject to the assistance
ratio as calculated by the pacer.
When assisting the garbage collector, a mutator goroutine will go
beyond paying off its allocation debt. It will build up extra credit
to amortize the overhead of the assist.
In fast-allocating applications with high assist ratios, building up
this credit can take the affected goroutine's entire time slice.
Reduce the penalty on each goroutine being selected to assist the GC
in two ways, to spread the responsibility more evenly.
First, do a consistent amount of extra scan work without regard for
the pacer's assistance ratio. Second, reduce the magnitude of the
extra scan work so it can be completed within a few hundred
microseconds.
Commentary on gcOverAssistWork is by Austin Clements, originally in
https://golang.org/cl/24704
Currently the pprof package gives almost no guidance for how to use it
and, despite the standard boilerplate used to create CPU and memory
profiles, this boilerplate appears nowhere in the pprof documentation.
Update the pprof package documentation to give the standard
boilerplate in a form people can copy, paste, and tweak. This
boilerplate is based on rsc's 2011 blog post on profiling Go programs
at https://blog.golang.org/profiling-go-programs, which is where I
always go when I need to copy-paste the boilerplate.
Change-Id: I74021e494ea4dcc6b56d6fb5e59829ad4bb7b0be
Reviewed-on: https://go-review.googlesource.com/25182 Reviewed-by: Rick Hudson <rlh@golang.org>
Jack Lindamood [Fri, 15 Jul 2016 20:28:27 +0000 (13:28 -0700)]
context: add test for WithDeadline in the past
Adds a test case for calling context.WithDeadline() where the deadline
exists in the past. This change increases the code coverage of the
context package.
net/http: make Transport.RoundTrip return raw Conn.Read error on peek failure
From at least Go 1.4 to Go 1.6, Transport.RoundTrip would return the
error value from net.Conn.Read directly when the initial Read (1 byte
Peek) failed while reading the HTTP response, if a request was
outstanding. While never a documented or tested promise, Go 1.7 changed the
behavior (starting at https://golang.org/cl/23160).
This restores the old behavior and adds a test (but no documentation
promises yet) while keeping the fix for spammy logging reported in #15446.
This looks larger than it is: it just changes errServerClosedConn from
a variable to a type, where the type preserves the underlying
net.Conn.Read error, for unwrapping later in Transport.RoundTrip.
Fixes #16465
Change-Id: I6fa018991221e93c0cfe3e4129cb168fbd98bd27
Reviewed-on: https://go-review.googlesource.com/25153 Reviewed-by: Andrew Gerrand <adg@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
It was removed in upstream Chrome https://codereview.chromium.org/2016863004
Rather than update to the latest version, make the minimal change for Go 1.7 and
change the "showToUser" boolean from true to false.
Tested by hand that it goes away after this change.
Updates #16247
Change-Id: I051f49da878e554b1a34a88e9abc70ab50e18780
Reviewed-on: https://go-review.googlesource.com/25117 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
David Chase [Wed, 20 Jul 2016 14:44:49 +0000 (10:44 -0400)]
cmd/compile: change phi location to be optimistic at backedges
This is:
(1) a simple trick that cuts the number of phi-nodes
(temporarily) inserted into the ssa representation by a factor
of 10, and can cut the user time to compile tricky inputs like
gogo/protobuf tests from 13 user minutes to 9.5, and memory
allocation from 3.4GB to 2.4GB.
(2) a fix to sparse lookup, that does not rely on
an assumption proven false by at least one pathological
input "etldlen".
These two changes fix unrelated compiler performance bugs,
both necessary to obtain good performance compiling etldlen.
Without them it takes 20 minutes or longer, with them it
completes in 2 minutes, without a gigantic memory footprint.
Updates #16407
Change-Id: Iaa8aaa8c706858b3d49de1c4865a7fd79e6f4ff7
Reviewed-on: https://go-review.googlesource.com/23136 Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Keith Randall [Tue, 19 Jul 2016 06:06:04 +0000 (23:06 -0700)]
cmd/compile: move phi args which are constants closer to the phi
entry:
x = MOVQconst [7]
...
b1:
goto b2
b2:
v = Phi(x, y, z)
Transform that program to:
entry:
...
b1:
x = MOVQconst [7]
goto b2
b2:
v = Phi(x, y, z)
This CL moves constant-generating instructions used by a phi to the
appropriate immediate predecessor of the phi's block.
We used to put all constants in the entry block. Unfortunately, in
large functions we have lots of constants at the start of the
function, all of which are used by lots of phis throughout the
function. This leads to the constants being live through most of the
function (especially if there is an outer loop). That's an O(n^2)
problem.
Note that most of the non-phi uses of constants have already been
folded into instructions (ADDQconst, MOVQstoreconst, etc.).
This CL may be generally useful for other instances of compiler
slowness, I'll have to check. It may cause some programs to run
slower, but probably not by much, as rematerializeable values like
these constants are allocated late (not at their originally scheduled
location) anyway.
This CL is definitely a minimal change that can be considered for 1.7.
We probably want to do a better job in the tighten pass generally, not
just for phi args. Leaving that for 1.8.
Ian Lance Taylor [Wed, 20 Jul 2016 22:40:10 +0000 (15:40 -0700)]
runtime: add explicit `INT $3` at end of Darwin amd64 sigtramp
The omission of this instruction could confuse the traceback code if a
SIGPROF occurred during a signal handler. The traceback code would
trace up to sigtramp, but would then get confused because it would see a
PC address that did not appear to be in the function.
runtime: support smaller physical pages than PhysPageSize
Most operations need an upper bound on the physical page size, which
is what sys.PhysPageSize is for (this is checked at runtime init on
Linux). However, a few operations need a *lower* bound on the physical
page size. Introduce a "minPhysPageSize" constant to act as this lower
bound and use it where it makes sense:
1) In addrspace_free, we have to query each page in the given range.
Currently we increment by the upper bound on the physical page
size, which means we may skip over pages if the true size is
smaller. Worse, we currently pass a result buffer that only has
enough room for one page. If there are actually multiple pages in
the range passed to mincore, the kernel will overflow this buffer.
Fix these problems by incrementing by the lower-bound on the
physical page size and by passing "1" for the length, which the
kernel will round up to the true physical page size.
2) In the write barrier, the bad pointer check tests for pointers to
the first physical page, which are presumably small integers
masquerading as pointers. However, if physical pages are smaller
than we think, we may have legitimate pointers below
sys.PhysPageSize. Hence, use minPhysPageSize for this test since
pointers should never fall below that.
In particular, this applies to ARM64 and MIPS. The runtime is
configured to use 64kB pages on ARM64, but by default Linux uses 4kB
pages. Similarly, the runtime assumes 16kB pages on MIPS, but both 4kB
and 16kB kernel configurations are common. This also applies to ARM on
systems where the runtime is recompiled to deal with a larger page
size. It is also a step toward making the runtime use only a
dynamically-queried page size.
Change-Id: I1fdfd18f6e7cbca170cc100354b9faa22fde8a69
Reviewed-on: https://go-review.googlesource.com/25020 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Austin Clements <austin@google.com>
The leak was reported internally on a sever canary that runs for days.
After a day server consumes 5.6GB, after 6 days -- 12.2GB.
The leak is exposed by the added benchmark.
The leak is fixed upstream in :
http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_thread.cc?view=diff&r1=276102&r2=276103&pathrev=276103
Fixes #16441
Change-Id: I9d4f0adef48ca6cf2cd781b9a6990ad4661ba49b
Reviewed-on: https://go-review.googlesource.com/25091 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Run-TryBot: Dmitry Vyukov <dvyukov@google.com>
Ian Lance Taylor [Tue, 19 Jul 2016 06:00:43 +0000 (23:00 -0700)]
runtime: add as many extra M's as needed
When a non-Go thread calls into Go, the runtime needs an M to run the Go
code. The runtime keeps a list of extra M's available. When the last
extra M is allocated, the needextram field is set to tell it to allocate
a new extra M as soon as it is running in Go. This ensures that an extra
M will always be available for the next thread.
However, if many threads need an extra M at the same time, this
serializes them all. One thread will get an extra M with the needextram
field set. All the other threads will see that there is no M available
and will go to sleep. The one thread that succeeded will create a new
extra M. One lucky thread will get it. All the other threads will see
that there is no M available and will go to sleep. The effect is
thundering herd, as all the threads looking for an extra M go through
the process one by one. This seems to have a particularly bad effect on
the FreeBSD scheduler for some reason.
With this change, we track the number of threads waiting for an M, and
create all of them as soon as one thread gets through. This still means
that all the threads will fight for the lock to pick up the next M. But
at least each thread that gets the lock will succeed, instead of going
to sleep only to fight again.
This smooths out the performance greatly on FreeBSD, reducing the
average wall time of `testprogcgo CgoCallbackGC` by 74%. On GNU/Linux
the average wall time goes down by 9%.
Fixes #13926
Fixes #16396
Change-Id: I6dc42a4156085a7ed4e5334c60b39db8f8ef8fea
Reviewed-on: https://go-review.googlesource.com/25047
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
net/smtp: document that the smtp package is frozen
This copies the frozen wording from the log/syslog package.
Fixes #16436
Change-Id: If5d478023328925299399f228d8aaf7fb117c1b4
Reviewed-on: https://go-review.googlesource.com/25080 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Gerrand <adg@golang.org>
[release-branch.go1.7] net/http, net/http/cgi: fix for CGI + HTTP_PROXY security issue
Because,
* The CGI spec defines that incoming request header "Foo: Bar" maps to
environment variable HTTP_FOO == "Bar". (see RFC 3875 4.1.18)
* The HTTP_PROXY environment variable is conventionally used to configure
the HTTP proxy for HTTP clients (and is respected by default for
Go's net/http.Client and Transport)
That means Go programs running in a CGI environment (as a child
process under a CGI host) are vulnerable to an incoming request
containing "Proxy: attacker.com:1234", setting HTTP_PROXY, and
changing where Go by default proxies all outbound HTTP requests.
This is CVE-2016-5386, aka https://httpoxy.org/
Fixes #16405
Change-Id: I6f68ade85421b4807785799f6d98a8b077e871f0
Reviewed-on: https://go-review.googlesource.com/25010
Run-TryBot: Chris Broadfoot <cbro@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-on: https://go-review.googlesource.com/25013