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.)
Updates #24151
Change-Id: Id44b4fecb2d620480acdfc65fea1473f7abbca7f
Reviewed-on: https://go-review.googlesource.com/c/go/+/231381
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
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.)
Change-Id: Iecdee8927df50c1d9daf904776b051de9f5e76ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/231380
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
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.
Updates #24151
Change-Id: I2ab78577fd936ba67969d3417284dbe46e4ae02f
Reviewed-on: https://go-review.googlesource.com/c/go/+/231379
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
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.
Fixes#27591
Change-Id: Id2d515f068a17ff796a32b30733abe44ad4f0339
Reviewed-on: https://go-review.googlesource.com/c/go/+/231378
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
If dst slice length is zero in makeslicecopy then the called mallocgc is
using a fast path to only return a pointer to runtime.zerobase.
There may be no heapBits for that address readable by
bulkBarrierPreWriteSrcOnly which will cause a panic.
Protect against this by not calling bulkBarrierPreWriteSrcOnly if
there is nothing to copy. This is the case for all cases where the
length of the destination slice is zero.
runtime.growslice and runtime.typedslicecopy have fast paths that
do not call bulkBarrierPreWrite for zero copy lengths either.
Fixes#38929
Change-Id: I78ece600203a0a8d24de5b6c9eef56f605d44e99
Reviewed-on: https://go-review.googlesource.com/c/go/+/232800
Reviewed-by: Keith Randall <khr@golang.org>
Missed in CL 221790
This is the only remaining use of math.Float64frombits in the .rules
file that isn't already guarded.
Fixes#38880
Change-Id: I11f71e3a48516748d8d2701c6cf6920a7bc9e216
Reviewed-on: https://go-review.googlesource.com/c/go/+/232859
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Currently linearAlloc manages an exclusive "end" address for the top of
its reserved space. While unlikely for a linearAlloc to be allocated
with an "end" address hitting the top of the address space, it is
possible and could lead to overflow.
Avoid overflow by chopping off the last byte from the linearAlloc if
it's bumping up against the top of the address space defensively. In
practice, this means that if 32-bit platforms map the top of the address
space and use the linearAlloc to acquire arenas, the top arena will not
be usable.
Fixes#35954.
Change-Id: I512cddcd34fd1ab15cb6ca92bbf899fc1ef22ff6
Reviewed-on: https://go-review.googlesource.com/c/go/+/231338
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Currently when checking if we can grow the heap into the current arena,
we do an addition which may overflow. This is particularly likely on
32-bit systems.
Avoid this situation by explicitly checking for overflow, and adding in
some comments about when overflow is possible, when it isn't, and why.
For #35954.
Change-Id: I2d4ecbb1ccbd43da55979cc721f0cd8d1757add2
Reviewed-on: https://go-review.googlesource.com/c/go/+/231337
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
I added routines that can acquire/release a particular rank without
acquiring/releasing an associated lock. I added lockRankGscan as a rank
for acquiring/releasing the Gscan bit.
castogscanstatus() and casGtoPreemptScan() are acquires of the Gscan
bit. casfrom_Gscanstatus() is a release of the Gscan bit. casgstatus()
is like an acquire and release of the Gscan bit, since it will wait if
Gscan bit is currently set.
We have a cycle between hchan and Gscan. The acquisition of Gscan and
then hchan only happens in syncadjustsudogs() when the G is suspended,
so the main normal ordering (get hchan, then get Gscan) can't be
happening. So, I added a new rank lockRankHchanLeaf that is used when
acquiring hchan locks in syncadjustsudogs. This ranking is set so no
other locks can be acquired except other hchan locks.
Fixes#38922
Change-Id: I58ce526a74ba856cb42078f7b9901f2832e1d45c
Reviewed-on: https://go-review.googlesource.com/c/go/+/228417
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Fix a minor bug where it should use Textp2 instead of Textp. This
doesn't affect correctness. It just made the pre-allocation less
effective.
Change-Id: Ib3fa8ab3c64037e3582933970d051f278286353b
Reviewed-on: https://go-review.googlesource.com/c/go/+/232837
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>
+----------------------------------------------------------------------+
| Hello, if you are reading this and run macOS, please test this code: |
| |
| $ GO111MODULE=on go get golang.org/dl/gotip@latest |
| $ gotip download |
| $ GODEBUG=x509roots=1 gotip test crypto/x509 -v -run TestSystemRoots |
+----------------------------------------------------------------------+
We currently have two code paths to extract system roots on macOS: one
uses cgo to invoke a maze of Security.framework APIs; the other is a
horrible fallback that runs "/usr/bin/security verify-cert" on every
root that has custom policies to check if it's trusted for SSL.
The fallback is not only terrifying because it shells out to a binary,
but also because it lets in certificates that are not trusted roots but
are signed by trusted roots, and because it applies some filters (EKUs
and expiration) only to roots with custom policies, as the others are
not passed to verify-cert. The other code path, of course, requires cgo,
so can't be used when cross-compiling and involves a large ball of C.
It's all a mess, and it broke oh-so-many times (#14514, #16532, #19436,
#20990, #21416, #24437, #24652, #25649, #26073, #27958, #28025, #28092,
#29497, #30471, #30672, #30763, #30889, #32891, #38215, #38365, ...).
Since macOS does not have a stable syscall ABI, we already dynamically
link and invoke libSystem.dylib regardless of cgo availability (#17490).
How that works is that functions in package syscall (like syscall.Open)
take the address of assembly trampolines (like libc_open_trampoline)
that jump to symbols imported with cgo_import_dynamic (like libc_open),
and pass them along with arguments to syscall.syscall (which is
implemented as runtime.syscall_syscall). syscall_syscall informs the
scheduler and profiler, and then uses asmcgocall to switch to a system
stack and invoke runtime.syscall. The latter is an assembly trampoline
that unpacks the Go ABI arguments passed to syscall.syscall, finally
calls the remote function, and puts the return value on the Go stack.
(This last bit is the part that cgo compiles from a C wrapper.)
We can do something similar to link and invoke Security.framework!
The one difference is that runtime.syscall and friends check errors
based on the errno convention, which Security doesn't follow, so I added
runtime.syscallNoErr which just skips interpreting the return value.
We only need a variant with six arguments because the calling convention
is register-based, and extra arguments simply zero out some registers.
That's plumbed through as crypto/x509/internal/macOS.syscall. The rest
of that package is a set of wrappers for Security.framework and Core
Foundation functions, like syscall is for libSystem. In theory, as long
as macOS respects ABI backwards compatibility (a.k.a. as long as
binaries built for a previous OS version keep running) this should be
stable, as the final result is not different from what a C compiler
would make. (One exception might be dictionary key strings, which we
make our own copy of instead of using the dynamic symbol. If they change
the value of those strings things might break. But why would they.)
Finally, I rewrote the crypto/x509 cgo logic in Go using those wrappers.
It works! I tried to make it match 1:1 the old logic, so that
root_darwin_amd64.go can be reviewed by comparing it to
root_cgo_darwin_amd64.go. The only difference is that we do proper error
handling now, and assume that if there is no error the return values are
there, while before we'd just check for nil pointers and move on.
I kept the cgo logic to help with review and testing, but we should
delete it once we are confident the new code works.
The nocgo logic is gone and we shall never speak of it again.
Fixes#32604Fixes#19561Fixes#38365
Awakens Cthulhu
Change-Id: Id850962bad667f71e3af594bdfebbbb1edfbcbb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/227037
Reviewed-by: Katie Hockman <katie@golang.org>
Also encode the certificates in a way that's more
consistent with TLS 1.3 (with a 24 byte length prefix).
Note that this will have an additional performance cost
requiring clients to do a full handshake every 7 days
where previously they were able to use the same ticket
indefinitely.
Updates #25256
Change-Id: Ic4d1ba0d92773c490b33b5f6c1320d557cc7347d
Reviewed-on: https://go-review.googlesource.com/c/go/+/231317
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
We might as well grow the stack at least as large as we'll need for
the frame that is calling morestack. It doesn't help with the
lots-of-small-frames case, but it may help a bit with the
few-big-frames case.
Update #18138
Change-Id: I1f49c97706a70e20b30433cbec99a7901528ea52
Reviewed-on: https://go-review.googlesource.com/c/go/+/225800
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
match:
m = make([]T, x); copy(m, s)
for pointer free T and x==len(s) rewrite to:
m = mallocgc(x*elemsize(T), nil, false); memmove(&m, &s, x*elemsize(T))
otherwise rewrite to:
m = makeslicecopy([]T, x, s)
This avoids memclear and shading of pointers in the newly created slice
before the copy.
With this CL "s" is only be allowed to bev a variable and not a more
complex expression. This restriction could be lifted in future versions
of this optimization when it can be proven that "s" is not referencing "m".
Triggers 450 times during make.bash..
Reduces go binary size by ~8 kbyte.
name old time/op new time/op delta
MakeSliceCopy/mallocmove/Byte 71.1ns ± 1% 65.8ns ± 0% -7.49% (p=0.000 n=10+9)
MakeSliceCopy/mallocmove/Int 71.2ns ± 1% 66.0ns ± 0% -7.27% (p=0.000 n=10+8)
MakeSliceCopy/mallocmove/Ptr 104ns ± 4% 99ns ± 1% -5.13% (p=0.000 n=10+10)
MakeSliceCopy/makecopy/Byte 70.3ns ± 0% 68.0ns ± 0% -3.22% (p=0.000 n=10+9)
MakeSliceCopy/makecopy/Int 70.3ns ± 0% 68.5ns ± 1% -2.59% (p=0.000 n=9+10)
MakeSliceCopy/makecopy/Ptr 102ns ± 0% 99ns ± 1% -2.97% (p=0.000 n=9+9)
MakeSliceCopy/nilappend/Byte 75.4ns ± 0% 74.9ns ± 2% -0.63% (p=0.015 n=9+9)
MakeSliceCopy/nilappend/Int 75.6ns ± 0% 76.4ns ± 3% ~ (p=0.245 n=9+10)
MakeSliceCopy/nilappend/Ptr 107ns ± 0% 108ns ± 1% +0.93% (p=0.005 n=9+10)
Fixes#26252
Change-Id: Iec553dd1fef6ded16197216a472351c8799a8e71
Reviewed-on: https://go-review.googlesource.com/c/go/+/146719
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Speed up repeated HMAC operations with the same key by not recomputing
the first block of the inner and outer hashes in Reset and Sum, saving
two block computations each time.
This is a significant win for applications which hash many small
messages with the same key. In x/crypto/pbkdf2 for example, this
optimization cuts the number of block computations in half, speeding it
up by 25%-40% depending on the hash function.
The hash function needs to implement binary.Marshaler and
binary.Unmarshaler for this optimization to work, so that we can save
and restore its internal state. All hash functions in the standard
library are marshalable (CL 66710) but if the hash isn't marshalable, we
fall back on the old behaviour.
Marshaling the hashes does add a couple unavoidable new allocations, but
this only has to be done once, so the cost is amortized over repeated
uses. To minimize impact to applications which don't (or can't) reuse
hmac objects, marshaling is performed in Reset (rather than in New),
since calling Reset seems like a good indication that the caller intends
to reuse the hmac object later.
I had to add a boolean field to the hmac state to remember if we've
marshaled the hashes or not. This is paid for by removing the size and
blocksize fields, which were basically unused except for some
initialization work in New, and to fulfill the Size and Blocksize
methods. Size and Blocksize can just be forwarded to the underlying
hash, so there doesn't really seem to be any reason to waste space
caching their values.
crypto/hmac benchmarks:
name old time/op new time/op delta
HMAC_Reset/SHA1/1K-2 4.06µs ± 0% 3.77µs ± 0% -7.29% (p=0.000 n=8+10)
HMAC_Reset/SHA1/32-2 1.08µs ± 0% 0.78µs ± 1% -27.67% (p=0.000 n=10+10)
HMAC_Reset/SHA256/1K-2 10.3µs ± 0% 9.4µs ± 0% -9.03% (p=0.000 n=10+10)
HMAC_Reset/SHA256/32-2 2.32µs ± 0% 1.42µs ± 0% -38.87% (p=0.000 n=10+10)
HMAC_Reset/SHA512/1K-2 8.22µs ± 0% 7.04µs ± 0% -14.32% (p=0.000 n=9+9)
HMAC_Reset/SHA512/32-2 3.08µs ± 0% 1.89µs ± 0% -38.54% (p=0.000 n=10+9)
HMAC_New/SHA1/1K-2 4.86µs ± 1% 4.93µs ± 1% +1.30% (p=0.000 n=10+9)
HMAC_New/SHA1/32-2 1.91µs ± 1% 1.95µs ± 1% +1.84% (p=0.000 n=10+9)
HMAC_New/SHA256/1K-2 11.2µs ± 1% 11.2µs ± 0% ~ (p=1.000 n=9+10)
HMAC_New/SHA256/32-2 3.22µs ± 2% 3.19µs ± 2% -1.07% (p=0.018 n=9+10)
HMAC_New/SHA512/1K-2 9.54µs ± 0% 9.66µs ± 1% +1.31% (p=0.000 n=9+10)
HMAC_New/SHA512/32-2 4.37µs ± 1% 4.46µs ± 1% +1.97% (p=0.000 n=10+9)
name old speed new speed delta
HMAC_Reset/SHA1/1K-2 252MB/s ± 0% 272MB/s ± 0% +7.86% (p=0.000 n=8+10)
HMAC_Reset/SHA1/32-2 29.7MB/s ± 0% 41.1MB/s ± 1% +38.26% (p=0.000 n=10+10)
HMAC_Reset/SHA256/1K-2 99.1MB/s ± 0% 108.9MB/s ± 0% +9.93% (p=0.000 n=10+10)
HMAC_Reset/SHA256/32-2 13.8MB/s ± 0% 22.6MB/s ± 0% +63.57% (p=0.000 n=10+10)
HMAC_Reset/SHA512/1K-2 125MB/s ± 0% 145MB/s ± 0% +16.71% (p=0.000 n=9+9)
HMAC_Reset/SHA512/32-2 10.4MB/s ± 0% 16.9MB/s ± 0% +62.69% (p=0.000 n=10+9)
HMAC_New/SHA1/1K-2 211MB/s ± 1% 208MB/s ± 1% -1.29% (p=0.000 n=10+9)
HMAC_New/SHA1/32-2 16.7MB/s ± 1% 16.4MB/s ± 1% -1.81% (p=0.000 n=10+9)
HMAC_New/SHA256/1K-2 91.3MB/s ± 1% 91.5MB/s ± 0% ~ (p=0.950 n=9+10)
HMAC_New/SHA256/32-2 9.94MB/s ± 2% 10.04MB/s ± 2% +1.09% (p=0.021 n=9+10)
HMAC_New/SHA512/1K-2 107MB/s ± 0% 106MB/s ± 1% -1.29% (p=0.000 n=9+10)
HMAC_New/SHA512/32-2 7.32MB/s ± 1% 7.18MB/s ± 1% -1.89% (p=0.000 n=10+9)
name old alloc/op new alloc/op delta
HMAC_Reset/SHA1/1K-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA1/32-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA256/1K-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA256/32-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA512/1K-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA512/32-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
HMAC_New/SHA1/1K-2 448B ± 0% 448B ± 0% ~ (all samples are equal)
HMAC_New/SHA1/32-2 448B ± 0% 448B ± 0% ~ (all samples are equal)
HMAC_New/SHA256/1K-2 480B ± 0% 480B ± 0% ~ (all samples are equal)
HMAC_New/SHA256/32-2 480B ± 0% 480B ± 0% ~ (all samples are equal)
HMAC_New/SHA512/1K-2 800B ± 0% 800B ± 0% ~ (all samples are equal)
HMAC_New/SHA512/32-2 800B ± 0% 800B ± 0% ~ (all samples are equal)
name old allocs/op new allocs/op delta
HMAC_Reset/SHA1/1K-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA1/32-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA256/1K-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA256/32-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA512/1K-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
HMAC_Reset/SHA512/32-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
HMAC_New/SHA1/1K-2 5.00 ± 0% 5.00 ± 0% ~ (all samples are equal)
HMAC_New/SHA1/32-2 5.00 ± 0% 5.00 ± 0% ~ (all samples are equal)
HMAC_New/SHA256/1K-2 5.00 ± 0% 5.00 ± 0% ~ (all samples are equal)
HMAC_New/SHA256/32-2 5.00 ± 0% 5.00 ± 0% ~ (all samples are equal)
HMAC_New/SHA512/1K-2 5.00 ± 0% 5.00 ± 0% ~ (all samples are equal)
HMAC_New/SHA512/32-2 5.00 ± 0% 5.00 ± 0% ~ (all samples are equal)
x/crypto/pbkdf2 benchmarks:
name old time/op new time/op delta
HMACSHA1-2 4.63ms ± 0% 3.40ms ± 0% -26.58% (p=0.000 n=10+9)
HMACSHA256-2 9.75ms ± 0% 5.98ms ± 0% -38.62% (p=0.000 n=9+10)
name old alloc/op new alloc/op delta
HMACSHA1-2 516B ± 0% 708B ± 0% +37.21% (p=0.000 n=10+10)
HMACSHA256-2 549B ± 0% 772B ± 0% +40.62% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
HMACSHA1-2 8.00 ± 0% 10.00 ± 0% +25.00% (p=0.000 n=10+10)
HMACSHA256-2 8.00 ± 0% 10.00 ± 0% +25.00% (p=0.000 n=10+10)
Fixes#19941
Change-Id: I7077a6f875be68d3da05f7b3664e18514861886f
Reviewed-on: https://go-review.googlesource.com/c/go/+/27458
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
The code writes text sections twice, one with Codeblk, one with
Datblk. The second write shouldn't be there.
May fix#38898.
Change-Id: I4ec70294059ec9aa0fc4cc69a3cd824f5843287b
Reviewed-on: https://go-review.googlesource.com/c/go/+/232661
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Lynn Boger <laboger@linux.vnet.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Lynn Boger <laboger@linux.vnet.ibm.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Reject base 128 encoded integers that aren't using minimal encoding,
specifically if the leading octet of an encoded integer is 0x80. This
only affects parsing of tags and OIDs, both of which expect this
encoding (see X.690 8.1.2.4.2 and 8.19.2).
Fixes#36881
Change-Id: I969cf48ac1fba7e56bac334672806a0784d3e123
GitHub-Last-Rev: fefc03d202
GitHub-Pull-Request: golang/go#38281
Reviewed-on: https://go-review.googlesource.com/c/go/+/227320
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The previous behavior directly contradicted the docs that have been in
place for years:
To unmarshal a JSON array into a slice, Unmarshal resets the
slice length to zero and then appends each element to the slice.
We could use reflect.New to create a new element and reflect.Append to
then append it to the destination slice, but benchmarks have shown that
reflect.Append is very slow compared to the code that manually grows a
slice in this file.
Instead, if we're decoding into an element that came from the original
backing array, zero it before decoding into it. We're going to be using
the CodeDecoder benchmark, as it has a slice of struct pointers that's
decoded very often.
Note that we still reuse existing values from arrays being decoded into,
as the documentation agrees with the existing implementation in that
case:
To unmarshal a JSON array into a Go array, Unmarshal decodes
JSON array elements into corresponding Go array elements.
The numbers with the benchmark as-is might seem catastrophic, but that's
only because the benchmark is decoding into the same variable over and
over again. Since the old decoder was happy to reuse slice elements, it
would save a lot of allocations by not having to zero and re-allocate
said elements:
name old time/op new time/op delta
CodeDecoder-8 10.4ms ± 1% 10.9ms ± 1% +4.41% (p=0.000 n=10+10)
name old speed new speed delta
CodeDecoder-8 186MB/s ± 1% 178MB/s ± 1% -4.23% (p=0.000 n=10+10)
name old alloc/op new alloc/op delta
CodeDecoder-8 2.19MB ± 0% 3.59MB ± 0% +64.09% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
CodeDecoder-8 76.8k ± 0% 92.7k ± 0% +20.71% (p=0.000 n=10+10)
We can prove this by moving 'var r codeResponse' into the loop, so that
the benchmark no longer reuses the destination pointer. And sure enough,
we no longer see the slow-down caused by the extra allocations:
name old time/op new time/op delta
CodeDecoder-8 10.9ms ± 0% 10.9ms ± 1% -0.37% (p=0.043 n=10+10)
name old speed new speed delta
CodeDecoder-8 177MB/s ± 0% 178MB/s ± 1% +0.37% (p=0.041 n=10+10)
name old alloc/op new alloc/op delta
CodeDecoder-8 3.59MB ± 0% 3.59MB ± 0% ~ (p=0.780 n=10+10)
name old allocs/op new allocs/op delta
CodeDecoder-8 92.7k ± 0% 92.7k ± 0% ~ (all equal)
I believe that it's useful to leave the benchmarks as they are now,
because the decoder does reuse memory in some cases. For example,
existing map elements are reused. However, subtle changes like this one
need to be benchmarked carefully.
Finally, add a couple of tests involving both a slice and an array of
structs.
Fixes#21092.
Change-Id: I8b1194f25e723a31abd146fbfe9428ac10c1389d
Reviewed-on: https://go-review.googlesource.com/c/go/+/191783
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The `yield := osyield` line doesn't serve any purpose, it's committed in `2015`, time to delete that line:)
Change-Id: I382d4d32cf320f054f011f3b6684c868cbcb0ff2
GitHub-Last-Rev: 7a0aa25e55
GitHub-Pull-Request: golang/go#36078
Reviewed-on: https://go-review.googlesource.com/c/go/+/210837
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This commit moves the isSelect bool below the ticket uint32. The
boolean was consuming 8 bytes of the struct. The uint32 was also
consuming 8 bytes, so we can pack isSelect below the uint32 and save 8
bytes. This reduces the sudog struct from 96 bytes to 88 bytes.
Change-Id: If555cdaf2f5eaa125e2590fc4d113dbc99750738
GitHub-Last-Rev: d63b4e086b
GitHub-Pull-Request: golang/go#36552
Reviewed-on: https://go-review.googlesource.com/c/go/+/214677
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Change-Id: I493bb7e5e9a9e1752236dea1e032b317da7f67f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/211560
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This CL sets positions for errors from cals to load within the load
call itself, similar to how the rest of the code in pkg.go sets
positions right after the error is set on the package.
This allows the code to ensure that we only add positions either for
ImportPathErrors, or if an error was passed into load, and was set
using setLoadPackageDataError. (Though I'm wondering if the call
to setLoadPackageDataError should be done before the call to load).
Fixes#38034
Change-Id: I0748866933b4c1a329954b4b96640bef702a4644
Reviewed-on: https://go-review.googlesource.com/c/go/+/228784
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Being lenient on those has caused enough security issues.
Spun out of CL 231419.
Fixes#38889
Change-Id: Idd3bc6adc22e08a30b3dabb146ce78d4105684cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/232277
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Before going around making changes, surface the current behavior in the
docs as a starting point. No behavior changes.
Change-Id: If8096cedbba7eda37694dbb7f438046d590c3bcc
Reviewed-on: https://go-review.googlesource.com/c/go/+/231377
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
This is a security hardening measure against HTTP request smuggling.
Thank you to ZeddYu for reporting this issue.
We weren't parsing things correctly anyway, allowing "identity" to be
combined with "chunked", and ignoring any Transfer-Encoding header past
the first. This is a delicate security surface that already broke
before, just be strict and don't add complexity to support cases not
observed in the wild (nginx removed "identity" support [1] and multiple
TE header support [2]) and removed by RFC 7230 (see page 81).
It'd probably be good to also drop support for anything other than
"chunked" in outbound TE headers, as "identity" is not a thing anymore,
and we are probably off-spec for anything other than "chunked", but it
should not be a security concern, so leaving it for now. See #38867.
[1]: https://hg.nginx.org/nginx/rev/fe5976aae0e3
[2]: https://hg.nginx.org/nginx/rev/aca005d232ff
Change-Id: If17d0827f9c6167a0b19a158e2bc5844ec803288
Reviewed-on: https://go-review.googlesource.com/c/go/+/231418
Reviewed-by: Katie Hockman <katie@golang.org>
Clients have to reject any HelloRetryRequest message that doesn't lead
to a change in the ClientHello. Instead, we were rejecting any HRR that
didn't select an alternative group, even if it sent a cookie, which
would change the CH.
The good news is that I know of no TLS servers that use or need HRRs
exclusively for cookies (which are mostly useful in DTLS as a way to
verify the source address). The bad news is that we poisoned the
ecosystem as Go 1.12 to 1.14 will reject such HRRs. Oops, hopefully no
one needed this.
No tests because neither Go nor s_server support cookies. This would
presumably get covered once we integrate BoGo.
Fixes#30149
Change-Id: I760fb1ded81148ac3096cf201cbc1e941374b83d
Reviewed-on: https://go-review.googlesource.com/c/go/+/231039
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Following CL 208126, do the same for MIPS.
Change-Id: I95f8fc99a234524119a4d29c7695676dc0ea1025
Reviewed-on: https://go-review.googlesource.com/c/go/+/208217
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
On some architectures, for async preemption the injected call
needs to clobber a register (usually REGTMP) in order to return
to the preempted function. As a consequence, the PC ranges where
REGTMP is live are not preemptible.
The uses of REGTMP are usually generated by the assembler, where
it needs to load or materialize a large constant or offset that
doesn't fit into the instruction. In those cases, REGTMP is not
live at the start of the instruction sequence. Instead of giving
up preemption in those cases, we could preempt it and restart the
sequence when resuming the execution. Basically, this is like
reissuing an interrupted instruction, except that here the
"instruction" is a Prog that consists of multiple machine
instructions. For this to work, we need to generate PC data to
mark the start of the Prog.
Currently this is only done for ARM64.
TODO: the split-stack function prologue is currently not async
preemptible. We could use this mechanism, preempt it and restart
at the function entry.
Change-Id: I37cb282f8e606e7ab6f67b3edfdc6063097b4bd1
Reviewed-on: https://go-review.googlesource.com/c/go/+/208126
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
There is a range of numbers lower than 0x7fff_ffff_ffff_ffff which
cannot be represented by a 64 bit float. We set that to the correct
limit beyond which conversions can happen properly.
It appears that the negative bound check can indeed by correctly handled
by I64TruncF64S. But we use the same limit for consistency.
Fixes#38839
Change-Id: Ib783a22cb331fba7e6955459f41c67f9ceb53461
Reviewed-on: https://go-review.googlesource.com/c/go/+/231837
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Record the caller when Cleanup is called to report it with t.Log
instead of unhelpful line in testing.go.
Fixes#38800
Change-Id: I3136f5d92a0e5a48f8b32a2e13b2521bc91d72d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/232237
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
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.
Fixes#38746
Change-Id: Ia7c5f76952329f60974bc033c29a5433610f3b28
Reviewed-on: https://go-review.googlesource.com/c/go/+/231977
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Replaced almost every use of Bytes with FillBytes.
Note that the approved proposal was for
func (*Int) FillBytes(buf []byte)
while this implements
func (*Int) FillBytes(buf []byte) []byte
because the latter was far nicer to use in all callsites.
Fixes#35833
Change-Id: Ia912df123e5d79b763845312ea3d9a8051343c0a
Reviewed-on: https://go-review.googlesource.com/c/go/+/230397
Reviewed-by: Robert Griesemer <gri@golang.org>
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:
github.com/google/pprof v0.0.0-20200229191704-1ebb73c60ed3
github.com/ianlancetaylor/demangle v0.0.0-20200414190113-039b1ae3a340
golang.org/x/arch v0.0.0-20200312215426-ff8b605520f4
golang.org/x/crypto v0.0.0-20200429183012-4b2356b1ed79
golang.org/x/mod v0.2.1-0.20200429172858-859b3ef565e2
golang.org/x/net v0.0.0-20200501053045-e0ff5e5a1de5
golang.org/x/sys v0.0.0-20200501145240-bc7a7d42d5c3
golang.org/x/text v0.3.3-0.20200430171850-afb9336c4530
golang.org/x/tools v0.0.0-20200504152539-33427f1b0364
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543
github.com/ianlancetaylor/demangle is considered in scope and updated.
github.com/google/pprof is out of scope and was not updated.
For #36905.
Change-Id: Icb6996eb0df11f16edd9a42e04434012c0336354
Reviewed-on: https://go-review.googlesource.com/c/go/+/231657
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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: 78e280e6d0
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>
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>
Introduces a few casts, mostly to fix rules that mix int64 and int32
off1 and off2.
Passes
GOARCH=arm64 gotip build -toolexec 'toolstash -cmp' -a std
Change-Id: I1ec75211f3bb8e521dcc5217cf29ab0655a84d79
Reviewed-on: https://go-review.googlesource.com/c/go/+/230840
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
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>
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.
Fixes#35908
Change-Id: I45886aee548431fe4031883ab1629a41e35f1727
GitHub-Last-Rev: 7be6b8d421
GitHub-Pull-Request: golang/go#36378
Reviewed-on: https://go-review.googlesource.com/c/go/+/213257
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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>
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>
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>
Adding the usage of PCALIGN directive for arm64, and updating some
details on using some directives defined in the textflag.h file.
Change-Id: I43d363e3337939bab69b856831caf06803a292d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/227801
Reviewed-by: Cherry Zhang <cherryyz@google.com>
CL 231397 is submitted too fast... Expand the comment to make it
a little clearer.
Change-Id: Ica9737aa7b51f97320bab74457388dcab8188370
Reviewed-on: https://go-review.googlesource.com/c/go/+/231597
Reviewed-by: Austin Clements <austin@google.com>
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.
Updates #35881
Co-authored-by: David Chase <drchase@google.com>
Change-Id: Ief0722300bc3f987098e4fd92b22b14ad6281d91
Reviewed-on: https://go-review.googlesource.com/c/go/+/219357
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
A symbol being reachable doesn't imply its type descriptor is
needed. Don't mark it.
If the type is converted to interface somewhere in the program,
there will be an explicit use of the type descriptor, which
will make it marked.
A println("hello") program before and after
-rwxr-xr-x 1 cherryyz primarygroup 1259824 Apr 30 23:00 hello
-rwxr-xr-x 1 cherryyz primarygroup 1169680 Apr 30 23:10 hello
Updates #38782.
Updates #6853.
Change-Id: I88884c126ce75ba073f1ba059c4b892c87d2ac96
Reviewed-on: https://go-review.googlesource.com/c/go/+/231397
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
(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>
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>
For latest http2 changes.
Which then required updating golang.org/x/sys in cmd too.
Change-Id: I3fac5f3a15f4c9381baaff597873ed0c6209dbac
Reviewed-on: https://go-review.googlesource.com/c/go/+/231457
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Refer to reflect.StringHeader in the godoc comment for type String
instead of reflect.SliceHeader.
Change-Id: I40fc016c7365510a12c41d4ca596f66d2892c3f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/231537
Reviewed-by: Brad Fitzpatrick <bradfitz@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, with
improvements on both linker speed and memory usage.
This is a clean merge.
Change-Id: I38516d6c4b41021bc61c1b9886e701de5fa2b0f1
Convert the part that uses relocations to use loader.ExtReloc
directly. It still uses sym.Symbols for now, but not sym.Relocs.
This reduces some memory usage: linking cmd/compile with external
linking,
name old allocs/op new allocs/op delta
Loadlibfull_GC 52.2MB ± 0% 13.9MB ± 0% -73.40% (p=0.008 n=5+5)
name old live-B new live-B delta
Loadlibfull_GC 75.5M ± 0% 61.9M ± 0% -18.02% (p=0.008 n=5+5)
Change-Id: I317ecbf516063c42b255b2caba310ea6281342d7
Reviewed-on: https://go-review.googlesource.com/c/go/+/231319
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
For now this will only avoid copying math/big/arith_riscv64.s
Change-Id: Ib236e4bf1a6a758649629268a6f512f307596e74
Reviewed-on: https://go-review.googlesource.com/c/go/+/231298
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reduces binary size by 4K, not counting the http2 changes (in CL
231119) that'll be bundled into this package in the future.
Updates golang/go#38782
Change-Id: Id360348707e076b8310a8f409e412d68dd2394b2
Reviewed-on: https://go-review.googlesource.com/c/go/+/231118
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
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>
Implement special case handling and testing to ensure
conformance with the C99 standard annex G.6 Complex arithmetic.
Fixes#29320
Change-Id: Id72eb4c5a35d5a54b4b8690d2f7176ab11028f1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/220689
Reviewed-by: Robert Griesemer <gri@golang.org>
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>
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>
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>
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>
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>
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.
This is a clean merge.
Change-Id: Ide5ad6fcec9cede99e9b21c4548929b4ba1f4185
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)
Change-Id: I8bc94c366ae792a5b0f23697b8e0108443a7a748
Reviewed-on: https://go-review.googlesource.com/c/go/+/231138
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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>
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.
Updates #35788.
Change-Id: I84289b37816a7d595d803c72a71b7f5c59d47e6b
Reviewed-on: https://go-review.googlesource.com/c/go/+/207998
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
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>
This CL changes the arm64 TBZ/TBNZ block from using Aux to using
a (typed) AuxInt. The corresponding rules have also been changed
to be typed.
Passes
GOARCH=arm64 gotip build -toolexec 'toolstash -cmp' -a std
Change-Id: I98d0cd2a791948f1db13259c17fb1b9b2807a043
Reviewed-on: https://go-review.googlesource.com/c/go/+/230839
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
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>
We call (or will call) ResolveABIAlias in many places. Doing zero
symbol check everytime is annoying. Fold the condition into
ResolveABIAlias.
Change-Id: I10485fe83b9cce2d19b6bd17dc42176f72dae48b
Reviewed-on: https://go-review.googlesource.com/c/go/+/231046
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>
Adddynrel2 is a function pointer. In dynrelocsym we pass &r to
it, which will cause r to escape. Pass it by value instead.
Linking cmd/compile,
name old alloc/op new alloc/op delta
Dodata_GC 15.8MB ± 0% 5.9MB ± 0% -62.55% (p=0.008 n=5+5)
Change-Id: Ib86005d1026ebaca57777b27ead037e613585f44
Reviewed-on: https://go-review.googlesource.com/c/go/+/231045
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>
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>
This change adjusts go command to pass -buildmode=pie to cmd/link,
if -buildmode is not explicitly provided.
Fixes#35192
Change-Id: Iec020131e676eb3e9a2df9eea1929b2af2b6df04
Reviewed-on: https://go-review.googlesource.com/c/go/+/230217
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
'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>
Race builds require C dependencies, but cross-compiled cgo builds are
not always possible, so don't suggest enabling CGO in those cases.
Fixes#37021
Change-Id: I1fd675efc9cef958a926bd63eac8e6858bc59d0a
GitHub-Last-Rev: cbf43c1bbb
GitHub-Pull-Request: golang/go#38670
Reviewed-on: https://go-review.googlesource.com/c/go/+/230202
Reviewed-by: Bryan C. Mills <bcmills@google.com>
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>
Reportedly some Docker images accept the prlimit64 system call,
used by syscall.prlimit, but prohibit the getrlimit and setrlimit
system calls.
Fixes#38604
Change-Id: I91ff9370450b4869098cc8e335bbb7b863060508
Reviewed-on: https://go-review.googlesource.com/c/go/+/230339
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
And remove "2" from some function names.
Change-Id: Ibf1089970d849a42f53976064ceb9ade20bf6eba
Reviewed-on: https://go-review.googlesource.com/c/go/+/231017
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
The fields aren't too useful for Go 1.2 and later, but they aren't
actually nil.
Fixes#38754
Change-Id: Ia13a224f623697a00dea8ba0225633e1b9308c9f
Reviewed-on: https://go-review.googlesource.com/c/go/+/230940
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This reverts commit 4f7053c87f.
Reason for revert: Newly added test is failing on several builders.
Change-Id: I22dcbfebf2f57735b2f479886bbeb623f95b132f
Reviewed-on: https://go-review.googlesource.com/c/go/+/231043
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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.
Fixes#36437
Change-Id: I9645ef73177c286c99547e3a0f3719fa07b35cb5
Reviewed-on: https://go-review.googlesource.com/c/go/+/229357
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
All five calls to wakep are protected by the same check of nmidle and
nmspinning. Move this check into wakep.
Change-Id: I2094eec211ce551e462e87614578f37f1896ba38
Reviewed-on: https://go-review.googlesource.com/c/go/+/230757
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
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>
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.
Updates #10848.
Change-Id: I2fc70d4a34dc545677b99f218b51023c7891bbbd
Reviewed-on: https://go-review.googlesource.com/c/go/+/231041
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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%
name old time/op new time/op delta
Template 178ms ± 7% 172ms ±14% -3.59% (p=0.005 n=19+19)
Unicode 71.0ms ±12% 69.8ms ±10% ~ (p=0.126 n=18+18)
GoTypes 655ms ± 8% 615ms ± 8% -6.11% (p=0.000 n=19+19)
Compiler 3.27s ± 6% 3.15s ± 7% -3.69% (p=0.001 n=20+20)
SSA 7.10s ± 5% 6.85s ± 8% -3.53% (p=0.001 n=19+20)
Flate 124ms ±15% 116ms ±22% -6.57% (p=0.024 n=18+19)
GoParser 156ms ±26% 147ms ±34% ~ (p=0.070 n=19+19)
Reflect 406ms ± 9% 387ms ±21% -4.69% (p=0.028 n=19+20)
Tar 163ms ±15% 162ms ±27% ~ (p=0.370 n=19+19)
XML 223ms ±13% 218ms ±14% ~ (p=0.157 n=20+20)
LinkCompiler 503ms ±21% 484ms ±23% ~ (p=0.072 n=20+20)
ExternalLinkCompiler 1.27s ± 7% 1.22s ± 8% -3.85% (p=0.005 n=20+19)
LinkWithoutDebugCompiler 294ms ±17% 273ms ±11% -7.16% (p=0.001 n=19+18)
(https://perf.golang.org/search?q=upload:20200428.8)
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%
(https://perf.golang.org/search?q=upload:20200428.9)
For #36365.
Change-Id: I69448e714f2a44430067ca97f6b78e08c0abed27
Reviewed-on: https://go-review.googlesource.com/c/go/+/230544
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
We're about to switch to having significantly fewer maps in the
liveness map, so switch from a dense representation to a sparse
representation.
Passes toolstash-check.
For #36365.
Change-Id: Icb17bd6ace17667a280bc5fba4039cae3020a8d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/230543
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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.
For #36365.
Change-Id: Ib3abfedfddc8e724d933a064fa4d573500627990
Reviewed-on: https://go-review.googlesource.com/c/go/+/230542
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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.
For #36365.
Change-Id: I066628938b02e78be5c81a6614295bcf7cc566c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/230541
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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.
For #36365.
Change-Id: I89839de0be8db3be3f0d3a7fb5fcf0b0b6ebc98a
Reviewed-on: https://go-review.googlesource.com/c/go/+/230540
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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.
For #36365.
Change-Id: Ic5bb463fd152cc320c815dc04cf62005261ae169
Reviewed-on: https://go-review.googlesource.com/c/go/+/230539
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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.
For #36365.
Change-Id: Ie217b6711f3741ccc437552d8ff88f961a73cee0
Reviewed-on: https://go-review.googlesource.com/c/go/+/229300
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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.
For #36365.
Change-Id: I68463bfd73cbee06cfc49999606410a59dd8f653
Reviewed-on: https://go-review.googlesource.com/c/go/+/229299
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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.
For #36365.
Change-Id: Idacd06b63e738982e840fe995d891bfd377ce23b
Reviewed-on: https://go-review.googlesource.com/c/go/+/229298
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Rusage.Maxrss is in bytes on Darwin but in KiB on Linux. Fix this
discrepancy so it's always in bytes.
Change-Id: Ic714abc3276566b8fe5e30565072092212610854
Reviewed-on: https://go-review.googlesource.com/c/go/+/230979
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
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>
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>
This allows writing
// F does a thing.
//go:noinline
func F()
without the //go:noinline or other directive (such as //line)
ending up looking like extra words in the doc comment.
Fixes#37974.
Change-Id: Ic738d72802cc2fa448f7633915e7126d2f76d8ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/224737
Reviewed-by: Robert Griesemer <gri@golang.org>
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.
Fixes#33094Fixes#23069
Change-Id: Id9829c20968e16db7194549f69c0eb5985044944
Reviewed-on: https://go-review.googlesource.com/c/go/+/229864
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Per suggestion in CL 202578, this CL drops the purego build tag used
within this package.
Change-Id: I33626c73d6602e321528544ee601741f7e634c1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/230677
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Michael Munday <mike.munday@ibm.com>
Adds a few instructions to ppc64enc.s that were missing from the
previous update.
Change-Id: Ieafce39e905cdf4da3bfb00fdd5a39ab28089cb3
Reviewed-on: https://go-review.googlesource.com/c/go/+/230437
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Once the symbol is compressed, we will not need the uncompressed
symbol content. Free its memory.
Linking cmd/compile,
name old live-B new live-B delta
Dwarfcompress_GC 42.7M ± 0% 37.9M ± 0% -11.31% (p=0.008 n=5+5)
Change-Id: Ib6cc73832946d158ff4f5b4f31be9c35ba7cf103
Reviewed-on: https://go-review.googlesource.com/c/go/+/230859
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>
Non functional, but required by the majority of the architectures.
Change-Id: I57601016c28ce665a9d434e283a1db8bded9b133
Reviewed-on: https://go-review.googlesource.com/c/go/+/230858
Run-TryBot: Jeremy Faller <jeremy@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
After LoadFull, we'll be using sym.Symbols mostly. We still need
the loader information for symbol index mappings and name
lookups, but not much else. Free some memory.
Linking cmd/compile,
name old time/op new time/op delta
Loadlibfull_GC 44.5M ± 0% 35.8M ± 0% -19.66% (p=0.008 n=5+5)
Archive_GC 46.4M ± 0% 37.6M ± 0% -18.89% (p=0.008 n=5+5)
Linking cmd/compile with external linking,
name old time/op new time/op delta
Loadlibfull_GC 82.5M ± 0% 57.4M ± 0% -30.41% (p=0.008 n=5+5)
Archive_GC 86.8M ± 0% 61.7M ± 0% -28.90% (p=0.008 n=5+5)
Change-Id: I6859d488172ef8968918b86de527fbfed6832ebf
Reviewed-on: https://go-review.googlesource.com/c/go/+/230300
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
When applying relocations, we need to resolve ABI aliases.
relocsym does that. Architecture-specific archreloc also needs to
do that. The old code doesn't do that since ABI aliases are
resolved in loadlibfull, or, in the old linker, in a much earlier
stage. We don't do this in the new linker, as we want to avoid
mutating relocations.
While here, move R_CONST and R_GOTOFF handling to generic code.
They appear on several architectures and the handling are same.
Should fix 386-clang and *bsd-386 builds.
Change-Id: I6681c94f0327555d6cf329d0a518c88848773671
Reviewed-on: https://go-review.googlesource.com/c/go/+/230857
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Support external linking for the new reloc pass as well, and
enable it on AMD64 and 386.
Change-Id: Ia71aec3d7c14e9d661e0748d2e988f29f220d1e1
Reviewed-on: https://go-review.googlesource.com/c/go/+/230308
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>
Remove 'Sub' field from sym.Symbol, replacing uses (those downstream of
loadlibfull) with loader method calls.
NB: removing the Outer field will have to wait for now; it is accessed
in archreloc methods that don't have access to link ctxt or loader
currently.
Change-Id: I2abe5906fc169c64b2ab7d5ad213619bea5a17c7
Reviewed-on: https://go-review.googlesource.com/c/go/+/230617
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This updates the PPC64.rules file to use the MOD instructions
that are available in power9. Prior to power9 this is done
using a longer sequence with multiply and divide.
Included in this change is removal of the REM* opcode variations
that set the CC or OV bits since their settings are based
on the DIV and are not appropriate for the REM.
Change-Id: Iceed9ce33e128e1911c15592ee674276ce8ba3fa
Reviewed-on: https://go-review.googlesource.com/c/go/+/229761
Run-TryBot: Lynn Boger <laboger@linux.vnet.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Prior to this commit, NewXxx could panic when passed an image.Rectangle
with one of width or height being negative. But it might not panic if
both were negative, because (bpp * w * h) could still be positive. After
this commit, it will panic if both are negative.
With overflow, NewXxx might not have panicked if (bpp * w * h), the
length passed to "make([]uint8, length)", was still non-negative (after
truncation), but even if w and h were valid (non-negative), the overall
byte slice wasn't long enough. Iterating over the pixels would possibly
panic later with index out of bounds. This change moves the panic
earlier, closer to where the mistake is.
Change-Id: I011feb2d53515fc3f0fe72bb6c23b3953772c577
Reviewed-on: https://go-review.googlesource.com/c/go/+/230220
Reviewed-by: Rob Pike <r@golang.org>
Convert some optimizations rules to strongly-typed versions. Similar to
CL 230338, this CL only converts rules that need no additional changes
(i.e: only need to change '->' to '=>').
This CL covers the rules from line 800 - 1219.
Passes toolstash-check
Change-Id: I94181a809fa38918b78301f1c0c680b7a8ab552f
Reviewed-on: https://go-review.googlesource.com/c/go/+/230738
Reviewed-by: Michael Munday <mike.munday@ibm.com>
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Only enabled for AMD64 when internal linking for now.
Change-Id: I2aa9ee47c0f7413ea7bbcdd31b8317c14220bba3
Reviewed-on: https://go-review.googlesource.com/c/go/+/230302
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Previously, looking up only IPv4 or IPv6 addresses was only possible
with DefaultResolver via ResolveIPAddr. Add this functionality to the
Resolver type with a new method, LookupIP. This largely brings Resolver
functionally to parity with the global functions. The name LookupIP is
used over ResolveIPAddr to be consistent with the other Resolver
methods.
There are two main benefits to (*Resolver).LookupIP over
(*Resolver).LookupHost. First is an ergonomic benefit. Wanting a
specific family of address is common enough to justify a method, evident
by the existence of ResolveIPAddr. Second, this opens the possibility of
not performing unnecessary DNS requests when only a specific family of
addresses are needed. This optimization is left to follow up work.
Updates #30452
Change-Id: I241f61019588022a39738f8920b0ddba900cecdd
Reviewed-on: https://go-review.googlesource.com/c/go/+/228641
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Allocate the runcases slice on the stack if the number
of select cases is small (up to 4).
Found while looking at production profiles of common
proto based RPC server framework code in Google which do
not have a large number of cases.
name old time/op new time/op delta
Select/1 147ns ± 2% 120ns ± 6% -18.32% (p=0.000 n=7+10)
Select/4 316ns ± 5% 249ns ± 2% -21.23% (p=0.000 n=10+10)
Select/8 516ns ± 3% 515ns ± 3% ~ (p=0.858 n=10+9)
name old alloc/op new alloc/op delta
Select/1 96.0B ± 0% 64.0B ± 0% -33.33% (p=0.000 n=10+10)
Select/4 336B ± 0% 208B ± 0% -38.10% (p=0.000 n=10+10)
Select/8 672B ± 0% 672B ± 0% ~ (all equal)
name old allocs/op new allocs/op delta
Select/1 4.00 ± 0% 3.00 ± 0% -25.00% (p=0.000 n=10+10)
Select/4 7.00 ± 0% 6.00 ± 0% -14.29% (p=0.000 n=10+10)
Select/8 11.0 ± 0% 11.0 ± 0% ~ (all equal)
Change-Id: I1687e74fc8e86606a27f03fa8a561bcfb68775d6
Reviewed-on: https://go-review.googlesource.com/c/go/+/230657
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Opening a connection with Connect should still create a derived
context with a timeout because some clients will not use a timeout
and the connection pool may open a connection asynchronously.
Likewise, if a connection close makes a network operation it should
provide some type of sane timeout for the operation.
Fixes#38185
Change-Id: I9b7ce2996c81c486170dcc84b12672a99610fa27
Reviewed-on: https://go-review.googlesource.com/c/go/+/230438
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
CL 220499 started marking readonly syms as SRODATA earlier,
so we can use that in the writebarrier pass now.
Passes toolstash-check.
Change-Id: Ic4d49714b8bffbe03c8e9a75ca96df4475bae732
Reviewed-on: https://go-review.googlesource.com/c/go/+/230559
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Remove the "ElfSym" and "LocalElfSym" fields from sym.Symbol,
replacing uses with loader method calls as needed.
Change-Id: I3828f13203ece2bdc03eeb09ab37a5c94e21a726
Reviewed-on: https://go-review.googlesource.com/c/go/+/230462
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Remove sym.Symbol 'Unit' field, replacing accesses to the field with
calls into the loader instead.
Change-Id: Ia1abd4c3d93036705dd624a49cb3d9cbe6a5188b
Reviewed-on: https://go-review.googlesource.com/c/go/+/230307
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Add new get/set methods to the loader for recording the ELF symbol
index for a given loader symbol. These are map-based, since it is
expected that many/most symbols will not need an ELF symbol table
entry.
Change-Id: I1102c3637775515ccc6650118e8b059468a2c3ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/230461
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Remove the 'Gotype' field from sym.Symbol, as it is now no longer
used. Store the loader.Sym for a symbol as a field in sym.Symbol
("SymIdx"). Then remove sym.Symbol 'File' field, and replace the field
accesses in question with calls into the loader instead.
Change-Id: I01c5504425006b8d3fe77fac2b69a86e198c7a5a
Reviewed-on: https://go-review.googlesource.com/c/go/+/230304
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>
None of the users of genasmsym are doing anything with the Gotype
field of sym.Symbol, so remove that param from the callback function.
Change-Id: Ie902c4cdbcc6b68d353daf5ce21a99012161a946
Reviewed-on: https://go-review.googlesource.com/c/go/+/230545
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
The span set data structure may leak blocks due to a race in the logic
to check whether it's safe to free a block. The simplest example of this
race is between two poppers:
1. Popper A claims slot spanSetEntries-2.
2. Popper B claims slot spanSetEntries-1.
3. Popper A gets descheduled before it subtracts from block.used.
4. Popper B subtracts from block.used, sees that claimed
spanSetEntries-1, but also that block.used != 0, so it returns.
5. Popper A comes back and subtracts from block.used, but it didn't
claim spanSetEntries-1 so it also returns.
The spine is left with a stale block pointer and the block later gets
overwritten by pushes, never to be re-used again.
The problem here is that we designate the claimer of slot
spanSetEntries-1 to be the one who frees the block, but that may not be
the thread that actually does the last subtraction from block.used.
Fixing this problem is tricky, and the fundamental problem there is that
block.used is not stable: it may be observed to be zero, but that
doesn't necessarily mean you're the last popper!
Do something simpler: keep a counter of how many pops have happened to a
given block instead of block.used. This counter monotonically increases
when a pop is _completely done_. Because this counter is monotonically
increasing, and only increases when a popper is done, then we know for
sure whichever popper is the last to increase it (i.e. its value is
spanSetBlockEntries) is also the last popper in the block. Because the
race described above still exists, the last popper may not be the one
which claimed the last slot in the block, but we know for certain nobody
else is popping from that block anymore so we can safely free it.
Finally, because pops serialize with pushes to the same slot, we need
not worry about concurrent pushers at all.
Updates #37487.
Change-Id: I6697219372774c8ca7d8ee6895eaa230a64ce9e1
Reviewed-on: https://go-review.googlesource.com/c/go/+/230497
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Make Wasm more like other architectures, writing data sections to
heap in Asmb instead of Asmb2. Then we can remove the
copy-on-write logic in applying relocations.
Change-Id: I26d5315ea9fba032fe4bdb9b5c7fe483611c4373
Reviewed-on: https://go-review.googlesource.com/c/go/+/230465
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>
Currently mcaches are flushed to mcentral after a bunch of memstats have
already been read. This is not safe (in the sense that it doesn't ensure
consisent memstats) since memstats may in general change when mcentral
data structures are manipulated.
Note that prior to the new mcentral implementation this was not a
problem because mcentral operations happened to never modify certain
memstats. As of the new mcentral implementation, we might for example
persistentalloc when uncaching a span, which would change memstats. This
can cause a skew between the value of sys (which currently is calculated
before mcaches are flushed) and the value of gc_sys and other_sys.
Fix this by moving mcache flushing to the very top of updatememstats.
Also leave a comment explaining that this must be done first, in
general, because mcentrals make no guarantee that they will not
influence memstats (and doing so would be unnecessarily restrictive).
Fixes#38712.
Change-Id: I15bacb313c54a46e380a945a71bb75db67169c1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/230498
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
This change is just the mechanical work of moving the wavefront past
address.
Change-Id: I519ec49fa8ba50760c7d23fc084fcd3bb0544546
Reviewed-on: https://go-review.googlesource.com/c/go/+/229700
Run-TryBot: Jeremy Faller <jeremy@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This is a second attempt at CL 230024, with
cmd/go/testdata/script/mod_retention.txt updated to perform a
version-independent comparison on the 'go' version added to a go.mod
file that lacks one.
Fixes#38708
Change-Id: I15dcd83b51ed5ec57946b419bcbaec41e85a46f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/230382
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
This reverts CL 229994.
Reason for revert: break AIX build.
This is nice to have but isn't critical. We can revisit later.
Change-Id: Ifc56a0a4c0fb36859cf7666ab149e25e0e5d4cc0
Reviewed-on: https://go-review.googlesource.com/c/go/+/230459
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
ctxt.Tlsg2 is supposed to be the embedded ArchSyms.Tlsg2.
Change-Id: I4f9711f83999d4a98bcf6d99c24fab756c580905
Reviewed-on: https://go-review.googlesource.com/c/go/+/230379
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Convert some optimizations rules to strongly-typed versions. So far, I
have only converted the rules that need no additional changes (i.e: only
need to change '->' to "=>").
This CL covers the rules from line 478 - line 800 in S390X.rules file.
Some compare and branch rules also fall in this range, but they were
already done previously in another CL.
Passes toolstash-check.
Change-Id: I9167c5f1a32f4fd6c29bacc13fff95e83b0533e0
Reviewed-on: https://go-review.googlesource.com/c/go/+/230338
Reviewed-by: Michael Munday <mike.munday@ibm.com>
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The compiler has better error messages for methods called without a
pointer receiver when one is expected. This change is similar to
CL 229801, but for method calls.
Also, added better error messages for functions called with the wrong
capitalization. I left the third TODO in this switch statement almost
as-is because I'm not sure that the extra complexity is worth it -
I adjusted the error to look like the one the compiler reports.
Fixesgolang/go#38658
Change-Id: Ie0ca2503e12f3659f112f0135cc27db1b027fdcb
Reviewed-on: https://go-review.googlesource.com/c/go/+/230380
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Linux 4.5 introduced (and Linux 5.3 refined) the copy_file_range
system call, which allows file systems the opportunity to implement
copy acceleration techniques. This commit adds support for
copy_file_range(2) to the os package.
Introduce a new ReadFrom method on *os.File, which makes *os.File
implement the io.ReaderFrom interface. If dst and src are both files,
this enables io.Copy(dst, src) to call dst.ReadFrom(src), which, in
turn, will call copy_file_range(2) if possible. If copy_file_range(2)
is not supported by the host kernel, or if either of dst or src
refers to a non-regular file, ReadFrom falls back to the regular
io.Copy code path.
Add internal/poll.CopyFileRange, which acquires locks on the
appropriate poll.FDs and performs the actual work, as well as
internal/syscall/unix.CopyFileRange, which wraps the copy_file_range
system call itself at the lowest level.
Rework file layout in internal/syscall/unix to accomodate the
additional system call numbers needed for copy_file_range.
Merge these definitions with the ones used by getrandom(2) into
sysnum_linux_$GOARCH.go files.
A note on additional optimizations: if dst and src both refer to pipes
in the invocation dst.ReadFrom(src), we could, in theory, use the
existing splice(2) code in package internal/poll to splice directly
from src to dst. Attempting this runs into trouble with the poller,
however. If we call splice(src, dst) and see EAGAIN, we cannot know
if it came from src not being ready for reading or dst not being
ready for writing. The write end of src and the read end of dst are
not under our control, so we cannot reliably use the poller to wait
for readiness. Therefore, it seems infeasible to use the new ReadFrom
method to splice between pipes directly. In conclusion, for now, the
only optimization enabled by the new ReadFrom method on *os.File is
the copy_file_range optimization.
Fixes#36817.
Change-Id: I696372639fa0cdf704e3f65414f7321fc7d30adb
Reviewed-on: https://go-review.googlesource.com/c/go/+/229101
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
IntSize is an untyped constant that does not need explicit conversion.
Annotating IntSize as an int and running github.com/mdempsky/unconvert
reveals these two cases.
Fixes#38682.
Change-Id: I014646b7457ddcde32474810153229dcf0c269c6
Reviewed-on: https://go-review.googlesource.com/c/go/+/230306
Run-TryBot: Akhil Indurti <aindurti@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Currently allocToCache assumes it can move the search address past the
block it allocated the cache from, which violates the property that
searchAddr should always point to mapped memory (i.e. memory represented
by pageAlloc.inUse).
This bug was already fixed once for pageAlloc.alloc in the Go 1.14
release via CL 216697, but that changed failed to take into account
allocToCache.
Fixes#38605.
Change-Id: Id08180aa10d19dc0f9f551a1d9e327a295560dff
Reviewed-on: https://go-review.googlesource.com/c/go/+/229577
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Originally, we use an assembly function that returns a boolean result to
tell whether the machine has vector facility or not. It is now no longer
needed when we can directly use cpu.S390X.HasVX variable. This CL
also removes the last occurence of hasVectorFacility function on s390x.
Change-Id: Id20cb746c21eacac5e13344b362e2d87adfe4317
Reviewed-on: https://go-review.googlesource.com/c/go/+/230337
Reviewed-by: Michael Munday <mike.munday@ibm.com>
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
symGroupType was needed for dodata. Now that we have converted
dodata to using the loader, stop overwriting it.
Change-Id: Ie94109c0b35dd2f71a19ebb38f8cf20b6a37c624
Reviewed-on: https://go-review.googlesource.com/c/go/+/229994
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
In dodata we overwrite symbol types to SDATA. Now we'll stop
doing that, so accept more symbol types here. This is basically
a list of all writeable types handled in dodata that could appear
in XCOFF.
Change-Id: Iee35369162f5acd59806a3f0e6c8d3682620067e
Reviewed-on: https://go-review.googlesource.com/c/go/+/230310
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Originally, we use an assembly function that returns a boolean result to
tell whether the machine has vector facility or not. It is now no longer
needed when we can directly use cpu.S390X.HasVX variable.
Change-Id: Ic1dae851982532bcfd9a9453416c112347f21d87
Reviewed-on: https://go-review.googlesource.com/c/go/+/230318
Reviewed-by: Michael Munday <mike.munday@ibm.com>
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Originally, we use an assembly function that returns a boolean result to
tell whether the machine has vector facility or not. It is now no longer
needed when we can directly use cpu.S390X.HasVX variable.
Change-Id: Ic3ffeb9e63238ef41406d97cdc42502145ddb454
Reviewed-on: https://go-review.googlesource.com/c/go/+/230319
Reviewed-by: Michael Munday <mike.munday@ibm.com>
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This CL allows the usage of KDSA instruction when it is available. The
instruction is designed to be resistant to side channel attacks and
offers performance improvement for ed25519.
Benchmarks:
name old time/op new time/op delta
Signing-8 120µs ±20% 62µs ±12% -48.40% (p=0.000 n=10+10)
Verification-8 325µs ±17% 69µs ±10% -78.80% (p=0.000 n=10+10)
name old alloc/op new alloc/op delta
Signing-8 448B ± 0% 0B -100.00% (p=0.000 n=10+10)
Verification-8 288B ± 0% 0B -100.00% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
Signing-8 5.00 ± 0% 0.00 -100.00% (p=0.000 n=10+10)
Verification-8 2.00 ± 0% 0.00 -100.00% (p=0.000 n=10+10)
Change-Id: I0330ce83d807370b419ce638bc2cae4cb3c250dc
Reviewed-on: https://go-review.googlesource.com/c/go/+/202578
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Munday <mike.munday@ibm.com>
This reverts CL 33677.
Reason for revert: NetBSD is broken
Updates #38649
Change-Id: Id60e3c97d3cb4fb0053dea03b95dbbb0b850c883
Reviewed-on: https://go-review.googlesource.com/c/go/+/230038
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Several new ones came from my testing (long, repeated runs) and one (assistQueue ->
spine) came from the staticlockranking builder (filed as issue 38441).
Fixes#38441
Change-Id: I4268da0d8b8cc51251eba6bd936110c8ab4c4e61
Reviewed-on: https://go-review.googlesource.com/c/go/+/229480
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Currently, the small object sweeper will sweep until it finds a free
slot or there are no more spans of that size class to sweep. In dense
heaps, this can cause sweeping for a given size class to take
unbounded time, and gets worse with larger heaps.
This CL limits the small object sweeper to try at most 100 spans
before giving up and allocating a fresh span. Since it's already shown
that 100 spans are completely full at that point, the space overhead
of this fresh span is at most 1%.
This CL is based on an experimental CL by Austin Clements (CL 187817)
and is updated to be part of the mcentral implementation, gated by
go115NewMCentralImpl.
Updates #18155.
Change-Id: I37a72c2dcc61dd6f802d1d0eac3683e6642b6ef8
Reviewed-on: https://go-review.googlesource.com/c/go/+/229998
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Austin Clements <austin@google.com>
Currently mcentral is implemented as a couple of linked lists of spans
protected by a lock. Unfortunately this design leads to significant lock
contention.
The span ownership model is also confusing and complicated. In-use spans
jump between being owned by multiple sources, generally some combination
of a gcSweepBuf, a concurrent sweeper, an mcentral or an mcache.
So first to address contention, this change replaces those linked lists
with gcSweepBufs which have an atomic fast path. Then, we change up the
ownership model: a span may be simultaneously owned only by an mcentral
and the page reclaimer. Otherwise, an mcentral (which now consists of
sweep bufs), a sweeper, or an mcache are the sole owners of a span at
any given time. This dramatically simplifies reasoning about span
ownership in the runtime.
As a result of this new ownership model, sweeping is now driven by
walking over the mcentrals rather than having its own global list of
spans. Because we no longer have a global list and we traditionally
haven't used the mcentrals for large object spans, we no longer have
anywhere to put large objects. So, this change also makes it so that we
keep large object spans in the appropriate mcentral lists.
In terms of the static lock ranking, we add the spanSet spine locks in
pretty much the same place as the mcentral locks, since they have the
potential to be manipulated both on the allocation and sweep paths, like
the mcentral locks.
This new implementation is turned on by default via a feature flag
called go115NewMCentralImpl.
Benchmark results for 1 KiB allocation throughput (5 runs each):
name \ MiB/s go113 go114 gotip gotip+this-patch
AllocKiB-1 1.71k ± 1% 1.68k ± 1% 1.59k ± 2% 1.71k ± 1%
AllocKiB-2 2.46k ± 1% 2.51k ± 1% 2.54k ± 1% 2.93k ± 1%
AllocKiB-4 4.27k ± 1% 4.41k ± 2% 4.33k ± 1% 5.01k ± 2%
AllocKiB-8 4.38k ± 3% 5.24k ± 1% 5.46k ± 1% 8.23k ± 1%
AllocKiB-12 4.38k ± 3% 4.49k ± 1% 5.10k ± 1% 10.04k ± 0%
AllocKiB-16 4.31k ± 1% 4.14k ± 3% 4.22k ± 0% 10.42k ± 0%
AllocKiB-20 4.26k ± 1% 3.98k ± 1% 4.09k ± 1% 10.46k ± 3%
AllocKiB-24 4.20k ± 1% 3.97k ± 1% 4.06k ± 1% 10.74k ± 1%
AllocKiB-28 4.15k ± 0% 4.00k ± 0% 4.20k ± 0% 10.76k ± 1%
Fixes#37487.
Change-Id: I92d47355acacf9af2c41bf080c08a8c1638ba210
Reviewed-on: https://go-review.googlesource.com/c/go/+/221182
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
This change implements the spanSet data structure which is based off of
the gcSweepBuf data structure. While the general idea is the same (one
has two of these which one switches between every GC cycle; one to push
to and one to pop from), there are some key differences.
Firstly, we never have a need to iterate over this data structure so
delete numBlocks and block. Secondly, we want to be able to pop from the
front of the structure concurrently with pushes to the back. As a result
we need to maintain both a head and a tail and this change introduces an
atomic headTail structure similar to the one used by sync.Pool. It also
implements popfirst in a similar way.
As a result of this headTail, we need to be able to explicitly reset the
length, head, and tail when it goes empty at the end of sweep
termination, so add a reset method.
Updates #37487.
Change-Id: I5b8ad290ec32d591e3c8c05e496c5627018074f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/221181
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
