go/parser will by default resolve objects as per the go/ast.Object type,
which is then used by gofmt's rewrite and simplify flags.
However, none of that is needed if neither of the flags is set,
so we can avoid the work entirely for a nice speed-up.
benchcmd -n 8 GofmtSrcCmd gofmt -l ~/tip/src/cmd
name old time/op new time/op delta
GofmtSrcCmd 957ms ± 7% 908ms ± 7% -5.12% (p=0.028 n=8+8)
name old user-time/op new user-time/op delta
GofmtSrcCmd 11.2s ± 1% 10.4s ± 1% -7.23% (p=0.001 n=7+7)
name old sys-time/op new sys-time/op delta
GofmtSrcCmd 325ms ±29% 286ms ±22% ~ (p=0.065 n=8+8)
name old peak-RSS-bytes new peak-RSS-bytes delta
GofmtSrcCmd 295MB ±17% 276MB ±15% ~ (p=0.328 n=8+8)
See #46485.
Change-Id: Iad1ae294953710c233f7837d7eb02e23d11c6185
Reviewed-on: https://go-review.googlesource.com/c/go/+/401454
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
This reverts CL400377, which restricted serials passed to
x509.CreateCertificate to <= 20 octets. Unfortunately this turns out to
be something _a lot_ of people get wrong. Since it's not particularly
obvious how to properly generate conformant serials, until we provide
an easier way for people to get this right, reverting this restriction
makes sense (possible solution discussed in #52444.)
Change-Id: Ia85a0ffe61e2e547abdaf1389c3e1ad29e28a2be
Reviewed-on: https://go-review.googlesource.com/c/go/+/401657
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Rename it TestIssue1560 since it no longer sleeps.
For #1560Fixes#45586
Change-Id: I338eee9de43e871da142143943e9435218438e90
Reviewed-on: https://go-review.googlesource.com/c/go/+/400194
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When the user is trying to list or build a package in a module that's
outside of the workspace provide a more clear message hinting to the
user that they can add the module to the workspace using go work use.
Fixes#51604
Change-Id: I1202ecb2f22fd6351bfdec88ed613b8167687fb7
Reviewed-on: https://go-review.googlesource.com/c/go/+/400014
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Auto-Submit: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Future CLs will be changing the provenance of these functions. Move the
declarations to the individual OS files now so that future CLs can
change only 1 OS at a time rather than changing all at once.
For #51087
Change-Id: I5e1bca71e670263d8c0faa586c1b6b4de1a114b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/388474
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This CL exports the existing ir.UintptrKeepAlive via the new directive
//go:uintptrkeepalive. This makes the compiler insert KeepAlives for
pointers converted to uintptr in calls, keeping them alive for the
duration of the call.
//go:uintptrkeepalive requires //go:nosplit, as stack growth can't
handle these arguments (it cannot know which are pointers). We currently
check this on the immediate function, but the actual restriction applies
to all transitive calls.
The existing //go:uintptrescapes is an extension of
//go:uintptrkeepalive which forces pointers to escape to the heap, thus
eliminating the stack growth issue.
This pragma is limited to the standard library.
For #51087
Change-Id: If9a19d484d3561b4219e5539b70c11a3cc09391e
Reviewed-on: https://go-review.googlesource.com/c/go/+/388095
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
CL 388095 will change this file significantly. Move it preemptively to
ensure git tracks the move properly.
For #51087
Change-Id: I1408aecf8675723041b64e54cf44cdec38cc655c
Reviewed-on: https://go-review.googlesource.com/c/go/+/388094
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This is a code simplification using the new API added in #50599.
Change-Id: Icb9628bcd0daa3dc2d653e9654b38099730137d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/401535
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
With #50599 implemented, base.AppendPWD is redundant if cmd.Env would
otherwise be nil, and calls to os.Environ followed by base.AppendPWD
can be replaced by a simpler call to cmd.Environ.
Updates #50599.
Change-Id: I94a22e2a4cc8e83c815ac41702ea0b1ee5034ecc
Reviewed-on: https://go-review.googlesource.com/c/go/+/401534
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Change-Id: If3d8391d81e8de869dbb3c857f0570817e8aa440
Reviewed-on: https://go-review.googlesource.com/c/go/+/400914
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Wayne Zuo <wdvxdr@golangcn.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Change-Id: I5698c7576a0f39ae62de7bea64286ac8e578d421
Reviewed-on: https://go-review.googlesource.com/c/go/+/400916
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Wayne Zuo <wdvxdr@golangcn.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Change-Id: I5ccbcea4c53658136b25ca608faec19eeec2e908
Reviewed-on: https://go-review.googlesource.com/c/go/+/400915
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Wayne Zuo <wdvxdr@golangcn.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This reverts CL 400877.
Reason for revert: broke TestScript/test_output_wait and the
regression test for #18153 on certain builders.
Fixes#52461.
Change-Id: I98627ce5e22088b0784be502c459480c41ba353a
Reviewed-on: https://go-review.googlesource.com/c/go/+/401494
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Per RFC 4158 Section 2.4.2, when we are discarding candidate
certificates during path building, use the SANs as well as subject and
public key when checking whether a certificate is already present in
the built path. This supports the case where a certificate in the chain
(typically a leaf) has the exact same subject and public key as another
certificate in the chain (typically its parent) but has SANs which don't
match.
Change-Id: I212c234e94a1f6afbe9691e4a3ba257461db3a7e
Reviewed-on: https://go-review.googlesource.com/c/go/+/401115
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Does what it says on the tin.
Fixes#51088
Change-Id: I12c0fa6bba1c1ce96c1ad31ba387c77a93f801c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/384894
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Once #50599 is implemented, the entries will be observable via the
Environ method. I find it confusing for later entries in the list to
jump arbitrarily far forward based on entries for the same key that no
longer exist.
This also fixes the deduplication logic for the degenerate Windows
keys observed in #49886, which were previously deduplicated as empty
keys.
(It does not do anything about the even-more-degenerate keys observed
in #52436.)
For #50599.
Change-Id: Ia7cd2200ec34ccc4b9d18631cb513194dc420c25
Reviewed-on: https://go-review.googlesource.com/c/go/+/401339
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
There is an issue where 'go test' will hang after the tests complete if
a test starts a sub-process that does not exit (see #24050).
However, go test only exhibits that behavior when a package name is
explicitly passed as an argument. If 'go test' is invoked without any
package arguments then the package in the working directory is assumed,
however in that case (and only that case) os.Stdout is used as the test
process's cmd.Stdout, which does *not* cause 'go test' wait for the
sub-process to exit (see #23019).
This change wraps os.Stdout in an io.Writer struct in this case, hiding
the *os.File from the os/exec package, causing cmd.Wait to always wait
for the full output from the test process and any of its sub-processes.
In other words, this makes 'go test' exhibit the same behavior as
'go test .' (or 'go test ./...' and so on).
Update #23019
Update #24050
Change-Id: Ica09bf156f3b017f9a31aad91ed0f16a7837195b
Reviewed-on: https://go-review.googlesource.com/c/go/+/400877
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Andrew Gerrand <adg@golang.org>
Auto-Submit: Andrew Gerrand <adg@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Support fastrand64 in the runtime, although fastrand uses wyrand to generate 64-bit random number, it still returns uint32. In some cases, we need to generate a 64-bit random number, the new API would be faster and easier to use, and at least we can use the new function in these places:
src/net/dnsclient.go:randInt()
src/hash/maphash/maphash.go:MakeSeed()
src/runtime/map.go:mapiterinit()
name time/op
Fastrand-16 0.09ns ± 5%
Fastrand64-16 0.09ns ± 6%
Change-Id: Ibb97378c7ca59bc7dc15535d4872fa58ea112e6a
Reviewed-on: https://go-review.googlesource.com/c/go/+/400734
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
overflowPad variable in bucketOf function is a holdover from a NaCl port
and never used now.
Change-Id: Ib68fdb054e1b6a655ffbfd34521a3f8773a22694
GitHub-Last-Rev: f281be9c11
GitHub-Pull-Request: golang/go#52449
Reviewed-on: https://go-review.googlesource.com/c/go/+/401274
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
The WHATWG Mime Sniffing Standard we follow dropped support for
.snd / audio/basic.
https://github.com/whatwg/mimesniff/issues/151
Change-Id: Iae1bd8b29063b06b5b3909a944e12ead5974a526
Reviewed-on: https://go-review.googlesource.com/c/go/+/400754
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Auto-Submit: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
When we have an error in a function type used in an expression
we don't know until we see an opening { whether we have a function
literal or a function type. Use "function type" as context because
that's always correct in the specific error message.
Change-Id: I9aad8fcddf31ae53daa53cebd2c2001f08eabde0
Reviewed-on: https://go-review.googlesource.com/c/go/+/401316
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
If the user provides the -json flag to explicitly specify fields, but
doesn't specify the Deps or DepsErrors fields, skip computing the deps
fields.
For #29666
Change-Id: I15596c374aba1af13bdf5808d11d54abdc838667
Reviewed-on: https://go-review.googlesource.com/c/go/+/392495
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Michael Matloob <matloob@golang.org>
Auto-Submit: Michael Matloob <matloob@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This test forces GOARCH to amd64, but currently uses the default GOOS.
This works on every OS that supports amd64, which is every OS we
support except AIX. Hence, on AIX this fails with an unsupported
GOOS/GOARCH combination.
Fix this by forcing GOOS to linux.
Fixes#52451.
Change-Id: I9321dd6386c7ef0fe2b47d77ed900aafc53f2a46
Reviewed-on: https://go-review.googlesource.com/c/go/+/401334
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This case was missed in CL 370894, and masked by the lack of an
Illumos TryBot.
Fixes#52452.
Change-Id: I7cda193e33c11a9d04eb888fdb5ec9218e6ed1b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/401294
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <valsorda@google.com>
An error message like "could not import os/user (exit status 1)"
(observed in https://go.dev/issue/52407) is fairly inscrutable.
On the other hand, srcimporter doesn't report errors with quite enough
structure to dump the entire stderr output from 'go tool cgo' without
potentially overwhelming the caller. Here, we split the difference by
describing which command failed but not printing the output of that
command.
For #52407, that would at least provide a stronger clue connecting
to #52408.
Change-Id: Iabdc95b17ba20a0f6ff38e5c7084e5081e1ef5e8
Reviewed-on: https://go-review.googlesource.com/c/go/+/400817
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The POSIX standard requires the PWD variable to be an absolute path.
Fixes#46832
Change-Id: I1938592538633e1a0a0958276f1fefc3c4808399
Reviewed-on: https://go-review.googlesource.com/c/go/+/378396
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Be explicit that we always mean non-interface types when we
talk about sets of types.
Also, clarify that the quantification "all non-interface types"
means all such types in all possible programs, not just the
current program.
Per suggestion from Philip Wadler.
Change-Id: Ibc7b5823164e547bfcee85d4e523e58c7c27ac8a
Reviewed-on: https://go-review.googlesource.com/c/go/+/398655
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Fixes#52438.
Change-Id: I5cbf8c448dba037e9e0c5fe8f209401d6bf7d43f
Reviewed-on: https://go-review.googlesource.com/c/go/+/401134
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Be careful before accessing an operand's expr field (which may
be nil in some rare cases).
While at it, factor out position information so that it's only
computed when there's an error, which is almost never.
In go/types, remove an unnecessary argument to Checker.overflow.
The code is otherwise ok as it's structured slightly differently
due to the way positions are recorded in AST nodes.
Fixes#52401.
Change-Id: I447ebd9bb0c33eb6bff5e7b4d5aee37ceb0a4b14
Reviewed-on: https://go-review.googlesource.com/c/go/+/400798
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Before Go 1.18, an embedded type name in an interface could not be
parenthesized. With generalized embedding of types in interfaces,
where one might write ~(chan<- int) for clarity (making clear that
the ~ applies to the entire channel type), it also makes sense to
permit (chan<- int), or (int) for that matter.
Adjust the parser accordingly to match the spec.
(go/types already accepts the notation as specified by the spec.)
Fixes#52391.
Change-Id: Ifdd9a199c5ccc3473b2dac40dbca31d2df10d12b
Reviewed-on: https://go-review.googlesource.com/c/go/+/400797
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
The kernel's RNG is fast enough, and buffering means taking locks, which
we don't want to do. So just remove all buffering. This also means the
randomness we get is "fresher". That also means we don't need any
locking, making this potentially faster if multiple cores are hitting
GetRandom() at the same time on newer Linuxes.
Also, change the build tag of the tests to be 'unix' instead of
enumerating them.
Change-Id: Ia773fab768270d2aa20c0649f4171c5326b71d02
Reviewed-on: https://go-review.googlesource.com/c/go/+/390038
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Run-TryBot: Jason Donenfeld <Jason@zx2c4.com>
Auto-Submit: Jason Donenfeld <Jason@zx2c4.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
We're using bufio to batch reads of /dev/urandom to 4k, but we weren't
doing the same on newer platforms with getrandom/getentropy. Since the
overhead is the same for these -- one syscall -- we should batch reads
of these into the same 4k buffer. While we're at it, we can simplify a
lot of the constant dispersal.
This also adds a new test case to make sure the buffering works as
desired.
Change-Id: I7297d4aa795c00712e6484b841cef8650c2be4ef
Reviewed-on: https://go-review.googlesource.com/c/go/+/370894
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Run-TryBot: Jason Donenfeld <Jason@zx2c4.com>
Auto-Submit: Jason Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
The x + (-y) => x - y rule is hitted 75 times while building stage 3 and tools
and make the linux-amd64 go binary 0.007% smaller.
It transform:
NEG AX
ADD BX, AX
Into:
SUB BX, AX
Which is 2X faster (assuming this assembly in a vacum).
The x ^ (-1) => ^x rule is not hitted in the toolchain.
It transforms:
XOR $-1, AX
Into:
NOT AX
Which is more compact as it doesn't encode the immediate.
Cache usage aside, this does not affect performance
(assuming this assembly in a vacum).
On my ryzen 3600, with some surrouding code, this randomly might be 2X faster,
I guess this has to do with loading the immediate into a temporary register.
Combined to an other rule that already exists it also rewrite manual two's
complement negation from:
XOR $-1, AX
INC AX
Into:
NEG AX
Which is 2X faster.
The other rules just eliminates similar trivial cases and help constants
folding.
This should generalise to other architectures.
Change-Id: Ia1e51b340622e7ed88e5d856f3b1aa424aa039de
GitHub-Last-Rev: ce35ff2efd
GitHub-Pull-Request: golang/go#52395
Reviewed-on: https://go-review.googlesource.com/c/go/+/400714
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Change-Id: If2887f84b3d14fac3c059fc5bad4186ec9d69d0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/401077
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Keith Randall <khr@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Even though the change in the behavior of 'runtime.GOROOT' was
not actually due to a change in the runtime package proper, I
suspect that users who notice it will look for the release note
in that section, not the 'cmd/go' section.
Fixes#51461.
Change-Id: I271752968d4152a7fdf3e170537e3072bf87ce86
Reviewed-on: https://go-review.googlesource.com/c/go/+/400814
Reviewed-by: Ian Lance Taylor <iant@google.com>
All APIs in the now-deprecated io/ioutil package have a direct
replacement in either the io or os package with the same signature,
with the notable exception of ioutil.ReadDir, as os.ReadDir has a
slightly different signature with fs.DirEntry rather than fs.FileInfo.
New code can easily make use of []fs.DirEntry directly,
but existing code may need to continue using []fs.FileInfo for backwards
compatibility reasons. For instance, I had a bit of code that exposed
the slice as a public API, like:
return ioutil.ReadDir(name)
It took me a couple of minutes to figure out what the exact equivalent
in terms of os.ReadDir would be, and a code sample would have helped.
Add one for future reference.
For #42026.
For #51927.
Change-Id: I76d46cd7d68fc609c873821755fdcfc299ffd56c
Reviewed-on: https://go-review.googlesource.com/c/go/+/399854
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
The linker performs a global analysis of all nosplit call chains to
check they fit in the stack space ensured by splittable functions.
That analysis has two problems right now:
1. It's inefficient. It performs a top-down analysis, starting with
every nosplit function and the nosplit stack limit and walking *down*
the call graph to compute how much stack remains at every call. As a
result, it visits the same functions over and over, often with
different remaining stack depths. This approach is historical: this
check was originally written in C and this approach avoided the need
for any interesting data structures.
2. If some call chain is over the limit, it only reports a single call
chain. As a result, if the check does fail, you often wind up playing
whack-a-mole by guessing where the problem is in the one chain, trying
to reduce the stack size, and then seeing if the link works or reports
a different path.
This CL completely rewrites the nosplit stack check. It now uses a
bottom-up analysis, computing the maximum stack height required by
every function's call tree. This visits every function exactly once,
making it much more efficient. It uses slightly more heap space for
intermediate storage, but still very little in the scheme of the
overall link. For example, when linking cmd/go, the new algorithm
virtually eliminates the time spent in this pass, and reduces overall
link time:
│ before │ after │
│ sec/op │ sec/op vs base │
Dostkcheck 7.926m ± 4% 1.831m ± 6% -76.90% (p=0.000 n=20)
TotalTime 301.3m ± 1% 296.4m ± 3% -1.62% (p=0.040 n=20)
│ before │ after │
│ B/op │ B/op vs base │
Dostkcheck 40.00Ki ± 0% 212.15Ki ± 0% +430.37% (p=0.000 n=20)
Most of this time is spent analyzing the runtime, so for larger
binaries, the total time saved is roughly the same, and proportionally
less of the overall link.
If the new implementation finds an error, it redoes the analysis,
switching to preferring quality of error reporting over performance.
For error reporting, it computes stack depths top-down (like the old
algorithm), and reports *all* paths that are over the stack limit,
presented as a tree for compactness. For example, this is the output
from a simple test case from test/nosplit with two over-limit paths
from f1:
main.f1: nosplit stack overflow
main.f1
grows 768 bytes, calls main.f2
grows 56 bytes, calls main.f4
grows 48 bytes
80 bytes over limit
grows 768 bytes, calls main.f3
grows 104 bytes
80 bytes over limit
While we're here, we do a few nice cleanups:
- We add a debug output flag, which will be useful for understanding
what our nosplit chains look like and which ones are close to
running over.
- We move the implementation out of the fog of lib.go to its own file.
- The implementation is generally more Go-like and less C-like.
Change-Id: If1ab31197f5215475559b93695c44a01bd16e276
Reviewed-on: https://go-review.googlesource.com/c/go/+/398176
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
