Robert Griesemer [Thu, 11 Sep 2014 19:57:51 +0000 (12:57 -0700)]
go/printer, gofmt: don't align map entries for irregular inputs
Details: Until now, when we saw a key:value pair that fit onto
a single line, we assumed that it should be formatted with a
vtab after the ':' for alignment of its value. This leads to
odd behavior if there are more than one such pair on a line.
This CL changes the behavior such that alignment is only used
for the first pair on a line. This preserves existing behavior
(in the std lib we have composite literals where the last line
contains multiple entries and the first entry's value is aligned
with the values on previous lines), and resolves this issue.
No impact on formatting of std lib, go.tools, go.exp, go.net.
A write *p = x that needs a write barrier (not all do)
now turns into runtime.writebarrierptr(p, x)
or one of the other variants.
The write barrier implementations are trivial.
The goal here is to emit the calls in the correct places
and to incur the cost of those function calls in the Go 1.4 cycle.
Performance on the Go 1 benchmark suite below.
Remember, the goal is to slow things down (and be correct).
We will look into optimizations in separate CLs, as part of
the process of comparing Go 1.3 against tip in order to make
sure Go 1.4 runs at least as fast as Go 1.3.
net: fix inconsistent behavior across platforms in SetKeepAlivePeriod
The previous implementation used per-socket TCP keepalive options
wrong. For example, it used another level socket option to control
TCP and it didn't use TCP_KEEPINTVL option when possible.
Matthew Dempsky [Wed, 10 Sep 2014 00:41:48 +0000 (17:41 -0700)]
runtime: cleanup openbsd semasleep implementation
The previous implementation had several subtle issues. It's not
clear if any of these could actually be causing the flakiness
problems on openbsd/386, but fixing them should only help.
1. thrsleep() is implemented internally as unlock, then test *abort
(if abort != nil), then tsleep(). Under the current code, that makes
it theoretically possible that semasleep()/thrsleep() could release
waitsemalock, then a racing semawakeup() could acquire the lock,
increment waitsemacount, and call thrwakeup()/wakeup() before
thrsleep() reaches tsleep(). (In practice, OpenBSD's big kernel lock
seems unlikely to let this actually happen.)
The proper way to avoid this is to pass &waitsemacount as the abort
pointer to thrsleep so thrsleep knows to re-check it before going to
sleep, and to wakeup if it's non-zero. Then we avoid any races.
(I actually suspect openbsd's sema{sleep,wakeup}() could be further
simplified using cas/xadd instead of locks, but I don't want to be
more intrusive than necessary so late in the 1.4 release cycle.)
2. semasleep() takes a relative sleep duration, but thrsleep() needs
an absolute sleep deadline. Instead of recomputing the deadline each
iteration, compute it once up front and use (*Timespec)(nil) to signify
no deadline. Ensures we retry properly if there's a spurious wakeup.
3. Instead of assuming if thrsleep() woke up and waitsemacount wasn't
available that we must have hit the deadline, check that the system
call returned EWOULDBLOCK.
4. Instead of assuming that 64-bit systems are little-endian, compute
timediv() using a temporary int32 nsec and then assign it to tv_nsec.
Anthony Martin [Wed, 10 Sep 2014 00:19:01 +0000 (17:19 -0700)]
runtime: call rfork on scheduler stack on Plan 9
A race exists between the parent and child processes after a fork.
The child needs to access the new M pointer passed as an argument
but the parent may have already returned and clobbered it.
Previously, we avoided this by saving the necessary data into
registers before the rfork system call but this isn't guaranteed
to work because Plan 9 makes no promises about the register state
after a system call. Only the 386 kernel seems to save them.
For amd64 and arm, this method won't work.
We eliminate the race by allocating stack space for the scheduler
goroutines (g0) in the per-process copy-on-write stack segment and
by only calling rfork on the scheduler stack.
The only thing I can see that is really Plan 9-specific
is that the stack pointer used for signal handling used
to have more mapped memory above it.
Specifically it used to have at most 88 bytes (StackTop),
so change the allocation of a 40-byte frame to a 128-byte frame.
No idea if this will work, but worth a try.
Note that "fix" here means get it back to timing out
instead of crashing.
The difference between the old and the new (from earlier) code
is that we set stackguard = stack.lo + StackGuard, while the old
code set stackguard = stack.lo. That 512 bytes appears to be
the difference between the profileloop function running and not running.
We don't know how big the system stack is, but it is likely MUCH bigger than 4k.
Give Go/C 8k.
Start the stack a few words below the actual top, so that
if something tries to read goexit's caller PC from the stack,
it won't fault on a bad memory address.
Today, heapdump does that.
Maybe tomorrow, traceback or something else will do that.
Make it not a bug.
Rob Pike [Tue, 9 Sep 2014 19:31:07 +0000 (12:31 -0700)]
testing: read coverage counters atomically
For -mode=atomic, we need to read the counters
using an atomic load to avoid a race. Not worth worrying
about when -mode=atomic is set during generation
of the profile, so we use atomic loads always.
We're carrying around a surprising amount of cruft from older schemes.
I am confident that precise stack scans and stack copying are here to stay.
Delete fallback code for when precise stack info is disabled.
Delete fallback code for when copying stacks is disabled.
Delete fallback code for when StackCopyAlways is disabled.
Delete Stktop chain - there is only one stack segment now.
Delete M.moreargp, M.moreargsize, M.moreframesize, M.cret.
Delete G.writenbuf (unrelated, just dead).
Delete runtime.lessstack, runtime.oldstack.
Delete many amd64 morestack variants.
Delete initialization of morestack frame/arg sizes (shortens split prologue!).
Replace G's stackguard/stackbase/stack0/stacksize/
syscallstack/syscallguard/forkstackguard with simple stack
bounds (lo, hi).
Update liblink, runtime/cgo for adjustments to G.
LGTM=khr
R=khr, bradfitz
CC=golang-codereviews, iant, r
https://golang.org/cl/137410043
I assumed they were the same when I wrote
cgocallback.go earlier today. Merge them
to eliminate confusion.
I can't tell what gomallocgc did before with
a nil type but without FlagNoScan.
I created a call like that in cgocallback.go
this morning, translating from a C file.
It was supposed to do what the C version did,
namely treat the block conservatively.
Now it will.
runtime: let stack copier update Panic structs for us
It already is updating parts of them; we're just getting lucky
retraversing them and not finding much to do.
Change argp to a pointer so that it will be updated too.
Existing tests break if you apply the change to adjustpanics
without also updating the type of argp.
Robert Griesemer [Mon, 8 Sep 2014 21:54:00 +0000 (14:54 -0700)]
go/parser: fix (pathological) corner case
Inside a control clause (if ... {}), composite
literals starting with a type name must be parenthesized.
A composite literal used in the array length expression
of an array composite literal is already parenthesized.
Not a valid program, but syntactically is should
be accepted.
This should make deferreturn nosplit all the way down,
which should fix the current windows/amd64 failure.
If not, I will change StackCopyAlways back to 0.
syscall: keep allocated C string live across call to Syscall
Given:
p := alloc()
fn_taking_ptr(p)
p is NOT recorded as live at the call to fn_taking_ptr:
it's not needed by the code following the call.
p was passed to fn_taking_ptr, and fn_taking_ptr must keep
it alive as long as it needs it.
In practice, fn_taking_ptr will keep its own arguments live
for as long as the function is executing.
But if instead you have:
p := alloc()
i := uintptr(unsafe.Pointer(p))
fn_taking_int(i)
p is STILL NOT recorded as live at the call to fn_taking_int:
it's not needed by the code following the call.
fn_taking_int is responsible for keeping its own arguments
live, but fn_taking_int is written to take an integer, so even
though fn_taking_int does keep its argument live, that argument
does not keep the allocated memory live, because the garbage
collector does not dereference integers.
The shorter form:
p := alloc()
fn_taking_int(uintptr(unsafe.Pointer(p)))
and the even shorter form:
fn_taking_int(uintptr(unsafe.Pointer(alloc())))
are both the same as the 3-line form above.
syscall.Syscall is like fn_taking_int: it is written to take a list
of integers, and yet those integers are sometimes pointers.
If there is no other copy of those pointers being kept live,
the memory they point at may be garbage collected during
the call to syscall.Syscall.
This is happening on Solaris: for whatever reason, the timing
is such that the garbage collector manages to free the string
argument to the open(2) system call before the system call
has been invoked.
Change the system call wrappers to insert explicit references
that will keep the allocations alive in the original frame
(and therefore preserve the memory) until after syscall.Syscall
has returned.
Should fix Solaris flakiness.
This is not a problem for cgo, because cgo wrappers have
correctly typed arguments.
The sighander has been run at the bottom of the
currently executing goroutine stack, but it's in C,
and we don't want C on our ordinary goroutine stacks.
Worse, it does a lot of stuff, and it might need more
stack space. There is scary code in traceback_windows.go
that talks about stack splits during sighandler.
Moving sighandler to g0 will eliminate the possibility
of stack splits and such, and then we can delete
traceback_windows.go entirely. Win win.
On the builder, all.bat passes with GOARCH=amd64
and all.bat gets most of the way with GOARCH=386
except for a DLL-loading test that I think is unrelated.
liblink, runtime: diagnose and fix C code running on Go stack
This CL contains compiler+runtime changes that detect C code
running on Go (not g0, not gsignal) stacks, and it contains
corrections for what it detected.
The detection works by changing the C prologue to use a different
stack guard word in the G than Go prologue does. On the g0 and
gsignal stacks, that stack guard word is set to the usual
stack guard value. But on ordinary Go stacks, that stack
guard word is set to ^0, which will make any stack split
check fail. The C prologue then calls morestackc instead
of morestack, and morestackc aborts the program with
a message about running C code on a Go stack.
This check catches all C code running on the Go stack
except NOSPLIT code. The NOSPLIT code is allowed,
so the check is complete. Since it is a dynamic check,
the code must execute to be caught. But unlike the static
checks we've been using in cmd/ld, the dynamic check
works with function pointers and other indirect calls.
For example it caught sigpanic being pushed onto Go
stacks in the signal handlers.
Fixes #8667.
LGTM=khr, iant
R=golang-codereviews, khr, iant
CC=golang-codereviews, r
https://golang.org/cl/133700043
Dave Cheney [Mon, 8 Sep 2014 06:06:41 +0000 (16:06 +1000)]
cmd/cc: fix undefined behaviour warning in bv.c
Fixes warning
# _/home/dfc/go/misc/cgo/test/backdoor
/home/dfc/go/src/cmd/cc/bv.c:43:11: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
Dave Cheney [Mon, 8 Sep 2014 05:36:21 +0000 (15:36 +1000)]
cmd/gc: fix undefined behaviour warning in subr.c
Fixes warning
/home/dfc/go/src/cmd/gc/subr.c:3469:8: runtime error: negation of -9223372036854775808 cannot be represented in type 'int64' (aka 'long'); cast to an unsigned type to negate this value to itself
This CL adjusts code referring to src/pkg to refer to src.
Immediately after submitting this CL, I will submit
a change doing 'hg mv src/pkg/* src'.
That change will be too large to review with Rietveld
but will contain only the 'hg mv'.
This CL will break the build.
The followup 'hg mv' will fix it.
For more about the move, see golang.org/s/go14nopkg.
runtime: implement time.now in assembly on plan9, solaris, windows
These all used a C implementation that contained 64-bit divide by 1000000000.
On 32-bit systems that ends up in the 64-bit C divide support, which makes
other calls and ends up using a fair amount of stack. We could convert them
to Go but then they'd still end up in software 64-bit divide code. That would
be okay, because Go code can split the stack, but it's still unnecessary.
Write time·now in assembly, just like on all the other systems, and use the
actual hardware support for 64/32 -> 64/32 division. This cuts the software
routines out entirely.
The actual code to do the division is copied and pasted from the sys_darwin_*.s files.
LGTM=alex.brainman
R=golang-codereviews, alex.brainman
CC=aram, golang-codereviews, iant, khr, r
https://golang.org/cl/136300043
panic: httptest: failed to listen on a port: listen tcp 127.0.0.1:0:
listen: An operation on a socket could not be performed because the
system lacked sufficient buffer space or because a queue was full.
Since we can't seem to understand what the test is trying to test,
and because it is causing problems on multiple systems,
delete it.
This is one of those "how did this ever work?" bugs.
The current build failures are happening because
a fault comes up while executing on m->curg on a
system-created thread using an m obtained from needm,
but TLS is set to m->g0, not m->curg. On fault,
sigtramp starts executing, assumes r10 (g) might be
incorrect, reloads it from TLS, and gets m->g0, not
m->curg. Then sighandler dutifully pushes a call to
sigpanic onto the stack and returns to it.
We're now executing on the m->curg stack but with
g=m->g0. Sigpanic does a stack split check, sees that
the SP is not in range (50% chance depending on relative
ordering of m->g0's and m->curg's stacks), and then
calls morestack. Morestack sees that g=m->g0 and
crashes the program.
The fix is to replace every change of g in asm_arm.s
with a call to a function that both updates g and
saves the updated g to TLS.
Why did it start happening? That's unclear.
Unfortunately there were other bugs in the initial
checkin that mask exactly which of a sequence of
CLs started the behavior where sigpanic would end
up tripping the stack split.
Fixes arm build.
Fixes #8675.
LGTM=iant
R=golang-codereviews, iant
CC=dave, golang-codereviews, khr, minux, r
https://golang.org/cl/135570043
After the three pending CLs listed below, there will be no more .goc files.
134580043 runtime: move stubs.goc code into runtime.c 133670043 runtime: fix windows syscalls for copying stacks 141180043 runtime: eliminate Go -> C -> block paths for Solaris
LGTM=bradfitz
R=golang-codereviews, bradfitz, dave
CC=golang-codereviews, iant, r
https://golang.org/cl/132680043
Syscall and everything it calls must be nosplit:
we cannot split a stack once Syscall has been invoked,
because we don't know which of its arguments are
pointers.
Increase NOSPLIT reservation from 192 to 384 bytes.
The problem is that the non-Unix systems (Solaris and Windows)
just can't make system calls in a small amount of space,
and then worse they do things that are complex enough
to warrant calling runtime.throw on failure.
We don't have time to rewrite the code to use less stack.
I'm not happy about this, but it's still a small amount.
The good news is that we're doing this to get to only
using copying stacks for stack growth. Once that is true,
we can drop the default stack size from 8k to 4k, which
should more than make up for the bytes we're losing here.
The gp->panicwrap adjustment is just fatally flawed.
Now that there is a Panic.argp field, update that instead.
That can be done on entry only, so that unwinding doesn't
need to worry about undoing anything. The wrappers
emit a few more instructions in the prologue but everything
else in the system gets much simpler.
It also fixes (without trying) a broken test I never checked in.
Fixes #7491.
LGTM=khr
R=khr
CC=dvyukov, golang-codereviews, iant, r
https://golang.org/cl/135490044
If there is doubt about passing arguments correctly
(as there is in this test), there should be doubt about
getting the results back intact too. Using 0 and 1
(especially 0 for success) makes it easy to get a PASS
accidentally when the return value is not actually
being propagated. Use less common values.
LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews, r
https://golang.org/cl/141110043
runtime: handle nil ptr load/store in arm software floating point
We cannot let a real panic start there, because there is C code
on the stack, and worse, there is an assembly frame with a
saved copy of the registers and we have no idea which ones
are pointers.
Instead, detect the nil ptr load/store and return out of the C
and assembly into a stub that will start the call to sigpanic.
Fixes GOARM=5 build.
LGTM=iant
R=golang-codereviews, iant
CC=dave, golang-codereviews, minux, r
https://golang.org/cl/138130043
sigprof and setcpuprofilerate coordinate the enabling/disabling
of the handler using a Mutex. This has always been a bit dodgy:
setcpuprofilerate must be careful to turn off signals before acquiring
the lock to avoid a deadlock.
Now the lock implementations use onM, and onM isn't okay on the
signal stack. We know how to make it okay, but it's more work than
is probably worth doing.
Since this is super-dodgy anyway, replace the lock with a simple
cas loop. It is only contended if setcpuprofilerate is being called,
and that doesn't happen frequently enough to care about the
raw speed or about using futexes/semaphores.
TBR to fix freebsd/amd64 and dragonfly/amd64 builds.
Happy to make changes in a follow-up CL.
The general kernel system call interface
takes 6 arguments: R0, R1, R2, R3, R4, R5.
Syscall is for calls that only need 3.
The amd64 and 386 versions zero the extra arg registers,
but the arm version does not.
func utimensat calls Syscall with 3 arguments.
The kernel expects a 4th argument.
That turns out to be whatever is in R3 at the time of the call.
CL 137160043 changed various pieces of code and apparently
changed the value left in R3 at the time of utimensat's Syscall.
This causes the kernel to return EINVAL.
Change linux/arm Syscall to zero R3, R4, R5, so that calls will
behave deterministically, even if they pass too few arguments.
Arguably, utimensat could be fixed too, but the predictable
zeroing is certainly worth doing, and once done utimensat's
use of Syscall is fine.
1. If onM is called on a g0 stack, it just calls the given function.
2. If onM is called on a gsignal stack, it calls badonm.
3. If onM is called on a curg stack, it switches to the g0 stack
and then calls the function.
In cases 1 and 2, if the program then crashes (and badonm always does),
we want to see what called onM, but the traceback stops at onM.
In case 3, the traceback must stop at onM, because the g0
stack we are renting really does stop at onM.
The current code stops the traceback at onM to handle 3,
at the cost of making 1 and 2 crash with incomplete traces.
Change traceback to scan past onM but in case 3 make it look
like on the rented g0 stack, onM was called from mstart.
The traceback already knows that mstart is a top-of-stack function.
Alternate fix at CL 132610043 but I think this one is cleaner.
This CL makes 3 the exception, while that CL makes 1 and 2 the exception.
Submitting TBR to try to get better stack traces out of the
freebsd/amd64 builder, but happy to make changes in a
followup CL.
cmd/dist: make textflag.h available in runtime, avoid android/linux conflicts
1) cmd/dist was copying textflag.h to the build include directory,
but only after compiling package runtime. So other packages could
use it, just not runtime. Copy earlier, so that runtime can use it too.
2) We decided for android that anything marked linux is also included
in the build. The generated linux-specific files in cmd/dist must therefore
have explicit +build !android tags, or else you can't have simultaneous
linux/arm and android/arm builds in a single client. The tag was already
there for at least one file, but it was missing from many others.
cmd/api: don't depend on os/user or USER to check api
The -nocgo builder failed because it has cgo disabled
as well as no USER environment variable:
http://build.golang.org/log/2250abb82f5022b72a12997b8ff89fcdeff094c9
# Checking API compatibility.
Error getting current user: user: Current not implemented on linux/amd64
exit status 1
The original conversion in CL 132090043 cut up
the function in an attempt to avoid converting most
of the code to Go. This contorts the control flow.
While debugging the onM signal stack bug,
I reconverted sigqueue.goc in its entirety.
This restores the original control flow, which is
much easier to understand.
The current conversion is correct, it's just complex
and will be hard to maintain. The new one is as
readable as the original code.
I uploaded sigqueue.goc as the initial copy of
sigqueue.go in the CL, so if you view the diffs
of sigqueue.go comparing against patch set 2 [sic]
it will show the actual starting point.
For example:
https://golang.org/cl/136160043/diff2/20001:60001/src/pkg/runtime/sigqueue.go
LGTM=dvyukov, iant
R=golang-codereviews, dvyukov, iant
CC=golang-codereviews, khr, r
https://golang.org/cl/136160043