Jeremy Faller [Tue, 25 Feb 2020 01:59:02 +0000 (20:59 -0500)]
[dev.link] cmd/link: create Target in order to make relocsym threadsafe
In its current form, relocsym requires the Link context -- largely in a
readonly state. Rather than passing around such heavy-weight object to
the function, this CL separates out the link target. From here, the
number of touchpoints (and thread unsafe portions) of relocing symbols
can be mentally reasoned about.
(NB: My personal intent is to make this more universal -- passing only
the necessary variables/state to reloc functions. This is a taste to see
if we like this form.)
Bryan C. Mills [Thu, 27 Feb 2020 20:52:32 +0000 (15:52 -0500)]
cmd/go/internal/search: consolidate package-pattern predicates into Match methods
This change consolidates predicates currently scattered throughout
various parts of the package and module loader into methods on the
search.Match type.
That not only makes them more concise, but also encourages
consistency, both in the code and in reasoning about the kinds of
patterns that need to be handled. (For example, the IsLocal predicate
was previously two different calls, either of which could be easily
forgotten at a given call site.)
Factored out from CL 185344 and CL 185345.
Updates #32917
Change-Id: Ifa450ffaf6101f673e0ed69ced001a487d6f9335
Reviewed-on: https://go-review.googlesource.com/c/go/+/221458
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Michael Matloob [Thu, 27 Feb 2020 21:18:56 +0000 (16:18 -0500)]
cmd/go/internal/modload: make AmbiguousImportError an ImportPathError
AmbiguousImportErrors will now be formatted like other ImportPathErrors:
this means that now the ambiguously imported package won't be printed
twice. Whereas the error message looked like the following:
can't load package: package example.com/m/importy: ambiguous import: found package example.com/m/importy in multiple directories:
$WORK/importy
$WORK/vendor/example.com/m/importy
It now looks like this:
can't load package: ambiguous import: found package example.com/m/importy in multiple directories:
$WORK/importy
$WORK/vendor/example.com/m/importy
Change-Id: I52a2074a6b3f5eb7d78d331d0852b7ea6b3735e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/221457
Run-TryBot: Michael Matloob <matloob@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Dmitri Shuralyov [Fri, 28 Feb 2020 03:53:18 +0000 (22:53 -0500)]
cmd/gofmt, go/format: sync internal.go
Apply CL 40930 to src/cmd/gofmt/internal.go to bring
it into sync with src/go/format/internal.go.
Also revert '\n' back to "\n\n" in one of the comments,
because the previous text was more accurate.
Gofmt replaces the "; " part of "package p; func _() {"
input with two newline characters, not one.
Updates #11844
Change-Id: I6bb8155a931b793311991d3cd8e006a2931b167a
Reviewed-on: https://go-review.googlesource.com/c/go/+/221497 Reviewed-by: Robert Griesemer <gri@golang.org>
cmd/compile: add ellipsis rule diagnostics to rulegen
These detect opportunities to convert a rule to use an ellipsis,
and provide better error messages when something goes wrong.
This change was used to generate all the preceding changes
converting rules to use ellipses. This change is at the end of those
changes rather than the beginning in order to avoid log spam during rule
generation (say during a git bisection).
The preceding changes collectively shrink the cmd/compile binary by ~2.2%.
Part of this detection is also warning when the presence of an
unmentioned aux or auxint could cause conversion to an ellipsis
rule to change the sematics of the rule.
For example:
(Div64 x y) -> (DIV x y)
looks like a promising rule for an ellipsis. However, Div64 has an auxint,
and (on most platforms) DIV does not. An ellipsis rule would keep the
auxint intact, rather than zeroing it, which can infere with CSE.
So this change flags this rule as doing implicit zeroing;
it should be replaced by
(Div64 [a] x y) -> (DIV x y)
which makes it clear that the auxint is being zeroed.
This detection is not foolproof, but it currently has no false positives.
If false positives arise in the future, we will need to gate the output.
Change-Id: Ie21f284579e5d6e75aa304d0deb024d41ede528b
Reviewed-on: https://go-review.googlesource.com/c/go/+/217014
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Daniel Martí [Thu, 27 Feb 2020 16:19:06 +0000 (16:19 +0000)]
cmd/go: version command should error when given bad args
For example, 'go version -m' happily gives you Go's own version, even
though the -m flag only makes sense when grabbing the version of a
binary on disk.
Similarly, if any of the directly named files can't be found, the tool
would succeed. That's acceptable if an error is encountered while
walking a large directory, but not when locating a path directly given
by the user.
These added test cases run even in short mode, as 'go build' is not
needed for them.
Change-Id: I7bb40b72853799e31d9f86cc5e999c8d57813eef
Reviewed-on: https://go-review.googlesource.com/c/go/+/221397
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
cmd/compile: optimize unsigned comparisons to 0/1 on amd64
Plus a bonus optimization I noticed while working on this.
There are no functions (besides the rewrite rules) whose text size
increases as a result of this change.
Updates #21439
The following per-package text size stats were generated by parsing the
output of compiling with -S and summing the function size reported on the
STEXT line. This gives a far more accurate picture of the impact
on generated code than merely looking at the object file size changes
or the resulting binary size changes. The latter are below, for reference.
Tim Cooper [Thu, 27 Feb 2020 18:26:36 +0000 (12:26 -0600)]
net/textproto: close channel to signal pipeline event completion
Change-Id: I7e4827b3428b48c67060789a528586a8907ca3db
Reviewed-on: https://go-review.googlesource.com/c/go/+/221418
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Michael Matloob [Thu, 20 Feb 2020 00:35:58 +0000 (00:35 +0000)]
cmd/go: roll forward "convert TestShadowingLogic to the script framework"
This rolls forward the change golang.org/cl/214431, which was reverted
in golang.org/cl/220217. The cl was broken because
TestVersionControlErrorMessageIncludesCorrectDirectory, which is going
to be removed in golang.org/cl/214429 hadn't been submitted yet.
Original change description:
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I87b3f9acb8575fbcbd58d454b5f9bac4923429b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/220178
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Than McIntosh [Wed, 19 Feb 2020 21:16:17 +0000 (16:16 -0500)]
[dev.link] cmd/link: simplify DWARF DIE symbol payload
Get rid of of the linker's dwSym struct (which wraps a loader.Loader
and a loader.Sym) in favor of just loader.Sym. This requires some minor
tweaks to the cmd/internal/dwarf interfaces.
Change-Id: Id3ffd7c41b2433ea04417040368700334bb0e611
Reviewed-on: https://go-review.googlesource.com/c/go/+/220982
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
We were assigning a cost of 2 for intrinsics:
One when we recognized an intrinsic,
and one for the OCALLFUNC node.
I believe that the intent was that intrinsics should
cost 1, since they are typically an arithmetic op,
and because they tend to occur in performance-sensitive code.
(Not that any of this is particularly principled right now.)
Stop charging when we recognize an intrinsic;
let the OCALLFUNC node cover the cost.
Ziheng Liu [Thu, 13 Feb 2020 21:20:30 +0000 (16:20 -0500)]
all: fix incorrect channel and API usage in some unit tests
This CL changes some unit test functions, making sure that these tests (and goroutines spawned during test) won't block.
Since they are just test functions, I use one CL to fix them all. I hope this won't cause trouble to reviewers and can save time for us.
There are three main categories of incorrect logic fixed by this CL:
1. Use testing.Fatal()/Fatalf() in spawned goroutines, which is forbidden by Go's document.
2. Channels are used in such a way that, when errors or timeout happen, the test will be blocked and never return.
3. Channels are used in such a way that, when errors or timeout happen, the test can return but some spawned goroutines will be leaked, occupying resource until all other tests return and the process is killed.
Change-Id: I3df931ec380794a0cf1404e632c1dd57c65d63e8
Reviewed-on: https://go-review.googlesource.com/c/go/+/219380
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
The gains from this aren't particularly impressive.
Still, it is cheap and easy, and
it will keep me from wondering about whether it
might help to add X every time I look at this function.
This updated function is pretty exhaustive;
I examined every op encountered in a call to isNonNegative
when compiling all the stuff hanging around in my GOPATH,
for both 386 and amd64.
(32 bit architectures were somewhat neglected before.)
Cherry Zhang [Wed, 26 Feb 2020 15:24:39 +0000 (10:24 -0500)]
cmd/dist: enable cgo and PIE tests on android/arm64
Now that android/arm64 supports internal linking PIE, enable the
test. While here, I realized that some cgo tests are also not
enabled on android/arm64. Enable them as well. Let's see if it
works.
Change-Id: Ibf186fe402ebf0bbec82873fd56d0eb752b48180
Reviewed-on: https://go-review.googlesource.com/c/go/+/221099
Run-TryBot: Cherry Zhang <cherryyz@google.com> Reviewed-by: Elias Naur <mail@eliasnaur.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
The SSA backend has rules to read the contents of readonly Lsyms.
However, this rule was failing to trigger for many readonly Lsyms.
This is because the readonly attribute that was set on the Node.Name
was not propagated to its Lsym until the dump globals phase, after SSA runs.
To work around this phase ordering problem, introduce Node.SetReadonly,
which sets Node.Name.Readonly and also configures the Lsym
enough that SSA can use it.
This change also fixes a latent problem in the rewrite rule function,
namely that reads past the end of lsym.P were treated as entirely zero,
instead of merely requiring padding with trailing zeros.
This change also adds an amd64 rule needed to fully optimize
the results of this change. It would be better not to need this,
but the zero extension that should handle this for us
gets optimized away too soon (see #36897 for a similar problem).
I have not investigated whether other platforms also need new
rules to take full advantage of the new optimizations.
Compiled code for (interface{})(true) on amd64 goes from:
There are often many values to clobber.
Allow passing them all in at once.
The goal is increased rule readability.
As a bonus, it shrinks cmd/compile by ~97k, almost half a percent.
Package SSA requires 1.2% less memory to compile.
The single-line changes were make via regex,
and the remaining multi-line clobbers were manually combined.
Passes toolstash-check -all.
Change-Id: Ib310e9265d3616211f8192c9040b4c8933824d19
Reviewed-on: https://go-review.googlesource.com/c/go/+/220691 Reviewed-by: Michael Munday <mike.munday@ibm.com>
Tobias Klauser [Tue, 25 Feb 2020 09:18:55 +0000 (10:18 +0100)]
cmd/link/internal/ld: bump NetBSD ABI version to 7.0
According to https://golang.org/wiki/NetBSD, NetBSD 7.0 is supported as
of Go 1.3 (with Go 1.5 recommended). NetBSD 6.0 was last supported in Go
1.9.7. Thus, bump the minimal ABI version to NetBSD 7.0
Joel Sing [Tue, 25 Feb 2020 16:58:59 +0000 (03:58 +1100)]
cmd/compile: improve Eq32/Neq32 on riscv64
Use SUBW to perform a 32-bit subtraction, rather than zero extending from
32 to 64 bits. This reduces Eq32 and Neq32 to two instructions, rather than
the four instructions required previously.
Joel Sing [Tue, 25 Feb 2020 16:17:01 +0000 (03:17 +1100)]
test: re-enable open-coded defer test on riscv64
Open-coded defers were fixed and re-enabled on riscv64, however this test was
inadvertantly left disabled.
Updates #36786
Change-Id: I128fc84baa3d51f50d173e19e52051dc4d9a07c3
Reviewed-on: https://go-review.googlesource.com/c/go/+/220920
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
TennyZhuang [Wed, 29 Jan 2020 03:47:49 +0000 (11:47 +0800)]
cmd/compile: output cost while inlining function with Debug['m'] > 1
The existing implementation outputs inline cost iff function cannot be inlined with Debug['m'] > 1, the cost info is also useful if the function is inlineable.
Michael Munday [Thu, 20 Feb 2020 17:46:08 +0000 (17:46 +0000)]
cmd/compile: remove Greater* and Geq* generic integer ops
The generic Greater and Geq ops can always be replaced with the Less and
Leq ops. This CL therefore removes them. This simplifies the compiler since
it reduces the number of operations that need handling in both code and in
rewrite rules. This will be especially true when adding control flow
optimizations such as the integer-in-range optimizations in CL 165998.
Change-Id: If0648b2b19998ac1bddccbf251283f3be4ec3040
Reviewed-on: https://go-review.googlesource.com/c/go/+/220417
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Michael Munday [Sun, 23 Feb 2020 22:08:24 +0000 (22:08 +0000)]
cmd/compile: canonicalize comparison argument order
Ensure that any comparison between two values has the same argument
order. This helps ensure that they can be eliminated during the
lowered CSE pass which will be particularly important if we eliminate
the Greater and Geq ops (see #37316).
CMP R0, R1
BLT L1
CMP R0, R1 // same order, can eliminate
BEQ L2
This does have some drawbacks. Notably comparisons might 'flip'
direction in the assembly output after even small changes to the
code or compiler. It should help make optimizations more reliable
however.
compilecmp master -> HEAD
master (218f4572f5): text/template: make reflect.Value indirections more robust
HEAD (f1661fef3e): cmd/compile: canonicalize comparison argument order
platform: linux/amd64
Elias Naur [Fri, 15 Nov 2019 23:30:19 +0000 (18:30 -0500)]
cmd/link: default to internal linking for android/arm64
The bootstrapping process (make.bash) on all other platforms use
internal linking. This change brings android/arm64 in line, fixing the
scary warning on our self-hosted Corellium builders:
warning: unable to find runtime/cgo.a
The linkmode default is changed to internal for all Android programs,
but in practice that won't matter outside our builders: using Go with
Android apps requires buildmode=c-shared which uses linkmode external.
Fixes #31343
Updates #31819
Change-Id: I3b3ada5ed69a7989e6d8e5960bbebf5e1c22aada
Reviewed-on: https://go-review.googlesource.com/c/go/+/207299
Run-TryBot: Elias Naur <mail@eliasnaur.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Emmanuel T Odeke [Mon, 4 Nov 2019 17:19:59 +0000 (09:19 -0800)]
crypto/x509: load roots from colon separated SSL_CERT_DIR in loadSystemRoots
"SSL_CERT_DIR" is meant to hold more than one directory, when a colon
is used as a delimiter. However, we assumed it'd be a single directory
for all root certificates.
OpenSSL and BoringSSL properly respected the colon separated
"SSL_CERT_DIR", as per:
* OpenSSL https://github.com/openssl/openssl/blob/12a765a5235f181c2f4992b615eb5f892c368e88/crypto/x509/by_dir.c#L153-L209
* BoringSSL https://github.com/google/boringssl/blob/3ba9586bc081f67903c89917f23e74a0662ba953/crypto/x509/by_dir.c#L194-L247
Michael Matloob [Fri, 24 Jan 2020 22:21:48 +0000 (17:21 -0500)]
cmd/go: emit an error for extraneous files in GOROOT/src in module mode
If there's a go file immediately in GOROOT/src, it was probably
accidentally added by the user. Since that package shouldn't
exist, return an error if a user tries to list it. We're only making
this change for GOPATH mode because we don't want to break cases
where users have been doing this historically, but want to fix
this case for the future.
This also leaves open the weird cases where files are placed directly
in vendor directories.
Fixes #36587
Change-Id: I9738e47b1e89fd5048cbb8dd28e44648834b8ea7
Reviewed-on: https://go-review.googlesource.com/c/go/+/216381
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Brad Fitzpatrick [Tue, 14 Jan 2020 18:15:25 +0000 (18:15 +0000)]
mime: fix ExtensionsByType bug when there are duplicates
Also, sort them so the results aren't random.
Thanks to @junedev for the bug report & repro.
Fixes #36524
Change-Id: Ic9197ebeceddfb3d0aee895d8fc12ce4d205b164
Reviewed-on: https://go-review.googlesource.com/c/go/+/214680 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Bryan C. Mills [Wed, 11 Dec 2019 14:12:25 +0000 (09:12 -0500)]
cmd/go/internal/{test,vet}: use a standard flag.FlagSet to parse flags
This removes much of the complexity of the implementation and use of
the cmd/go/internal/cmdflag package, and makes the behavior of GOFLAGS
in 'go test' and 'go vet' more consistent with other subcommands.
Some of the complexity reduction has been offset by code comments and
bug fixes, particularly for the handling of GOPATH arguments and flag
terminators ('--').
Fixes #32471
Fixes #18682
Change-Id: I1f6e46a7c679062e1e409e44a2b9f03b9172883b
Reviewed-on: https://go-review.googlesource.com/c/go/+/211358 Reviewed-by: Jay Conrod <jayconrod@google.com>
As far as I can tell, there is no public documentation on this topic,
which cost me several days of debugging.
I am possibly unusual in that I run binaries in production with the
race detector turned on, but I think that others who do the same may
want to be aware of the risk.
Updates #26813.
Updates #37233.
Change-Id: I1f8111bd01d0000596e6057b7cb5ed017d5dc655
Reviewed-on: https://go-review.googlesource.com/c/go/+/220586 Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Corne van der Plas [Tue, 19 Nov 2019 12:22:55 +0000 (13:22 +0100)]
cmd/link: Revert -buildmode=pie to internal linking
When internal linking was broken buildmode PIE is set to external
linking. Now internal linking is fixed, -buildmode=pie can default to
internal linking again.
Change-Id: I32ae689b91018d326f31a2442a1beaf68dddf13c
Reviewed-on: https://go-review.googlesource.com/c/go/+/220595
Run-TryBot: Jean de Klerk <deklerk@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Keith Randall [Mon, 6 Jan 2020 23:36:42 +0000 (15:36 -0800)]
reflect: when Converting between float32s, don't lose signal NaNs
When converting from float32->float64->float32, any signal NaNs
get converted to quiet NaNs. Avoid that so using reflect.Value.Convert
between two float32 types keeps the signal bit of NaNs.
Update #36400
Change-Id: Ic4dd04c4be7189d2171d12b7e4e8f7cf2fb22bb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/213497
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Keith Randall [Mon, 6 Jan 2020 19:23:08 +0000 (11:23 -0800)]
cmd/compile: don't allow NaNs in floating-point constant ops
We store 32-bit floating point constants in a 64-bit field, by
converting that 32-bit float to 64-bit float to store it, and convert
it back to use it.
That works for *almost* all floating-point constants. The exception is
signaling NaNs. The round trip described above means we can't represent
a 32-bit signaling NaN, because conversions strip the signaling bit.
To fix this issue, just forbid NaNs as floating-point constants in SSA
form. This shouldn't affect any real-world code, as people seldom
constant-propagate NaNs (except in test code).
Additionally, NaNs are somewhat underspecified (which of the many NaNs
do you get when dividing 0/0?), so when cross-compiling there's a
danger of using the compiler machine's NaN regime for some math, and
the target machine's NaN regime for other math. Better to use the
target machine's NaN regime always.
This has been a bug since 1.10, and there's an easy workaround
(declare a global varaible containing the signaling NaN pattern, and
use that as the argument to math.Float32frombits) so we'll fix it in
1.15.
Cherry Zhang [Mon, 24 Feb 2020 21:24:29 +0000 (16:24 -0500)]
cmd/link: improve gap detection in TestPIESize
In CL 210180 we detect gaps between PT_LOAD segments and subtract
them from size calculation. The code there only works when
PT_LOAD segments are next to each other. But it is possible that
there are other segments in between (e.g. a GNU_RELRO segment).
Relax the gap detection to count gaps between PT_LOAD segments
regardless of whether they are next to each other.
Updates #36023.
Updates #35545.
Change-Id: I8b94506359fa649a4478acc742d86d4b16022dbc
Reviewed-on: https://go-review.googlesource.com/c/go/+/220654
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
os/exec: use environment variables for user token when present
Builds upon the changes from #32000 which supported sourcing environment
variables for a new process from the environment of a Windows user token
when supplied.
But due to the logic of os/exec, the Env field of a process was
always non-nil when it reached that change.
This change moves the logic up to os/exec, specifically when
os.ProcAttr is being built for the os.StartProcess call, this
ensures that if a user token has been supplied and no Env slice has
been provided on the command it will be sourced from the user's
environment.
If no token is provided, or the program is compiled for any other
platform than Windows, the default environment will be sourced from
syscall.Environ().
Fixes #35314
Change-Id: I4c1722e90b91945eb6980d5c5928183269b50487
GitHub-Last-Rev: 32216b7291418f9285147a93ed6d0ba028f94ef2
GitHub-Pull-Request: golang/go#37402
Reviewed-on: https://go-review.googlesource.com/c/go/+/220587
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>