Michael Pratt [Fri, 21 Aug 2020 15:49:56 +0000 (11:49 -0400)]
runtime: check held locks with staticlockranking
When lock ranking is enabled, we can now assert that lock preconditions
are met by checking that the caller holds required locks on function
entry.
This change adds the infrastructure to add assertions. Actual assertions
will be added for various locks in subsequent changes.
Some functions are protected by locks that are not directly accessible
in the function. In that case, we can use assertRankHeld to check that
any lock with the rank is held. This is less precise, but it avoids
requiring passing the lock into the functions.
Updates #40677
Change-Id: I843c6874867f975e90a063f087b6e2ffc147877b
Reviewed-on: https://go-review.googlesource.com/c/go/+/245484
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Michael Pratt <mpratt@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
Dmitri Shuralyov [Thu, 17 Sep 2020 16:32:24 +0000 (12:32 -0400)]
all: update vendored dependencies during Go 1.16 development
The Go 1.16 development cycle has started. This is the time to update
all golang.org/x/... module versions that contribute packages to the
std and cmd modules in the standard library to latest master versions.
Those versions have already gone through code review, and now they
will undergo additional testing during the development period.
If there are new issues in these dependencies discovered, we have
development period to deal with that. We will do this update once
more at the end of the development cycle, by the code freeze, and
so doing it now will make that update smaller and safer.
Overall, this change will help us build confidence that the
Go 1.16 release and its selected dependencies will be robust.
Also increment the Go language version to 1.16 in standard library
go.mod files.
This change was created with a program from CL 256357 patch set 1
(which updates golang.org/x modules only) and the bundle tool at
CL 255053 patch set 1:
$ updatestd -goroot=$HOME/gotip -branch=master
> go version
go version devel +eda1d40544 Mon Sep 21 16:50:07 2020 +0000 darwin/amd64
> go env GOROOT
/Users/dmitshur/gotip
> go version -m /Users/dmitshur/go/bin/bundle
/Users/dmitshur/go/bin/bundle: go1.15.2
path golang.org/x/tools/cmd/bundle
mod golang.org/x/tools (devel) # CL 255053 PS 1
dep golang.org/x/mod v0.3.0 h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=
dep golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
debug/dwarf: speed up SkipChildren for compilation units
For a common pattern of iterating only over top-level compilation units (CU)
Reader.SkipChildren has decode and meterialize all CU subentries just
to skip them, because DW_TAG_compile_unit does not have DW_AT_sibling.
However, CUs have total size encoded before the unit and we already parse them
and know all unit sizes.
Optimize Reader.SkipChildren to use that size when skipping CUs children.
This speeds up iteration over a 1.3GB object file from 7.5s to 0.73s.
Change-Id: I2a8f00955159b4bd13571409f4817805f934cb69
Reviewed-on: https://go-review.googlesource.com/c/go/+/256217
Run-TryBot: Dmitry Vyukov <dvyukov@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Than McIntosh <thanm@google.com>
Michael Pratt [Fri, 18 Sep 2020 21:53:12 +0000 (17:53 -0400)]
runtime: expand gopark documentation
unlockf is called after the G is put into _Gwaiting, meaning another G
may have readied this one before unlockf is called.
This is implied by the current doc, but add additional notes to call out
this behavior, as it can be quite surprising.
Updates #40641
Change-Id: I60b1ccc6a4dd9ced8ad2aa1f729cb2e973100b59
Reviewed-on: https://go-review.googlesource.com/c/go/+/256058
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com>
Michael Anthony Knyszek [Mon, 10 Aug 2020 20:02:22 +0000 (20:02 +0000)]
runtime: disable stack shrinking in activeStackChans race window
Currently activeStackChans is set before a goroutine blocks on a channel
operation in an unlockf passed to gopark. The trouble is that the
unlockf is called *after* the G's status is changed, and the G's status
is what is used by a concurrent mark worker (calling suspendG) to
determine that a G has successfully been suspended. In this window
between the status change and unlockf, the mark worker could try to
shrink the G's stack, and in particular observe that activeStackChans is
false. This observation will cause the mark worker to *not* synchronize
with concurrent channel operations when it should, and so updating
pointers in the sudog for the blocked goroutine (which may point to the
goroutine's stack) races with channel operations which may also
manipulate the pointer (read it, dereference it, update it, etc.).
Fix the problem by adding a new atomically-updated flag to the g struct
called parkingOnChan, which is non-zero in the race window above. Then,
in isShrinkStackSafe, check if parkingOnChan is zero. The race is
resolved like so:
* Blocking G sets parkingOnChan, then changes status in gopark.
* Mark worker successfully suspends blocking G.
* If the mark worker observes parkingOnChan is non-zero when checking
isShrinkStackSafe, then it's not safe to shrink (we're in the race
window).
* If the mark worker observes parkingOnChan as zero, then because
the mark worker observed the G status change, it can be sure that
gopark's unlockf completed, and gp.activeStackChans will be correct.
The risk of this change is low, since although it reduces the number of
places that stack shrinking is allowed, the window here is incredibly
small. Essentially, every place that it might crash now is replaced with
no shrink.
This change adds a test, but the race window is so small that it's hard
to trigger without a well-placed sleep in park_m. Also, this change
fixes stackGrowRecursive in proc_test.go to actually allocate a 128-byte
stack frame. It turns out the compiler was destructuring the "pad" field
and only allocating one uint64 on the stack.
Fixes #40641.
Change-Id: I7dfbe7d460f6972b8956116b137bc13bc24464e8
Reviewed-on: https://go-review.googlesource.com/c/go/+/247050
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com>
Trust: Michael Knyszek <mknyszek@google.com>
runtime: improve error messages after allocating a stack that is too big
In the current implementation, we can observe crashes after calling
debug.SetMaxStack and allocating a stack larger than 4GB since
stackalloc works with 32-bit sizes. To avoid this, we define an upper
limit as the largest feasible point we can grow a stack to and provide a
better error message when we get a stack overflow.
Fixes #41228
Change-Id: I55fb0a824f47ed9fb1fcc2445a4dfd57da9ef8d4
Reviewed-on: https://go-review.googlesource.com/c/go/+/255997
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Giovanni Bajo <rasky@develer.com> Reviewed-by: Keith Randall <khr@golang.org>
unicode/utf8: document the handling of runes out of range in EncodeRune
Document the way EncodeRune currently handles runes which are
out of range. Also add an example showing that behaviour.
Change-Id: I0f8e7645ae053474ec319085a2bb6d7f73bc137c
Reviewed-on: https://go-review.googlesource.com/c/go/+/255998 Reviewed-by: Rob Pike <r@golang.org> Reviewed-by: Giovanni Bajo <rasky@develer.com>
Trust: Giovanni Bajo <rasky@develer.com>
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Go Bot <gobot@golang.org>
surechen [Wed, 6 May 2020 12:04:35 +0000 (20:04 +0800)]
math: Remove redundant local variable Ln2
Use the const variable Ln2 in math/const.go for function acosh.
Change-Id: I5381d03dd3142c227ae5773ece9be6c8f377615e
Reviewed-on: https://go-review.googlesource.com/c/go/+/232517 Reviewed-by: Robert Griesemer <gri@golang.org>
Trust: Robert Griesemer <gri@golang.org>
Trust: Giovanni Bajo <rasky@develer.com>
Jay Lee [Fri, 18 Sep 2020 05:49:09 +0000 (05:49 +0000)]
time: support colon at start of TZ value
According to POSIX, there are three formats for TZ variable. When
it refers to timezone file, it should starts with a colon. This commit
removes the colon if it exists, so that it keeps compatible with both
the spec and the old behavior.
Change-Id: I30cfeaea530d24e174de309952338cb1146694a5
GitHub-Last-Rev: 11d83d11ca2eca9d542036cf5e23559388fd323e
GitHub-Pull-Request: golang/go#27570
Reviewed-on: https://go-review.googlesource.com/c/go/+/134217
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Change-Id: I61a13f7d6410e4931efaa20307bdf1cc0037afc0
Reviewed-on: https://go-review.googlesource.com/c/go/+/255200
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Robert Griesemer <gri@golang.org>
Trust: Ian Lance Taylor <iant@golang.org>
Change-Id: I2233a6a157ec8feffaefd6a8ee65b1c38778c1cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/255238 Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Giovanni Bajo <rasky@develer.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Giovanni Bajo <rasky@develer.com>
Keith Randall [Fri, 18 Sep 2020 21:19:22 +0000 (14:19 -0700)]
reflect: use zero buffer to back the Value returned by Zero
In the common case (<1KB types), no allocation is required
by reflect.Zero.
Also use memclr instead of memmove in Set when the source
is known to be zero.
Fixes #33136
Change-Id: Ic66871930fbb53328032e587153ebd12995ccf55
Reviewed-on: https://go-review.googlesource.com/c/go/+/192331
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Martin Möhrmann <moehrmann@google.com>
cmd/link: don't mark a symbol's GoType reachable when -linkshared
In CL 231397, we stopped marking symbols' GoType reachable in
general, but not when -linkshared. It was left as a TODO. This CL
addresses it.
The problem was that the type names are mangled in the shared
library, so we need to mangle the name consistently in the
executable as well (regardless of whether the symbol is reachable
or not), so that the GCProg generation code can find the
corresponding symbol from the shared library.
Change-Id: I1040747402929a983ec581109f1681a77893682e
Reviewed-on: https://go-review.googlesource.com/c/go/+/255964
Trust: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Keith Randall [Thu, 20 Aug 2020 21:22:30 +0000 (14:22 -0700)]
runtime/debug: provide Addr method for errors from SetPanicOnFault
When we're building a panic that's triggered by a memory fault when
SetPanicOnFault has been called, include an Addr method. This
method reports the address at which the fault occurred.
Fixes #37023
RELNOTE=yes
Change-Id: Idff144587d6b75070fdc861a36efec76f4ec7384
Reviewed-on: https://go-review.googlesource.com/c/go/+/249677
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
David Chase [Wed, 15 Jul 2020 13:07:00 +0000 (09:07 -0400)]
cmd/compile: generate late-lowering static calls
This is testing for the limited case of zero or one SSA-able results.
One regression is that the later expansion of "Dereference" into
Move into argument slots thwarts the
MOVE A -> B, MOVE B -> C
replaced-by
MOVE A -> B, MOVE A -> C
optimization; the second move is written instead as a Dereference at the
phase where the optimization occurs, and because the target of the
dereference is not visible in the dereference, it's not possible to verify
that A and B or A and C don't overlap in some peculiar way (and for results
fed to args, they can).
Regression is repaired in a later CL by changing when calls are expanded.
Change-Id: Ia0f48a9d483d5a54a99657a24b12b25b8edde55f
Reviewed-on: https://go-review.googlesource.com/c/go/+/242782
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Jay Conrod [Fri, 18 Sep 2020 15:48:18 +0000 (11:48 -0400)]
cmd/go/internal/modload: don't report path errors when loading retractions
When we load module retractions from the latest version of a module,
it's possible that the latest version has a different module path than
the version we're loading. This can happen when a module is renamed or
a go.mod file is added for the first time.
We should not report an error in this case. Retractions should still
apply to old aliases of a module.
Fixes #41350
Change-Id: If1bc0b6b2b26fc7023e02fc211aa0cd8eb00796e
Reviewed-on: https://go-review.googlesource.com/c/go/+/255961
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
cmd/link: propagate UsedInIface through method descriptor
The linker prunes methods that are not directly reachable if the
receiver type is never converted to interface. A type can be
converted to interface using reflection through other types.
The linker already takes this into consideration but it missed
the case that the intermediate is a method descriptor. Handle
this case.
Change-Id: I590efc5da163c326db8d43583908a2ef67f65d9d
Reviewed-on: https://go-review.googlesource.com/c/go/+/255858
Trust: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
Matthew Dempsky [Fri, 18 Sep 2020 04:38:42 +0000 (21:38 -0700)]
cmd/compile: more comprehensive tests for #24991
The revised test now checks that unsafe-uintptr correctly works for
variadic uintptr parameters too, and the CL corrects the code so this
code compiles again.
The pointers are still not kept alive properly. That will be fixed by
a followup CL. But this CL at least allows programs not affected by
that to build again.
Updates #24991.
Updates #41460.
Change-Id: If4c39167b6055e602213fb7522c4f527c43ebda9
Reviewed-on: https://go-review.googlesource.com/c/go/+/255877
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Than McIntosh [Thu, 17 Sep 2020 19:35:31 +0000 (15:35 -0400)]
cmd/compile,cmd/asm: dump sym ABI versions for -S=2
When -S=2 is in effect for the compiler/assembler, include symbol ABI
values for defined symbols and relocations. This is intended to help
make it easier to distinguish between a symbol and its ABI wrapper.
Change-Id: Ifbf71372392075f15363b40e882b2132406b7d6d
Reviewed-on: https://go-review.googlesource.com/c/go/+/255718
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: David Chase <drchase@google.com>
Than McIntosh [Thu, 17 Sep 2020 19:31:07 +0000 (15:31 -0400)]
cmd/asm: make asm -S flag consistent with compile -S flag
Change things so that the -S command line option for the assembler
works the same as -S in the compiler, e.g. you can use -S=2 to
get additional detail.
Change-Id: I7bdfba39a98e67c7ae4b93019e171b188bb99a2d
Reviewed-on: https://go-review.googlesource.com/c/go/+/255717
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Jay Conrod [Thu, 17 Sep 2020 19:54:13 +0000 (15:54 -0400)]
cmd/go: flip relationship between load and modload
Previously, modload imported load, but it mainly just did so in order
to install callbacks to the modload API. This was important during vgo
development, but there's no longer a strong reason to do this. Nothing
modload imports strongly depends on load, so there's little danger of
a dependency cycle.
This change deletes the callbacks in load and instead, makes load call
exported functions in modload directly. In the future, these functions
may have different signatures than their GOPATH counterparts.
Change-Id: Ifde5c3ffebd190b5bd184924ec447d272b936f27
Reviewed-on: https://go-review.googlesource.com/c/go/+/255719
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Keith Randall [Thu, 17 Sep 2020 16:55:23 +0000 (09:55 -0700)]
cmd/compile: propagate go:notinheap implicitly
//go:notinheap
type T int
type U T
We already correctly propagate the notinheap-ness of T to U. But we
have an assertion in the typechecker that if there's no explicit
//go:notinheap associated with U, then report an error. Get rid of
that error so that implicit propagation is allowed.
Adjust the tests so that we make sure that uses of types like U
do correctly report an error when U is used in a context that might
cause a Go heap allocation.
Fixes #41451
Update #40954
Update #41432
Change-Id: I1692bc7cceff21ebb3f557f3748812a40887118d
Reviewed-on: https://go-review.googlesource.com/c/go/+/255637
Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
cmd/addr2line: don't assume that GOROOT_FINAL is clean
Fixes #41447
Change-Id: I4460c1c7962d02c41622a5ea1a3c4bc3714a1873
Reviewed-on: https://go-review.googlesource.com/c/go/+/255477
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
David Chase [Mon, 13 Jul 2020 18:44:14 +0000 (14:44 -0400)]
cmd/compile: add variable length TRESULTS type for SSA use.
This type is very much like TTUPLE, but not just for pairs.
Used to describe results of a pre-expansion function call.
(will later probably also be used to describe the incoming args).
Change-Id: I811850cfcc2b3de85085eb4c2eca217c04c330b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/242360
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Alex Opie [Thu, 17 Sep 2020 04:31:50 +0000 (04:31 +0000)]
cmd/go: use the correct linker config in the buildID hash
The linker config is hashed into the buildID; however,
the GOROOT_FINAL environment variable that is
actually used when -trimpath is specified was not
reflected in that hash. This change fixes that.
Fixes #38989
Change-Id: I418a21a9f6293ca63c101d22b501dfdba8e91ac6
GitHub-Last-Rev: 4cf82920e4a76173c5cb5359b059e87ee7fc7f51
GitHub-Pull-Request: golang/go#40296
Reviewed-on: https://go-review.googlesource.com/c/go/+/243557
Run-TryBot: Jay Conrod <jayconrod@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
Jay Conrod [Tue, 15 Sep 2020 16:59:05 +0000 (12:59 -0400)]
cmd/go: prepare tests for GO111MODULE=on by default
Set GO111MODULE=off explicitly in tests specific to GOPATH mode.
Added a go.mod file to other tests that assumed GOPATH mode.
Fixed an issue in the build metadata file generated in
modload/build.go, which did not end with a newline. This broke the
build_dash_x test, which expects to be able to run the script printed
by 'go build -x' to produce the same result. The script is broken if
the build metadata file doesn't end with a newline.
Change-Id: I59f2a492a9f5a66f6c4aa702f429909d5c5e815d
Reviewed-on: https://go-review.googlesource.com/c/go/+/255051
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Lynn Boger [Mon, 31 Aug 2020 13:43:40 +0000 (09:43 -0400)]
cmd/compile: use combined shifts to improve array addressing on ppc64x
This change adds rules to find pairs of instructions that can
be combined into a single shifts. These instruction sequences
are common in array addressing within loops. Improvements can
be seen in many crypto packages and the hash packages.
These are based on the extended mnemonics found in the ISA
sections C.8.1 and C.8.2.
Some rules in PPC64.rules were moved because the ordering prevented
some matching.
Joel Sing [Tue, 18 Aug 2020 17:07:26 +0000 (03:07 +1000)]
cmd/internal/obj/riscv: clean up lowerJALR
This cleans up the last of the direct obj.Prog rewriting, removing lowerJALR
and replacing it with correct handling for AJALR during instruction encoding.
Change-Id: Ieea125bde30d4c0edd2d9ed1e50160543aa8f330
Reviewed-on: https://go-review.googlesource.com/c/go/+/249077 Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Joel Sing <joel@sing.id.au>
syscall, cmd/go/internal/lockedfile/internal/filelock: add and use Flock on illumos
Copy the syscall wrapper from golang.org/x/sys/unix CL 255377 to provide
Flock on illumos and switch cmd/go/internal/lockedfile/internal/filelock
to use it.
Matthew Dempsky [Wed, 16 Sep 2020 22:41:47 +0000 (15:41 -0700)]
cmd/compile: skip looking for OCLOSURE nodes in xtop
xtop holds package's top-level declaration statements, but OCLOSURE
only appears in expression contexts. xtop will instead hold the
synthetic ODCLFUNC representing OCLOSURE's function body.
This CL makes the loop consistent with the later phases that only look
for ODCLFUNC nodes in xtop.
Passes toolstash-check.
Change-Id: I852a10ef1bf75bb3351e3da0357ca8b2e26aec6e
Reviewed-on: https://go-review.googlesource.com/c/go/+/255340
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
David Chase [Fri, 26 Jun 2020 22:19:01 +0000 (18:19 -0400)]
cmd/compile: make translation to calls for SSA look more "value-oriented"
The existing translation assumes an in-memory return values, thus it returns
the address of the result(s). Most consumers immediately load from the
address to get the value, and in late call expansion that is the favored idiom,
and it is also the favored idiom when arguments and results use registers
instead of memory.
Change-Id: Ie0ccc70f399682a42509d847b330ef3956462d56
Reviewed-on: https://go-review.googlesource.com/c/go/+/240186
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
David Chase [Fri, 12 Jun 2020 17:48:26 +0000 (13:48 -0400)]
cmd/compile: introduce special ssa Aux type for calls
This is prerequisite to moving call expansion later into SSA,
and probably a good idea anyway. Passes tests.
This is the first minimal CL that does a 1-for-1 substitution
of *ssa.AuxCall for *obj.LSym. Next step (next CL) is to make
this change for all calls so that additional information can
be stored in AuxCall.
Change-Id: Ia3a7715648fd9fb1a176850767a726e6f5b959eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/237680
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Alberto Donizetti [Wed, 16 Sep 2020 11:13:50 +0000 (13:13 +0200)]
cmd/compile: be more specific in cannot assign errors
"cannot assign to" compiler errors are very laconic: they never
explain why the lhs cannot be assigned to (with one exception, when
assigning to a struct field in a map).
This change makes them a little more specific, in two more cases: when
assigning to a string, or to a const; by giving a very brief reason
why the lhs cannot be assigned to.
Change-Id: I244cca7fc3c3814e00e0ccadeec62f747c293979
Reviewed-on: https://go-review.googlesource.com/c/go/+/255199
Trust: Alberto Donizetti <alb.donizetti@gmail.com>
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Michael Anthony Knyszek [Wed, 16 Sep 2020 17:08:55 +0000 (17:08 +0000)]
runtime: actually fix locking in BenchmarkMSpanCountAlloc
I just submitted CL 255297 which mostly fixed this problem, but totally
forgot to actually acquire/release the heap lock. Oops.
Updates #41391.
Change-Id: I45b42f20a9fc765c4de52476db3654d4bfe9feb3
Reviewed-on: https://go-review.googlesource.com/c/go/+/255298
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com> Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Keith Randall [Thu, 27 Aug 2020 21:05:52 +0000 (14:05 -0700)]
cmd/compile: make go:notinheap error message friendlier for cgo
Update #40954
Change-Id: Ifaab7349631ccb12fc892882bbdf7f0ebf3d845f
Reviewed-on: https://go-review.googlesource.com/c/go/+/251158
Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Keith Randall [Wed, 26 Aug 2020 21:17:35 +0000 (14:17 -0700)]
cmd/cgo: use go:notinheap for anonymous structs
They can't reasonably be allocated on the heap. Not a huge deal, but
it has an interesting and useful side effect.
After CL 249917, the compiler and runtime treat pointers to
go:notinheap types as uintptrs instead of real pointers (no write
barrier, not processed during stack scanning, ...). That feature is
exactly what we want for cgo to fix #40954. All the cases we have of
pointers declared in C, but which might actually be filled with
non-pointer data, are of this form (JNI's jobject heirarch, Darwin's
CFType heirarchy, ...).
Fixes #40954
Change-Id: I44a3b9bc2513d4287107e39d0cbbd0efd46a3aae
Reviewed-on: https://go-review.googlesource.com/c/go/+/250940
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Keith Randall [Wed, 26 Aug 2020 21:07:35 +0000 (14:07 -0700)]
cmd/compile: allow aliases to go:notinheap types
The alias doesn't need to be marked go:notinheap. It gets its
notinheap-ness from the target type.
Without this change, the type alias test in the notinheap.go file
generates these two errors:
notinheap.go:62: misplaced compiler directive
notinheap.go:63: type nih must be go:notinheap
The first is a result of go:notinheap pragmas not applying
to type alias declarations.
The second is the result of then trying to match the notinheap-ness
of the alias and the target type.
Add a few more go:notinheap tests while we are here.
Update #40954
Change-Id: I067ec47698df6e9e593e080d67796fd05a1d480f
Reviewed-on: https://go-review.googlesource.com/c/go/+/250939
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Trust: Keith Randall <khr@golang.org>
Michael Anthony Knyszek [Wed, 16 Sep 2020 16:22:28 +0000 (16:22 +0000)]
runtime: fix leak and locking in BenchmarkMSpanCountAlloc
CL 249917 made the mspan in MSpanCountAlloc no longer stack-allocated
(for good reason), but then allocated an mspan on each call and did not
free it, resulting in a leak. That allocation was also not protected by
the heap lock, which could lead to data corruption of mheap fields and
the spanalloc.
To fix this, export some functions to allocate/free dummy mspans from
spanalloc (with proper locking) and allocate just one up-front for the
benchmark, freeing it at the end. Then, update MSpanCountAlloc to accept
a dummy mspan.
Note that we need to allocate the dummy mspan up-front otherwise we
measure things like heap locking and fixalloc performance instead of
what we actually want to measure: how fast we can do a popcount on the
mark bits.
Fixes #41391.
Change-Id: If6629a6ec1ece639c7fb78532045837a8c872c04
Reviewed-on: https://go-review.googlesource.com/c/go/+/255297
Run-TryBot: Michael Knyszek <mknyszek@google.com> Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Michael Knyszek <mknyszek@google.com>
Joel Sing [Thu, 23 Jul 2020 17:38:38 +0000 (03:38 +1000)]
cmd/link: make it easier to debug an elfrelocsect size mismatch
Change-Id: I54976b004b4db006509f5e0781b1c2e46cfa09ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/244577
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: Joel Sing <joel@sing.id.au>
cmd/internal/obj/arm64: optimize the instruction of moving long effective stack address
Currently, when the offset of "MOVD $offset(Rn), Rd" is a large positive
constant or a negative constant, the assembler will load this offset from
the constant pool.This patch gets rid of the constant pool by encoding the
offset into two ADD instructions if it's a large positive constant or one
SUB instruction if negative. For very large negative offset, it is rarely
used, here we don't optimize this case.
Optimized case 1: MOVD $-0x100000(R7), R0
Before: LDR 0x67670(constant pool), R27; ADD R27.UXTX, R0, R7
After: SUB $0x100000, R7, R0
Martin Möhrmann [Mon, 14 Sep 2020 14:30:43 +0000 (16:30 +0200)]
cmd/compile: unify reflect, string and slice copy runtime functions
Use a common runtime slicecopy function to copy strings or slices
into slices. This deduplicates similar code previously used in
reflect.slicecopy and runtime.stringslicecopy.
Change-Id: I09572ff0647a9e12bb5c6989689ce1c43f16b7f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/254658
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Martin Möhrmann <moehrmann@google.com> Reviewed-by: Keith Randall <khr@golang.org>
Ian Lance Taylor [Wed, 2 Sep 2020 00:01:00 +0000 (17:01 -0700)]
cmd/cgo: don't translate bitfields into Go fields
The cgo tool would sometimes emit a bitfield at an offset that did not
correspond to the C offset, such as for the example in the new test.
Change-Id: I61b2ca10ee44a42f81c13ed12865f2060168fed5
Reviewed-on: https://go-review.googlesource.com/c/go/+/252378
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Henrique Vicente [Mon, 17 Feb 2020 01:22:47 +0000 (02:22 +0100)]
os/signal: add NotifyContext to cancel context using system signals
Fixes #37255
Change-Id: Ic0fde3498afefed6e4447f8476e4da7c1faa7145
Reviewed-on: https://go-review.googlesource.com/c/go/+/219640
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Giovanni Bajo <rasky@develer.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/go/internal/modget: factor out functions for argument resolution
For #37438
For #41315
For #36460
Change-Id: I17041c35ec91ff6ffb547e0f32572673d191b1ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/254820
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Jay Conrod [Wed, 9 Sep 2020 20:53:08 +0000 (16:53 -0400)]
cmd/go: default to -mod=readonly in most commands
For #40728
Change-Id: I6618f1b5a632e8b353a483a83bb0cdf4ef6df72c
Reviewed-on: https://go-review.googlesource.com/c/go/+/251881
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
cmd/go/internal/modget: warn about unmatched packages exactly once
Due to an inverted condition, we were emitting a "matched no packages"
warning twice in some cases and not at all in others.
For #41315
Change-Id: I472cd2d4f75811c8734852f2bdd7346f4c612816
Reviewed-on: https://go-review.googlesource.com/c/go/+/254819
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> Reviewed-by: Michael Matloob <matloob@golang.org>
Daniel Martí [Thu, 10 Sep 2020 21:53:59 +0000 (22:53 +0100)]
cmd/go: relax version's error on unexpected flags
In https://golang.org/cl/221397 we made commands like "go version -v"
error, since both of the command's flags only make sense when arguments
follow them. Without arguments, the command only reports Go's own
version, and the flags are most likely a mistake.
However, the script below is entirely reasonable:
export GOFLAGS=-v # make all Go commands verbose
go version
go build
After the previous CL, "go version" would error. Instead, only error if
the flag was passed explicitly, and not via GOFLAGS.
The patch does mean that we won't error on "GOFLAGS=-v go version -v",
but that very unlikely false negative is okay. The error is only meant
to help the user not misuse the flags, anyway - it's not a critical
error of any sort.
To reuse inGOFLAGS, we move it to the base package and export it there,
since it's where the rest of the GOFLAGS funcs are.
Fixes #41264.
Change-Id: I74003dd25d94bacf9ac507b5cad778fd65233321
Reviewed-on: https://go-review.googlesource.com/c/go/+/254157
Trust: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Jay Conrod [Tue, 15 Sep 2020 14:14:15 +0000 (10:14 -0400)]
cmd/go: fix broken mod_outside test
Since CL 254369, 'go mod graph' now reports an error when invoked
outside a module. This broke the mod_outside test, which expected
'go mod graph' to succeed with no output.
Change-Id: Ic30ee68f1f4c4d33795bdf7df70a7631fb9395e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/255017
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Jay Conrod [Fri, 11 Sep 2020 17:30:43 +0000 (13:30 -0400)]
cmd/go: add modload.NeedRoot mode for commands that need module root
This makes error reporting a bit more consistent for 'go mod'
subcommands. Most of these commands only work in module mode when a
go.mod file is present.
Setting modload.ForceUseModules reports an error when GO111MODULE=off.
Setting modload.RootMode to modload.NeedRoot reports an error when no
go.mod file is present.
Change-Id: I1daa8d2971cb8658e0c804765839d903734a412e
Reviewed-on: https://go-review.googlesource.com/c/go/+/254369 Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Michael Matloob <matloob@golang.org>
Jay Conrod [Wed, 2 Sep 2020 17:14:36 +0000 (13:14 -0400)]
cmd/go: implement 'go install pkg@version'
With this change, 'go install' will install executables in module mode
without using or modifying the module in the current directory, if
there is one.
For #40276
Change-Id: I922e71719b3a4e0c779ce7a30429355fc29930bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/254365
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Michael Matloob <matloob@golang.org>
Xiangdong Ji [Tue, 8 Sep 2020 09:55:20 +0000 (09:55 +0000)]
cmd/compile: rewrite some ARM64 rules to use typed aux
Passes toolstash-check -all.
Change-Id: I7ec36bc048f3031c8201107e6fc5d1257271dbf1
Reviewed-on: https://go-review.googlesource.com/c/go/+/234379
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Alberto Donizetti <alb.donizetti@gmail.com> Reviewed-by: Keith Randall <khr@golang.org>
internal/poll, internal/syscall/unix, net: enable writev on illumos
Illumos supports iovec read/write. Add the writev wrapper to
internal/syscall/unix and use it to implement internal/poll.writev for
net.(*netFD).writeBuffers.
Change-Id: Ie256c2f96aba8e61fb21991788789a049425f792
Reviewed-on: https://go-review.googlesource.com/c/go/+/254638
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Though it's not a problem at this moment, some attempts by @mdempsky to
move order/walk/instrument into buildssa was failed, due to SSA cache
corruption.
This commit fixes that reentrancy issue, by making generated functions
to be pumped through the same compile workqueue that normal functions
are compiled. We do this by adding them to xtop, instead of calling
funccompile directly in geneq/genhash/genwrapper. In dumpdata, we look
for uncompiled functions in xtop instead of compilequeue, then finish
compiling them.
Updates #38463
Fixes #33485
Change-Id: Ic9f0ce45b56ae2ff3862f17fd979253ddc144bb5
Reviewed-on: https://go-review.googlesource.com/c/go/+/254617
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Mon, 14 Sep 2020 19:53:36 +0000 (12:53 -0700)]
cmd/compile: better dclcontext handling in func{hdr,body}
funchdr and funcbody currently assume that either (1) Curfn == nil &&
dclcontext == PEXTERN, or (2) Curfn != nil && dclcontext == PAUTO.
This is a reasonable assumption during parsing. However, these
functions end up getting used in other contexts, and not all callers
are so disciplined about Curfn/dclcontext handling.
This CL changes them to save/restore arbitrary Curfn/dclcontext pairs
instead. This is necessary for the followup CL, which pushes fninit
earlier. Otherwise, Curfn/dclcontext fall out of sync, and funchdr
panics.
Passes toolstash-check.
Updates #33485.
Change-Id: I19b1be23db1bad6475345ae5c81bbdc66291a3a7
Reviewed-on: https://go-review.googlesource.com/c/go/+/254838
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Changkun Ou [Mon, 14 Sep 2020 08:24:59 +0000 (10:24 +0200)]
testing: fix panicking tests hang if Cleanup calls FailNow
Previously, it was impossible to call FailNow in a Cleanup.
Because it can terminate a panicking goroutine and cause its
parent hangs on t.signal channel. This CL sends the signal
in a deferred call to prevent the hang.
Fixes #41355
Change-Id: I4552d3a7ea763ef86817bf9b50c0e37fb34bf20f
Reviewed-on: https://go-review.googlesource.com/c/go/+/254637 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Emmanuel Odeke <emm.odeke@gmail.com>
cmd/api: omit outside dependencies when listing the packages in "std"
As of CL 251159, when 'go list -deps std' is run within GOROOT/src, it
treats the vendored external dependencies as real module dependencies,
not standard-library "vendor/" packages (which still exist in that
case, but are treated as distinct packages outside the "std" module).
Fixes #41358
Updates #30241
Change-Id: Ic23eae9829d90e74a340d49ca9052e9191597410
Reviewed-on: https://go-review.googlesource.com/c/go/+/254738
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Martin Möhrmann [Mon, 13 Jul 2020 16:12:20 +0000 (18:12 +0200)]
runtime: add 24 byte allocation size class
This CL introduces a 24 byte allocation size class which
fits 3 pointers on 64 bit and 6 pointers on 32 bit architectures.
Notably this new size class fits a slice header on 64 bit
architectures exactly while previously a 32 byte size class
would have been used for allocating a slice header on the heap.
The main complexity added with this CL is that heapBitsSetType
needs to handle objects that aren't 16-byte aligned but contain
more than a single pointer on 64-bit architectures.
Due to having a non 16 byte aligned size class on 32 bit a
h.shift of 2 is now possible which means a heap bitmap byte might
only be partially written. Due to this already having been
possible on 64 bit before the heap bitmap code only needed
minor adjustments for 32 bit doublecheck code paths.
Note that this CL changes the slice capacity allocated by append
for slice growth to a target capacity of 17 to 24 bytes.
On 64 bit architectures the capacity of the slice returned by
append([]byte{}, make([]byte, 24)...)) is 32 bytes before and
24 bytes after this CL. Depending on allocation patterns of the
specific Go program this can increase the number of total
alloctions as subsequent appends to the slice can trigger slice
growth earlier than before. On the other side if the slice is
never appended to again above its capacity this will lower heap
usage by 8 bytes.
This CL changes the set of size classes reported in the
runtime.MemStats.BySize array due to it being limited to a
total of 61 size classes. The new 24 byte size class is now
included and the 20480 byte size class is not included anymore.
Change-Id: I9ecf2a10024513abef8fbfbe519e44e0b29b6167
Reviewed-on: https://go-review.googlesource.com/c/go/+/242258
Trust: Martin Möhrmann <moehrmann@google.com>
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Martin Möhrmann <martisch@uos.de>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Keith Randall <khr@golang.org>
Change-Id: I1f67085a1e7c40f60282c2fea7104fb44a01e310
Reviewed-on: https://go-review.googlesource.com/c/go/+/254739
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
This CL ensures that responses served via CGI and FastCGI
have a Content-Type header based on the content of the
response if not explicitly set by handlers.
If the implementers of the handler did not explicitly
specify a Content-Type both CGI implementations would default
to "text/html", potentially causing cross-site scripting.
Thanks to RedTeam Pentesting GmbH for reporting this.
Dominic Della Valle [Sun, 13 Sep 2020 17:03:48 +0000 (17:03 +0000)]
make.bat: fix compare between GOROOT and srcdir paths, when either contains whitespace.
CL 96455 brings CL 57753 to Windows
However, a path comparison within it was left unquoted.
If the Go source directory resides in a path containing whitespace,
the interpreter will compare against the first portion of the path string,
and treat the remainder as an expression.
This patch amends that.
For example, consider the path
`C:\Users\Dominic Della Valle\Projects\Go\goroot\src`
Issuing `make.bat` will print out `'Della' is not recognized as an internal or external command, operable program or batch file.` before proceeding.
Change-Id: Ifcec159baeec940c29c61aa721c64c13c6fd8c14
GitHub-Last-Rev: 809ddbb4dbc80d834f8108ca44c2826016d78d1c
GitHub-Pull-Request: golang/go#41319
Reviewed-on: https://go-review.googlesource.com/c/go/+/253898
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Giovanni Bajo <rasky@develer.com>
Trust: Giovanni Bajo <rasky@develer.com>
Daniel Martí [Sat, 15 Aug 2020 14:20:50 +0000 (16:20 +0200)]
cmd/go: avoid flag.FlagSet.VisitAll at init time
We want to error early if GOFLAGS contains any flag that isn't known to
any cmd/go command. Thus, at init time we would recursively use VisitAll
on each of the flagsets to populate a map of all registered flags.
This was unfortunate, as populating said map constituted a whole 5% of
the run-time of 'go env GOARCH'. This is because VisitAll is pretty
expensive; it copies all the maps from the flagset's map to a slice,
sorts the slice, then does one callback per flag.
First, this was a bit wasteful. We only ever needed to query the
knownFlag map if GOFLAGS wasn't empty. If it's empty, there's no work to
do, thus we can skip the map populating work.
Second and most important, we don't actually need the map at all. A
flag.FlagSet already has a Lookup method, so we can simply recursively
call those methods for each flag in GOFLAGS. Add a hasFlag func to make
that evident.
This mechanism is different; its upfront cost is none, but it will
likely mean a handful of map lookups for each flag in GOFLAGS. However,
that tradeoff is worth it; we don't expect GOFLAGS to contain thousands
of flags. The most likely scenario is less than a dozen flags, in which
case constructing a "unified" map is not at all a net win.
One possible reason the previous mechanism was that way could be
AddKnownFlag. Thankfully, the one and only use of that API was removed
last year when Bryan cleaned up flag parsing in cmd/go.
The wins for the existing benchmark with an empty GOFLAGS are
significant:
name old time/op new time/op delta
ExecGoEnv-8 575µs ± 1% 549µs ± 2% -4.44% (p=0.000 n=7+8)
name old sys-time/op new sys-time/op delta
ExecGoEnv-8 1.69ms ± 1% 1.68ms ± 2% ~ (p=0.281 n=7+8)
name old user-time/op new user-time/op delta
ExecGoEnv-8 1.80ms ± 1% 1.66ms ± 2% -8.09% (p=0.000 n=7+8)
To prove that a relatively large number of GOFLAGS isn't getting
noticeably slower, we measured that as well, via benchcmd and GOFLAGS
containing 50 valid flags:
GOFLAGS=$(yes -- -race | sed 50q) benchcmd -n 500 GoEnvGOFLAGS go env GOARCH
And the result, while noisy, shows no noticeable difference (note that
it measures 3ms instead of 0.6ms since it's sequential):
name old time/op new time/op delta
GoEnvGOFLAGS 3.04ms ±32% 3.03ms ±35% ~ (p=0.156 n=487+481)
Finally, we've improved the existing Go benchmark. Now it's parallel,
and it also reports sys-time and user-time, which are useful metrics.
Change-Id: I9b4551415cedf2f819eb184a02324b8bd919e2bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/248757 Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>