Russ Cox [Thu, 6 Aug 2015 01:22:10 +0000 (21:22 -0400)]
net/url: restrict :port checking to [ipv6]:port form
Go 1.4 and earlier accepted mysql://x@y(z:123)/foo
and I don't see any compelling reason to break that.
The CL during Go 1.5 that broke this syntax was
trying to fix #11208 and was probably too aggressive.
I added a test case for #11208 to make sure that stays
fixed.
Relaxing the check did not re-break #11208 nor did
it cause any existing test to fail. I added a test for the
mysql://x@y(z:123)/foo syntax being preserved.
Russ Cox [Wed, 5 Aug 2015 13:53:56 +0000 (09:53 -0400)]
crypto/tls: fix ConnectionState().VerifiedChains for resumed connection
Strengthening VerifyHostname exposed the fact that for resumed
connections, ConnectionState().VerifiedChains was not being saved
and restored during the ClientSessionCache operations.
Do that.
This change just saves the verified chains in the client's session
cache. It does not re-verify the certificates when resuming a
connection.
There are arguments both ways about this: we want fast, light-weight
resumption connections (thus suggesting that we shouldn't verify) but
it could also be a little surprising that, if the verification config
is changed, that would be ignored if the same session cache is used.
On the server side we do re-verify client-auth certificates, but the
situation is a little different there. The client session cache is an
object in memory that's reset each time the process restarts. But the
server's session cache is a conceptual object, held by the clients, so
can persist across server restarts. Thus the chance of a change in
verification config being surprisingly ignored is much higher in the
server case.
Jed Denlea [Tue, 4 Aug 2015 01:00:44 +0000 (18:00 -0700)]
net/http: close server conn after broken trailers
Prior to this change, broken trailers would be handled by body.Read, and
an error would be returned to its caller (likely a Handler), but that
error would go completely unnoticed by the rest of the server flow
allowing a broken connection to be reused. This is a possible request
smuggling vector.
Adam Langley [Wed, 5 Aug 2015 17:55:41 +0000 (10:55 -0700)]
crypto/tls: update testing certificates.
This change alters the certificate used in many tests so that it's no
longer self-signed. This allows some tests to exercise the standard
certificate verification paths in the future.
Change-Id: I9c3fcd6847eed8269ff3b86d9b6966406bf0642d
Reviewed-on: https://go-review.googlesource.com/13244 Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Adam Langley <agl@golang.org> Reviewed-by: Adam Langley <agl@golang.org>
Austin Clements [Wed, 5 Aug 2015 15:07:47 +0000 (11:07 -0400)]
runtime: don't recheck heap trigger for periodic GC
88e945f introduced a non-speculative double check of the heap trigger
before actually starting a concurrent GC. This was necessary to fix a
race for heap-triggered GC, but broke sysmon-triggered periodic GC,
since the heap check will of course fail for periodically triggered
GC.
Fix this by telling startGC whether or not this GC was triggered by
heap size or a timer and only doing the heap size double check for GCs
triggered by heap size.
Russ Cox [Wed, 5 Aug 2015 14:20:02 +0000 (10:20 -0400)]
doc/go1.5: fix hyperlink for runtime/trace
Missed in CL 13074.
Change-Id: Ic0600341abbc423cd8d7b2201bf50e3b0bf398a7
Reviewed-on: https://go-review.googlesource.com/13167 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Wed, 5 Aug 2015 13:12:18 +0000 (09:12 -0400)]
go/build: enable cgo on freebsd/arm
Now that it works we need to turn it back on.
Fixes #10119.
Change-Id: I9c62d3026f7bb62c49a601ad73f33bf655372915
Reviewed-on: https://go-review.googlesource.com/13162 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Wed, 5 Aug 2015 14:19:12 +0000 (10:19 -0400)]
cmd/go: skip external tests on freebsd-arm builder
It is just far too slow.
I have a CL for Go 1.6 that makes many of these into internal tests.
That will improve the coverage.
It does not matter much, because basically none of the go command
tests are architecture dependent, so the other builders will catch
any problems.
Fixes freebsd-arm builder.
Change-Id: I8b2f6ac2cc1e7657019f7731c6662dc43e20bfb5
Reviewed-on: https://go-review.googlesource.com/13166 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Wed, 5 Aug 2015 14:11:59 +0000 (10:11 -0400)]
internal/testenv: add Builder, to report builder name
This works after golang.org/cl/13120 is running on the
coordinator (maybe it already is).
Change-Id: I4053d8e2f32fafd47b927203a6f66d5858e23376
Reviewed-on: https://go-review.googlesource.com/13165 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Wed, 5 Aug 2015 03:44:06 +0000 (23:44 -0400)]
runtime: align stack pointer during initcgo call on arm
This is what is causing freebsd/arm to crash mysteriously when using cgo.
The bug was introduced in golang.org/cl/4030, which moved this code out
of rt0_go and into its own function. The ARM ABI says that calls must
be made with the stack pointer at an 8-byte boundary, but only FreeBSD
seems to crash when this is violated.
Fixes #10119.
Change-Id: Ibdbe76b2c7b80943ab66b8abbb38b47acb70b1e5
Reviewed-on: https://go-review.googlesource.com/13161 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Dave Cheney <dave@cheney.net>
Andrew Gerrand [Wed, 5 Aug 2015 01:58:44 +0000 (11:58 +1000)]
doc: adjust installation instructions dynamically for a given download
This change allows the download page to redirect the user to
/doc/install?download=filename so the user can see installation
instructions specific to the file they are downloading.
This change also expands the "Test your Go installation" section
to instruct the user to create a workspace, hopefully leading
to less confusion down the line.
It also changes the front page download link to go directly
to the downloads page, which will in turn take them to the
installation instructions (the original destination).
This is related to this change to the tools repo:
https://golang.org/cl/13180
Change-Id: I658327bdb93ad228fb1846e389b281b15da91b1d
Reviewed-on: https://go-review.googlesource.com/13151 Reviewed-by: Chris Broadfoot <cbro@golang.org>
Austin Clements [Mon, 3 Aug 2015 22:06:05 +0000 (18:06 -0400)]
runtime: fix assist utilization computation
When commit 510fd13 enabled assists during the scan phase, it failed
to also update the code in the GC controller that computed the assist
CPU utilization and adjusted the trigger based on it. Fix that code so
it uses the start of the scan phase as the wall-clock time when
assists were enabled rather than the start of the mark phase.
Austin Clements [Mon, 3 Aug 2015 21:48:47 +0000 (17:48 -0400)]
runtime: revise assist ratio aggressively
At the start of a GC cycle, the garbage collector computes the assist
ratio based on the total scannable heap size. This was intended to be
conservative; after all, this assumes the entire heap may be reachable
and hence needs to be scanned. But it only assumes that the *current*
entire heap may be reachable. It fails to account for heap allocated
during the GC cycle. If the trigger ratio is very low (near zero), and
most of the heap is reachable when GC starts (which is likely if the
trigger ratio is near zero), then it's possible for the mutator to
create new, reachable heap fast enough that the assists won't keep up
based on the assist ratio computed at the beginning of the cycle. As a
result, the heap can grow beyond the heap goal (by hundreds of megs in
stress tests like in issue #11911).
We already have some vestigial logic for dealing with situations like
this; it just doesn't run often enough. Currently, every 10 ms during
the GC cycle, the GC revises the assist ratio. This was put in before
we switched to a conservative assist ratio (when we really were using
estimates of scannable heap), and it turns out to be exactly what we
need now. However, every 10 ms is far too infrequent for a rapidly
allocating mutator.
This commit reuses this logic, but replaces the 10 ms timer with
revising the assist ratio every time the heap is locked, which
coincides precisely with when the statistics used to compute the
assist ratio are updated.
Austin Clements [Mon, 3 Aug 2015 13:46:50 +0000 (09:46 -0400)]
runtime: make sweep proportional to spans bytes allocated
Proportional concurrent sweep is currently based on a ratio of spans
to be swept per bytes of object allocation. However, proportional
sweeping is performed during span allocation, not object allocation,
in order to minimize contention and overhead. Since objects are
allocated from spans after those spans are allocated, the system tends
to operate in debt, which means when the next GC cycle starts, there
is often sweep debt remaining, so GC has to finish the sweep, which
delays the start of the cycle and delays enabling mutator assists.
For example, it's quite likely that many Ps will simultaneously refill
their span caches immediately after a GC cycle (because GC flushes the
span caches), but at this point, there has been very little object
allocation since the end of GC, so very little sweeping is done. The
Ps then allocate objects from these cached spans, which drives up the
bytes of object allocation, but since these allocations are coming
from cached spans, nothing considers whether more sweeping has to
happen. If the sweep ratio is high enough (which can happen if the
next GC trigger is very close to the retained heap size), this can
easily represent a sweep debt of thousands of pages.
Fix this by making proportional sweep proportional to the number of
bytes of spans allocated, rather than the number of bytes of objects
allocated. Prior to allocating a span, both the small object path and
the large object path ensure credit for allocating that span, so the
system operates in the black, rather than in the red.
Combined with the previous commit, this should eliminate all sweeping
from GC start up. On the stress test in issue #11911, this reduces the
time spent sweeping during GC (and delaying start up) by several
orders of magnitude:
mean 99%ile max
pre fix 1 ms 11 ms 144 ms
post fix 270 ns 735 ns 916 ns
Austin Clements [Mon, 3 Aug 2015 13:25:23 +0000 (09:25 -0400)]
runtime: always give concurrent sweep some heap distance
Currently it's possible for the next_gc heap size trigger computed for
the next GC cycle to be less than the current allocated heap size.
This means the next cycle will start immediately, which means there's
no time to perform the concurrent sweep between GC cycles. This places
responsibility for finishing the sweep on GC itself, which delays GC
start-up and hence delays mutator assist.
Fix this by ensuring that next_gc is always at least a little higher
than the allocated heap size, so we won't trigger the next cycle
instantly.
runtime: assist the GC during GC startup and shutdown
Currently there are two sensitive periods during which a mutator can
allocate past the heap goal but mutator assists can't be enabled: 1)
at the beginning of GC between when the heap first passes the heap
trigger and sweep termination and 2) at the end of GC between mark
termination and when the background GC goroutine parks. During these
periods there's no back-pressure or safety net, so a rapidly
allocating mutator can allocate past the heap goal. This is
exacerbated if there are many goroutines because the GC coordinator is
scheduled as any other goroutine, so if it gets preempted during one
of these periods, it may stay preempted for a long period (10s or 100s
of milliseconds).
Normally the mutator does scan work to create back-pressure against
allocation, but there is no scan work during these periods. Hence, as
a fall back, if a mutator would assist but can't yet, simply yield the
CPU. This delays the mutator somewhat, but more importantly gives more
CPU time to the GC coordinator for it to complete the transition.
This is obviously a workaround. Issue #11970 suggests a far better but
far more invasive way to fix this.
Updates #11911. (This very nearly fixes the issue, but about once
every 15 minutes I get a GC cycle where the assists are enabled but
don't do enough work.)
runtime: recheck GC trigger before actually starting GC
Currently allocation checks the GC trigger speculatively during
allocation and then triggers the GC without rechecking. As a result,
it's possible for G 1 and G 2 to detect the trigger simultaneously,
both enter startGC, G 1 actually starts GC while G 2 gets preempted
until after the whole GC cycle, then G 2 immediately starts another GC
cycle even though the heap is now well under the trigger.
Fix this by re-checking the GC trigger non-speculatively just before
actually kicking off a new GC cycle.
This contributes to #11911 because when this happens, we definitely
don't finish the background sweep before starting the next GC cycle,
which can significantly delay the start of concurrent scan.
Vincent Batts [Mon, 3 Aug 2015 16:26:38 +0000 (12:26 -0400)]
archive/tar: don't treat multiple file system links as a tar hardlink
Do not assume that if stat shows multiple links that we should mark the
file as a hardlink in the tar format. If the hardlink link was not
referenced, this caused a link to "/". On an overlay file system, all
files have multiple links.
The caller must keep the inode references and set TypeLink, Size = 0,
and LinkName themselves.
Change-Id: I873b8a235bc8f8fbb271db74ee54232da36ca013
Reviewed-on: https://go-review.googlesource.com/13045 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Rob Pike [Mon, 3 Aug 2015 03:57:25 +0000 (13:57 +1000)]
go/types: remove the renaming import of go/constant
For niceness, when go/exact was moved from x/tools, it
was renamed go/constant.
For simplicity, when go/types was moved from x/tools, its
imports of (now) go/constant were done with a rename:
import exact "go/constant"
This kept the code just as it was before and avoided the issue
of what to call the internal constant called, um, constant.
But not all was hidden, as the text of some fields of structs and
the like leaked the old name, so things like "exact.Value" appeared
in type definitions and function signatures in the documentation.
This is unacceptable.
Fix the documentation issue by fixing the code. Rename the constant
constant constant_, and remove the renaming import.
This should go into 1.5. It's mostly a mechanical change, is
internal to the package, and fixes the documentation. It contains
no semantic changes except to fix a benchmark that was broken
in the original transition.
Andy Maloney [Mon, 3 Aug 2015 14:15:52 +0000 (10:15 -0400)]
doc: Mention contributor agreement immediately after Gerrit
I walked through the steps for a contribution but ended up
with an error when doing "git mail" because I didn't have a
signed agreement.
Added a section to check for or create one through Gerrit right
after the user has created the account and logged in.
Moved some info from copyright section to the new section.
Change-Id: I79bbd3e18fc3a742fa59a242085da14be9e19ba0
Reviewed-on: https://go-review.googlesource.com/13062 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Andrew Gerrand <adg@golang.org>
This was confusing when I was trying to fix go build -o.
Perhaps due to that fix, this can now be simplified from
three functions to one.
Change-Id: I878a6d243b14132a631e7c62a3bb6d101bc243ea
Reviewed-on: https://go-review.googlesource.com/13027 Reviewed-by: Ian Lance Taylor <iant@golang.org>
«
If the arguments to build are a list of .go files, build treats
them as a list of source files specifying a single package.
When compiling a single main package, build writes
the resulting executable to an output file named after
the first source file ('go build ed.go rx.go' writes 'ed' or 'ed.exe')
or the source code directory ('go build unix/sam' writes 'sam' or 'sam.exe').
The '.exe' suffix is added when writing a Windows executable.
When compiling multiple packages or a single non-main package,
build compiles the packages but discards the resulting object,
serving only as a check that the packages can be built.
The -o flag, only allowed when compiling a single package,
forces build to write the resulting executable or object
to the named output file, instead of the default behavior described
in the last two paragraphs.
»
There is a change in behavior here, namely that 'go build -o x.a x.go'
where x.go is not a command (not package main) did not write any
output files (back to at least Go 1.2) but now writes x.a.
This seems more reasonable than trying to explain that -o is
sometimes silently ignored.
Otherwise the behavior is unchanged.
The lines being deleted in goFilesPackage look like they are
setting up 'go build x.o' to write 'x.a', but they were overridden
by the p.target = "" in runBuild. Again back to at least Go 1.2,
'go build x.go' for a non-main package has never produced
output. It seems better to keep it that way than to change it,
both for historical consistency and for consistency with
'go build strings' and 'go build std'.
All of this behavior is now tested.
Fixes #10865.
Change-Id: Iccdf21f366fbc8b5ae600a1e50dfe7fc3bff8b1c
Reviewed-on: https://go-review.googlesource.com/13024 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Dave Day <djd@golang.org>
Brad Fitzpatrick [Mon, 3 Aug 2015 09:51:21 +0000 (11:51 +0200)]
net/http: deflake TestZeroLengthPostAndResponse
It was failing with multiple goroutines a few out of every thousand
runs (with errRequestCanceled) because it was using the same
*http.Request for all 5 RoundTrips, but the RoundTrips' goroutines
(notably the readLoop method) were all still running, sharing that
same pointer. Because the response has no body (which is what
TestZeroLengthPostAndResponse tests), the readLoop was marking the
connection as reusable early (before the caller read until the body's
EOF), but the Transport code was clearing the Request's cancelation
func *AFTER* the caller had already received it from RoundTrip. This
let the test continue looping and do the next request with the same
pointer, fetch a connection, and then between getConn and roundTrip
have an invariant violated: the Request's cancelation func was nil,
tripping this check:
if !pc.t.replaceReqCanceler(req.Request, pc.cancelRequest) {
pc.t.putIdleConn(pc)
return nil, errRequestCanceled
}
The solution is to clear the request cancelation func in the readLoop
goroutine in the no-body case before it's returned to the caller.
This now passes reliably:
$ go test -race -run=TestZeroLengthPostAndResponse -count=3000
I think we've only seen this recently because we now randomize scheduling
of goroutines in race mode (https://golang.org/cl/11795). This race
has existed for a long time but the window was hard to hit.
Change-Id: Idb91c582919f85aef5b9e5ef23706f1ba9126e9a
Reviewed-on: https://go-review.googlesource.com/13070
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Mikio Hara [Mon, 3 Aug 2015 03:43:25 +0000 (12:43 +0900)]
runtime: skip TestCgoCallbackGC on dragonfly
Updates #11990.
Change-Id: I6c58923a1b5a3805acfb6e333e3c9e87f4edf4ba
Reviewed-on: https://go-review.googlesource.com/13050 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Rob Pike [Mon, 3 Aug 2015 01:05:05 +0000 (11:05 +1000)]
doc: document new linker -X syntax in go1.5.html
Fixes #11973.
Change-Id: Icffa3213246663982b7cc795982e0923e272f405
Reviewed-on: https://go-review.googlesource.com/12919 Reviewed-by: Ian Lance Taylor <iant@golang.org>
net/http: close server conn after request body error
HTTP servers attempt to entirely consume a request body before sending a
response. However, when doing so, it previously would ignore any errors
encountered.
Unfortunately, the errors triggered at this stage are indicative of at
least a couple problems: read timeouts and chunked encoding errors.
This means properly crafted and/or timed requests could lead to a
"smuggled" request.
The fix is to inspect the errors created by the response body Reader,
and treat anything other than io.EOF or ErrBodyReadAfterClose as
fatal to the connection.
Robert Griesemer [Fri, 31 Jul 2015 18:15:11 +0000 (11:15 -0700)]
spec: fixed various example code snippets
Per suggestions by Peter Olsen (https://github.com/pto).
Fixes #11964.
Change-Id: Iae261ac14f75abf848f5601f59d7fe6e95b6805a
Reviewed-on: https://go-review.googlesource.com/13006 Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/go: fix go get x/... matching internal directories
Fixes #11960.
Change-Id: I9361a9f17f4eaf8e4f54b4ba380fd50a4b9cf003
Reviewed-on: https://go-review.googlesource.com/13023 Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/go: fix disallow of p/vendor/x during vendor experiment
The percolation of errors upward in the load process could
drop errors, meaning that a build tree could, depending on the
processing order, import the same directory as both "p/vendor/x"
and as "x". That's not supposed to be allowed. But then, worse,
the build would generate two jobs for building that directory,
which would use the same work space and overwrite each other's files,
leading to very strange failures.
Two fixes:
1. Fix the propagation of errors upward (prefer errors over success).
2. Check explicitly for duplicated packages before starting a build.
New test for #1.
Since #2 can't happen, tested #2 by hand after reverting fix for #1.
Fixes #11913.
Change-Id: I6d2fc65f93b8fb5f3b263ace8d5f68d803a2ae5c
Reviewed-on: https://go-review.googlesource.com/13022 Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/compile, runtime: fix placement of map bucket overflow pointer on nacl
On most systems, a pointer is the worst case alignment, so adding
a pointer field at the end of a struct guarantees there will be no
padding added after that field (to satisfy overall struct alignment
due to some more-aligned field also present).
In the runtime, the map implementation needs a quick way to
get to the overflow pointer, which is last in the bucket struct,
so it uses size - sizeof(pointer) as the offset.
NaCl/amd64p32 is the exception, as always.
The worst case alignment is 64 bits but pointers are 32 bits.
There's a long history that is not worth going into, but when
we moved the overflow pointer to the end of the struct,
we didn't get the padding computation right.
The compiler computed the regular struct size and then
on amd64p32 added another 32-bit field.
And the runtime assumed it could step back two 32-bit fields
(one 64-bit register size) to get to the overflow pointer.
But in fact if the struct needed 64-bit alignment, the computation
of the regular struct size would have added a 32-bit pad already,
and then the code unconditionally added a second 32-bit pad.
This placed the overflow pointer three words from the end, not two.
The last two were padding, and since the runtime was consistent
about using the second-to-last word as the overflow pointer,
no harm done in the sense of overwriting useful memory.
But writing the overflow pointer to a non-pointer word of memory
means that the GC can't see the overflow blocks, so it will
collect them prematurely. Then bad things happen.
Correct all this in a few steps:
1. Add an explicit check at the end of the bucket layout in the
compiler that the overflow field is last in the struct, never
followed by padding.
2. When padding is needed on nacl (not always, just when needed),
insert it before the overflow pointer, to preserve the "last in the struct"
property.
3. Let the compiler have the final word on the width of the struct,
by inserting an explicit padding field instead of overwriting the
results of the width computation it does.
4. For the same reason (tell the truth to the compiler), set the type
of the overflow field when we're trying to pretend its not a pointer
(in this case the runtime maintains a list of the overflow blocks
elsewhere).
5. Make the runtime use "last in the struct" as its location algorithm.
This fixes TestTraceStress on nacl/amd64p32.
The 'bad map state' and 'invalid free list' failures no longer occur.
cmd/internal/obj/arm: fix large stack offsets on nacl/arm
The code already fixed large non-stack offsets
but explicitly excluded stack references.
Perhaps you could get away with that before,
but current versions of nacl reject such stack
references. Rewrite them the same as the others.
For #11956 but probably not the last problem.
Change-Id: I0db4e3a1ed4f88ccddf0d30228982960091d9fb7
Reviewed-on: https://go-review.googlesource.com/13010 Reviewed-by: Dave Cheney <dave@cheney.net>
Etcd and kubernetes have hit this.
See https://bugzilla.redhat.com/show_bug.cgi?id=1248071
Change-Id: I6231013efa0a19ee74f7ebacd1024adb368af83a
Reviewed-on: https://go-review.googlesource.com/12951 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Ian Lance Taylor [Thu, 30 Jul 2015 22:20:12 +0000 (15:20 -0700)]
cmd/go: permit installing into a subdirectory of $GOPATH/bin
In https://golang.org/cl/12080 we forbade installing cross-compiled
binaries into a subdirectory of $GOBIN, in order to fix
https://golang.org/issue/9769. However, that fix was too aggressive,
in that it also forbade installing into a subdirectory of $GOPATH/bin.
This patch permits installing cross-compiled binaries into a
subdirectory $GOPATH/bin while continuing to forbid installing into a
subdirectory of $GOBIN.
Fixes #11778.
Change-Id: Ibc9919554e8c275beff54ec8bf919cfaa03b11ba
Reviewed-on: https://go-review.googlesource.com/12938
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Rob Pike [Thu, 30 Jul 2015 23:03:58 +0000 (09:03 +1000)]
doc: solaris info added to go1.5.html
Fixes #11952.
Change-Id: I548f9d75c6223bf79bdf654ef733f1568e3d5804
Reviewed-on: https://go-review.googlesource.com/12990 Reviewed-by: Ian Lance Taylor <iant@golang.org>
The spec didn't specify several aspects of expression switches:
- The switch expression is evaluated exactly once.
- Switch expressions evaluating to an untyped value are converted
to the respective default type before use.
- An (untyped) nil value is not permitted as expression switch
value. (We could permit it relatively easily, but gc doesn't,
and disallowing it is in symmetry with the rules for var decls
without explicit type and untyped initializer expressions.)
- The comparison x == t between each case expression x and
switch expression value t must be valid.
- (Some) duplicate constant case expressions are not permitted.
This change also clarifies the following issues:
4524: mult. equal int const switch case values should be illegal
-> spec issue fixed
6398: switch w/ no value uses bool rather than untyped bool
-> spec issue fixed
11578: allows duplicate switch cases -> go/types bug
11667: int overflow in switch expression -> go/types bug
11668: use of untyped nil in switch -> not a gc bug
Change-Id: Ie4431e74f095b85b4b5c07d087c3d29acf46d138
Reviewed-on: https://go-review.googlesource.com/12902 Reviewed-by: Ian Lance Taylor <iant@golang.org>
runtime/cgo: fix darwin/amd64 signal handling setup
Was not allocating space for the frame above sigpanic,
nor was it pushing the LR into the right place.
Because traceback past sigpanic only needs the
LR for faulting leaves, this was not noticed too much.
But it did break the sync/atomic nil deref tests.
Change-Id: Icba53fffa193423aab744c37f21ee893ce2ee3ac
Reviewed-on: https://go-review.googlesource.com/12926 Reviewed-by: David Crawshaw <crawshaw@golang.org>
David Chase [Thu, 30 Jul 2015 16:31:18 +0000 (12:31 -0400)]
cmd/compile: add case for ODOTTYPE to escwalk
ODOTTYPE should be treated a whole lot like ODOT,
but it was missing completely from the switch in
escwalk and thus escape status did not propagate
to fields.
Since interfaces are required to trigger this bug,
the test was added to escape_iface.go.
runtime: change arm software div/mod call sequence not to modify stack
Instead of pushing the denominator argument on the stack,
the denominator is now passed in m.
This fixes a variety of bugs related to trying to take stack traces
backwards from the middle of the software div/mod routines.
Some of those bugs have been kludged around in the past,
but others have not. Instead of trying to patch up after breaking
the stack, this CL stops breaking the stack.
This is an update of https://golang.org/cl/19810043,
which was rolled back in https://golang.org/cl/20350043.
The problem in the original CL was that there were divisions
at bad times, when m was not available. These were divisions
by constant denominators, either in C code or in assembly.
The Go compiler knows how to generate division by multiplication
for constant denominators, but the C compiler did not.
There is no longer any C code, so that's taken care of.
There was one problematic DIV in runtime.usleep (assembly)
but https://golang.org/cl/12898 took care of that one.
So now this approach is safe.
Reject DIV/MOD in NOSPLIT functions to keep them from
coming back.
Fixes #6681.
Fixes #6699.
Fixes #10486.
Change-Id: I09a13c76ad08ba75b3bd5d46a3eb78e66a84ab38
Reviewed-on: https://go-review.googlesource.com/12899 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Ian Lance Taylor [Thu, 30 Jul 2015 05:04:09 +0000 (22:04 -0700)]
cmd/cgo: discard trailing zero-sized fields in a non-empty C struct
In order to fix issue #9401 the compiler was changed to add a padding
byte to any non-empty Go struct that ends in a zero-sized field. That
causes the Go version of such a C struct to have a different size than
the C struct, which can considerable confusion. Change cgo so that it
discards any such zero-sized fields, so that the Go and C structs are
the same size.
This is a change from previous releases, in that it used to be
possible to refer to a zero-sized trailing field (by taking its
address), and with this change it no longer is. That is unfortunate,
but something has to change. It seems better to visibly break
programs that do this rather than to silently break programs that rely
on the struct sizes being the same.
runtime: replace divide with multiply in runtime.usleep on arm
We want to adjust the DIV calling convention to use m,
and usleep can be called without an m, so switch to a
multiplication by the reciprocal (and test).
Step toward a fix for #6699 and #10486.
Change-Id: Iccf76a18432d835e48ec64a2fa34a0e4d6d4b955
Reviewed-on: https://go-review.googlesource.com/12898 Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/internal/obj/arm: fix line numbers after constant pool
If a function is large enough to need to flush the constant pool
mid-function, the line number assignment code was forcing the
line numbers not just for the constant pool but for all the instructions
that follow it. This made the line number information completely
wrong for all but the beginning of large functions on arm.
Same problem in code copied into arm64.
This broke runtime/trace's TestTraceSymbolize.
Fixes arm build.
Change-Id: I84d9fb2c798c4085f69b68dc766ab4800c7a6ca4
Reviewed-on: https://go-review.googlesource.com/12894 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
This allows running a cross-compile like
GOOS=darwin GOARCH=arm go build std
to check that everything builds.
Otherwise there is a redefinition error because both
root_nocgo_darwin.go and root_darwin_armx.go
supply initSystemRoots.
Change-Id: Ic95976b2b698d28c629bfc93d8dac0048b023578
Reviewed-on: https://go-review.googlesource.com/12897 Reviewed-by: Ian Lance Taylor <iant@golang.org>
net: allow longer timeout in dialClosedPort test on windows
The test expects the dial to take 1.0 seconds
on Windows and allows it to go to 1.095 seconds.
That's far too optimistic.
Recent failures are reporting roughly 1.2 seconds.
Let it have 1.5.
Change-Id: Id69811ccb65bf4b4c159301a2b4767deb6ee8d28
Reviewed-on: https://go-review.googlesource.com/12895 Reviewed-by: Ian Lance Taylor <iant@golang.org>
math/rand: warn against using package for security-sensitive work
Urge users of math/rand to consider using crypto/rand when doing
security-sensitive work.
Related to issue #11871. While we haven't reached consensus on how
to make the package inherently safer, everyone agrees that the docs
for math/rand can be improved.
Change-Id: I576a312e51b2a3445691da6b277c7b4717173197
Reviewed-on: https://go-review.googlesource.com/12900 Reviewed-by: Rob Pike <r@golang.org>
cmd/compile: fix uninitialized memory during type switch assertE2I2
Fixes arm64 builder crash.
The bug is possible on all architectures; you just have to get lucky
and hit a preemption or a stack growth on entry to assertE2I2.
The test stacks the deck.
Change-Id: I8419da909b06249b1ad15830cbb64e386b6aa5f6
Reviewed-on: https://go-review.googlesource.com/12890 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Rob Pike <r@golang.org>
The skips added in CL 12579, based on incorrect time stamps,
should be sufficient to identify and exclude all the time-related
flakiness on these systems.
runtime/trace: record event sequence numbers explicitly
Nearly all the flaky failures we've seen in trace tests have been
due to the use of time stamps to determine relative event ordering.
This is tricky for many reasons, including:
- different cores might not have exactly synchronized clocks
- VMs are worse than real hardware
- non-x86 chips have different timer resolution than x86 chips
- on fast systems two events can end up with the same time stamp
Stop trying to make time reliable. It's clearly not going to be for Go 1.5.
Instead, record an explicit event sequence number for ordering.
Using our own counter solves all of the above problems.
The trace still contains time stamps, of course. The sequence number
is just used for ordering.
Should alleviate #10554 somewhat. Then tickDiv can be chosen to
be a useful time unit instead of having to be exact for ordering.
Separating ordering and time stamps lets the trace parser diagnose
systems where the time stamp order and actual order do not match
for one reason or another. This CL adds that check to the end of
trace.Parse, after all other sequence order-based checking.
If that error is found, we skip the test instead of failing it.
Putting the check in trace.Parse means that cmd/trace will pick
up the same check, refusing to display a trace where the time stamps
do not match actual ordering.
Using net/http's BenchmarkClientServerParallel4 on various CPU counts,
not tracing vs tracing:
The layout code has to date insisted on stack frames that are 16-aligned
including the saved LR, and it ensured this by growing the frame itself.
This breaks code that refers to values near the top of the frame by positive
offset from SP, and in general it's too magical: if you see TEXT xxx, $N,
you expect that the frame size is actually N, not sometimes N and sometimes N+8.
This led to a serious bug in the compiler where ambiguously live values
were not being zeroed correctly, which in turn triggered an assertion
in the GC about finding only valid pointers. The compiler has been
fixed to always emit aligned frames, and the hand-written assembly
has also been fixed.
Now that everything is aligned, make unaligned an error instead of
something to "fix" silently.
The nosplit stack overflow checks were confused about morestack.
The comment about not having correct SP information at the call
to morestack was true, but that was a real bug, not something to
work around. I fixed that problem in CL 12144. With that fixed,
no need to special-case morestack in the way done here.
This cleanup and simplification of the code was the first step
to fixing a bug that happened when I started working on the
arm64 frame size adjustments, but the cleanup was sufficient
to make the bug go away.
runtime, reflect: use correctly aligned stack frame sizes on arm64
arm64 requires either no stack frame or a frame with a size that is 8 mod 16
(adding the saved LR will make it 16-aligned).
The cmd/internal/obj/arm64 has been silently aligning frames, but it led to
a terrible bug when the compiler and obj disagreed on the frame size,
and it's just generally confusing, so we're going to make misaligned frames
an error instead of something that is silently changed.
This CL prepares by updating assembly files.
Note that the changes in this CL are already being done silently by
cmd/internal/obj/arm64, so there is no semantic effect here,
just a clarity effect.