This adds a straight-forward implementation of the functionality.
A more performant version could be added that unrolls the loop
as is done in google.golang.org/protobuf/encoding/protowire,
but usages that demand high performance can use that package instead.
Fixes#51644
Change-Id: I9d3b615a60cdff47e5200e7e5d2276adf4c93783
Reviewed-on: https://go-review.googlesource.com/c/go/+/400176
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
In issue #17671, there are a endless loop if printing
the panic value panics, CL 30358 has fixed that.
As issue #52257 pointed out, above change should not
discard the value from panic while panicking.
With this CL, when we recover from a panic in error.Error()
or stringer.String(), and the recovered value is string,
then we can print it normally.
Fixes#52257
Change-Id: Icfcc4a1a390635de405eea04904b4607ae9e3055
Reviewed-on: https://go-review.googlesource.com/c/go/+/399874
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
The noopt builder is broken, because with -N we get two OpSB opcodes
(one for the function as a whole, one introduced by the jumptable
rewrite rule), and they fight each other for a register.
Without -N, the two OpSB get CSEd, so optimized builds are ok.
Maybe we fix regalloc to deal with this case, but it's simpler
(and maybe more correct?) to disable jump tables with -N.
Change-Id: I75c87f12de6262955d1df787f47c53de976f8a5f
Reviewed-on: https://go-review.googlesource.com/c/go/+/400455
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reorganize the way we rewrite expression switches on strings, so that
jump tables are naturally used for the outer switch on the string length.
The changes to the prove pass in this CL are required so as to not repeat
the test for string length in each case.
name old time/op new time/op delta
SwitchStringPredictable 2.28ns ± 9% 2.08ns ± 5% -9.04% (p=0.000 n=10+10)
SwitchStringUnpredictable 10.5ns ± 1% 9.5ns ± 1% -9.08% (p=0.000 n=9+10)
Update #5496
Update #34381
Change-Id: Ie6846b1dd27f3e472f7c30dfcc598c68d440b997
Reviewed-on: https://go-review.googlesource.com/c/go/+/395714
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@google.com>
So that the inliner knows all the other cases are dead and doesn't
accumulate any cost for them.
The canonical case for this is switching on runtime.GOOS, which occurs
several places in the stdlib.
Fixes#50253
Change-Id: I44823aaebb6c1b03c9b0c12d10086db81954350f
Reviewed-on: https://go-review.googlesource.com/c/go/+/399694
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Performance is kind of hard to exactly quantify.
One big difference between jump tables and the old binary search
scheme is that there's only 1 branch statement instead of O(n) of
them. That can be both a blessing and a curse, and can make evaluating
jump tables very hard to do.
The single branch can become a choke point for the hardware branch
predictor. A branch table jump must fit all of its state in a single
branch predictor entry (technically, a branch target predictor entry).
With binary search that predictor state can be spread among lots of
entries. In cases where the case selection is repetitive and thus
predictable, binary search can perform better.
The big win for a jump table is that it doesn't consume so much of the
branch predictor's resources. But that benefit is essentially never
observed in microbenchmarks, because the branch predictor can easily
keep state for all the binary search branches in a microbenchmark. So
that benefit is really hard to measure.
So predictable switch microbenchmarks are ~useless - they will almost
always favor the binary search scheme. Fully unpredictable switch
microbenchmarks are better, as they aren't lying to us quite so
much. In a perfectly unpredictable situation, a jump table will expect
to incur 1-1/N branch mispredicts, where a binary search would incur
lg(N)/2 of them. That makes the crossover point at about N=4. But of
course switches in real programs are seldom fully unpredictable, so
we'll use a higher crossover point.
Beyond the branch predictor, jump tables tend to execute more
instructions per switch but have no additional instructions per case,
which also argues for a larger crossover.
As far as code size goes, with this CL cmd/go has a slightly smaller
code segment and a slightly larger overall size (from the jump tables
themselves which live in the data segment).
This is a case where some FDO (feedback-directed optimization) would
be really nice to have. #28262
Some large-program benchmarks might help make the case for this
CL. Especially if we can turn on branch mispredict counters so we can
see how much using jump tables can free up branch prediction resources
that can be gainfully used elsewhere in the program.
name old time/op new time/op delta
Switch8Predictable 1.89ns ± 2% 1.27ns ± 3% -32.58% (p=0.000 n=9+10)
Switch8Unpredictable 9.33ns ± 1% 7.50ns ± 1% -19.60% (p=0.000 n=10+9)
Switch32Predictable 2.20ns ± 2% 1.64ns ± 1% -25.39% (p=0.000 n=10+9)
Switch32Unpredictable 10.0ns ± 2% 7.6ns ± 2% -24.04% (p=0.000 n=10+10)
Fixes#5496
Update #34381
Change-Id: I3ff56011d02be53f605ca5fd3fb96b905517c34f
Reviewed-on: https://go-review.googlesource.com/c/go/+/357330
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Name the arguments in a way that is more self-describing.
Many code editor tools show a snippet of the function and
its arguments. However, "x" and "y" are not helpful in determining
which is the sign and which is the magnitude,
short of reading the documentation itself.
Name the sign argument as "sign" to be explicit.
This follows the same naming convention as IsInf.
Change-Id: Ie3055009e475f96c92d5ea7bfe9828eed908c78b
Reviewed-on: https://go-review.googlesource.com/c/go/+/400177
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
In CreateCertificate, if there are no extensions, don't include the
extensions SEQUENCE in the encoded certificate.
Why, you might ask, does the encoding/asn1 tag 'optional' not do
the same thing as 'omitempty'? Good question, no clue, fixing that
would probably break things in horrific ways.
Fixes#52319
Change-Id: I84fdd5ff3e4e0b0a59e3bf86e7439753b1e1477b
Reviewed-on: https://go-review.googlesource.com/c/go/+/399827
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
The ABI mangling code skips symbols that are not loaded from Go
objects. Usually that is fine, as other symbols don't need name
mangling. But trampolines are linker generated and have the same
symbol version (ABI) as the underlying symbol. We need to avoid
symbol name collisions for trampolines, such as a trampoline to
f<ABI0> and a trampoline to f<ABIInternal>. We could explicitly
incorportate the ABI into the trampoline name. But as we already
have the name mangling scheme we could just use that.
The original code excludes external symbols probably because
symbols from C object don't need mangling. But a C symbol and a
Go symbol shouldn't have same name, and so the condition won't
apply.
Also exclude static symbols as they don't need mangling.
Change-Id: I298eb1d64bc0c3da0154f0146b95c4d26ca2f47a
Reviewed-on: https://go-review.googlesource.com/c/go/+/399894
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
They are already in a good order. The sort here does nothing, as
all the SymValues are 0. Sorting just arbitrarily permutes items
because everything is equal and the sort isn't stable.
Not sure why the ordering of these symbols matter. That ordering was
added in CL 243223.
Change-Id: Iee153394afdb39387701cfe0375bc022cf4bd489
Reviewed-on: https://go-review.googlesource.com/c/go/+/399540
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Keith Randall <khr@google.com>
check_testdata/check_testdata.go used the encoding of the corpus entry
file, rather than the input string itself, when checking the expected
size of the minimized value. Instead, use the actual byte length, which
should bypass flakiness.
While we are here, use somewhat simpler fuzz targets, that use byte
slices rather than strings, and only execute the targets when fuzzing (
skipping the 'run' phase.)
Fixes#52285
Change-Id: I48c3780934891eec4a9e38d93abb4666091cb580
Reviewed-on: https://go-review.googlesource.com/c/go/+/399814
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
The Faccessat call checks the user, group, or other permission bits of a
file to see if the calling process can access it. The test to see if the
group permissions should be used was made with the wrong group id, using
the process's group id rather than the file's group id. Fix this to use
the correct group id.
No test since we cannot easily change file permissions when not running
as root and the test is meaningless if running as root.
For #52313
Change-Id: I4e2c84754b0af7830b40fd15dedcbc58374d75ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/399539
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When we added VCS stamping in the Go 1.18 release, we defaulted to
-buildvcs=true, on the theory that most folks will actually want VCS
information stamped.
We also made -buildvcs=true error out if a VCS directory is found and
no VCS tool is available, on the theory that a user who builds with
'-buildvcs=true' will be very surprised if the VCS metadata is
silently missing.
However, that causes a problem for CI environments that don't have the
appropriate VCS tool installed. (And we know that's a common situation
because we're in that situation ourselves — see #46693!)
The new '-buildvcs=auto' setting provides a middle ground: it stamps
VCS information by default when the tool is present (and reports
explicit errors if the tool errors out), but omits the metadata
when the tool isn't present at all.
Fixes#51748.
Updates #51999.
Change-Id: Iebc955c2af0abca9b7517f62ca48b1d944eb2df4
Reviewed-on: https://go-review.googlesource.com/c/go/+/398855
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The existing implementation of `load.resolveEmbed`
uses an expression like `path[len(pkgdir)+1:]`.
Though the `+1` is intended to remove a prefix slash,
the expression returns an incorrect path when `pkgdir`
is "/". (ex.: when removing "/" from "/foo", want "foo",
but got "oo")
It seems that `str.TrimFilePathPrefix` would solve
the problem, but the function contains the same bug.
So, this commit fixes `str.TrimFilePathPrefix` then
applies it to `load.resolveEmbed` to solve the issue.
The fix is quite simple. First, remove prefix. Then
check whether the remained first letter is equal to
`filepath.Separator`. If so, remove it then return.
Fixed#49570
Change-Id: I26ab727ee4dfcbf51ed9bd0a573957ced2154515
Reviewed-on: https://go-review.googlesource.com/c/go/+/396694
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Code generators may reasonably expect to find the GOROOT for which the
code is being generated.
If the generator invokes 'go run' (which ought to be reasonable to do)
and the user has set 'GOFLAGS=trimpath' (which also ought to be
reasonable), then either 'go generate' or 'go run' needs to set GOROOT
explicitly.
I would argue that it is more appropriate for 'go generate' to set
GOROOT than for 'go run' to do so, since a user may reasonably invoke
'go run' to reproduce a user-reported bug in a standalone Go program,
but should not invoke 'go generate' except to regenerate code for a Go
package.
Updates #51461.
Change-Id: Iceba233b4eebd57c40cf5dcd4af9031d210dc9d8
Reviewed-on: https://go-review.googlesource.com/c/go/+/399157
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
After CL 398014 fixed a compiler deadlock on syntax errors,
this CL adds a test case and more details for that.
How it was fixed:
CL 57751 introduced a channel "sem" to limit the number of
simultaneously open files.
Unfortunately, when the number of syntax processing goroutines
exceeds this limit, will easily trigger deadlock.
In the original implementation, "sem" only limited the number
of open files, not the number of concurrent goroutines, which
will cause extra goroutines to block on "sem". When the p.err
of the following iteration happens to be held by the blocking
goroutine, it will fall into a circular wait, which is a deadlock.
CL 398014 fixed the above deadlock, also see issue #52127.
First, move "sem <- struct{}{}" to the outside of the syntax
processing goroutine, so that the number of concurrent goroutines
does not exceed the number of open files, to ensure that all
goroutines in execution can eventually write to p.err.
Second, move the entire syntax processing logic into a separate
goroutine to avoid blocking on the producer side.
Change-Id: I1bb89bfee3d2703784f0c0d4ded82baab2ae867a
Reviewed-on: https://go-review.googlesource.com/c/go/+/399054
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Now that gofmt is reformatting these, we can't get away with
not knowing about directives such as //export and //extern (for gccgo).
Otherwise "//export foo" and "//extern foo" turn into "// export foo",
and "// extern foo", which are completely different meanings.
For #51082.
Change-Id: Id0970331fa0b52ab5fa621631b5fa460767068bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/399734
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
