This API was added only for BoringCrypto, never shipped in standard
Go. This API is also not compatible with the expected future evolution
of crypto/x509, as we move closer to host verifiers on macOS and Windows.
If we want to merge BoringCrypto into the main tree, it is best not to
have differing API. So instead of a hook set by crypto/tls, move the
actual check directly into crypto/x509, eliminating the need for
exposed API.
For #51940.
Change-Id: Ia2ae98c745de818d39501777014ea8166cab0b03
Reviewed-on: https://go-review.googlesource.com/c/go/+/395878
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
One annoying difference between dev.boringcrypto and master is that
there is not a clear separation between low-level (math/big-free)
crypto and high-level crypto, because crypto/internal/boring imports
both encoding/asn1 and math/big.
This CL removes both those problematic imports and aligns the
dependency rules in the go/build test with the ones in the main
branch.
To remove encoding/asn1, the crypto/internal/boring APIs change to
accepting and returning encoded ASN.1, leaving crypto/ecdsa to do the
marshaling and unmarshaling, which it already contains code to do.
To remove math/big, the crypto/internal/boring package defines
type BigInt []uint, which is the same representation as a big.Int's
internal storage. The new package crypto/internal/boring/bbig provides
conversions between BigInt and *big.Int. The boring package can then
be in the low-level crypto set, and any package needing to use bignum
APIs (necessarily in the high-level crypto set) can import bbig to
convert.
To simplify everything we hide from the test the fact that
crypto/internal/boring imports cgo. Better to pretend it doesn't and
keep the prohibitions that other packages like crypto/aes must not use
cgo (outside of BoringCrypto).
$ git diff origin/master src/go/build/deps_test.go
diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go
index 6ce872e297..a63979cc93 100644
--- a/src/go/build/deps_test.go
+++ b/src/go/build/deps_test.go
@@ -402,9 +402,13 @@ var depsRules = `
NET, log
< net/mail;
+ NONE < crypto/internal/boring/sig;
+ sync/atomic < crypto/internal/boring/fipstls;
+ crypto/internal/boring/sig, crypto/internal/boring/fipstls < crypto/tls/fipsonly;
+
# CRYPTO is core crypto algorithms - no cgo, fmt, net.
# Unfortunately, stuck with reflect via encoding/binary.
- encoding/binary, golang.org/x/sys/cpu, hash
+ crypto/internal/boring/sig, encoding/binary, golang.org/x/sys/cpu, hash
< crypto
< crypto/subtle
< crypto/internal/subtle
@@ -413,6 +417,8 @@ var depsRules = `
< crypto/ed25519/internal/edwards25519/field, golang.org/x/crypto/curve25519/internal/field
< crypto/ed25519/internal/edwards25519
< crypto/cipher
+ < crypto/internal/boring
+ < crypto/boring
< crypto/aes, crypto/des, crypto/hmac, crypto/md5, crypto/rc4,
crypto/sha1, crypto/sha256, crypto/sha512
< CRYPTO;
@@ -421,6 +427,7 @@ var depsRules = `
# CRYPTO-MATH is core bignum-based crypto - no cgo, net; fmt now ok.
CRYPTO, FMT, math/big, embed
+ < crypto/internal/boring/bbig
< crypto/rand
< crypto/internal/randutil
< crypto/ed25519
@@ -443,7 +450,8 @@ var depsRules = `
< golang.org/x/crypto/hkdf
< crypto/x509/internal/macos
< crypto/x509/pkix
- < crypto/x509
+ < crypto/x509;
+ crypto/internal/boring/fipstls, crypto/x509
< crypto/tls;
# crypto-aware packages
@@ -653,6 +661,9 @@ func findImports(pkg string) ([]string, error) {
}
var imports []string
var haveImport = map[string]bool{}
+ if pkg == "crypto/internal/boring" {
+ haveImport["C"] = true // kludge: prevent C from appearing in crypto/internal/boring imports
+ }
fset := token.NewFileSet()
for _, file := range files {
name := file.Name()
For #51940.
Change-Id: I26fc752484310d77d22adb06495120a361568d04
Reviewed-on: https://go-review.googlesource.com/c/go/+/395877
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
The standard Go implementations are allocation-free.
Making the BoringCrypto ones the same helps avoid
surprises, including in some of our own tests.
For #51940.
Change-Id: Ic9c5dc46f5e29ca85f571244be2b380ec2cf89c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/395876
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
The conversion via byte slices is inefficient; we can convert via word slices
and avoid the copy entirely.
For #51940.
Change-Id: I06f747e0acffffae427d9706d43bdacf146c027d
Reviewed-on: https://go-review.googlesource.com/c/go/+/395875
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
The dev.boringcrypto branch has historically forced use of boringcrypto
with no additional configuration flags. The previous CL undid that.
This CL redoes it, so that direct uses of dev.boringcrypto don't lapse
unexpectedly into not having boringcrypto enabled.
When dev.boringcrypto is merged into master, we will undo this change
as part of the merge, so that the only final difference between master
and dev.boringcrypto will be this CL.
For #51940.
Change-Id: I816593a0b30b4e71093a7da9451bae7807d7167e
Reviewed-on: https://go-review.googlesource.com/c/go/+/402597
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
A plain make.bash in this tree will produce a working,
standard Go toolchain, not a BoringCrypto-enabled one.
The BoringCrypto-enabled one will be created with:
GOEXPERIMENT=boringcrypto ./make.bash
For #51940.
Change-Id: Ia9102ed993242eb1cb7f9b93eca97e81986a27b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/395881
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Proposal #42477 asked for a way to apply conditional build tags
to syso files (which have no source code to hold //go:build lines).
We ended up suggesting that the standard answer should be to
put the syso in its own package and then import that package from
a source file that is itself conditionally compiled.
A followup comment on that issue pointed out a problem that I did
not understand until I tried to use this approach myself: the cgo
build fails by default, because the link step only uses syso files from
the current package. You have to override this explicitly by arranging
to pass a “ignore unresolved symbols” flag to the host linker.
Many users will not know how to do this.
(I don't know how to do this off the top of my head.)
If we want users to use this approach, we should make it work better.
This CL does that, by including the syso files from dependencies of
the current package in the link step.
For #51940.
Change-Id: I53a0371b2df17e39a000a645b7686daa6a98722d
Reviewed-on: https://go-review.googlesource.com/c/go/+/402596
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
When we add GOEXPERIMENT=boringcrypto, the bootstrap process
will not converge if the compiler itself depends on the boringcrypto
cgo-based implementations of sha1 and sha256.
Using notsha256 avoids boringcrypto and makes bootstrap converge.
Removing md5 is not strictly necessary but it seemed worthwhile to
be consistent.
For #51940.
Change-Id: Iba649507e0964d1a49a1d16e463dd23c4e348f14
Reviewed-on: https://go-review.googlesource.com/c/go/+/402595
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Package notsha256 implements the NOTSHA256 hash,
defined as bitwise NOT of SHA-256.
It will be used from the Go compiler toolchain where an
arbitrary hash is needed and the code currently reaches
for MD5, SHA1, or SHA256. The problem with all of those
is that when we add GOEXPERIMENT=boringcrypto, the
bootstrap process will not converge if the compiler itself
depends on the boringcrypto cgo code.
Using notsha256 avoids boringcrypto.
It is possible that I don't fully understand the convergence
problem and that there is a way to make the compiler converge
when using cgo, but keeping cgo out of the compiler seems safest.
It also makes clear that (except for the hack in codesign)
the code using this package doesn't care which hash is used.
For #51940.
Change-Id: Ie7c661183eacf8413a9d2074c96cbb9361e125ef
Reviewed-on: https://go-review.googlesource.com/c/go/+/402594
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
replace for string's end trimming TrimFunc -> TrimRightFunc
strings.TrimSpace string's end trimming should use more specific TrimRightFunc instead of common TrimFunc (because start has already trimmed before)
Change-Id: I827f1a25c141e61edfe1f8b11f6e8cd685f8b384
GitHub-Last-Rev: 040607a831
GitHub-Pull-Request: golang/go#46862
Reviewed-on: https://go-review.googlesource.com/c/go/+/329731
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Replaced github username with my real name and added to AUTHORS.
Change-Id: Id68c30eeb31b3f0b7cecc434462029843758f9f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/402794
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Fix typo in comment describing IsWordChar.
Change-Id: Ia283813cf5662e218ee6d0411fb0c1b1ad1021f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/393435
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This extends CL 402190 from Linux to the rest of the Unix OSes.
Marking sigtramp as TOPFRAME allows gentraceback to stop tracebacks at
the end of a signal handler, since there is not much beyond sigtramp.
Change-Id: I8b7f5d55d41889f59c0a79c65351b9b0b2d77717
Reviewed-on: https://go-review.googlesource.com/c/go/+/402934
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
On LR machine, consider F calling G calling H, which grows stack.
The stack looks like
...
G's frame:
... locals ...
saved LR = return PC in F <- SP points here at morestack
H's frame (to be created)
At morestack, we save
gp.sched.pc = H's morestack call
gp.sched.sp = H's entry SP (the arrow above)
gp.sched.lr = return PC in G
Currently, when unwinding through morestack (if _TraceJumpStack
is set), we switch PC and SP but not LR. We then have
frame.pc = H's morestack call
frame.sp = H's entry SP (the arrow above)
As LR is not set, we load it from stack at *sp, so
frame.lr = return PC in F
As the SP hasn't decremented at the morestack call,
frame.fp = frame.sp = H's entry SP
Unwinding a frame, we have
frame.pc = old frame.lr = return PC in F
frame.sp = old frame.fp = H's entry SP a.k.a. G's SP
The PC and SP don't match. The unwinding will go off if F and G
have different frame sizes.
Fix this by preserving the LR when switching stack.
Also add code to detect infinite loop in unwinding.
TODO: add some test. I can reproduce the infinite loop (or throw
with added check) but the frequency is low.
May fix#52116.
Change-Id: I6e1294f1c6e55f664c962767a1cf6c466a0c0eff
Reviewed-on: https://go-review.googlesource.com/c/go/+/400575
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Eric Fang <eric.fang@arm.com>
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Now gc can generate the same assembly code.
Change-Id: Iac503003e14045d63e2def66408c13cee516aa37
Reviewed-on: https://go-review.googlesource.com/c/go/+/402575
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Wayne Zuo <wdvxdr@golangcn.org>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
After CL 402974, we do not typecheck an inline body in ImportedBody
anymore, thus the inTypeCheckInl check is un-necessary.
Change-Id: Ia6154a57db507c3e00b67333f1e3b910a6de7d05
Reviewed-on: https://go-review.googlesource.com/c/go/+/402975
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Keith Randall <khr@golang.org>
Now, 1.17 is the least supported version, the compiler always write
type information when exporting function bodies. So we can get rid of
go117ExportTypes constant and all its conditional checking codes.
Change-Id: I9ac616509c30601e94f99426049d814328253395
Reviewed-on: https://go-review.googlesource.com/c/go/+/402974
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
There are several tests in the runtime that need to force various
things to escape to the heap. This CL centralizes this functionality
into runtime.Escape, defined in export_test.
Change-Id: I2de2519661603ad46c372877a9c93efef8e7a857
Reviewed-on: https://go-review.googlesource.com/c/go/+/402178
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Austin Clements <austin@google.com>
Auto-Submit: Austin Clements <austin@google.com>
This gives explicit names to the possible states of throwing (-1, 0, 1).
m.throwing is now one of:
throwTypeOff: not throwing, previously == 0
throwTypeUser: user throw, previously == -1
throwTypeRuntime: runtime throw, previously == 1
For runtime throws, we now always include frame metadata and system
goroutines regardless of GOTRACEBACK to aid in debugging the runtime.
For user throws, we no longer include frame metadata or runtime frames,
unless GOTRACEBACK=system or higher.
For #51485.
Change-Id: If252e2377a0b6385ce7756b937929be4273a56c0
Reviewed-on: https://go-review.googlesource.com/c/go/+/390421
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Austin Clements <austin@google.com>
"User" throws are throws due to some invariant broken by the application.
"System" throws are due to some invariant broken by the runtime,
environment, etc (i.e., not the fault of the application).
This CL sends "user" throws through the new fatal. Currently this
function is identical to throw, but with a different name to clearly
differentiate the throw type in the stack trace, and hopefully be a bit
more clear to users what it means.
This CL changes a few categories of throw to fatal:
1. Concurrent map read/write.
2. Deadlock detection.
3. Unlock of unlocked sync.Mutex.
4. Inconsistent results from syscall.AllThreadsSyscall.
"Thread exhaustion" and "out of memory" (usually address space full)
throws are additional throws that are arguably the fault of user code,
but I've left off for now because there is no specific invariant that
they have broken to get into these states.
For #51485
Change-Id: I713276a6c290fd34a6563e6e9ef378669d74ae32
Reviewed-on: https://go-review.googlesource.com/c/go/+/390420
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Currently throw() in the signal handler results in "fatal error: unknown
return pc from runtime.sigreturn ...".
Marking sigtramp as TOPFRAME allows gentraceback to stop tracebacks at
the end of a signal handler, since there is not much beyond sigtramp.
This is just done on Linux for now, but may apply to other Unix systems
as well.
Change-Id: I96edcb945283f417a5bfe00ce2fb2b1a0d578692
Reviewed-on: https://go-review.googlesource.com/c/go/+/402190
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
RawSyscall is used in a variety of rather unsafe conditions, such as
after fork in forkAndExecInChild1. Disable race instrumentation to avoid
calling TSAN in unsafe conditions.
For #51087
Change-Id: I47c35e6f0768c77ddab99010ea0404c45ad2f1da
Reviewed-on: https://go-review.googlesource.com/c/go/+/402914
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TestAlias checks systematically for aliasing issues, where passing the
same value for an argument and the receiver leads to incorrect results.
We had a number of issues like that over the years:
- #31084: Lsh on arm64
- #30217: GCD
- #22830: Exp due to divLarge
- #22265: ModSqrt
- #20490: Add and Sub
- #11284: GCD
This CL also fixes two new minor bugs that the test found. A wrong
result would be returned by
- Exp when the modulo and the receiver alias
- Rand when the limit is negative and it aliases the receiver
The test runs in ~0.05s with the default -quickchecks value.
Change-Id: I8354069ec9886e40c60f2642342ee08e604befb7
Reviewed-on: https://go-review.googlesource.com/c/go/+/168257
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
This makes Gerrit recognize the rename of the field implementation and
facilitates the review. No code changes.
For #52182
Change-Id: I827004e175db1ae2fcdf17d0f586ff21503d27e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/390754
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Marshal behavior for invalid points is undefined, so don't use it to
check if points are valid.
For #52182
Change-Id: If167893bc4b029f71bb2528564f2bd96bee7221c
Reviewed-on: https://go-review.googlesource.com/c/go/+/382994
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
There was no way to use an interface because the methods on the Point
types return concrete Point values, as they should.
A couple somewhat minor annoyances:
- Allocations went up due to #48849. This is fine here, where
math/big causes allocations anyway, but would probably not be fine
in nistec itself.
- Carrying the newPoint/newGenerator functions around as a field is
a little weird, even if type-safe. It also means we have to make
what were functions methods so they can access newPoint to return
the zero value. This is #35966.
For #52182
Change-Id: I050f3a27f15d3f189818da80da9de0cba0548931
Reviewed-on: https://go-review.googlesource.com/c/go/+/360015
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Not quite golang.org/wiki/TargetSpecific compliant, but almost.
The only substantial code change is in randFieldElement: it used to use
Params().BitSize instead of Params().N.BitLen(), which is semantically
incorrect, even if the two values are the same for all named curves.
For #52182
Change-Id: Ibc47450552afe23ea74fcf55d1d799d5d7e5487c
Reviewed-on: https://go-review.googlesource.com/c/go/+/315273
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
This test tends to be flaky on 32-bit systems.
There's not enough bits in the hash output, so we
expect a nontrivial number of collisions, and it is
often quite a bit higher than expected.
Fixes#43130
Change-Id: If35413b7c45eed778a08b834dacf98009ceca840
Reviewed-on: https://go-review.googlesource.com/c/go/+/402456
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Keith Randall <khr@google.com>
This change refactors the scavenger into a type whose methods represent
the actual function and scheduling of the scavenger. It also stubs out
access to global state in order to make it testable.
This change thus also adds a test for the scavenger. In writing this
test, I discovered the lack of a behavior I expected: if the
pageAlloc.scavenge returns < the bytes requested scavenged, that means
the heap is exhausted. This has been true this whole time, but was not
documented or explicitly relied upon. This change rectifies that. In
theory this means the scavenger could spin in run() indefinitely (as
happened in the test) if shouldStop never told it to stop. In practice,
shouldStop fires long before the heap is exhausted, but for future
changes it may be important. At the very least it's good to be
intentional about these things.
While we're here, I also moved the call to stopTimer out of wake and
into sleep. There's no reason to add more operations to a context that's
already precarious (running without a P on sysmon).
Change-Id: Ib31b86379fd9df84f25ae282734437afc540da5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/384734
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change completes the proposal laid out in #44163. With #44313
resolved, we now ensure that stopped Ms are able to wake up and become
dedicated GC workers. As a result, idle GC workers are in theory no
longer required to be a proxy for scheduling dedicated mark workers.
And, with at least one dedicated mark worker running (which is
non-preemptible) we ensure the GC makes progress in all circumstances
when at least one is running. Currently we ensure at least one idle mark
worker is available at all times because it's possible before #44313
that a dedicated worker doesn't ever get scheduled, leading to a
deadlock if user goroutines block on a GC completing. But now that extra
idle mark worker should be unnecessary to ensure GC progress when at
least one dedicated mark worker is going to be scheduled.
Fixes#44163.
Change-Id: I62889ef2db4e69d44da883e8e6eebcfe5398c86d
Reviewed-on: https://go-review.googlesource.com/c/go/+/395634
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change moves several scheduling decisions made by schedule into
findrunnable. The main motivation behind this change is the fact that
stopped Ms can't become dedicated or fractional GC workers. The main
reason for this is that when a stopped M wakes up, it stays in
findrunnable until it finds work, which means it will never consider GC
work. On that note, it'll also never consider becoming the trace reader,
either.
Another way of looking at it is that this change tries to make
findrunnable aware of more sources of work than it was before. With this
change, any M in findrunnable should be capable of becoming a GC worker,
resolving #44313. While we're here, let's also make more sources of
work, such as the trace reader, visible to handoffp, which should really
be checking all sources of work. With that, we also now correctly handle
the case where StopTrace is called from the last live M that is also
locked (#39004). stoplockedm calls handoffp to start a new M and handle
the work it cannot, and once we include the trace reader in that, we
ensure that the trace reader gets scheduled.
This change attempts to preserve the exact same ordering of work
checking to reduce its impact.
One consequence of this change is that upon entering schedule, some
sources of work won't be checked twice (i.e. the local and global
runqs, and timers) as they do now, which in some sense gives them a
lower priority than they had before.
Fixes#39004.
Fixes#44313.
Change-Id: I5d8b7f63839db8d9a3e47cdda604baac1fe615ce
Reviewed-on: https://go-review.googlesource.com/c/go/+/393880
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change reduces the maximum number of idle mark workers during
periodic (currently every 2 minutes) GC cycles to 1.
Idle mark workers soak up all available and unused Ps, up to GOMAXPROCS.
While this provides some throughput and latency benefit in general, it
can cause what appear to be massive CPU utilization spikes in otherwise
idle applications. This is mostly an issue for *very* idle applications,
ones idle enough to trigger periodic GC cycles. This spike also tends to
interact poorly with auto-scaling systems, as the system might assume
the load average is very low and suddenly see a massive burst in
activity.
The result of this change is not to bring down this 100% (of GOMAXPROCS)
CPU utilization spike to 0%, but rather
min(25% + 1/GOMAXPROCS*100%, 100%)
Idle mark workers also do incur a small latency penalty as they must be
descheduled for other work that might pop up. Luckily the runtime is
pretty good about getting idle mark workers off of Ps, so in general
the latency benefit from shorter GC cycles outweighs this cost. But, the
cost is still non-zero and may be more significant in idle applications
that aren't invoking assists and write barriers quite as often.
We can't completely eliminate idle mark workers because they're
currently necessary for GC progress in some circumstances. Namely,
they're critical for progress when all we have is fractional workers. If
a fractional worker meets its quota, and all user goroutines are blocked
directly or indirectly on a GC cycle (via runtime.GOMAXPROCS, or
runtime.GC), the program may deadlock without GC workers, since the
fractional worker will go to sleep with nothing to wake it.
Fixes#37116.
For #44163.
Change-Id: Ib74793bb6b88d1765c52d445831310b0d11ef423
Reviewed-on: https://go-review.googlesource.com/c/go/+/393394
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
OpenBSD has a coarse sleep granularity that rounds up to 10 ms
increments. This can cause significant STW delays, among other issues.
As far as I can tell, there's only 1 tightly timed sleep without an
explicit wakeup for which this actually matters.
Fixes#52475.
Change-Id: Ic69fc11096ddbbafd79b2dcdf3f912fde242db24
Reviewed-on: https://go-review.googlesource.com/c/go/+/401638
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Right now we export alloc count metrics via the runtime/metrics package
and mark them as monotonic, but that's not actually true. As an
optimization, the runtime assumes a span is always fully allocated
before being uncached, and updates the accounting as such. In the rare
case that it's wrong, the span has enough information to back out what
did not get allocated.
This change uses 16 bits of padding in the mspan to house another field
that represents the amount of mspan slots filled just as the mspan is
cached. This is information is enough to get an exact count, allowing us
to make the metrics truly monotonic.
Change-Id: Iaff3ca43f8745dc1bbb0232372423e014b89b920
Reviewed-on: https://go-review.googlesource.com/c/go/+/377516
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Change-Id: Ic0e882fc6666c9adcd5f2dffc96e201f3146fa0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/402180
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
sigtramp on openbsd-arm64 is teetering on the edge of the nosplit stack
limit. Add more headroom by calling sigtrampgo using ABIInternal, which
eliminates a 48-byte ABI wrapper frame.
openbsd-amd64 has slightly more space, but is also close to the limit,
so convert it as well.
Other operating systems don't have it as bad, but many have nearly
identical implementations of sigtramp, so I have converted them as well.
I've omitted darwin-arm64 and solaris, as those are quite different and
would benefit from not needing ifdef for both cases.
For #51485.
Change-Id: I70512645d4208b346a59d5e5d03836a45833b1d7
Reviewed-on: https://go-review.googlesource.com/c/go/+/390814
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
We're turning up Macs in a network environment that clashes with this
test. I don't think it's critical to get it working, so skip it.
For #49149.
Change-Id: I925e3ecc5356c4cefd208bdcff3d98021215d0b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/402181
Reviewed-by: Alex Rakoczy <alex@golang.org>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Auto-Submit: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The previous implementation of helperCommand relied on running a
well-known Test function which implemented all known commands.
That not only added Skip noise in the test's output, but also (and
more importantly) meant that the commands could not write directly to
stdout in the usual way, since the testing package hijacks os.Stdout
for its own use.
The new implementation addresses the above issues, and also ensures
that all registered commands are actually used, reducing the risk of
an unused command sticking around after refactoring.
It also sets the subprocess environment variable directly in the test
process, instead of on each individual helper command's Env field,
allowing helper commands to be used without an explicit Env.
Updates #50599.
(Also for #50436.)
Change-Id: I189c7bed9a07cfe47a084b657b88575b1ee370b9
Reviewed-on: https://go-review.googlesource.com/c/go/+/401934
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
This makes clearer that skipStdinCopyError is always defined and never
overridden in tests.
Secondarily, it may also help reduce init-time work and allow the
linker and/or inliner to better optimize this package.
(Noticed while prototyping #50436.)
Change-Id: I4f3c1bc146384a98136a4039f82165ed106c14b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/401897
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
When we call time.Unix(s, ns), the internal representation is
s + 62135596800, where 62135596800 is the number of
seconds from Jan 1 1 to Jan 1 1970.
If quickcheck generates numbers too close to 2^63,
the addition can wraparound to make a very negative
internal 64-bit value. Wraparounds are not guarded
against, since they would not arise in any reasonable program,
so just avoid testing near them.
Fixes#52409.
Change-Id: Id466c8a34a49055ab26f2687a6b2b657cb64bed6
Reviewed-on: https://go-review.googlesource.com/c/go/+/402177
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Accept ~x as ordinary unary expression in the parser but recognize
such expressions as invalid in the type checker.
This change opens the door to recognizing complex type constraint
literals such as `*E|~int` in `[P *E|~int]` and parse them correctly
instead of reporting a parse error because `P*E|~int` syntactically
looks like an incorrect array length expression (binary expression
where the RHS of | is an invalid unary expression ~int).
As a result, the parser is more forgiving with expressions but the
type checker will reject invalid uses as before.
We could pass extra information into the binary/unary expression
parse functions to prevent the use of ~ in invalid situations but
it doesn't seem worth the trouble. In fact it may be advantageous
to allow a more liberal expression syntax especially in the presence
of errors (better parser synchronization after an error).
Preparation for fixing #49482.
Change-Id: I119e8bd9445dfa6460fcd7e0658e3554a34b2769
Reviewed-on: https://go-review.googlesource.com/c/go/+/402255
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Change-Id: I46543e188bf25384e529a9d5a3095033ac618bbd
Reviewed-on: https://go-review.googlesource.com/c/go/+/402057
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Fixes#30715
Change-Id: Ia3712d248b6dc86abef71ccea6e705a571933d53
GitHub-Last-Rev: 6ae68402a5
GitHub-Pull-Request: golang/go#49359
Reviewed-on: https://go-review.googlesource.com/c/go/+/361397
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
