Benchmarking suggests about a 14-17% reduction in user build time,
about 3.5-7.8% reduction for wall time. This helps most builds
because small packages are common. Latest benchmarks (after the last
round of improvement):
(12 processors) https://perf.golang.org/search?q=upload:20221102.20
(GOMAXPROCS=2) https://perf.golang.org/search?q=upload:20221103.1
(48 processors) https://perf.golang.org/search?q=upload:20221102.19
(The number of compiler workers is capped at min(4, GOMAXPROCS))
An earlier, similar version of this CL at one point observed a 27%
reduction in user build time (building 40+ benchmarks, 20 times), but
the current form is judged to be the most reliable; it may be
profitable to tweak the numbers slightly later, and/or to adjust the
number of compiler workers.
We've talked about doing this in the past, the "new"(ish) metrics
package makes it a more tractable proposition.
The method here is:
1. If os.Getenv("GOGC") is empty, then increase GOGC to a large value,
calculated to grow the heap to 32 + 4 * compile_parallelism before a
GC occurs (e.g., on a >= 4 processor box, 64M). In practice,
sometimes GC occurs before that, but this still results in fewer GCs
and saved time. This is "heap goal".
2. Use a finalizer to approximately detect when GC occurs, and use
runtime metrics to track progress towards the goal heap size,
readjusting GOGC to retarget it as necessary. Reset GOGC to 100 when
the heap is "close enough" to the goal.
One feared failure mode of doing this is that the finalizer will be
slow to run and the heap will grow exceptionally large before GOGC is
reset; I monitored the heap size at reset and exit across several
boxes with a variety of processor counts and extra noise
(including several builds in parallel, including a laptop with a busy
many-tabs browser running) and overshoot effectively does not occur.
In some cases the compiler's heap grows so rapidly that estimated live
exceeds the GC goal, but this is not delayed-finalizer overshoot; the
compiler is just using that much memory. In a small number of cases
(3% of GCs in make.bash) the new goal is larger than predicted by as
much as 38%, so check for that and redo the adjustment.
I considered instead using the maximum heap size limit +
GC-detecting-finalizer + reset instead, but to me that seemed like it
might have a worse bad-case outcome; if the reset is delayed, it's
possible the GC would start running frequently, making it harder to
run the finalizer, reach 50% utilization, and the extra GCs would
lose the advantage. This might also perform badly in the case that a
rapidly growing heap outruns goal. In practice, this sort of
overshoot hasn't been observed, and a goal of 64M is small enough to
tolerate plenty of overshoot anyway.
This version of the CL includes a comment urging anyone who sees the
code and thinks it would work for them, to update a bug (to be
created if the CL is approved) with information about their
situation/experience, so that we may consider creating some more
official and reliable way of obtaining the same result.
Change-Id: I45df1c927c1a7d7503ade1abd1a3300e27516633
Reviewed-on: https://go-review.googlesource.com/c/go/+/436235
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
The stack pointer must lie within system stack limits
when Control Flow Guard (CFG) is enabled on Windows.
This CL updates runtime.sigtramp to honor this restriction by
porting some code from the windows/arm64 version, which
already supports CFG.
Fixes#53560
Change-Id: I7f88f9ae788b2bac38aac898b2567f1bea62f8f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/437559
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
In following with Roland's TODO, switch TestDisableSHA1ForCertOnly to ParseRevocationList(...) over ParseCRL(...).
Change-Id: I8cdaf04ad0a1c8b94303415ae41933657067041e
GitHub-Last-Rev: bb2ef760e4
GitHub-Pull-Request: golang/go#56541
Reviewed-on: https://go-review.googlesource.com/c/go/+/447036
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
This adds a -d debug flag "fmahash" for hashcode search for
floating point architecture-dependent problems. This variable has no
effect on architectures w/o fused-multiply-add.
This was rebased onto the GOSSAHASH renovation so that this could have
its own dedicated environment variable, and so that it would be
cheap (a nil check) to check it in the normal case.
Includes a basic test of the trigger plumbing.
Sample use (on arm64, ppc64le, s390x):
% GOCOMPILEDEBUG=fmahash=001110110 \
go build -o foo cmd/compile/internal/ssa/testdata/fma.go
fmahash triggered main.main:24 101111101101111001110110
GOFMAHASH triggered main.main:20 010111010000101110111011
1.0000000000000002 1.0000000000000004 -2.220446049250313e-16
exit status 1
The intended use is in conjunction with github.com/dr2chase/gossahash,
which will probably acquire a flag "-fma" to streamline its use. This
tool+use was inspired by an ad hoc use of this technique "in anger"
to debug this very problem. This is also a dry-run for using this
same technique to identify code sensitive to loop variable
lifetime/capture, should we make that change.
Example intended use, with current search tool (using old environment
variable), for a test example:
gossahash -e GOFMAHASH GOMAGIC=GOFMAHASH go run fma.go
Trying go args=[...], env=[GOFMAHASH=1 GOMAGIC=GOFMAHASH]
go failed (81 distinct triggers): exit status 1
Trying go args=[...], env=[GOFMAHASH=11 GOMAGIC=GOFMAHASH]
go failed (39 distinct triggers): exit status 1
Trying go args=[...], env=[GOFMAHASH=011 GOMAGIC=GOFMAHASH]
go failed (18 distinct triggers): exit status 1
Trying go args=[...], env=[GOFMAHASH=0011 GOMAGIC=GOFMAHASH]
Trying go args=[...], env=[GOFMAHASH=1011 GOMAGIC=GOFMAHASH]
...
Trying go args=[...], env=[GOFMAHASH=0110111011 GOMAGIC=GOFMAHASH]
Trying go args=[...], env=[GOFMAHASH=1110111011 GOMAGIC=GOFMAHASH]
go failed (2 distinct triggers): exit status 1
Trigger string is 'GOFMAHASH triggered math.qzero:427 111111101010011110111011', repeated 6 times
Trigger string is 'GOFMAHASH triggered main.main:20 010111010000101110111011', repeated 1 times
Trying go args=[...], env=[GOFMAHASH=01110111011 GOMAGIC=GOFMAHASH]
go failed (1 distinct triggers): exit status 1
Trigger string is 'GOFMAHASH triggered main.main:20 010111010000101110111011', repeated 1 times
Review GSHS_LAST_FAIL.0.log for failing run
FINISHED, suggest this command line for debugging:
GOSSAFUNC='main.main:20 010111010000101110111011' \
GOFMAHASH=01110111011 GOMAGIC=GOFMAHASH go run fma.go
Change-Id: Ifa22dd8f1c37c18fc8a4f7c396345a364bc367d5
Reviewed-on: https://go-review.googlesource.com/c/go/+/394754
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
On some platforms, building PIE needs external linking, which
cannot run if cgo is not available.
Change-Id: I6d504aed0f0442cda0355d0beac606ad365e2046
Reviewed-on: https://go-review.googlesource.com/c/go/+/447616
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Currently in PGO we use a percentage threshold to determine if a
callsite is hot. This CL uses a different method -- treating the
hottest callsites that make up cumulatively top X% of total edge
weights as hot (X=95 for now). This default might work better for
a wider range of profiles. (The absolute threshold can still be
changed by a flag.)
For #55022.
Change-Id: I7e3b6f0c3cf23f9a89dd5994c10075b498bf14ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/447016
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
`TestScripts/svn` test suite fails if the host does not have a TZ
database installed.
This CL updates those tests so SVN formats dates using UTC, which
don't require a TZ database.
Fixes#56527
Change-Id: I20f3c03c3cedd7d748f4623dddc66bd04d1df318
Reviewed-on: https://go-review.googlesource.com/c/go/+/447335
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Currently, FromDir identifies a VCS checkout directory just by checking
whether it contains a specified file. This is not enough. For example,
although there is a ".git" file (a plain file, not a directory) in a
git submodule directory, this directory is not a git repository root.
This change takes the file mode into account. As of now, the filename
and file mode for the supported VCS tools are:
- Mercurial: .hg directory
- Git: .git directory
- Bazaar: .bzr directory
- Subversion: .svn directory
- Fossil: .fslckout plain file
- Fossil: _FOSSIL_ plain file
This CL effectively reverts CL 30948 for #10322.
Fixes#53640.
Change-Id: Iea316c7e983232903bddb7e7f6dbaa55e8498685
GitHub-Last-Rev: 7a2d6ff6f9
GitHub-Pull-Request: golang/go#56296
Reviewed-on: https://go-review.googlesource.com/c/go/+/443597
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
This is an oversight from https://go-review.googlesource.com/c/go/+/419875,
where script commands were refactored and factored out to a new package.
For #27494.
Change-Id: Ie606cab39f60859ee1da5165dcc94c8470c94325
Reviewed-on: https://go-review.googlesource.com/c/go/+/447575
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Per discussion with Roland Shoemaker, this updates
x509.CreateRevocationList to mirror the behavior of
x509.CreateCertificate, creating an internal struct for the ASN.1
encoding of the CRL. This allows us to switch the Issuer field type to
asn1.RawValue, bypassing the round-tripping issues of pkix.Name in most
scenarios.
Per linked ticket, this resolves issues where a non-Go created
certificate can be used to create CRLs which aren't correctly attested
to that certificate. In rare cases where the CRL issuer is validated
against the certificate's issuer (such as the linked JDK example), this
can result in failing to check this CRL for the candidate certificate.
Fixes#53754
Change-Id: If0adc053c081d6fb0b1ce47324c877eb2429a51f
GitHub-Last-Rev: 033115dd5e
GitHub-Pull-Request: golang/go#53985
Reviewed-on: https://go-review.googlesource.com/c/go/+/418834
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Two edge cases that were mentioned in the docs are actually impossible:
* For NIST curves, ECDH can't fail, since the zero scalar is rejected
by NewPrivateKey, the identity point is rejected by NewPublicKey,
and NIST curves are a prime-order group.
Let's call the inputs to scalar multiplication k and P, and the
order of the group q. If k[P] is the identity, and also q[P] is the
identity by definition, then P's order is a divisor of q-k, because
k[P] + [q-k]P = q[P] = I
P's order is either 1 or q, and can only be a divisor of q-k if it's
1 (so P is the identity), or if k is zero.
* For X25519, PrivateKey.PublicKey can't return the all-zero value,
since no value is equivalent to zero after clamping.
Clamping unsets the lowest three bit, sets the second-to-highest
bit, and unsets the top bit; this means that a scalar equivalent to
zero needs to be a multiple of 8*q, and needs to be between 2**254
and 2**255-1, but 8*p > 2**255-1.
Tests for other exotic edge cases such as non-canonical point encodings,
clamping, points on the twist, and low-order components are covered by
x/crypto/wycheproof.
Change-Id: I731a878c58bd59aee5636211dc0f19ad8cfae9db
Reviewed-on: https://go-review.googlesource.com/c/go/+/425463
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
In https://build.golang.org/log/d2eb315305bf3d513c490e7f85d56e9a016aacd2,
we observe a failure in TestPipeLookPathLeak due to an additional
descriptor (7) that was open at the start of the test being closed while
the test executes.
I haven't dug much into the failure, but it seems plausible to me that the
descriptor may have been opened by libc for some reason, and may have been
closed due to some sort of idle timeout or the completion of a background
initialization routine.
Since the test is looking for a leak, and closing an existing descriptor
does not indicate a leak, let's not fail the test if an existing descriptor
is unexpectedly closed.
Updates #5071.
Change-Id: I03973ddff6592c454cfcc790d6e56accd051dd52
Reviewed-on: https://go-review.googlesource.com/c/go/+/447235
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The -C flag is like tar -C or make -C: it changes to the named directory
early in command startup, before anything else happens.
Fixes#50332.
Change-Id: I8e4546f69044cb3a028d4d26dfba482b08cb845d
Reviewed-on: https://go-review.googlesource.com/c/go/+/421436
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Currently, with PGO, the inliner uses node weights to decide if a
function is inlineable (with a larger budget). But the actual
inlining is determined by the weight of the call edge. There is a
discrepancy that, if a callee node is hot but the call edge is not,
it would not inlined, and marking the callee inlineable would of
no use.
Instead of using two kinds of weights, we just use the edge
weights to decide inlineability. If a function is the callee of a
hot call edge, its inlineability is determined with a larger
threshold. For a function that exceeds the regular inlining budget,
it is still inlined only when the call edge is hot, as it would
exceed the regular inlining cost for non-hot call sites, even if
it is marked inlineable.
For #55022.
Change-Id: I93fa9919fc6bcbb394e6cfe54ec96a96eede08f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/447015
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
CC-BY-3.0 was shiny and new back in 2009, but CC-BY-4.0 is
generally preferred now. Update our CC-BY uses to CC-BY-4.0.
Google lawyers signed off on the overall CC-BY-4.0 update
and Renee French signed off on the update of the gopher license.
See also CL 447156.
Change-Id: I3908910d6011ed733271e595f761c773351b30f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/447275
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
We don't have a formatter for these files, so check here that
they are in the right form to allow 'cat next/*.txt >go1.X.txt'
at the end of each cycle.
Fix the api files that the check finds.
Change-Id: I0c5e4ab11751c7d0afce32503131d487313f41c0
Reviewed-on: https://go-review.googlesource.com/c/go/+/431335
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
We were mishandling {{range $i = .}}, treating it as though it were
{{range $i := .}}. That happened to work if $i were the most recently
declared variable, but not otherwise.
Fixes#56490
Change-Id: I222a009d671d86c06a980a54388e05f12101c00b
Reviewed-on: https://go-review.googlesource.com/c/go/+/446795
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
net.IP states that a 16-byte slice can still be an IPv4 address.
But after netip.Addr is introduced, it requires extra care to keep
it as an IPv4 address when converting it to a netip.Addr using
netip.AddrFromSlice.
To address this issue, let's change the cgo resolver to return
4-byte net.IP for IPv4. The change will save us 12 bytes too.
Please note that the go resolver already return IPv4 as 4-byte
slice.
The test TestResolverLookupIP has been modified to cover this
behavior. So no new test is added.
Fixes#53554.
Change-Id: I0dc2a59ad785c0c67a7bc22433105529f055997f
GitHub-Last-Rev: bd7bb2f17b
GitHub-Pull-Request: golang/go#53638
Reviewed-on: https://go-review.googlesource.com/c/go/+/415580
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
For #56041
Change-Id: I6c98458b5c0d3b3636a53ee04fc97221f3fd8bbc
Reviewed-on: https://go-review.googlesource.com/c/go/+/444817
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: xie cui <523516579@qq.com>
Randomized feature enable/disable might be something we use to
help users debug any problems with changed loop variable capture,
and there's another CL that would like to use it to help in
locating places where "fused" multiply add instructions change
program behavior.
This CL:
- adds the ability to include an integer parameter (e.g. line number)
- replumbed the environment variable into a flag to simplify go build cache management
- but added an environment variable to allow flag setting through the environment
- which adds the possibility of switching on a different variable
(if there's one built-in for variable capture, it shouldn't be GOSSAHASH)
- cleaned up the checking code
- adds tests for all the intended behavior
- removes the case for GSHS_LOGFILE; TBD whether we'll need to put that back
or if there is another way.
Change-Id: I8503e1bb3dbc4a743aea696e04411ea7ab884787
Reviewed-on: https://go-review.googlesource.com/c/go/+/443063
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: David Chase <drchase@google.com>
There does not seem to be any point to this sync.Once.
I noticed because I was surveying uses of sync.Once to
understand usage patterns. This seems to be a dreg left over
from some earlier instance of the code.
Change-Id: I99dd258d865a41d0e8f6cfa55887855e477fb9c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/445755
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Fuzz tests are meant to be run just like ordinary tests,
so copy the same loop cpu and count loops used in testing.go
(and benchmark.go) into fuzz.go.
Change-Id: Ic585df8ccc577869c877b1055e0493803dbeb828
Reviewed-on: https://go-review.googlesource.com/c/go/+/443377
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
It adds support for no-reload option, as specified in resolv.conf(5):
no-reload (since glibc 2.26)
Sets RES_NORELOAD in _res.options. This option
disables automatic reloading of a changed
configuration file.
Change-Id: I11182c5829434503f719ed162014f2301e3ba8d4
GitHub-Last-Rev: 7ae44be2d5
GitHub-Pull-Request: golang/go#56489
Reviewed-on: https://go-review.googlesource.com/c/go/+/446555
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Fixes AIX build.
Change-Id: Icbb33896017bbcc488a8baff20e10eb0e14ea4b9
Reviewed-on: https://go-review.googlesource.com/c/go/+/447095
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: Bryan Mills <bcmills@google.com>
The global ListOfHotCallSites set is used to communicate between
CanInline and InlineCalls the set of call sites that InlineCalls may
increase the budget for.
CanInline clears this map on each call, thus assuming that
InlineCalls(x) is called immediately after CanInline(x). This assumption
is false, as CanInline (among other cases) is recursive (CanInline ->
hairyVisitor.doNode -> inlCallee -> CanInline).
When this assumption proves false, we will lose the opportunity to
inline hot calls.
This CL is the least invasive fix for this. ListOfHotCallSites is
actually just a subset of the candHotEdgeMap, with CallSiteInfo.Callee
cleared. candHotEdgeMap doesn't actually need to distinguish based on
Callee, so we can drop callee from candHotEdgeMap as well and just use
that directly [1].
Later CLs should do more work to remove the globals entirely.
For cmd/compile, this inceases the number of PGO inlined functions by
~50% for one set of PGO parameters. I have no evaluated performance
impact.
[1] This is something that we likely want to change in the future.
For #55022.
Change-Id: I57735958d651f6dfa9bd296499841213d20e1706
Reviewed-on: https://go-review.googlesource.com/c/go/+/446755
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
ErrWaitDelay is not expected to occur in this test, but if it does
it indicates a failure mode very different from the “failed to start”
catchall that we log for other non-ExitError errors.
Updates #50436.
Change-Id: I3f4d87d502f772bf471fc17303d5a6b483446f8f
Reviewed-on: https://go-review.googlesource.com/c/go/+/446876
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
I noticed some test failures in the build dashboard after CL 445597
that made me realize the grace period should be based on the test
timeout, not the Context timeout: if the test itself sets a short
timeout for a command, we still want to give the test process enough
time to consume and log its output.
I also put some more thought into how one might debug a test hang, and
realized that in that case we don't want to set a WaitDelay at all:
instead, we want to leave the processes in their stuck state so that
they can be investigated with tools like `ps` and 'lsof'.
Updates #50436.
Change-Id: I65421084f44eeaaaec5dd2741cd836e9e68dd380
Reviewed-on: https://go-review.googlesource.com/c/go/+/446875
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
We reused p so we were deleting the same directory twice instead of two
different directories. Fix that.
For #47257
Change-Id: I315ad87d0a9182e00ae4c11b82986227e2b02e17
Reviewed-on: https://go-review.googlesource.com/c/go/+/447115
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Otherwise given a file like defs_nacl_amd64p32.go.~1~ we will add
"nacl" and "amd64p32" to AllTags. This was causing the
cmd/go/internal/modindex tests to fail on my system, since I had
an old editor backup file lying around.
Change-Id: Ib1c5d835e4871addae6dc78cee07c9839bb880e2
Reviewed-on: https://go-review.googlesource.com/c/go/+/446395
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
The go1 benchmark suite does a lot of work at package init time, which
makes it take quite a while to run even if you're not running any of
the benchmarks, or if you're only running a subset of them. This leads
to an awkward workaround in dist test to compile but not run the
package, unlike roughly all other packages. It also reduces isolation
between benchmarks by affecting the starting heap size of all
benchmarks.
Fix this by initializing all data required by a benchmark when that
benchmark runs, and keeping it local so it gets freed by the GC and
doesn't leak between benchmarks. Now, none of the benchmarks depend on
global state.
Re-initializing the data on each benchmark run does add overhead to an
actual benchmark run, as each benchmark function is called several
times with different values of b.N. A full run of all benchmarks at
the default -benchtime=1s now takes ~10% longer; higher -benchtimes
would be less. It would be quite difficult to cache this data between
invocations of the same benchmark function without leaking between
different benchmarks and affecting GC overheads, as the testing
package doesn't provide any mechanism for this.
This reduces the time to run the binary with no benchmarks from 1.5
seconds to 10 ms, and also reduces the memory required to do this from
342 MiB to 17 MiB.
To make sure data was not leaking between different benchmarks, I ran
the benchmarks with -shuffle=on. The variance remained low: mostly
under 3%. A few benchmarks had higher variance, but in all cases it
was similar to the variance between this change.
This CL naturally changes the measured performance of several of the
benchmarks because it dramatically changes the heap size and hence GC
overheads. However, going forward the benchmarks should be much better
isolated.
For #37486.
Change-Id: I252ebea703a9560706cc1990dc5ad22d1927c7a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/443336
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Austin Clements <austin@google.com>
The result of the call to fmt.Errorf was unused. It was clearly
intending to print the message, not simply construct an error.
Change-Id: I14856214c521a51fe4b45690e6c35fbb17e66577
Reviewed-on: https://go-review.googlesource.com/c/go/+/443375
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
If TestArenaCollision cannot reserve the address range it expects to
reserve, it currently fails somewhat mysteriously. Detect this case
and skip the test. This could lead to test rot if we wind up always
skipping this test, but it's not clear that there's a better answer.
If the test does fail, we now also log what it thinks it reserved so
the failure message is more useful in debugging any issues.
Fixes#49415Fixes#54597
Change-Id: I05cf27258c1c0a7a3ac8d147f36bf8890820d59b
Reviewed-on: https://go-review.googlesource.com/c/go/+/446877
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Check for and reject environment variables containing NULs.
The conventions for passing environment variables to subprocesses
cause most or all systems to interpret a NUL as a separator. The
syscall package rejects environment variables containing a NUL
on most systems, but erroniously did not do so on Windows. This
causes an environment variable such as "FOO=a\x00BAR=b" to be
interpreted as "FOO=a", "BAR=b".
Check for and reject NULs in environment variables passed to
syscall.StartProcess on Windows.
Add a redundant check to os/exec as extra insurance.
Fixes#56284
Fixes CVE-2022-41716
Change-Id: I2950e2b0cb14ebd26e5629be1521858f66a7d4ae
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1609434
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
TryBot-Result: Security TryBots <security-trybots@go-security-trybots.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/446916
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Track the running tests and when they started,
so that we can report the running tests on a test timeout.
% go test -timeout=5s
panic: test timed out after 5s
running tests:
TestTCPSpuriousConnSetupCompletion (4s)
... stack traces as usual ...
% go test -run=Script -timeout=10s cmd/go
vcs-test.golang.org rerouted to http://127.0.0.1:65168https://vcs-test.golang.org rerouted to https://127.0.0.1:65169
go test proxy running at GOPROXY=http://127.0.0.1:65170/mod
panic: test timed out after 10s
running tests:
TestScript (10s)
TestScript/mod_get_patchcycle (0s)
TestScript/mod_get_prefer_incompatible (0s)
TestScript/mod_get_promote_implicit (0s)
TestScript/mod_get_pseudo (0s)
TestScript/mod_get_pseudo_other_branch (0s)
TestScript/mod_get_pseudo_prefix (0s)
TestScript/mod_get_test (0s)
TestScript/mod_get_trailing_slash (0s)
TestScript/mod_get_update_unrelated_sum (0s)
TestScript/mod_gobuild_import (0s)
TestScript/mod_gomodcache (0s)
TestScript/mod_gonoproxy (0s)
TestScript/mod_load_badchain (0s)
TestScript/mod_overlay (0s)
TestScript/test_fuzz_minimize (6s)
TestScript/test_fuzz_minimize_dirty_cov (7s)
... stack traces as usual ...
Change-Id: I3a6647c029097becc06664ebd76a2597c7ed7b8e
Reviewed-on: https://go-review.googlesource.com/c/go/+/446176
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Use a slightly different line number pragma when emitting instrumented
code, so as to ensure that we don't get any changes in the
"-gcflags=-m" output for coverage vs non-coverage.
Fixes#56475.
Change-Id: I3079171fdf83c0434ed6ea0ce3eb2797c2280c55
Reviewed-on: https://go-review.googlesource.com/c/go/+/446259
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Change the 'PrepareForCoverageBuild' helper function to provide more
sensible defaults in the case where Go packages are listed on the
command line (e.g. cases such as "go run -cover mumble.go").
With the old implementation, if module mode was enabled, we would only
instrument packages in the main module, meaning that if you did
something like this:
$ ls go.mod
go.mod
$ GOCOVERDATA=/tmp/cov go run -cover testdata/somefile.go
$
no coverage profiles would be generated at all. This is due to the
fact that the pseudo-package created by the Go command internally when
building "somefile.go" is not considered part of the main module.
This patch moves the default to "packages explicitly mentioned on the
command line, plus packages in the main module", which will make more
sense to users passing specific packages and *.go files on the command
line. Examples:
// Here cmd/compile is part the Go standard library + commands
// (which we exclude from instrumentation by default), but since
// 'cmd/compile' is mentioned on the command line, we will instrument
// just that single package (not any of its deps).
$ cd $GOROOT/src
$ go build -o gc.exe -cover cmd/compile
$ GOCOVERDATA=/tmp/cov ./gc.exe ...
...
$
// Here we're running a Go file named on the command line, where
// the pseudo-package for the command line is not part of the
// main module, but it makes sense to instrument it anyhow.
$ cd ~/go/k8s.io/kubernetes
$ GOCOVERDATA=/tmp/cov go run -cover test/typecheck/testdata/bad/bad.go
...
$
This patch also simplifies the logic and improves flow/comments in
in the helper function PrepareForCoverageBuild.
Change-Id: Id8fc8571157dac8c09e44cc73baa05aeba1640ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/445918
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
This patch fixes a typo/bug introduced in CL 441858 where when pattern
matching a coverage counter access we were looking at an assingment
node instead of the assignment LHS, and fixes a similar problem in
atomic counter update pattern matching introduced in CL 444835. In
both of these cases the bug was not caught because the test intended
to lock down the behavior was written incorrectly (wasn't
instrumenting what the test author thought it was instrumenting,
ouch).
Change-Id: I6e6ac3beacf12ef1a817de5527340b639f0bb044
Reviewed-on: https://go-review.googlesource.com/c/go/+/446258
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Than McIntosh <thanm@google.com>
When coverage testing a local package (defined by a relative import
path such as "./foo/bar") the convention when "-coverprofile" is used
has been to capture source files by full pathname, as opposed to
recording the full import path or the invented import path
("command-line-arguments/") created by the go command in the case of
building named Go files. Doing this makes it much easier to use
collected profiles with "go tool -cover -html=<profile>".
The support for this feature/convention wound up being inadvertantly
dropped during the GOEXPERIMENT=coverageredesign implementation; this
patch restores it.
Fixes#56433.
Change-Id: Ib9556fdc86011b00c155caa614ab23e5148f3eb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/445917
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Change the macOS implementation to use libc calls.
Using libc calls directly is what we do for all the runtime and os syscalls.
Doing so here as well improves consistency and also makes it possible
to cross-compile (from non-Mac systems) macOS binaries that use the
native name resolver.
Fixes#12524.
Change-Id: I011f4fcc5c50fbb5396e494889765dcbb9342336
Reviewed-on: https://go-review.googlesource.com/c/go/+/446178
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Also removes no-longer-needed "Any" field from compiler's DebugFlags.
Test/use case for this is the fmahash CL.
Change-Id: I214f02c91f30fc2ce53caf75fa5e2b905dd33429
Reviewed-on: https://go-review.googlesource.com/c/go/+/445495
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: David Chase <drchase@google.com>
This an end-to-end test that sets GOROOT to a symlink
to the distribution, approximating copying it to a new
location, and checks that packages in the standard library
are not stale, as they would be if paths were embedded
in artifacts.
For #47257
Change-Id: I4ed04df36656ad946a2f6f5ce3194e76e06372e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/445358
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
