Keith Randall [Mon, 4 May 2020 16:50:20 +0000 (09:50 -0700)]
cmd/compile: restrict bit test rewrite rules
The {AND,OR,XOR}const ops can only take an int32 as an argument.
Make sure that when rewriting a BTx op to one of these, the result
has no high-order bits.
crypto/x509: don't read symlinked root certs from disk twice
On Linux distros at least, it's common for cert directories to have
symlinks pointing to other certs or even other symlinks. An example from
Debian stretch's /etc/ssl/certs directory:
Dmitri Shuralyov [Fri, 1 May 2020 22:58:41 +0000 (18:58 -0400)]
all: update vendored dependencies for Go 1.15 release
The Go 1.15 code freeze has just started. This is the time to update
all golang.org/x/... module versions that contribute packages to the
std and cmd modules in the standard library to latest master versions.
Those versions have already gone through code review, and now they
will undergo additional testing during the freeze period.
If there are new issues in these dependencies discovered, we have
the freeze period to deal with that. By the end of the freeze period,
we will have confidence that the Go 1.15 release and the dependency
versions it has selected are robust.
If one of the Go 1.15.x minor releases requires changing code in one of
the vendored packages, we'll be able to do so on top of the versions
that are selected here, and not be forced to use versions that came
from different time periods, or try to jump across multiple untested
versions in a minor release.
The dependency versions that are selected in this commit are:
Kezhu Wang [Mon, 4 May 2020 02:28:39 +0000 (02:28 +0000)]
reflect: keep RO flags unchanged in Value.Addr
Currently, Value.Addr collapses flagRO, which is a combination of
flagEmbedRO and flagStickyRO, to flagStickyRO. This causes exported
fields of unexported anonymous field from Value.Addr.Elem read only.
This commit fix this by keeping all bits of flagRO from origin
value in Value.Addr. This should be safe due to following reasons:
* Result of Value.Addr is not CanSet because of it is not CanAddr
but not flagRO.
* Addr.Elem get same flagRO as origin, so it should behave same as
origin in CanSet.
Fixes #32772.
Change-Id: I79e086628c0fb6569a50ce63f3b95916f997eda1
GitHub-Last-Rev: 78e280e6d06865661b5835def74c252c94a92800
GitHub-Pull-Request: golang/go#32787
Reviewed-on: https://go-review.googlesource.com/c/go/+/183937 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Jay Conrod [Fri, 1 May 2020 19:30:58 +0000 (15:30 -0400)]
cmd/go/internal/modload: make QueryPattern consider current versions
QueryPattern will now look up the current version of a module (if any)
before invoking queryProxy. This changes the interpretation of some
patterns (like "upgrade") and avoids the need to download earlier
versions for earlier versions when the current version is
+incompatible.
Fixes #37574
Change-Id: I4089d6099236493df13a7f88a252b5e5e556d383
Reviewed-on: https://go-review.googlesource.com/c/go/+/231599 Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
CL 208617 introduced syscall.utf16PtrToString and
internal/syscall/windows.UTF16PtrToString functions.
Original version of CL 208617 did not include syscall.utf16PtrToString
and internal/syscall/windows.UTF16PtrToString max parameter. The
parameter was added by Brad at the request of Ian. Ian said:
"In some cases it seems at least possible that the null terminator is
not present. I think it would be safer if we passed a maximum length
here."
The syscall.utf16PtrToString and
internal/syscall/windows.UTF16PtrToString function are designed to work
with only null terminated strings. So max parameter is superfluous.
This change removes max parameter.
Updates #34972
Change-Id: Ifea65dbd86bca8a08353579c6b9636c6f963d165
Reviewed-on: https://go-review.googlesource.com/c/go/+/228858
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Daniel Kumor [Tue, 7 Jan 2020 02:16:40 +0000 (02:16 +0000)]
net/http/httputil: handle escaped paths in SingleHostReverseProxy
When forwarding a request, a SingleHostReverseProxy appends the
request's path to the target URL's path. However, if certain path
elements are encoded, (such as %2F for slash in either the request or
target path), simply joining the URL.Path elements is not sufficient,
since the field holds the decoded path.
Since 87a605, the RawPath field was added which holds a decoding
hint for the URL. When joining URL paths, this decoding hint needs
to be taken into consideration.
As an example, if the target URL.Path is /a/b, and URL.RawPath
is /a%2Fb, joining the path with /c should result in /a/b/c
in URL.Path, and /a%2Fb/c in RawPath.
The added joinURLPath function combines the two URL's Paths,
while taking into account escaping, and replaces the previously used
singleJoiningSlash in NewSingleHostReverseProxy.
Joel Sing [Sat, 25 Apr 2020 18:34:34 +0000 (04:34 +1000)]
cmd/compile: use SEQZ pseudo instruction in RISCV64 boolean rules
This makes the intent clearer, allows for another ellipsis and will aid
in future rewriting. While here, document boolean loads to explain register
contents.
Change-Id: I933db2813826d88819366191fbbea8fcee5e4dda
Reviewed-on: https://go-review.googlesource.com/c/go/+/230120 Reviewed-by: Keith Randall <khr@golang.org>
Ian Lance Taylor [Fri, 1 May 2020 19:26:30 +0000 (12:26 -0700)]
syscall: if Setctty, require that Ctty be a child descriptor
Ctty was always handled as a child descriptor, but in some cases
passing a parent descriptor would also work. This depended on
unpredictable details of the implementation. Reject those cases to
avoid confusion.
Also reject setting both Setctty and Foreground, as they use Ctty
in incompatible ways. It's unlikely that any programs set both fields,
as they don't make sense together.
Fixes #29458
Change-Id: Ieba2d625711fd4b82c8e65e1feed02fd1fb25e6d
Reviewed-on: https://go-review.googlesource.com/c/go/+/231638
Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Damien Neil <dneil@google.com>
Dmitri Shuralyov [Fri, 28 Feb 2020 01:20:39 +0000 (20:20 -0500)]
cmd/gofmt, go/format, go/printer: move number normalization to printer
Normalization of number prefixes and exponents was added in CL 160184
directly in cmd/gofmt. The same behavior change needs to be applied in
the go/format package. This is done by moving the normalization code
into go/printer, behind a new StdFormat mode, which is then re-used
by both cmd/gofmt and go/format.
Note that formatting of Go source code changes over time, so the exact
byte output produced by go/printer may change between versions of Go
when using StdFormat mode. What is guaranteed is that the new formatting
is equivalent Go code.
Clients looking to format Go code with standard formatting consistent
with cmd/gofmt and go/format would need to start using this flag, but
a better alternative is to use the go/format package instead.
Benchstat numbers on go test go/printer -bench=BenchmarkPrint:
name old time/op new time/op delta
Print-8 4.56ms ± 1% 4.57ms ± 0% ~ (p=0.700 n=3+3)
name old alloc/op new alloc/op delta
Print-8 467kB ± 0% 467kB ± 0% ~ (p=1.000 n=3+3)
name old allocs/op new allocs/op delta
Print-8 17.2k ± 0% 17.2k ± 0% ~ (all equal)
That benchmark data doesn't contain any numbers that need to be
normalized. More work needs to be performed when formatting Go code
with numbers, but it is unavoidable to produce standard formatting.
Fixes #37476.
For #37453.
Change-Id: If50bde4035c3ee6e6ff0ece5691f6d3566ffe8d5
Reviewed-on: https://go-review.googlesource.com/c/go/+/231461
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@golang.org>
This commit adds a new option to the x86 assembler. If the
GOAMD64 environment variable is set to alignedjumps (the
default) and we're doing a 64 bit build, the assembler will
make sure that neither stand alone nor macro-fused jumps will
end on or cross 32 byte boundaries. To achieve this, functions
are aligned on 32 byte boundaries, rather than 16 bytes, and
jump instructions are padded to ensure that they do not
cross or end on 32 byte boundaries. Jumps are padded
by adding a NOP instruction of the appropriate length before
the jump.
The commit is likely to result in larger binary sizes when
GOAMD64=alignedjumps. On the binaries tested so far, an
increase of between 1.4% and 1.5% has been observed.
Matthew Dempsky [Wed, 30 Nov 2016 01:27:15 +0000 (17:27 -0800)]
go/types: add UsesCgo config to support _cgo_gotypes.go
(Reland of golang.org/cl/33677.)
This CL adds a UsesCgo config setting to go/types to specify that the
_cgo_gotypes.go file generated by cmd/cgo has been provided as a
source file. The type checker then internally resolves C.bar qualified
identifiers to _Cfoo_bar as appropriate.
It also adds support to srcimporter to automatically run cgo.
Unfortunately, this functionality is not compatible with overriding
OpenFile, because cmd/cgo and gcc will directly open files.
Updates #16623.
Updates #35721.
Change-Id: Ib179d55c8c589916f98ceeae0b9a3e746157253a
Reviewed-on: https://go-review.googlesource.com/c/go/+/231459
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@golang.org>
Matthew Dempsky [Fri, 1 May 2020 06:05:51 +0000 (23:05 -0700)]
cmd/cgo: use type aliases for #define type macros
Cgo's initial design for handling "#define foo int*" involved
rewriting "C.foo" to "*_Ctype_int" everywhere. But now that we have
type aliases, we can declare "type _Ctype_foo = *_Ctype_int" once, and
then rewrite "C.foo" to just "_Ctype_foo".
This is important for go/types's UsesCgo mode, where go/types needs to
be able to figure out a type for each C.foo identifier using only the
information written into _cgo_gotypes.go.
Fixes #38649.
Change-Id: Ia0f8c2d82df81efb1be5bc26195ea9154c0af871
Reviewed-on: https://go-review.googlesource.com/c/go/+/230037
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Cherry Zhang [Fri, 1 May 2020 14:14:00 +0000 (10:14 -0400)]
cmd: merge branch 'dev.link' into master
In the dev.link branch we continued developing the new object
file format support and the linker improvements described in
https://golang.org/s/better-linker . Since the last merge, more
progress has been made to improve the new linker, with
improvements on both linker speed and memory usage.
Ian Lance Taylor [Fri, 24 Apr 2020 04:15:03 +0000 (21:15 -0700)]
syscall: document exact meaning of Ctty field
The Ctty field is a child descriptor number when Setctty is set,
but a parent descriptor when Foreground is set. This is absurd
but changing either behavior breaks existing programs.
With this change we at least document how it works.
For #29458
Change-Id: If9cf0a1a1e6ed0d4a4edae5043016d5b4ee3308b
Reviewed-on: https://go-review.googlesource.com/c/go/+/229768
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Damien Neil <dneil@google.com>
Change-Id: I59a2ed52563851c693b2c8dfce7e3cde640f62a9
Reviewed-on: https://go-review.googlesource.com/c/go/+/231120 Reviewed-by: Ian Lance Taylor <iant@golang.org>
internal/unsafeheader: consolidate stringHeader and sliceHeader declarations into an internal package
The new package "internal/unsafeheader" depends only on "unsafe", and
provides declarations equivalent to reflect.StringHeader and
reflect.SliceHeader but with Data fields of the proper unsafe.Pointer
type (instead of uintptr).
Unlike the types it replaces, the "internal/unsafeheader" package has
a regression test to ensure that its header types remain equivalent to
the declarations provided by the "reflect" package.
Since "internal/unsafeheader" has almost no dependencies, it can be
used in other low-level packages such as "syscall" and "reflect".
This change is based on the corresponding x/sys change in CL 231177.
Fixes #37805
Updates #19367
Change-Id: I7a6d93ef8dd6e235bcab94e7c47270aad047af31
Reviewed-on: https://go-review.googlesource.com/c/go/+/231223 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Than McIntosh [Thu, 30 Apr 2020 20:07:31 +0000 (16:07 -0400)]
[dev.link] cmd/link: tweaks to data alignment processing
Now that the loader's internal storage mechanism for symbol alignment
is array-based and not map-based, we can go back to computing symbol
alignment in the parallel-by-section section of dodata.
With this patch plus the previous one, this produces a small
kubelet speedup:
$ benchstat out.devlink.txt out.align.txt
name old time/op new time/op delta
RelinkKubelet 13.3s ± 2% 13.1s ± 2% -1.27% (p=0.000 n=20+20)
RelinkKubelet-WithoutDebug 7.36s ± 5% 7.14s ± 3% -3.00% (p=0.000 n=20+20)
Change-Id: I9eb0e8fea6aeb12f188f499e9031d5a3a23232c7
Reviewed-on: https://go-review.googlesource.com/c/go/+/231221
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
Than McIntosh [Thu, 30 Apr 2020 20:01:03 +0000 (16:01 -0400)]
[dev.link] cmd/link/internal/loader: change storage mechanism for sym alignment
Switch the storage mechanism for symbol alignment away from a map and
to a slice of uint8 values per symbol, where value K indicates
alignment 2^K. Intended to help speed up alignment get/set in dodata.
Change-Id: I26416e455c808f697dd0d7f6d2582247ee5c5b40
Reviewed-on: https://go-review.googlesource.com/c/go/+/231220
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
Anton Kuklin [Fri, 24 Apr 2020 23:33:30 +0000 (02:33 +0300)]
cmd: disable *.go domains lookup in go get command
Using 'go get x.go' instead of 'go build x.go' or some other
go command is a common mistake. By that mistake, a user gets
a misleading error message about unsuccessful `x.go` domain lookup.
This improvement handles such cases, by validating, whether the
argument hasn't specified version, has .go suffix, and either has
no slashes or such file locally exists. Handled both GOPATH
and GOMOD modes.
Fixes #38478
Change-Id: I583a4ef7f7ca8901deb07ebc811e2b3c0e828fa6
Reviewed-on: https://go-review.googlesource.com/c/go/+/229938 Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Robert Griesemer [Thu, 30 Apr 2020 20:24:05 +0000 (13:24 -0700)]
strconv: fix for parseFloatPrefix
parseFloatPrefix accepts a string if it has a valid floating-point
number as prefix. Make sure that "infi", "infin", ... etc. are
accepted as valid numbers "inf" with suffix "i", "in", etc. This
is important for parsing complex numbers such as "0+infi".
This change does not affect the correctness of ParseFloat because
ParseFloat rejects strings that contain a suffix after a valid
floating-point number.
Updates #36771.
Change-Id: Ie1693a8ca2f8edf07b57688e0b35751b7100d39d
Reviewed-on: https://go-review.googlesource.com/c/go/+/231237
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
[dev.link] cmd/link: use more compact representation for external relocations
Currently, for external relocations, the ExtReloc structure
contains all the fields of the relocation. In fact, many of the
fields are the same with the original relocation. So, instead, we
can just use an index to reference the original relocation and
not expand the fields.
There is one place where we modify relocation type: changing
R_DWARFSECTREF to R_ADDR. Get away with it by changing
downstreams.
It also makes it easier to retrieve the reloc variant.
This reduces some allocation. Linking cmd/compile with external
linking,
name old alloc/op new alloc/op delta
Reloc_GC 34.1MB ± 0% 22.7MB ± 0% -33.30% (p=0.000 n=5+4)
Change-Id: Id08a89ed2aee705296886d3b95014b806a0d55cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/231217
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
In the dev.link branch we continued developing the new object
file format support and the linker improvements described in
https://golang.org/s/better-linker . Since the last merge, more
progress has been made to improve the new linker.
Than McIntosh [Thu, 30 Apr 2020 14:19:28 +0000 (10:19 -0400)]
[dev.link] cmd/link: performance changes for relocsym
Revise the signature for "relocsym" to reflect the fact that many of
its arguments are invariant: push the invariant args into a struct and
pass the struct by reference.
Add a facility for doing batch allocation of external relocations in
relocsym, so that we don't wind up with wasted space due to the
default "append" behavior.
This produces a small speedup in linking kubelet:
$ benchstat out.devlink.txt out.dodata.txt
name old time/op new time/op delta
RelinkKubelet 14.2s ± 2% 13.8s ± 2% -3.11% (p=0.000 n=19+19)
RelinkKubelet-WithoutDebug 8.02s ± 3% 7.73s ± 3% -3.67% (p=0.000 n=20+20)
Michael Anthony Knyszek [Thu, 30 Apr 2020 19:13:41 +0000 (19:13 +0000)]
runtime: add scavenge -> traceBuf to lock partial order
Under the scavenge lock it's possible to ready a goroutine (or now
injectglist, which has mostly the same effect) which could cause an
unpark trace event to be emitted. If there's no active trace buffer for
the P, then we might acquire the lock. The total order between the two
is correct, but there's no partial order edge between them. Add in the
edge.
Change-Id: I3fc5d86a3b6bdd0b5648181fb76b5ebc90c3d69f
Reviewed-on: https://go-review.googlesource.com/c/go/+/231197
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Dan Scales <danscales@google.com>
Michael Anthony Knyszek [Tue, 19 Nov 2019 17:32:17 +0000 (17:32 +0000)]
runtime: wake scavenger and update address on sweep done
This change modifies the semantics of waking the scavenger: rather than
wake on any update to pacing, wake when we know we will have work to do,
that is, when the sweeper is done. The current scavenger runs over the
address space just once per GC cycle, and we want to maximize the chance
that the scavenger observes the most attractive scavengable memory in
that pass (i.e. free memory with the highest address), so the timing is
important. By having the scavenger awaken and reset its search space
when the sweeper is done, we increase the chance that the scavenger will
observe the most attractive scavengable memory, because no more memory
will be freed that GC cycle (so the highest scavengable address should
now be available).
Furthermore, in applications that go idle, this means the background
scavenger will be awoken even if another GC doesn't happen, which isn't
true today.
However, we're unable to wake the scavenger directly from within the
sweeper; waking the scavenger involves modifying timers and readying
goroutines, the latter of which may trigger an allocation today (and the
sweeper may run during allocation!). Instead, we do the following:
1. Set a flag which is checked by sysmon. sysmon will clear the flag and
wake the scavenger.
2. Wake the scavenger unconditionally at sweep termination.
The idea behind this policy is that it gets us close enough to the state
above without having to deal with the complexity of waking the scavenger
in deep parts of the runtime. If the application goes idle and sweeping
finishes (so we don't reach sweep termination), then sysmon will wake
the scavenger. sysmon has a worst-case 20 ms delay in responding to this
signal, which is probably fine if the application is completely idle
anyway, but if the application is actively allocating, then the
proportional sweeper should help ensure that sweeping ends very close to
sweep termination, so sweep termination is a perfectly reasonable time
to wake up the scavenger.
Michael Anthony Knyszek [Fri, 24 Apr 2020 14:46:28 +0000 (14:46 +0000)]
runtime: make the scavenger's pacing logic more defensive
This change adds two bits of logic to the scavenger's pacing. Firstly,
it checks to make sure we scavenged at least one physical page, if we
released a non-zero amount of memory. If we try to release less than one
physical page, most systems will release the whole page, which could
lead to memory corruption down the road, and this is a signal we're in
this situation.
Secondly, the scavenger's pacing logic now checks to see if the time a
scavenging operation takes is measured to be exactly zero or negative.
The exact zero case can happen if time update granularity is too large
to effectively capture the time the scavenging operation took, like on
Windows where the OS timer frequency is generally 1ms. The negative case
should not happen, but we're being defensive (against kernel bugs, bugs
in the runtime, etc.). If either of these cases happen, we fall back to
Go 1.13 behavior: assume the scavenge operation took around 10µs per
physical page. We ignore huge pages in this case because we're in
unknown territory, so we choose to be conservative about pacing (huge
pages could only increase the rate of scavenging).
Currently, the scavenger is broken on Windows because the granularity of
time measurement is around 1 ms, which is too coarse to measure how fast
we're scavenging, so we often end up with a scavenging time of zero,
followed by NaNs and garbage values in the pacing logic, which usually
leads to the scavenger sleeping forever.
Fixes #38617.
Change-Id: Iaaa2a4cbb21338e1258d010f7362ed58b7db1af7
Reviewed-on: https://go-review.googlesource.com/c/go/+/229997
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>
Than McIntosh [Wed, 29 Apr 2020 22:46:44 +0000 (18:46 -0400)]
[dev.link] cmd/link: minor performance tweaks in dodata
Tweak doDataSect to reduce symbol sorting overhead, and calculate size
ahead of allocating the ctxt.datap slice. Yields a small speedup
(2-3%) linking kubelet.
Change-Id: I82869f5276caa4bee9f6e6f41da2b240e601ce50
Reviewed-on: https://go-review.googlesource.com/c/go/+/231047
Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
[dev.link] cmd/link: unescape relocs passed to Archreloc2
Archreloc2 is a function pointer. It will escape its pointer
arguments. In relocsym, as we pass &r and &rr to Archreloc2, it
causes them to escape, even if Archreloc2 is not actually called.
Instead, pass r by value. loader.Reloc2 is a small structure
which is intended to be passed by value.
For rr, as Archreloc2 will likely return true, we speculatively
add it to extRelocs slice and use that space to pass to
Archreloc2.
Linking cmd/compile,
name old alloc/op new alloc/op delta
Dwarfcompress_GC 110MB ± 0% 24MB ± 0% -78.34% (p=0.008 n=5+5)
Reloc_GC 24.6MB ± 0% 0.0MB ± 0% -100.00% (p=0.029 n=4+4)
Linking cmd/compile using external linking
name old alloc/op new alloc/op delta
Reloc_GC 152MB ± 0% 36MB ± 0% -76.07% (p=0.008 n=5+5)
Change-Id: I1415479e0c17ea9787f9a62453dce00ad9ea792f
Reviewed-on: https://go-review.googlesource.com/c/go/+/231077
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
net/http/httputil: don't append to X-Forwarded-For in ReverseProxy when nil
Fixes #38079
Change-Id: Iac02d7f9574061bb26d1d9a41bb6ee6cc38934e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/230937 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Daniel Martí [Fri, 24 Apr 2020 09:53:17 +0000 (10:53 +0100)]
cmd/go: make 'mod verify' use multiple CPUs
'go mod verify' checksums one module zip at a time, which is
CPU-intensive on most modern machines with fast disks. As a result, one
can see a CPU bottleneck when running the command on, for example, a
module where 'go list -m all' lists ~440 modules:
$ /usr/bin/time go mod verify
all modules verified
11.47user 0.77system 0:09.41elapsed 130%CPU (0avgtext+0avgdata 24284maxresident)k
0inputs+0outputs (0major+4156minor)pagefaults 0swaps
Instead, verify up to GOMAXPROCS zips at once, which should line up
pretty well with the amount of processors we can use on a machine. The
results below are obtained via 'benchcmd -n 5 GoModVerify go mod verify'
on the same large module.
name old time/op new time/op delta
GoModVerify 9.35s ± 1% 3.03s ± 2% -67.60% (p=0.008 n=5+5)
name old user-time/op new user-time/op delta
GoModVerify 11.2s ± 1% 16.3s ± 3% +45.38% (p=0.008 n=5+5)
name old sys-time/op new sys-time/op delta
GoModVerify 841ms ± 9% 865ms ± 8% ~ (p=0.548 n=5+5)
name old peak-RSS-bytes new peak-RSS-bytes delta
GoModVerify 27.8MB ±13% 50.7MB ±27% +82.01% (p=0.008 n=5+5)
The peak memory usage nearly doubles, and there is some extra overhead,
but it seems clearly worth the tradeoff given that we see a ~3x speedup
on my laptop with 4 physical cores. The vast majority of developer
machines nowadays should have 2-4 cores at least.
No test or benchmark is included; one can benchmark 'go mod verify'
directly, as I did above. The existing tests also cover correctness,
including any data races via -race.
Fixes #38623.
Change-Id: I45d8154687a6f3a6a9fb0e2b13da4190f321246c
Reviewed-on: https://go-review.googlesource.com/c/go/+/229817
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Rob Pike [Tue, 28 Apr 2020 15:45:59 +0000 (01:45 +1000)]
cmd/cover: include a package name in the HTML title
A recent change added a title to the HTML coverage report but
neglected to include the package name. Add the package name here.
It's a little trickier than you'd think because there may be multiple
packages and we don't want to parse the files, so we just extract
a directory name from the path of the first file. This will almost
always be right, and has the advantage that it gives a better result
for package main. There are rare cases it will get wrong, but that
will be no hardship.
If this turns out not to be good enough, we can refine it.
Fixes #38609
Change-Id: I2201f6caef906e0b0258b90d7de518879041fe72
Reviewed-on: https://go-review.googlesource.com/c/go/+/230517 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Robert Griesemer [Tue, 28 Apr 2020 23:54:44 +0000 (16:54 -0700)]
strconv: implement parseFloatPrefix returning no. of bytes consumed
parseFloatPrefix will make it easier to implement ParseComplex.
Verified that there's no relevant performance impact:
Benchmarks run on a "quiet" MacBook Pro, 3.3GHz Dual-Core Intel Core i7,
with 16GB 2133MHz LPDDR3 RAM running macOS 10.15.4.
Change-Id: I8ff66b582ae8b468d89c9ffc35c569c735cf0341
Reviewed-on: https://go-review.googlesource.com/c/go/+/230737 Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/compile: omit file:pos for non-existent, permission errors
Omits printing the file:line:column when trying to open either
* non-existent files
* files without permission
Given:
go tool compile x.go
For either of x.go not existing, or if no read permissions:
* Before:
x.go:0: open x.go: no such file or directory
x.go:0: open x.go: permission denied
* After:
open x.go: no such file or directory
open x.go: permission denied
While here, noticed an oddity with the Linux builders, that appear
to always be running under root, hence the test for permission errors
with 0222 -W-*-W-*-W- can't pass on linux-amd64 builders.
The filed bug is #38608.
When I browsed the source code, I saw that there is no corresponding example of this function. I am not sure if there is a need for an increase, this is my first time to submit CL.
Change-Id: Idbf4e1e1ed2995176a76959d561e152263a2fd26
Reviewed-on: https://go-review.googlesource.com/c/go/+/230741
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@golang.org>
Matthew Dempsky [Wed, 29 Apr 2020 23:45:05 +0000 (16:45 -0700)]
net/http/cgi: replace constant map with switch statement
The switch statement can be statically optimized by the compiler,
whereas similarly optimizing the map index expression would require
additional compiler analysis to detect the map is never mutated.
cmd/compile,runtime: stack maps only at calls, remove register maps
Currently, we emit stack maps and register maps at almost every
instruction. This was originally intended to support non-cooperative
preemption, but was only ever used for debug call injection. Now debug
call injection also uses conservative frame scanning. As a result,
stack maps are only needed at call sites and register maps aren't
needed at all except that we happen to also encode unsafe-point
information in the register map PCDATA stream.
This CL reduces stack maps to only appear at calls, and replace full
register maps with just safe/unsafe-point information.
This is all protected by the go115ReduceLiveness feature flag, which
is defined in both runtime and cmd/compile.
This CL significantly reduces binary sizes and also speeds up compiles
and links:
name old exe-bytes new exe-bytes delta
BinGoSize 15.0MB ± 0% 14.1MB ± 0% -5.72%
name old pcln-bytes new pcln-bytes delta
BinGoSize 3.14MB ± 0% 2.48MB ± 0% -21.08%
The binary size improvement is even slightly better when you include
the CLs leading up to this. Relative to the parent of "cmd/compile:
mark PanicBounds/Extend as calls":
name old exe-bytes new exe-bytes delta
BinGoSize 15.0MB ± 0% 14.1MB ± 0% -6.18%
name old pcln-bytes new pcln-bytes delta
BinGoSize 3.22MB ± 0% 2.48MB ± 0% -22.92%
cmd/compile: don't emit stack maps for write barrier calls
These are necessarily deeply non-preemptible, so there's no point in
emitting stack maps for them. We already mark them as unsafe points,
so this only affects the runtime, since user code does not emit stack
maps at unsafe points. SSAGenState.PrepareCall also excludes them when
it's sanity checking call stack maps.
Right now this only drops a handful of unnecessary stack maps from the
runtime, but we're about to start emitting stack maps only at calls
for user code, too. At that point, this will matter much more.
The compiler currently conflates whether a Value has a stack map with
whether it's an unsafe point. For the most part, unsafe-points don't
have stack maps, so this is mostly fine, but call instructions can be
both an unsafe-point *and* have a stack map. For example, none of the
instructions in a nosplit function should be preemptible, but calls
must still have stack maps in case the called function grows the stack
or get preempted.
Currently, the compiler can't distinguish this case, so calls in
nosplit functions are marked as safe-points just because they have
stack maps. This is particularly problematic if a nosplit function
calls another nosplit function, since this can introduce a preemption
point where there should be none.
We realized this was a problem for split-stack prologues a while back,
and CL 207349 changed the encoding of unsafe-points to use the
register map index instead of the stack map index so we could record
both a stack map and an unsafe-point at the same instruction. But this
was never extended into the compiler.
This CL fixes this problem in the compiler. We make LivenessIndex
slightly more abstract by separating unsafe-point marks from stack and
register map indexes. We map this to the PCDATA encoding later when
producing Progs. This isn't enough to fix the whole problem for
nosplit functions, because obj still adds prologues and marks those as
preemptible, but it's a step in the right direction.
I checked this CL by comparing maps before and after this change in
the runtime and net/http. In net/http, unsafe-points match exactly; at
anything that isn't an unsafe-point, both the stack and register maps
are unchanged by this CL. In the runtime, at every point that was a
safe-point before this change, the stack maps agree (and mostly the
runtime doesn't have register maps at all now). In both, all CALLs
(except write barrier calls) have stack maps.
Currently, this function conflates two (easily conflated!) concepts:
whether a Value is a safe-point and whether it has a stack map. In
particular, call Values may not be a safe-point, but may need a stack
map anyway in case the called function grows the stack.
Hence, rename this function to "hasStackMap", since that's really what
it represents.
PanicBounds and PanicExtend are lowered to runtime calls (with a
non-Go ABI), but are not currently marked as calls. Since liveness
analysis only emits stack maps at calls in the runtime, this means
these panic call sites in the runtime won't get a stack map. These
almost immediately turn into throws in the runtime, but there's still
a chance they'll try to grow the stack first, which would lead to a
different panic.
To fix this, mark these operations as calls.
Outside the runtime, we currently emit stack maps for everything that
isn't an unsafe-point, so these panic calls get stack maps by default.
However, we're about to move to emitting stack maps only at call
sites, at which point this will start to matter outside the runtime as
well.
I confirmed that this has no effect on anything but PCDATA/FUNCDATA in
runtime and net/http.
runtime: use conservative scanning for debug calls
A debugger can inject a call at almost any PC, which causes
significant complications with stack scanning and growth. Currently,
the runtime solves this using precise stack maps and register maps at
nearly all PCs, but these extra maps require roughly 5% of the binary.
These extra maps were originally considered worth this space because
they were intended to be used for non-cooperative preemption, but are
now used only for debug call injection.
This CL switches from using precise maps to instead using conservative
frame scanning, much like how non-cooperative preemption works. When a
call is injected, the runtime flushes all potential pointer registers
to the stack, and then treats that frame as well as the interrupted
frame conservatively.
The limitation of conservative frame scanning is that we cannot grow
the goroutine stack. That's doable because the previous CL switched to
performing debug calls on a new goroutine, where they are free to grow
the stack.
With this CL, there are no remaining uses of precise register maps
(though we still use the unsafe-point information that's encoded in
the register map PCDATA stream), and stack maps are only used at call
sites.
runtime: perform debug call injection on a new goroutine
Currently, when a debugger injects a call, that call happens on the
goroutine where the debugger injected it. However, this requires
significant runtime complexity that we're about to remove.
To prepare for this, this CL switches to a different approach that
leaves the interrupted goroutine parked and runs the debug call on a
new goroutine. When the debug call returns, it resumes the original
goroutine.
This should be essentially transparent to debuggers. It follows the
exact same call injection protocol and ensures the whole protocol
executes indivisibly on a single OS thread. The only difference is
that the current G and stack now change part way through the protocol.
Currently, newproc1 allocates, initializes, and schedules a new
goroutine. We're about to change debug call injection in a way that
will need to create a new goroutine without immediately scheduling it.
To prepare for that, make scheduling the responsibility of newproc1's
caller. Currently, there's exactly one caller (newproc), so this
simply shifts that responsibility.
Martin Möhrmann [Wed, 29 Apr 2020 10:23:32 +0000 (12:23 +0200)]
bytes, strings: align requirements for functions passed to FieldFuncs
golang.org/cl/229763 removed the documentation of requirements of
the function passed to FieldsFunc. The current implementation does
not require functions to return consistent results but this had not
been the case for previous implementations.
Add the requirement for consistent results back to the documentation
to allow for future implementations to be more allocation efficient
for an output with more than 32 fields. This is possible with a two
pass algorithm first determining the number of fields used to allocate
the output slice and then splitting the input into fields.
While at it align the documentation of bytes.FieldsFunc with
strings.FieldFunc.
Fixes #38630
Change-Id: Iabbf9ca3dff0daa41f4ec930a21a3dd98e19f122
Reviewed-on: https://go-review.googlesource.com/c/go/+/230797
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@golang.org>
Jay Conrod [Thu, 19 Dec 2019 18:43:24 +0000 (13:43 -0500)]
cmd/go: trim source paths when compiling C with -trimpath
When then go command is run with -trimpath, it will now use
-fdebug-prefix-map when invoking the C compiler (if supported) to
replace the source root directory with a dummy root directory.
This should prevent source directories from appearing either literally
or in compressed DWARF in linked binaries.
Updates #36072
Change-Id: Iedd08d5e886f81e981f11248a1be4ed4f58bdd29
Reviewed-on: https://go-review.googlesource.com/c/go/+/212101
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Richard Miller [Wed, 29 Apr 2020 10:20:28 +0000 (11:20 +0100)]
cmd/go/internal/modload: use lockedfile to read path-replacement go.mod files
When parsing go.mod files found via file-path replacements, it's safer to
use lockedfile.Read instead of ioutil.ReadFile, in case of overwriting by
other concurrent go commands.
Change-Id: I7dcac3bb5ada84bee1eb634b39f813c461ef103a
Reviewed-on: https://go-review.googlesource.com/c/go/+/230838 Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Previously, non-standard attributes in Name.Names were being
omitted when printed using Name.String(). Now, any non-standard
attributes that would not already be printed in Name.String()
are being added temporarily to Name.ExtraNames to be printed.