Xiangdong Ji [Wed, 6 May 2020 09:54:40 +0000 (09:54 +0000)]
cmd/compile: fix incorrect rewriting to if condition
Some ARM64 rewriting rules convert 'comparing to zero' conditions of if
statements to a simplified version utilizing CMN and CMP instructions to
branch over condition flags, in order to save one Add or Sub caculation.
Such optimizations lead to wrong branching in case an overflow/underflow
occurs when executing CMN or CMP.
Fix the issue by introducing new block opcodes that don't honor the
overflow/underflow flag, in the following categories:
Block-Op Meaning ARM condition codes
1. LTnoov less than MI
2. GEnoov greater than or equal PL
3. LEnoov less than or equal MI || EQ
4. GTnoov greater than NEQ & PL
The backend generates two consecutive branch instructions for 'LEnoov'
and 'GTnoov' to model their expected behavior. A slight change to 'gc'
and amd64/386 backends is made to unify the code generation.
Add a test 'TestCondRewrite' as justification, it covers 32 incorrect rules
identified on arm64, more might be needed on other arches, like 32-bit arm.
Add two benchmarks profiling the aforementioned category 1&2 and category
3&4 separetely, we expect the first two categories will show performance
improvement and the second will not result in visible regression compared with
the non-optimized version.
This change also updates TestFormats to support using %#x.
Examples exhibiting where does the issue come from:
1: 'if x + 3 < 0' might be converted to:
before:
CMN $3, R0
BGE <else branch> // wrong branch is taken if 'x+3' overflows
after:
CMN $3, R0
BPL <else branch>
2: 'if y - 3 > 0' might be converted to:
before:
CMP $3, R0
BLE <else branch> // wrong branch is taken if 'y-3' underflows
after:
CMP $3, R0
BMI <else branch>
BEQ <else branch>
Benchmark data from different kinds of arm64 servers, 'old' is the non-optimized
version (not the parent commit), generally the optimization version outperforms.
S1:
name old time/op new time/op delta
CondRewrite/SoloJump 13.6ns ± 0% 12.9ns ± 0% -5.15% (p=0.000 n=10+10)
CondRewrite/CombJump 13.8ns ± 1% 12.9ns ± 0% -6.32% (p=0.000 n=10+10)
S2:
name old time/op new time/op delta
CondRewrite/SoloJump 11.6ns ± 0% 10.9ns ± 0% -6.03% (p=0.000 n=10+10)
CondRewrite/CombJump 11.4ns ± 0% 10.8ns ± 1% -5.53% (p=0.000 n=10+10)
S3:
name old time/op new time/op delta
CondRewrite/SoloJump 7.36ns ± 0% 7.50ns ± 0% +1.79% (p=0.000 n=9+10)
CondRewrite/CombJump 7.35ns ± 0% 7.75ns ± 0% +5.51% (p=0.000 n=8+9)
S4:
name old time/op new time/op delta
CondRewrite/SoloJump-224 11.5ns ± 1% 10.9ns ± 0% -4.97% (p=0.000 n=10+10)
CondRewrite/CombJump-224 11.9ns ± 0% 11.5ns ± 0% -2.95% (p=0.000 n=10+10)
S5:
name old time/op new time/op delta
CondRewrite/SoloJump 10.0ns ± 0% 10.0ns ± 0% -0.45% (p=0.000 n=9+10)
CondRewrite/CombJump 9.93ns ± 0% 9.77ns ± 0% -1.53% (p=0.000 n=10+9)
Volker Dobler [Tue, 26 May 2020 14:55:28 +0000 (16:55 +0200)]
net/http: clarify that AddCookie only sanitizes the Cookie being added
AddCookie properly encodes a cookie and appends it to the Cookie header
field but does not modify or sanitize what the Cookie header field
contains already. If a user manualy sets the Cookie header field to
something not conforming to RFC 6265 then a cookie added via AddCookie
might not be retrievable.
Daniel Martí [Tue, 24 Sep 2019 17:14:10 +0000 (18:14 +0100)]
encoding/xml: only initialize nil struct fields when decoding
fieldInfo.value used to initialize nil anonymous struct fields if they
were encountered. This behavior is wanted when decoding, but not when
encoding. When encoding, the value should never be modified, and these
nil fields should be skipped entirely.
To fix the bug, add a bool argument to the function which tells the
code whether we are encoding or decoding.
Finally, add a couple of tests to cover the edge cases pointed out in
the original issue.
Daniel Martí [Wed, 20 May 2020 17:03:31 +0000 (17:03 +0000)]
Revert "encoding/json: reuse values when decoding map elements"
This reverts golang.org/cl/179337.
Reason for revert: broke a few too many reasonably valid Go programs.
The previous behavior was perhaps less consistent, but the docs were
never very clear about when the decoder merges with existing values,
versus replacing existing values altogether.
Fixes #39149.
Change-Id: I1c1d857709b8398969fe421aa962f6b62f91763a
Reviewed-on: https://go-review.googlesource.com/c/go/+/234559
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>
Bryan C. Mills [Tue, 26 May 2020 20:11:22 +0000 (16:11 -0400)]
os: in Symlink, stat the correct target path for drive-relative targets on Windows
Previously, when the target (“old”) path passed to os.Symlink was a
“root-relative” Windows path,¹ we would erroneously prepend
destination (“new”) path when determining which path to Stat,
resulting in an invalid path which was then masked by the lack of
error propagation for the Stat call (#39183).
If the link target is a directory (rather than a file), that would
result in the symlink being created without the
SYMBOLIC_LINK_FLAG_DIRECTORY flag, which then fails in os.Open.
Cherry Zhang [Wed, 27 May 2020 15:37:00 +0000 (11:37 -0400)]
cmd/compile: always use StackMapDontCare as register map index when reg map is not used
When go115ReduceLiveness is true (so we don't emit actual
register maps), use StackMapDontCare consistently for the
register map index, so RegMapValid is always false.
This fixes a compiler crash when doing -live=2 debug print.
Russ Cox [Fri, 22 May 2020 00:46:05 +0000 (20:46 -0400)]
net/http: handle body rewind in HTTP/2 connection loss better
In certain cases the HTTP/2 stack needs to resend a request.
It obtains a fresh body to send by calling req.GetBody.
This call was missing from the path where the HTTP/2
round tripper returns ErrSkipAltProtocol, meaning fall back
to HTTP/1.1. The result was that the HTTP/1.1 fallback
request was sent with no body at all.
This CL changes that code path to rewind the body before
falling back to HTTP/1.1. But rewinding the body is easier
said than done. Some requests have no GetBody function,
meaning the body can't be rewound. If we need to rewind and
can't, that's an error. But if we didn't read anything, we don't
need to rewind. So we have to track whether we read anything,
with a new ReadCloser wrapper. That in turn requires adding
to the couple places that unwrap Body values to look at the
underlying implementation.
This CL adds the new rewinding code in the main retry loop
as well.
The new rewindBody function also takes care of closing the
old body before abandoning it. That was missing in the old
rewind code.
Thanks to Aleksandr Razumov for CL 210123
and to Jun Chen for CL 234358, both of which informed
this CL.
Fixes #32441.
Change-Id: Id183758526c087c6b179ab73cf3b61ed23a2a46a
Reviewed-on: https://go-review.googlesource.com/c/go/+/234894
Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Cherry Zhang [Wed, 27 May 2020 00:02:43 +0000 (20:02 -0400)]
cmd/link: actually close the output file
When the output file is mmap'd, OutBuf.Close currently munmap the
file but doesn't actually close the file descriptor. This CL
makes it actually close the FD.
Jay Conrod [Thu, 21 May 2020 14:59:29 +0000 (10:59 -0400)]
cmd/go: report error for empty GOPROXY list
If GOPROXY is "", we set it to the default value,
"https://proxy.golang.org,direct". However, if GOPROXY is a non-empty
string that doesn't contain any URLs or keywords, we treat it as
either "off" or "noproxy", which can lead to some strange errors.
This change reports an error for this kind of GOPROXY value.
For #39180
Change-Id: If2e6e39d6f74c708e5ec8f90e9d4880e0e91894f
Reviewed-on: https://go-review.googlesource.com/c/go/+/234857
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Michael Matloob <matloob@golang.org>
Katie Hockman [Tue, 26 May 2020 19:36:58 +0000 (15:36 -0400)]
crypto/x509: allow setting MaxPathLen to -1 without IsCA
This fixes a bug in CL 228777 which disallowed
a MaxPathLen of -1 without IsCA, even though the
x509.Certificate documentation indicates that
MaxPathLen of -1 is considered "unset".
Paul D. Weber [Tue, 26 May 2020 07:00:30 +0000 (07:00 +0000)]
cmd/link/internal/ld/lib.go: use lld on Android
Set linker explicitly to lld because the default does not work on NDK
versions r19c, r20, r20b and r21. NDK 18b (or earlier) based builds
will need to specify -fuse-ld=gold.
Fixes #38838
Change-Id: Ib75f71fb9896b843910f41bd12aa1e36868fa9b3
GitHub-Last-Rev: eeaa171604b59d8ad3d86944ebf21ee758e92f95
GitHub-Pull-Request: golang/go#39217
Reviewed-on: https://go-review.googlesource.com/c/go/+/235017 Reviewed-by: Elias Naur <mail@eliasnaur.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Elias Naur <mail@eliasnaur.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Joel Sing [Tue, 19 May 2020 16:13:57 +0000 (02:13 +1000)]
runtime, syscall: correct openbsd/arm and openbsd/arm64 syscalls for OpenBSD 6.7
Add two no op instructions following svc on openbsd/arm64 and swi on openbsd/arm.
All except some of the most recent arm64 processors have a speculative execution
flaw that occurs across a syscall boundary, which cannot be mitigated in the
kernel. In order to protect against this leak a speculation barrier needs to be
placed after an svc or swi instruction.
In order to avoid the performance impact of these instructions, the OpenBSD 6.7
kernel returns execution two instructions past the svc or swi call. For now two
hardware no ops are added, which allows syscalls to work with both 6.6 and 6.7.
These should be replaced with real speculation barriers once OpenBSD 6.8 is
released.
Updates #36435
Change-Id: I06153cb0998199242cca8761450e53599c3e7de4
Reviewed-on: https://go-review.googlesource.com/c/go/+/234381
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Elias Naur [Mon, 25 May 2020 13:27:05 +0000 (15:27 +0200)]
cmd/link/internal/ld: consider alternative linkers in linkerFlagSupported
CL 235017 is about to change the default Android linker to lld. lld doesn't
support the --compress-debug-sections=zlib-gnu flag, but linkerFlagSupported
doesn't take any alternative linkers specified with -fuse-ld into account.
Updates #38838
Change-Id: I5f7422c06d40dedde2e4b070fc48398e8f822190
Reviewed-on: https://go-review.googlesource.com/c/go/+/235157
Run-TryBot: Elias Naur <mail@eliasnaur.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Tobias Klauser [Sat, 23 May 2020 22:09:20 +0000 (00:09 +0200)]
cmd/go: fix parallel chatty tests on solaris-amd64 builder
The parallel chatty tests added in CL 229085 fail on the
solaris-amd64-oraclerel builder, because a +NN:NN offset time zone is
used. Allow for the `+` character in the corresponding regex to fix
these tests. Also move the '-' to the end of the character class, so it
is not interpreted as the range 9-T.
Change-Id: Iec9ae82ba45d2490176f274f0dc6812666eae718
Reviewed-on: https://go-review.googlesource.com/c/go/+/234978
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
matsuyoshi [Sat, 23 May 2020 07:23:04 +0000 (16:23 +0900)]
os: use same link in UserCacheDir/UserConfigDir doc
Change-Id: I94c385243c37589f56aadaa30336b400adf31308
Reviewed-on: https://go-review.googlesource.com/c/go/+/234959 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Roland Shoemaker [Tue, 12 May 2020 18:06:42 +0000 (11:06 -0700)]
encoding/asn1: document what Unmarshal returns in rest
Specifically, this change documents the behavior of Unmarshal when a
SEQUENCE contains trailing elements.
For context Unmarshal treats trailing elements of a SEQUENCE that do not
have matching struct fields as valid, as this is how ASN.1 structures
are typically extended. This can be somewhat confusing as you might
expect those elements to be appended to rest, but rest is really only
for trailing data unrelated to the structure being parsed (i.e. if you
append a second sequence to b, it would be returned in rest).
Russ Cox [Fri, 22 May 2020 00:53:45 +0000 (20:53 -0400)]
time: simplify Duration.String example
The existing example is needlessly complex.
You have to know that t.Sub returns a Duration
and also have to mentally subtract the two times
to understand what duration should be printed.
Rewrite to focus on just the Duration.String operation.
Austin Clements [Thu, 14 May 2020 20:55:39 +0000 (16:55 -0400)]
runtime: detect and report zombie slots during sweeping
A zombie slot is a slot that is marked, but isn't allocated. This can
indicate a bug in the GC, or a bad use of unsafe.Pointer. Currently,
the sweeper has best-effort detection for zombie slots: if there are
more marked slots than allocated slots, then there must have been a
zombie slot. However, this is imprecise since it only compares totals
and it reports almost no information that may be helpful to debug the
issue.
Add a precise check that compares the mark and allocation bitmaps and
reports detailed information if it detects a zombie slot.
No appreciable effect on performance as measured by the sweet
benchmarks:
Bryan C. Mills [Thu, 21 May 2020 18:16:50 +0000 (14:16 -0400)]
all: use a hermetic "go" tool in standard-library tests
The go/build package uses the "go" tool from the user's environment,
but its tests should not assume that that tool is in any particular
state, let alone appropriate for running the test.
Instead, explicitly use testenv.GoTool, adding it to $PATH in a
TestMain when necessary.
Fixes #39199
Fixes #39198
Change-Id: I56618a55ced473e75dd96eeb3a8f7084e2e64d02
Reviewed-on: https://go-review.googlesource.com/c/go/+/234880
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alexander Rakoczy <alex@golang.org>
Michael Pratt [Thu, 7 May 2020 22:13:21 +0000 (18:13 -0400)]
runtime: disable preemption in startTemplateThread
When a locked M wants to start a new M, it hands off to the template
thread to actually call clone and start the thread. The template thread
is lazily created the first time a thread is locked (or if cgo is in
use).
stoplockedm will release the P (_Pidle), then call handoffp to give the
P to another M. In the case of a pending STW, one of two things can
happen:
1. handoffp starts an M, which does acquirep followed by schedule, which
will finally enter _Pgcstop.
2. handoffp immediately enters _Pgcstop. This only occurs if the P has
no local work, GC work, and no spinning M is required.
If handoffp starts an M, and must create a new M to do so, then newm
will simply queue the M on newmHandoff for the template thread to do the
clone.
When a stop-the-world is required, stopTheWorldWithSema will start the
stop and then wait for all Ps to enter _Pgcstop. If the template thread
is not fully created because startTemplateThread gets stopped, then
another stoplockedm may queue an M that will never get created, and the
handoff P will never leave _Pidle. Thus stopTheWorldWithSema will wait
forever.
A sequence to trigger this hang when STW occurs can be visualized with
two threads:
Note that the P in T2 is stuck sitting in _Pidle. Since the template
thread isn't running, the new M will not be started complete the
transition to _Pgcstop.
To resolve this, we disable preemption around the assignment of
haveTemplateThread and the creation of the template thread in order to
guarantee that if handTemplateThread is set then the template thread
will eventually exist, in the presence of stops.
Jean de Klerk [Mon, 4 May 2020 20:06:34 +0000 (14:06 -0600)]
testing: reformat test chatty output
In #24929, we decided to stream chatty test output. It looks like,
foo_test.go:138: TestFoo/sub-1: hello from subtest 1
foo_test.go:138: TestFoo/sub-2: hello from subtest 2
In this CL, we refactor the output to be grouped by === CONT lines, preserving
the old test-file-before-log-line behavior:
=== CONT TestFoo/sub-1
foo_test.go:138 hello from subtest 1
=== CONT TestFoo/sub-2
foo_test.go:138 hello from subtest 2
This should remove a layer of verbosity from tests, and make it easier to group
together related lines. It also returns to a more familiar format (the
pre-streaming format), whilst still preserving the streaming feature.
Michael Anthony Knyszek [Tue, 19 May 2020 16:33:17 +0000 (16:33 +0000)]
runtime: synchronize StartTrace and StopTrace with sysmon
Currently sysmon is not stopped when the world is stopped, which is
in general a difficult thing to do. The result of this is that when
tracing starts and the value of trace.enabled changes, it's possible
for sysmon to fail to emit an event when it really should. This leads to
traces which the execution trace parser deems inconsistent.
Fix this by putting all of sysmon's work behind a new lock sysmonlock.
StartTrace and StopTrace both acquire this lock after stopping the world
but before performing any work in order to ensure sysmon sees the
required state change in tracing. This change is expected to slow down
StartTrace and StopTrace, but will help ensure consistent traces are
generated.
Updates #29707.
Fixes #38794.
Change-Id: I64c58e7c3fd173cd5281ffc208d6db24ff6c0284
Reviewed-on: https://go-review.googlesource.com/c/go/+/234617
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Michael Pratt <mpratt@google.com>
Cherry Zhang [Mon, 18 May 2020 19:23:46 +0000 (15:23 -0400)]
cmd/link: fix size calculation for file space preallocation on darwin
On darwin, we preallocate file storage space with fcntl
F_ALLOCATEALL in F_PEOFPOSMODE mode. This is specified as
allocating from the physical end of the file. So the size we give
it should be the increment, instead of the total size.
Than McIntosh [Fri, 15 May 2020 16:19:07 +0000 (12:19 -0400)]
cmd/compile: delay inlinable method compilation for -c=1
When the concurrent back end is not enabled, it is possible to have a
scenario where: we compile a specific inlinable non-pointer-receiver
method T.M, then at some point later on in the compilation we visit a
type that triggers generation of a pointer-receiver wrapper (*T).M,
which then results in an inline of T.M into (*T).M. This introduces
subtle differences in the DWARF as compared with when the concurrent
back end is enabled (in the concurrent case, by the time we run the
SSA back end on T.M is is marked as being inlined, whereas in the
non-current case it is not marked inlined).
As a fix, at the point where we would normally compile a given
function in the xtop list right away, if the function is a method AND
is inlinable AND hasn't been inlined, then delay its compilation until
compileFunctions (so as to make sure that when we do compile it, all
possible inlining has been complete). In addition, make sure that
the abstract function symbol for the inlined function gets recorded
correctly.
Fixes #38068.
Change-Id: I57410ab5658bd4ee5b4b80750518e9b20fd6ba52
Reviewed-on: https://go-review.googlesource.com/c/go/+/234178
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: Keith Randall <khr@golang.org>
Jay Conrod [Wed, 20 May 2020 21:54:55 +0000 (17:54 -0400)]
cmd/go: rank errUseProxy lower when handling proxy errors
modfetch.TryProxies ranks errors returned by GOPROXY entries by
usefulness. It returns the error of the highest rank from the last
proxy. Errors from "direct" and "noproxy" are most useful, followed by
errors other than ErrNotExist, followed by ErrNotExist.
This change ranks errUseProxy with ErrNotExist even though it's
reported by "noproxy". There is almost always a more useful message
than "path does not match GOPRIVATE/GONOPROXY".
Fixes #39180
Change-Id: Ifa5b96462d7bf411e6d2d951888465c839d42471
Reviewed-on: https://go-review.googlesource.com/c/go/+/234687
Run-TryBot: Jay Conrod <jayconrod@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Cherry Zhang [Mon, 18 May 2020 18:14:11 +0000 (14:14 -0400)]
runtime: add a barrier after a new span is allocated
When copying a stack, we
1. allocate a new stack,
2. adjust pointers pointing to the old stack to pointing to the
new stack.
If the GC is running on another thread concurrently, on a machine
with weak memory model, the GC could observe the adjusted pointer
(e.g. through gp._defer which could be a special heap-to-stack
pointer), but not observe the publish of the new stack span. In
this case, the GC will see the adjusted pointer pointing to an
unallocated span, and throw. Fixing this by adding a publication
barrier between the allocation of the span and adjusting pointers.
One testcase for this is TestDeferHeapAndStack in long mode. It
fails reliably on linux-mips64le-mengzhuo builder without the fix,
and passes reliably after the fix.
Ian Lance Taylor [Wed, 20 May 2020 03:23:58 +0000 (20:23 -0700)]
syscall: preserve Windows file permissions for O_CREAT|O_TRUNC
On Windows, calling syscall.Open(file, O_CREAT|O_TRUNC, 0) for a file
that already exists would change the file to be read-only.
That is not how the Unix syscall.Open behaves, so avoid it on
Windows by calling CreateFile twice if necessary.
Fixes #38225
Change-Id: I70097fca8863df427cc8a97b9376a9ffc69c6318
Reviewed-on: https://go-review.googlesource.com/c/go/+/234534
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Yasuhiro Matsumoto [Thu, 14 May 2020 09:17:49 +0000 (18:17 +0900)]
cmd/go: use temporary file for output of gcc command on Windows
On Windows, some of gcc command (like msys2 native) output NUL as a file.
Fixes #36000
Change-Id: I02c632fa2d710a011d79f24d5beee4bc57ad994e
Reviewed-on: https://go-review.googlesource.com/c/go/+/233977
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
David Chase [Fri, 1 May 2020 21:30:33 +0000 (17:30 -0400)]
cmd/go: remove GOAMD64 environment variable
This removes the GOAMD64 environment variable and its documentation.
The value is instead supplied by a compiled-in constant.
Note that function alignment is also dependent on the value of
the (removed) flag; it is 32 for aligned jumps, 16 if not.
When the flag-dependent logic is removed, it will be 32.
Bryan C. Mills [Tue, 19 May 2020 05:29:11 +0000 (01:29 -0400)]
runtime: allocate fewer bytes during TestEINTR
This will hopefully address the occasional "runtime: out of memory"
failures observed on the openbsd-arm-jsing builder:
https://build.golang.org/log/c296d866e5d99ba401b18c1a2ff3e4d480e5238c
Also make the "spin" and "winch" loops concurrent instead of
sequential to cut down the test's running time.
Finally, change Block to coordinate by closing stdin instead of
sending SIGINT. The SIGINT handler wasn't necessarily registered by
the time the signal was sent.
Updates #20400
Updates #39043
Change-Id: Ie12fc75b87e33847dc25a12edb4126db27492da6
Reviewed-on: https://go-review.googlesource.com/c/go/+/234538
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Michael Anthony Knyszek [Mon, 18 May 2020 16:06:11 +0000 (16:06 +0000)]
runtime: don't use (addrRange).subtract in removeGreaterEqual
Currently in (*addrRanges).removeGreaterEqual we use
(addrRange).subtract with a range from specified address to "infinity"
which is supposed to be maxOffAddr. However, maxOffAddr is necessarily
an inclusive bound on the address space, because on many platforms an
exclusive bound would overflow back to 0.
On some platforms like mips and mipsle, the address space is smaller
than what's representable in a pointer, so if there's a range which hits
the top of the address space (such as in the pageAlloc tests), the limit
doesn't overflow, but maxOffAddr is inclusive, so any attempt to prune
this range with (*addrRange).removeGreaterEqual causes a failure, since
the range passed to subtract is contained within the address range which
touches the top of the address space.
Another problem with using subtract here is that addr and
maxOffAddr.addr() may not be in the same segment which could cause
makeAddrRange to panic. While this unlikely to happen, on some platforms
such as Solaris it is possible.
Fix these issues by not using subtract at all. Create a specific
implementation of (addrRange).removeGreaterEqual which side-steps all of
this by not having to worry about the top of the address space at all.
Michael Hudson-Doyle [Wed, 20 May 2020 02:39:13 +0000 (14:39 +1200)]
cmd/go: accept smart quotes when checking for missing gold in TestNoteReading
Fixes #39157
Change-Id: Ia983f5e66698519cd19c1565cfb80e86d08fdfc6
Reviewed-on: https://go-review.googlesource.com/c/go/+/234380 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Bryan C. Mills [Tue, 19 May 2020 18:17:05 +0000 (14:17 -0400)]
crypto/x509: save the temp dir in TestReadUniqueDirectoryEntries
In CL 231958, TempDir was changed to create a new temp directory on
each allocation, on the theory that it is easy to save in a variable
for callers that want the same directory repeatedly. Apply that
transformation here.
Katie Hockman [Mon, 18 May 2020 20:49:04 +0000 (16:49 -0400)]
crypto/tls: remove version check when unmarshaling sessionState
This was causing issues when fuzzing with
TestMarshalUnmarshal since the test would
occassionally set the version to VersionTLS13,
which would fail when unmarshaling. The check
doesn't add much in practice, and there is no
harm in removing it to de-flake the test.
Jay Conrod [Thu, 14 May 2020 18:52:17 +0000 (14:52 -0400)]
cmd: update golang.org/x/mod to v0.3.0 (same commit)
v0.3.0 is a tag on 859b3ef565e2, the version that was already being
used. This change is a no-op, except for letting us use a release
version instead of a pseudo-version.
For #36905
Change-Id: I70b8ce2a3f1451f5602c469501362d7a6a673b12
Reviewed-on: https://go-review.googlesource.com/c/go/+/234002
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org>
Richard Miller [Mon, 18 May 2020 08:34:17 +0000 (09:34 +0100)]
runtime: don't enable notes (=signals) too early in Plan 9
The Plan 9 runtime startup was enabling notes (like Unix signals)
before the gsignal stack was allocated. This left a small window
of time where an interrupt (eg by the parent killing a subprocess
quickly after exec) would cause a null pointer dereference in
sigtramp. This would leave the interrupted process suspended in
'broken' state instead of exiting. We've observed this on the
builders, where it can make a test time out waiting for the broken
process to terminate.
Updates #38772
Change-Id: I54584069fd3109595f06c78724c1f6419e028aab
Reviewed-on: https://go-review.googlesource.com/c/go/+/234397
Run-TryBot: David du Colombier <0intro@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: David du Colombier <0intro@gmail.com>
Cherry Zhang [Thu, 14 May 2020 23:22:59 +0000 (19:22 -0400)]
cmd/link: detect trampoline of deferreturn call
The runtime needs to find the PC of the deferreturn call in a few
places. So for functions that have defer, we record the PC of
deferreturn call in its funcdata.
For very large binaries, the deferreturn call could be made
through a trampoline. The current code of finding deferreturn PC
fails in this case. This CL handles the trampoline as well.
Fixes #39049.
Change-Id: I929be54d6ae436f5294013793217dc2a35f080d4
Reviewed-on: https://go-review.googlesource.com/c/go/+/234105
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>
Cherry Zhang [Thu, 14 May 2020 16:16:10 +0000 (12:16 -0400)]
cmd/link: fix SCONST symbol handling on darwin
Don't include SCONST symbols in the symbol table when
NotInSymbolTable is set. This is what the old code (genasmsym)
does.
In fact, SCONST symbol is only emitted by the field tracking
code, and is always NotInSymbolTable. So we should just not
include them at all, or not generate SCONST symbols at all. But
at this late stage I'll just restore the old behavior.
Michael Anthony Knyszek [Tue, 12 May 2020 16:08:50 +0000 (16:08 +0000)]
runtime: make maxOffAddr reflect the actual address space upper bound
Currently maxOffAddr is defined in terms of the whole 64-bit address
space, assuming that it's all supported, by using ^uintptr(0) as the
maximal address in the offset space. In reality, the maximal address in
the offset space is (1<<heapAddrBits)-1 because we don't have more than
that actually available to us on a given platform.
On most platforms this is fine, because arenaBaseOffset is just
connecting two segments of address space, but on AIX we use it as an
actual offset for the starting address of the available address space,
which is limited. This means using ^uintptr(0) as the maximal address in
the offset address space causes wrap-around, especially when we just
want to represent a range approximately like [addr, infinity), which
today we do by using maxOffAddr.
To fix this, we define maxOffAddr more appropriately, in terms of
(1<<heapAddrBits)-1.
This change also redefines arenaBaseOffset to not be the negation of the
virtual address corresponding to address zero in the virtual address
space, but instead directly as the virtual address corresponding to
zero. This matches the existing documentation more closely and makes the
logic around arenaBaseOffset decidedly simpler, especially when trying
to reason about its use on AIX.
Fixes #38966.
Change-Id: I1336e5036a39de846f64cc2d253e8536dee57611
Reviewed-on: https://go-review.googlesource.com/c/go/+/233497
Run-TryBot: Michael Knyszek <mknyszek@google.com> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Michael Pratt <mpratt@google.com>
Michael Munday [Wed, 13 May 2020 15:46:16 +0000 (16:46 +0100)]
cmd/compile: fix tuple selector bug in CSE pass
When tuple generators and selectors are eliminated as part of the
CSE pass we may end up with tuple selectors that are in different
blocks to the tuple generators that they correspond to. This breaks
the invariant that tuple generators and their corresponding
selectors must be in the same block. Therefore after CSE this
situation must be corrected.
Unfortunately the fixup code did not take into account that selectors
could be eliminated by CSE. It assumed that only the tuple generators
could be eliminated. In some situations this meant that it got into
a state where it was replacing references to selectors with references
to dead selectors in the wrong block.
To fix this we move the fixup code after the CSE rewrites have been
applied. This removes any difficult-to-reason-about interactions
with the CSE rewriter.
Fixes #38916.
Change-Id: I2211982dcdba399d03299f0a819945b3eb93b291
Reviewed-on: https://go-review.googlesource.com/c/go/+/233857
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Bryan C. Mills [Wed, 13 May 2020 03:06:14 +0000 (23:06 -0400)]
cmd/go: terminate TestScript commands more aggressively when the test times out
- Avoid starting subprocesses when the test is already very close to
timing out. The overhead of starting and stopping processes may
cause the test to exceed its deadline even if each individual
process is signaled soon after it is started.
- If a command does not shut down quickly enough after receiving
os.Interrupt, send it os.Kill using the same style of grace period
as in CL 228438.
- Fail the test if a background command whose exit status is not
ignored is left running at the end of the test. We have no reliable
way to distinguish a failure due to the termination signal from an
unexpected failure, and the termination signal varies across
platforms (so may cause failure on one platform but success on
another).
For #38797
Change-Id: I767898cf551dca45579bf01a9d1bb312e12d6193
Reviewed-on: https://go-review.googlesource.com/c/go/+/233526
Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com>
Bryan C. Mills [Wed, 6 May 2020 20:22:15 +0000 (16:22 -0400)]
cmd/go: do not ignore permission errors when matching patterns
While reviewing CL 228784, I noticed that various filepath.WalkFunc
implementations within cmd/go were dropping non-nil errors.
Those errors turn out to be significant, at least in some cases: for
example, they can cause packages to appear to be missing when any
parent of the directory had the wrong permissions set.
(This also turned up a bug in the existing list_dedup_packages test,
which was accidentally passing a nonexistent directory instead of the
intended duplicate path.)
Change-Id: Ia09a0a33aa7a966d9f132d3747d6c674a5370b2d
Reviewed-on: https://go-review.googlesource.com/c/go/+/232579
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Bryan C. Mills [Wed, 13 May 2020 16:43:46 +0000 (12:43 -0400)]
runtime: reduce timing sensitivity in TestEINTR
- Don't assume that a process interrupted at 100μs intervals will have
enough remaining time to make progress. (Stop sending signals
in between signal storms to allow the process to quiesce.)
- Don't assume that a child process that spins for 1ms will block long
enough for the parent process to receive signals or make meaningful
progress. (Instead, have the child block indefinitely, and unblock
it explicitly after the signal storm.)
For #39043
Updates #22838
Updates #20400
Change-Id: I85cba23498c346a637e6cfe8684ca0c478562a93
Reviewed-on: https://go-review.googlesource.com/c/go/+/233877
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Ian Lance Taylor [Fri, 8 May 2020 04:34:54 +0000 (21:34 -0700)]
internal/poll, os: loop on EINTR
Historically we've assumed that we can install all signal handlers
with the SA_RESTART flag set, and let the system restart slow functions
if a signal is received. Therefore, we don't have to worry about EINTR.
This is only partially true, and we've added EINTR checks already for
connect, and open/read on Darwin, and sendfile on Solaris.
Other cases have turned up in #36644, #38033, and #38836.
Also, #20400 points out that when Go code is included in a C program,
the C program may install its own signal handlers without SA_RESTART.
In that case, Go code will see EINTR no matter what it does.
So, go ahead and check for EINTR. We don't check in the syscall package;
people using syscalls directly may want to check for EINTR themselves.
But we do check for EINTR in the higher level APIs in os and net,
and retry the system call if we see it.
This change looks safe, but of course we may be missing some cases
where we need to check for EINTR. As such cases turn up, we can add
tests to runtime/testdata/testprogcgo/eintr.go, and fix the code.
If there are any such cases, their handling after this change will be
no worse than it is today.
For #22838
Fixes #20400
Fixes #36644
Fixes #38033
Fixes #38836
Change-Id: I7e46ca8cafed0429c7a2386cc9edc9d9d47a6896
Reviewed-on: https://go-review.googlesource.com/c/go/+/232862
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Keith Randall [Thu, 7 May 2020 20:44:51 +0000 (13:44 -0700)]
cmd/compile: in prove, zero right shifts of positive int by #bits - 1
Taking over Zach's CL 212277. Just cleaned up and added a test.
For a positive, signed integer, an arithmetic right shift of count
(bit-width - 1) equals zero. e.g. int64(22) >> 63 -> 0. This CL makes
prove replace these right shifts with a zero-valued constant.
These shifts may arise in source code explicitly, but can also be
created by the generic rewrite of signed division by a power of 2.
// Signed divide by power of 2.
// n / c = n >> log(c) if n >= 0
// = (n+c-1) >> log(c) if n < 0
// We conditionally add c-1 by adding n>>63>>(64-log(c))
(first shift signed, second shift unsigned).
(Div64 <t> n (Const64 [c])) && isPowerOfTwo(c) ->
(Rsh64x64
(Add64 <t> n (Rsh64Ux64 <t>
(Rsh64x64 <t> n (Const64 <typ.UInt64> [63]))
(Const64 <typ.UInt64> [64-log2(c)])))
(Const64 <typ.UInt64> [log2(c)]))
If n is known to be positive, this rewrite includes an extra Add and 2
extra Rsh. This CL will allow prove to replace one of the extra Rsh with
a 0. That replacement then allows lateopt to remove all the unneccesary
fixups from the generic rewrite.
There is a rewrite rule to handle this case directly:
(Div64 n (Const64 [c])) && isNonNegative(n) && isPowerOfTwo(c) ->
(Rsh64Ux64 n (Const64 <typ.UInt64> [log2(c)]))
But this implementation of isNonNegative really only handles constants
and a few special operations like len/cap. The division could be
handled if the factsTable version of isNonNegative were available.
Unfortunately, the first opt pass happens before prove even has a
chance to deduce the numerator is non-negative, so the generic rewrite
has already fired and created the extra Ops discussed above.
Fixes #36159
By Printf count, this zeroes 137 right shifts when building std and cmd.
Alberto Donizetti [Sat, 9 May 2020 15:10:32 +0000 (17:10 +0200)]
runtime: fix dead link in gcc_androd.c file
Old url 404s because the file no longer exists on master; change it to
point to the android 10 release branch.
Change-Id: If0f8b645f2c746f9fc8bbd68f4d1fe41868493ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/232809 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Steven Hartland [Fri, 8 May 2020 14:17:55 +0000 (14:17 +0000)]
net: only enable broadcast on sockets which support it
Only enable broadcast on SOCK_DGRAM and SOCK_RAW sockets, SOCK_STREAM
and others don't support it.
Don't enable SO_BROADCAST on UNIX domain sockets as they don't support it.
This caused failures on WSL which strictly checks setsockopt calls
unlike other OSes which often silently ignore bad options.
Also return error for setsockopt call for SO_BROADCAST on Windows
matching all other platforms but for IPv4 only as it's not supported
on IPv6 as per:
https://docs.microsoft.com/en-us/windows/win32/winsock/socket-options
Fixes #38954
Change-Id: I0503fd1ce96102b17121af548b66b3e9c2bb80d3
Reviewed-on: https://go-review.googlesource.com/c/go/+/232807
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Ayan George [Fri, 8 May 2020 04:13:44 +0000 (00:13 -0400)]
image/png: remove too early declaration of "n"
Before this commit, the code declares and assigns "n" with the result of
io.ReadFull() -- but the value is not used. The variable is then reused
later in the function.
This commit removes the first declaration of "n" and declares it closer
to where it is used.
Change-Id: I7ffe19a10f2a563c306bb6fe6562493435b9dc5a
Reviewed-on: https://go-review.googlesource.com/c/go/+/232917 Reviewed-by: Rob Pike <r@golang.org>
Daniel Martí [Fri, 8 May 2020 13:14:15 +0000 (14:14 +0100)]
testing: tests and benchmarks can assume flag.Parsed
testing.M.Run has this bit of code:
if !flag.Parsed() {
flag.Parse()
}
It makes sense, and it's common knowledge for many Go developers that
test flags are automatically parsed by the time tests and benchmarks are
run. However, the docs didn't clarify that. The previous wording only
mentioned that flag.Parse isn't run before TestMain, which doesn't
necessarily mean that it's run afterwards.
Fixes #38952.
Change-Id: I85f7a9dce637a23c5cb9abc485d47415c1a1ca27
Reviewed-on: https://go-review.googlesource.com/c/go/+/232806 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Daniel Martí [Wed, 29 May 2019 10:09:50 +0000 (11:09 +0100)]
encoding/json: reuse values when decoding map elements
When we decode into a struct, each input key-value may be decoded into
one of the struct's fields. Particularly, existing data isn't dropped,
so that some sub-fields can be decoded into without zeroing all other
data.
However, decoding into a map behaved in the opposite way. Whenever a
key-value was decoded, it completely replaced the previous map element.
If the map contained any non-zero data in that key, it's dropped.
Instead, try to reuse the existing element value if possible. If the map
element type is a pointer, and the value is non-nil, we can decode
directly into it. If it's not a pointer, make a copy and decode into
that copy, as map element values aren't addressable.
This means we have to parse and convert the map element key before the
value, to be able to obtain the existing element value. This is fine,
though. Moreover, reporting errors on the key before the value follows
the input order more closely.
Finally, add a test to explore the four combinations, involving pointer
and non-pointer, and non-zero and zero values. A table-driven test
wasn't used, as each case required different checks, such as checking
that the non-nil pointer case doesn't end up with a different pointer.
Fixes #31924.
Change-Id: I5ca40c9963a98aaf92f26f0b35843c021028dfca
Reviewed-on: https://go-review.googlesource.com/c/go/+/179337
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Cherry Zhang [Fri, 8 May 2020 19:08:55 +0000 (15:08 -0400)]
cmd/internal/obj/arm64: fix 32-bit BITCON test
The BITCON test, isbitcon, assumes 32-bit constants are expanded
repeatedly, i.e. by copying the low 32 bits to high 32 bits,
instead of zero extending. We already do such expansion in
progedit. In con32class when classifying 32-bit constants, we
should use the expanded constant, instead of zero-extending it.
TODO: we could have better encoding for things like ANDW $-1, Rx.
Robert Griesemer [Fri, 8 May 2020 19:13:11 +0000 (12:13 -0700)]
strconv: fix ParseComplex for strings with separators
The recently added function parseFloatPrefix tested the entire
string for correct placement of separators rather than just the
consumed part. The 4-char fix is in readFloat (atof.go:303).
Added more tests. Also added some white space for nicer
grouping of the test cases.
While at it, removed the need for calling testing.Run.
Fixes #38962.
Change-Id: Ifce84f362bb4ede559103f8d535556d3de9325f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/233017 Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/compile: omit file:pos for non-existent errors
Omits printing the file:line:column when trying to
open non-existent files
Given:
go tool compile x.go
* Before:
x.go:0: open x.go: no such file or directory
* After:
open x.go: no such file or directory
Reverts the revert in CL 231043 by only fixing the case
of non-existent errors which is what the original bug
was about. The fix for "permission errors" will come later
on when I have bandwidth to investigate the differences
between running with root and why os.Open works for some
builders and not others.
Daniel Martí [Fri, 27 Mar 2020 23:56:09 +0000 (23:56 +0000)]
encoding/json: don't mangle strings in an edge case when decoding
The added comment contains some context. The original optimization
assumed that each call to unquoteBytes (or unquote) followed its
corresponding call to rescanLiteral. Otherwise, unquoting a literal
might use d.safeUnquote from another re-scanned literal.
Unfortunately, this assumption is wrong. When decoding {"foo": "bar"}
into a map[T]string where T implements TextUnmarshaler, the sequence of
calls would be as follows:
Note that the call to UnmarshalText happens in literalStore, which
repeats the work to unquote the input string literal. But, since that
happens after we've re-scanned "bar", we're using the wrong safeUnquote
field value.
In the added test case, the second string had a non-zero number of safe
bytes, and the first string had none since it was all non-ASCII. Thus,
"safely" unquoting a number of the first string's bytes could cut a rune
in half, and thus mangle the runes.
A rather simple fix, without a full revert, is to only allow one use of
safeUnquote per call to unquoteBytes. Each call to rescanLiteral when
we have a string is soon followed by a call to unquoteBytes, so it's no
longer possible for us to use the wrong index.
Also add a test case from #38126, which is the same underlying bug, but
affecting the ",string" option.
Before the fix, the test would fail, just like in the original two issues:
--- FAIL: TestUnmarshalRescanLiteralMangledUnquote (0.00s)
decode_test.go:2443: Key "开源" does not exist in map: map[开���:12345开源]
decode_test.go:2458: Unmarshal unexpected error: json: invalid use of ,string struct tag, trying to unmarshal "\"aaa\tbbb\"" into string
Fixes #38105.
For #38126.
Change-Id: I761e54924e9a971a4f9eaa70bbf72014bb1476e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/226218
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Michael Anthony Knyszek [Tue, 28 Apr 2020 21:30:57 +0000 (21:30 +0000)]
runtime: use offAddr in more parts of the runtime
This change uses the new offAddr type in more parts of the runtime where
we've been implicitly switching from the default address space to a
contiguous view. The purpose of offAddr is to represent addresses in the
contiguous view of the address space, and to make direct computations
between real addresses and offset addresses impossible. This change thus
improves readability in the runtime.
Michael Anthony Knyszek [Tue, 28 Apr 2020 21:09:17 +0000 (21:09 +0000)]
runtime: make addrRange[s] operate on offset addresses
Currently addrRange and addrRanges operate on real addresses. That is,
the addresses they manipulate don't include arenaBaseOffset. When added
to an address, arenaBaseOffset makes the address space appear contiguous
on platforms where the address space is segmented. While this is
generally OK because even those platforms which have a segmented address
space usually don't give addresses in a different segment, today it
causes a mismatch between the scavenger and the rest of the page
allocator. The scavenger scavenges from the highest addresses first, but
only via real address, whereas the page allocator allocates memory in
offset address order.
So this change makes addrRange and addrRanges, i.e. what the scavenger
operates on, use offset addresses. However, lots of the page allocator
relies on an addrRange containing real addresses.
To make this transition less error-prone, this change introduces a new
type, offAddr, whose purpose is to make offset addresses a distinct
type, so any attempt to trivially mix real and offset addresses will
trigger a compilation error.
This change doesn't attempt to use offAddr in all of the runtime; a
follow-up change will look for and catch remaining uses of an offset
address which doesn't use the type.
Michael Anthony Knyszek [Mon, 10 Feb 2020 23:11:30 +0000 (23:11 +0000)]
runtime: avoid re-scanning scavenged and untouched memory
Currently the scavenger will reset to the top of the heap every GC. This
means if it scavenges a bunch of memory which doesn't get used again,
it's going to keep re-scanning that memory on subsequent cycles. This
problem is especially bad when it comes to heap spikes: suppose an
application's heap spikes to 2x its steady-state size. The scavenger
will run over the top half of that heap even if the heap shrinks, for
the rest of the application's lifetime.
To fix this, we maintain two numbers: a "free" high watermark, which
represents the highest address freed to the page allocator in that
cycle, and a "scavenged" low watermark, which represents how low of an
address the scavenger got to when scavenging. If the "free" watermark
exceeds the "scavenged" watermark, then we pick the "free" watermark as
the new "top of the heap" for the scavenger when starting the next
scavenger cycle. Otherwise, we have the scavenger pick up where it left
off.
With this mechanism, we only ever re-scan scavenged memory if a random
page gets freed very high up in the heap address space while most of the
action is happening in the lower parts. This case should be exceedingly
unlikely because the page reclaimer walks over the heap from low address
to high addresses, and we use a first-fit address-ordered allocation
policy.
Updates #35788.
Change-Id: Id335603b526ce3a0eb79ef286d1a4e876abc9cab
Reviewed-on: https://go-review.googlesource.com/c/go/+/218997
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: David Chase <drchase@google.com>
Michael Anthony Knyszek [Thu, 21 Nov 2019 17:05:14 +0000 (17:05 +0000)]
runtime: remove scavAddr in favor of address ranges
This change removes the concept of s.scavAddr in favor of explicitly
reserving and unreserving address ranges. s.scavAddr has several
problems with raciness that can cause the scavenger to miss updates, or
move it back unnecessarily, forcing future scavenge calls to iterate
over searched address space unnecessarily.
This change achieves this by replacing scavAddr with a second addrRanges
which is cloned from s.inUse at the end of each sweep phase. Ranges from
this second addrRanges are then reserved by scavengers (with the
reservation size proportional to the heap size) who are then able to
safely iterate over those ranges without worry of another scavenger
coming in.
Fixes #35788.
Change-Id: Ief01ae170384174875118742f6c26b2a41cbb66d
Reviewed-on: https://go-review.googlesource.com/c/go/+/208378
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Austin Clements <austin@google.com>
Daniel Martí [Tue, 31 Mar 2020 11:20:15 +0000 (12:20 +0100)]
encoding/json: properly encode strings with ",string" again
golang.org/cl/193604 fixed one bug when one encodes a string with the
",string" option: if SetEscapeHTML(false) is used, we should not be
using HTML escaping for the inner string encoding. The CL correctly
fixed that.
The CL also tried to speed up this edge case. By avoiding an entire new
call to Marshal, the new Issue34127 benchmark reduced its time/op by
45%, and lowered the allocs/op from 3 to 2.
However, that last optimization wasn't correct:
Since Go 1.2 every string can be marshaled to JSON without error
even if it contains invalid UTF-8 byte sequences. Therefore
there is no need to use Marshal again for the only reason of
enclosing the string in double quotes.
JSON string encoding isn't just about adding quotes and taking care of
invalid UTF-8. We also need to escape some characters, like tabs and
newlines.
The new code failed to do that. The bug resulted in the added test case
failing to roundtrip properly; before our fix here, we'd see an error:
invalid use of ,string struct tag, trying to unmarshal "\"\b\f\n\r\t\"\\\"" into string
If you pay close attention, you'll notice that the special characters
like tab and newline are only encoded once, not twice. When decoding
with the ",string" option, the outer string decode works, but the inner
string decode fails, as we are now decoding a JSON string with unescaped
special characters.
The fix we apply here isn't to go back to Marshal, as that would
re-introduce the bug with SetEscapeHTML(false). Instead, we can use a
new encode state from the pool - it results in minimal performance
impact, and even reduces allocs/op further. The performance impact seems
fair, given that we need to check the entire string for characters that
need to be escaped.
name old time/op new time/op delta
Issue34127-8 89.7ns ± 2% 100.8ns ± 1% +12.27% (p=0.000 n=8+8)
name old alloc/op new alloc/op delta
Issue34127-8 40.0B ± 0% 32.0B ± 0% -20.00% (p=0.000 n=8+8)
name old allocs/op new allocs/op delta
Issue34127-8 2.00 ± 0% 1.00 ± 0% -50.00% (p=0.000 n=8+8)
Instead of adding another standalone test, we convert an existing
"string tag" test to be table-based, and add another test case there.
One test case from the original CL also had to be amended, due to the
same problem - when escaping '<' due to SetEscapeHTML(true), we need to
end up with double escaping, since we're using ",string".
Fixes #38173.
Change-Id: I2b0df9e4f1d3452fff74fe910e189c930dde4b5b
Reviewed-on: https://go-review.googlesource.com/c/go/+/226498
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Since the ConnectionState will now be available during
verification, some code was moved around in order to
initialize and make available as much of the fields on
Conn as possible before the ConnectionState is verified.
Automatically rotate session ticket keys for servers
that don't already have sessionTicketKeys and that
haven't called SetSessionTicketKeys.
Now, session ticket keys will be rotated every 24 hours
with a lifetime of 7 days. This adds a small performance
cost to existing clients that don't provide a session
ticket encrypted with a fresh enough session ticket key,
which would require a full handshake.
Roland Shoemaker [Wed, 29 Apr 2020 18:24:15 +0000 (18:24 +0000)]
encoding/asn1: sort order of 'SET of' components during Marshal
Per X690 Section 11.6 sort the order of SET of components when generating
DER. This CL makes no changes to Unmarshal, meaning unordered components
will still be accepted, and won't be re-ordered during parsing.
In order to sort the components a new encoder, setEncoder, which is similar
to multiEncoder is added. The functional difference is that setEncoder
encodes each component to a [][]byte, sorts the slice using a sort.Sort
interface, and then writes it out to the destination slice. The ordering
matches the output of OpenSSL.
Filippo Valsorda [Fri, 1 May 2020 02:35:35 +0000 (22:35 -0400)]
crypto/x509: treat hostnames with colons as invalid
Colons are port separators, so it's risky to allow them in hostnames.
Per the CL 231377 rule, if we at least consider them invalid we will not
apply wildcard processing to them, making behavior a little more
predictable.
We were considering hostnames with colons valid (against spec) because
that meant we'd not ignore them in Common Name. (There was at least
one deployment that was putting colons in Common Name and expecting it
to verify.)
Now that Common Name is ignored by default, those clients will break
again, so it's a good time to drop the exception. Hopefully they moved
to SANs, where invalid hostnames are checked 1:1 (ignoring wildcards)
but still work. (If they didn't, this change means they can't use
GODEBUG=x509ignoreCN=0 to opt back in, but again you don't get to use a
legacy deprecated field AND invalid hostnames.)
Filippo Valsorda [Fri, 1 May 2020 01:24:25 +0000 (21:24 -0400)]
crypto/x509: treat certificate names with trailing dots as invalid
Trailing dots are not allowed in certificate fields like CN and SANs
(while they are allowed and ignored as inputs to verification APIs).
Move to considering names with trailing dots in certificates as invalid
hostnames.
Following the rule of CL 231378, these invalid names lose wildcard
processing, but can still match if there is a 1:1 match, trailing dot
included, with the VerifyHostname input.
They also become ignored Common Name values regardless of the
GODEBUG=x509ignoreCN=X value, because we have to ignore invalid
hostnames in Common Name for #24151. The error message automatically
accounts for this, and doesn't suggest the environment variable. You
don't get to use a legacy deprecated field AND invalid hostnames.
(While at it, also consider wildcards in VerifyHostname inputs as
invalid hostnames, not that it should change any observed behavior.)
Filippo Valsorda [Fri, 1 May 2020 00:43:59 +0000 (20:43 -0400)]
crypto/x509: ignore Common Name by default
Common Name has been deprecated for 20 years, and has horrible
interactions with Name Constraints. The browsers managed to drop it last
year, let's try flicking the switch to disabled by default.
Return helpful errors for things that would get unbroken by flipping the
switch back with the environment variable.
Had to refresh a test certificate that was too old to have SANs.
Filippo Valsorda [Fri, 1 May 2020 00:20:56 +0000 (20:20 -0400)]
crypto/x509: require perfect matches for invalid hostnames
When the input or SAN dNSNames are not valid hostnames, the specs don't
define what should happen, because this should ideally never happen, so
everything we do is undefined behavior. Browsers get to just return an
error, because browsers can assume that the resolving layer is DNS. We
can't, names can be resolved by anything implementing a Dial function,
and the crypto/x509 APIs can also be used directly without actual
networks in sight.
Trying to process invalid hostnames leads to issues like #27591 where
wildcards glob stuff they aren't expected to, because wildcards are only
defined on hostnames.
Try to rationalize the behavior like this: if both the VerifyHostname
input and the SAN dNSNames are a valid hostname, follow the specs;
otherwise, only accept perfect 1:1 case-insensitive matches (without
wildcards or trailing dot processing).
This should allow us to keep supporting weird names, with less
unexpected side-effects from undefined behavior. Also, it's a rule, even
if completely made up, so something we can reason about and code against.
The commonName field does allow any string, but no specs define how to
process it. Processing it differently from dNSNames would be confusing,
and allowing it to match invalid hostnames is incompatible with Name
Constraint processing (#24151).
This does encourage invalid dNSNames, regrettably, but we need some way
for the standard API to match weird names, and the alternative of
keeping CN alive sounds less appealing.