osyield is called in code paths that are not allowed to split
stack, e.g. casgstatus called from entersyscall/exitsyscall.
It is nosplit on all other platforms. Mark it nosplit on OpenBSD
as well.
Change-Id: I3fed5d7f58b3d50610beca6eed2c7e902b8ec52c
Reviewed-on: https://go-review.googlesource.com/c/go/+/319969
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Joel Sing <joel@sing.id.au>
If the client request never makes it to the server, the outstanding
accept is never broken. Change the test to always close the listening
socket when the client request completes.
Updates #45358
Change-Id: I744a91dfa11704e7e528163d7669c394e90456dc
Reviewed-on: https://go-review.googlesource.com/c/go/+/319275
Trust: Heschi Kreinick <heschi@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
The Context.ImportDir method in the go/build package sets Package.Root
to $GOPATH, if a package is inside a GOPATH workspace. The
loadPackageData function keeps this value even when modules are enabled.
Override Package.Root when modules are enabled, instead of just set its
value when Context.ImportDir was unable to set it.
Add a regression test.
Fixes#46119
Change-Id: I900a33fe13a445cb771e2952d0d830f1b4a5921f
Reviewed-on: https://go-review.googlesource.com/c/go/+/319209
Reviewed-by: Bryan C. Mills <bcmills@google.com>
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>
Currently all script tests use the UNIX path separator with the cd
command, causing the PWD environment variable to have the incorrect path
separator on Windows.
Call filepath.FromSlash on the cd command argument.
Update the testdata/script/README to document that the cd argument must
use slashes.
Add a regression test.
To reproduce this issue, a test must use the cd command followed by a
stdout or stderr command containing the pattern $PWD.
Change-Id: Ib2dc5f185cc2476451402787996d14df91f7dddb
Reviewed-on: https://go-review.googlesource.com/c/go/+/319311
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
There was a space missing in the first line shown below, and an extra
space in the second line shown. Thanks Peter Bourgon for noting this.
BEFORE:
$ go help build | grep -A1 'has some limitations'
has some limitations:importantly, cgo files included from outside the
include path must be in the same directory as the Go package they are
AFTER:
$ go help build | grep -A1 'has some limitations'
has some limitations: importantly, cgo files included from outside the
include path must be in the same directory as the Go package they are
Note that I edited alldocs.go by hand here, as the mkalldocs.sh script
produces a lot more changes, for example adding the -insecure flag
documentation in. Not sure what's wrong there.
Change-Id: I303f6d6b42b0e24cec0748a949dc23beec64b917
Reviewed-on: https://go-review.googlesource.com/c/go/+/319949
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
According to the armv8-a reference manual, conditions AL and NV are not allowed
for instructions CINC, CINV, CNEG, CSET and CSETM. This CL adds this check and
the corresponding test cases.
Change-Id: Icb496b7b13a353f41491f2de4d939a5cd88abb04
Reviewed-on: https://go-review.googlesource.com/c/go/+/317912
Reviewed-by: eric fang <eric.fang@arm.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Trust: eric fang <eric.fang@arm.com>
Run-TryBot: eric fang <eric.fang@arm.com>
TryBot-Result: Go Bot <gobot@golang.org>
In the deadcode path we mark runtime.unreachableMethod symbol,
which is a special symbol used for redirecting unreachable
methods. Currently this code is conditioned on not -linkshared.
This is wrong. It should be marked with -linkshared mode as well.
In fact, -linkshared should only affect the entry symbol. Change
the code accordingly.
Change-Id: I252abf850212a930f275589ef0035a43e52cb9cc
Reviewed-on: https://go-review.googlesource.com/c/go/+/319893
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
We redirect references to unreachable methods to
runtime.unreachableMethod. We choose to use ABIInternal symbol
for this, because runtime.unreachableMethod is a defined Go
function.
When linking against shared libraries, and ABI wrappers are not
enabled, the imported function symbols are all ABI0 and aliased
to ABIInternal. We need to resolve ABI alias in this case.
Change-Id: Idd64ef46ce0b5f54882ea0069ce0d59dc9b7a599
Reviewed-on: https://go-review.googlesource.com/c/go/+/319891
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
A .tbd file is a macOS text-based stub library and is a valid input to
the macOS linker. This change adds .tbd to the allow-list for acceptable
linker flags.
Fixesgolang/go#44263
Change-Id: Ie5439a13325dbc908e42f95ec70aca518bb549f9
GitHub-Last-Rev: 6055c3b5fa
GitHub-Pull-Request: golang/go#44276
Reviewed-on: https://go-review.googlesource.com/c/go/+/292269
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Ian Lance Taylor <iant@golang.org>
It is unclear what the future holds for the go line in go.mod files.
Perhaps at some point we will switch to semver numbering.
Perhaps at some point we will allow specifying minor versions
or even betas and release candidates.
Those kinds of changes are difficult today because the go line
is parsed in dependency modules, meaning that older
versions of the Go toolchain need to understand newer go lines.
This CL makes that case - parsing a go line in a dependency's
go.mod file - a bit more lax about how to find the version.
It allows a leading v and any trailing non-digit-prefixed string
after the MAJOR.MINOR section.
There are no concrete plans to make use of any of these changes,
but if in the future we want to make them, having a few Go releases
under out belt that will accept the syntax in dependencies will
make any changes significantly easier.
Change-Id: I79bb84bba4b769048ac4b14d5c275eb9a3f270c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/317690
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
In Perl mode, (|a)* should match an empty string at the start of the
input. Instead it matches as many a's as possible.
Because (|a)+ is handled correctly, matching only an empty string,
this leads to the paradox that e* can match more text than e+
(for e = (|a)) and that e+ is sometimes different from ee*.
This is a very old bug that ultimately derives from the picture I drew
for e* in https://swtch.com/~rsc/regexp/regexp1.html. The picture is
correct for longest-match (POSIX) regexps but subtly wrong for
preferred-match (Perl) regexps in the case where e has a preferred
empty match. Pointed out by Andrew Gallant in private mail.
The current code treats e* and e+ as the same structure, with
different entry points. In the case of e* the preference list ends up
not quite in the right order, in part because the “before e” and
“after e” states are the same state. Splitting them apart fixes the
preference list, and that can be done by compiling e* as if it were
(e+)?.
Like with any bug fix, there is a very low chance of breaking a
program that accidentally depends on the buggy behavior.
RE2, Go, and Rust all have this bug, and we've all agreed to fix it,
to keep the implementations in sync.
Fixes#46123.
Change-Id: I70e742e71e0a23b626593b16ddef3c1e73b413b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/318750
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
The DWARF standard requires that the DIEs in a subprogram
corresponding to input and output parameters appear in declaration
order; this patch adds some new code in dwarfgen to enforce this
ordering (relying on the existing fn.Dcl ordering is not sufficient).
Prior to the register ABI, it was easy to keep vars/decls sorted
during DWARF generation since you could always rely on frame offset;
with the ABI sorting by frame offset no longer gives you the original
declaration order in all cases.
Fixes#46055.
Change-Id: I0e070cb781d6453caba896e5d3bee7cd5388050d
Reviewed-on: https://go-review.googlesource.com/c/go/+/318829
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
A user encountered a debug/dwarf crash when running the dwarf2json
tool (https://github.com/volatilityfoundation/dwarf2json) on a
debug-built copy of the linux kernel. In this crash, the DWARF type
reader was trying to examine the contents of an array type while that
array type was still in the process of being constructed (due to
cycles in the type graph).
To avoid such situations, this patch extends the mechanism introduced
in https://go-review.googlesource.com/18459 (which handles typedef
types) to delay fixup of array types as well.
Change-Id: I303f6ce5db1ca4bd79da3581957dfc2bfc17cc01
Reviewed-on: https://go-review.googlesource.com/c/go/+/319329
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Yi Chou <yich@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
To pull in CL 318212.
For #41184
Change-Id: Iec0d7bee2d3a5874c24a55ec20efd7746bf70902
Reviewed-on: https://go-review.googlesource.com/c/go/+/319410
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Don't add them to files in vendor and cmd/vendor though. These will be
pulled in by updating the respective dependencies.
For #41184
Change-Id: Icc57458c9b3033c347124323f33084c85b224c70
Reviewed-on: https://go-review.googlesource.com/c/go/+/319389
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
The table was not rendered correctly because one line missed a
column.
Change-Id: I1373e4e9fb8b8f2dcd9fd0db339083362cce9b71
Reviewed-on: https://go-review.googlesource.com/c/go/+/319291
Trust: Cherry Mui <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
This CL add runtime.memmove inlining for AMD64 and ARM64.
According to ssa dump from testcases generic rules can't inline
memmomve properly due to one of the arguments is Phi operation. But this
Phi op will be optimized out by later optimization stages. As a result
memmove can be inlined during arch-specific rules.
The commit add new optimization rules to arch-specific rules that can
inline runtime.memmove if it possible during lowering stage.
Optimization fires 5 times in Go source-code using regabi.
Fixes#41662
Change-Id: Iaffaf4c482d068b5f0683d141863892202cc8824
Reviewed-on: https://go-review.googlesource.com/c/go/+/289151
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: David Chase <drchase@google.com>
As a follow up to an earlier change[1] to add ARMv8+LSE instructions in
the compiler generated atomic intrinsics, make the same change in the
runtime library. Since not all ARMv8 systems support LSE instructions,
they are protected by a feature-flag branch.
[1]: golang.org/cl/234217 commit: ecc3f5112e
Change-Id: I0e2fb22e78d5eddb6547863667a8865946679a00
Reviewed-on: https://go-review.googlesource.com/c/go/+/310591
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Heschi Kreinick <heschi@google.com>
It is valid to see SPWRITE functions at the top of a GC stack traceback,
in the case where they self-preempted during the stack growth check
and haven't actually modified SP in a traceback-unfriendly manner yet.
The current check is therefore too aggressive.
isAsyncSafePoint is taking care of not async-preempting SPWRITE functions
because it doesn't async-preempt any assembly functions at all.
But perhaps it will in the future.
To keep a check that SPWRITE assembly functions are not async-preempted,
add one in preemptPark. Then relax the check in traceback to avoid
triggering on self-preempted SPWRITE functions.
The long and short of this is that the assembly we corrected in x/crypto
issue #44269 was incredibly dodgy but not technically incompatible with
the Go runtime. After this change, the original x/crypto assembly no longer
causes GC traceback crashes during "GOGC=1 go test -count=1000".
But we'll still leave the corrected assembly.
This also means that we don't need to worry about diagnosing SPWRITE
assembly functions that may exist in the wild. They will be skipped for
async preemption and no harm no foul.
Fixes#44269, which was open pending some kind of check for
bad SPWRITE functions in the wild. (No longer needed.)
Change-Id: I6000197b62812bbd2cd92da28eab422634cf75a8
Reviewed-on: https://go-review.googlesource.com/c/go/+/317669
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
We already check mmap errors on some code paths, but we missed
one. Add error check there.
Change-Id: Ic0e9cb0eb03c805de40802cfc5d5500e3e065d99
Reviewed-on: https://go-review.googlesource.com/c/go/+/319290
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
When linking a very large binary, the section address may not fit
in int32. Don't truncate it.
Change-Id: Ibcc8d74bf5662611949e547ce44ca8b973de383f
Reviewed-on: https://go-review.googlesource.com/c/go/+/319289
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
The regression test introduced in https://golang.org/cl/318770 broke the
the nocgo builders.
Update the cgo package used in the test to ensure that it can be build
both with cgo enabled and disabled.
Change-Id: Iab0486f0b85ac5e5a22fdf8a1998edd50cbb4d96
Reviewed-on: https://go-review.googlesource.com/c/go/+/319210
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
The documentation of the go list -find flag says that the Deps list will
be empty. However the current implementation adds implicit imports when
supporting Cgo or SWIG and when linking a main package.
Update the documentation of PackageOpts.IgnoreImport to clarify that
both explicit and implicit imports are ignored.
Add a regression test.
Fixes#46092
Change-Id: I37847528d84adb7a18eb6ff29e4af4b4318a66fe
Reviewed-on: https://go-review.googlesource.com/c/go/+/318770
Trust: Jay Conrod <jayconrod@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
As a cleanup, golang.org/cl/307914 unintentionally caused the idle GC
work recheck to drop sched.lock between acquiring a P and committing to
keep it (once a worker G was found).
This is unsafe, as releasing a P requires extra checks once sched.lock
is taken (such as for runSafePointFn). Since checkIdleGCNoP does not
perform these extra checks, we can now race with other users.
In the case of #45975, we may hang with this sequence:
1. M1: checkIdleGCNoP takes sched.lock, gets P1, releases sched.lock.
2. M2: forEachP takes sched.lock, iterates over sched.pidle without
finding P1, releases sched.lock.
3. M1: checkIdleGCNoP puts P1 back in sched.pidle.
4. M2: forEachP waits forever for P1 to run the safePointFn.
Change back to the old behavior of releasing sched.lock only after we
are certain we will keep the P. Thus if we put it back its removal from
sched.pidle was never visible.
Fixes#45975
For #45916
For #45885
For #45884
Change-Id: I191a1800923b206ccaf96bdcdd0bfdad17b532e9
Reviewed-on: https://go-review.googlesource.com/c/go/+/318569
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
openbsd/mips64 is now the only openbsd port that uses non-libc syscall - revise
build tags to reflect this.
Update #36435
Change-Id: I357b2dd2926d058e25e618fcca42c388587598a8
Reviewed-on: https://go-review.googlesource.com/c/go/+/317919
Trust: Joel Sing <joel@sing.id.au>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
The current implementation uses UTF-aware functions
like strings.EqualFold and strings.ToLower.
This could, in some cases, cause http smuggling.
Change-Id: I0e76a993470a1e1b1b472f4b2859ea0a2b22ada0
Reviewed-on: https://go-review.googlesource.com/c/go/+/308009
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Roberto Clapis <roberto@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
This change alters the CurveParam methods to upgrade from the generic
curve implementation to the specific P224 or P256 implementations when
called on the embedded CurveParams. This removes the trap of using
elliptic.P224().Params() instead of elliptic.P224(), for example, which
results in using the generic implementation instead of the optimized
constant time one. For P224 this is done for all of the CurveParams
methods, except Params, as the optimized implementation covers all
these methods. For P256 this is only done for ScalarMult and
ScalarBaseMult, as despite having implementations of addition and
doubling they aren't exposed and instead the generic implementation is
used. For P256 an additional check that there actually is a specific
implementation is added, as unlike the P224 implementation the P256 one
is only available on certain platforms.
This change takes the simple, fast approach to checking this, it simply
compares pointers. This removes the most obvious class of mistakes
people make, but still allows edge cases where the embedded CurveParams
pointer has been dereferenced (as seen in the unit tests) or when someone
has manually constructed their own CurveParams that matches one of the
standard curves. A more complex approach could be taken to also address
these cases, but it would require directly comparing all of the
CurveParam fields which would, in the worst case, require comparing
against two standard CurveParam sets in the ScalarMult and
ScalarBaseMult paths, which are likely to be the hottest already.
Updates #34648
Change-Id: I82d752f979260394632905c15ffe4f65f4ffa376
Reviewed-on: https://go-review.googlesource.com/c/go/+/233939
Trust: Roland Shoemaker <roland@golang.org>
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Recently some tsan tests were enabled on ppc64le which had not
been enabled before. This resulted in failures on systems with
tsan available, and while debugging it was determined that
there were other issues related to the use of signals with cgo.
Signals were not being forwarded within programs linked against
libtsan because the nocgo sigaction was being called for ppc64le
with or without cgo. Adding callCgoSigaction and calling that
allows signals to be registered so that signal forwarding works.
For linux-ppc64 and aix-ppc64, this won't change. On linux-ppc64
there is no cgo. I can't test aix-ppc64 so those owners can enable
it if they want.
In reviewing comments about sigtramp in sys_linux_arm64 it was
noted that a previous issue in arm64 due to missing callee save
registers could also be a problem on ppc64x, so code was added
to save and restore those.
Also, the use of R31 as a temp register in some cases caused an
issue since it is a nonvolatile register in C and was being clobbered
in cases where the C code expected it to be valid. The code sequences to
load these addresses were changed to avoid the use of R31 when loading
such an address.
To get around a vet error, the stubs_ppc64x.go file in runtime
was split into stubs_ppc64.go and stubs_ppc64le.go.
Updates #45040
Change-Id: Ia4ecff950613cbe1b89471790b1d3819d5b5cfb9
Reviewed-on: https://go-review.googlesource.com/c/go/+/306369
Trust: Lynn Boger <laboger@linux.vnet.ibm.com>
Run-TryBot: Lynn Boger <laboger@linux.vnet.ibm.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Carlos Eduardo Seo <carlos.seo@linaro.org>
go/build.ImportDir returns a *build.Package with various lists of
files. If a file is invalid for some reason, for example, because it
has a different package name than other files, it's added to
InvalidGoFiles in addition to GoFiles, TestGoFiles, or other lists.
Previously, files with parse errors or build constraint errors were
not included in these lists, which causes problems for tools that use
'go list' since InvalidGoFiles is not printed. With this change, files
with any kind of error are added to one of the GoFiles lists.
Fixes#39986
Change-Id: Iee007b5092293eb4420c8a39ce731805fe32135f
Reviewed-on: https://go-review.googlesource.com/c/go/+/241577
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>
If a package has files with conflicting package names, go/build
empirically populates the first name encountered and puts the
remaining files in InvalidGoFiles. That foiled our check for packages
whose name is either unpopulated or "main", since the "package main"
could be found in a source file after the first.
Instead, we now treat any package with a nonzero set of InvalidGoFiles
as potentially a main package. This biases toward over-reporting
errors, but we would rather over-report than under-report.
If we fix#45999, we will be able to make these error checks more
precise.
Updates #42088Fixes#45827Fixes#39986
Change-Id: I588314341b17961b38660192c2130678dc03023e
Reviewed-on: https://go-review.googlesource.com/c/go/+/317300
Trust: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
For #45827
Change-Id: I4d3268d66fb0927161f44b353faef11aa4551e40
Reviewed-on: https://go-review.googlesource.com/c/go/+/317298
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
For #45827
For #39986
Updates #45999
Change-Id: I0c919b6a2e56e7003b90425487eafe0f0eadc609
Reviewed-on: https://go-review.googlesource.com/c/go/+/317299
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Encode the length of type names and tags in a varint encoding
instead of a fixed 2-byte encoding. This allows lengths longer
than 65535 (which can happen for large unnamed structs).
Removed the alignment check for #14962, it isn't relevant any more
since we're no longer reading pointers directly out of this data
(it is encoded as an offset which is copied out bytewise).
Fixes#44155
Update #14962
Change-Id: I6084f6027e5955dc16777c87b0dd5ea2baa49629
Reviewed-on: https://go-review.googlesource.com/c/go/+/318249
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This test is also now flakey on this platform.
Updates #36435
Updates #42464
Change-Id: Idedb81478178ffffe7a9c125a6e8bbd83458f9ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/315794
Trust: Joel Sing <joel@sing.id.au>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Convert the syscall package on openbsd/arm to use libc rather than performing
direct system calls.
Updates #36435
Change-Id: Iff3a91c959292cbf4e0a09c7fd34efc8e88ff83f
Reviewed-on: https://go-review.googlesource.com/c/go/+/315793
Trust: Joel Sing <joel@sing.id.au>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Use libc rather than performing direct system calls for the runtime on
openbsd/arm.
Updates #36435
Change-Id: If64a96a61c80b9748792f8a85a8f16ed6ebca91f
Reviewed-on: https://go-review.googlesource.com/c/go/+/315792
Trust: Joel Sing <joel@sing.id.au>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Switch openbsd/arm to locking via libc, rather than performing direct
system calls.
Update #36435
Change-Id: I190abb1aa544d2cb406fe412960ec106c9716f87
Reviewed-on: https://go-review.googlesource.com/c/go/+/315791
Trust: Joel Sing <joel@sing.id.au>
Reviewed-by: Cherry Mui <cherryyz@google.com>
This switches openbsd/arm to thread creation via pthreads, rather than doing
direct system calls.
Update #36435
Change-Id: Ia8749e3723a9967905c33b6d93dfd9be797a486c
Reviewed-on: https://go-review.googlesource.com/c/go/+/315790
Trust: Joel Sing <joel@sing.id.au>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Spreading function compilation across multiple goroutines results in
non-deterministic output. This is how cmd/compile has historically
behaved for concurrent builds, but is troublesome for non-concurrent
builds, particularly because it interferes with "toolstash -cmp".
I spent some time trying to think of a simple, unified algorithm that
can concurrently schedule work but gracefully degrades to a
deterministic build for single-worker builds, but I couldn't come up
with any. The simplest idea I found was to simply abstract away the
operation of scheduling work so that we can have alternative
deterministic vs concurrent modes.
Change-Id: I08afa00527ce1844432412f4f8553781c4e323df
Reviewed-on: https://go-review.googlesource.com/c/go/+/318229
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Like for P-224, we do the constant time selects to hide the
point-at-infinity special cases of addition, but not the P = Q case,
which presumably doesn't happen in normal operations.
Runtime increases by about 50%, as expected, since on average we were
able to skip half the additions, and the additions reasonably amounted
to half the runtime. Still, the Fiat code is so much faster than big.Int
that we're still more than three time faster overall than pre-CL 315271.
name old time/op new time/op delta
pkg:crypto/elliptic goos:darwin goarch:arm64
ScalarBaseMult/P521-8 4.18ms ± 3% 1.35ms ± 1% -67.64% (p=0.000 n=10+10)
ScalarMult/P521-8 4.17ms ± 2% 1.36ms ± 1% -67.45% (p=0.000 n=10+10)
pkg:crypto/ecdsa goos:darwin goarch:arm64
Sign/P521-8 4.23ms ± 1% 1.44ms ± 1% -66.02% (p=0.000 n=9+10)
Verify/P521-8 8.31ms ± 2% 2.73ms ± 2% -67.08% (p=0.000 n=9+9)
GenerateKey/P521-8 4.15ms ± 2% 1.35ms ± 2% -67.41% (p=0.000 n=10+10)
Updates #40171
Change-Id: I782f2b7f33dd60af9b3b75e46d920d4cb47f719f
Reviewed-on: https://go-review.googlesource.com/c/go/+/315274
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
Trust: Katie Hockman <katie@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Fiat Cryptography (https://github.com/mit-plv/fiat-crypto) is a project
that produces prime order field implementations (the code that does
arithmetic modulo a prime number) based on a formally verified model.
The formal verification covers some of the most subtle and hard to test
parts of an elliptic curve implementation, like carry chains. It would
probably have prevented #20040 and #43786.
This CL imports a 64-bit implementation of the P-521 base field,
replacing the horribly slow and catastrophically variable time big.Int
CurveParams implementation.
The code in p521_fiat64.go is generated reproducibly by fiat-crypto,
building and running the Dockerfile according to the README.
The code in fiat/p521.go is a thin and idiomatic wrapper around the
fiat-crypto code. It includes an Invert method generated with the help
of github.com/mmcloughlin/addchain.
The code in elliptic/p521.go is a line-by-line port of the CurveParams
implementation. Lsh(x, N) was replaced with repeated Add(x, x) calls.
Mul(x, x) was replaced with Square(x). Mod calls were removed, as all
operations are modulo P. Likewise, Add calls to bring values back to
positive were removed. The ScalarMult ladder implementation is now
constant time, copied from p224ScalarMult. Only other notable changes
are adding a p512Point type to keep (x, y, z) together, and making
addJacobian and doubleJacobian methods on that type, with the usual
receiver semantics to save 4 allocations per step.
This amounts to a proof of concept, and is far from a mature elliptic
curve implementation. Here's a non-exhaustive list of things that need
improvement, most of which are pre-existing issues with crypto/elliptic.
Some of these can be fixed without API change, so can't.
- Marshal and Unmarshal still use the slow, variable time big.Int
arithmetic. The Curve interface does not expose field operations, so
we'll have to make our own abstraction.
- Point addition uses an incomplete Jacobian formula, which has variable
time behaviors for points at infinity and equal points. There are
better, complete formulae these days, but I wanted to keep this CL
reviewable against the existing code.
- The scalar multiplication ladder is still heavily variable time. This
is easy to fix and I'll do it in a follow-up CL, but I wanted to keep
this one easier to review.
- Fundamentally, values have to go in and out of big.Int representation
when they pass through the Curve interface, which is both slow and
slightly variable-time.
- There is no scalar field implementation, so crypto/ecdsa ends up using
big.Int for signing.
- Extending this to P-384 would involve either duplicating all P-521
code, or coming up with some lower-level interfaces for the base
field. Even better, generics, which would maybe let us save heap
allocations due to virtual calls.
- The readability and idiomaticity of the autogenerated code can
improve, although we have a clear abstraction and well-enforced
contract, which makes it unlikely we'll have to resort to manually
modifying the code. See mit-plv/fiat-crypto#949.
- We could also have a 32-bit implementation, since it's almost free to
have fiat-crypto generate one.
Anyway, it's definitely better than CurveParams, and definitely faster.
name old time/op new time/op delta
pkg:crypto/elliptic goos:darwin goarch:arm64
ScalarBaseMult/P521-8 4.18ms ± 3% 0.86ms ± 2% -79.50% (p=0.000 n=10+9)
ScalarMult/P521-8 4.17ms ± 2% 0.85ms ± 6% -79.68% (p=0.000 n=10+10)
pkg:crypto/ecdsa goos:darwin goarch:arm64
Sign/P521-8 4.23ms ± 1% 0.94ms ± 0% -77.70% (p=0.000 n=9+8)
Verify/P521-8 8.31ms ± 2% 1.75ms ± 4% -78.99% (p=0.000 n=9+10)
GenerateKey/P521-8 4.15ms ± 2% 0.85ms ± 2% -79.49% (p=0.000 n=10+9)
name old alloc/op new alloc/op delta
pkg:crypto/elliptic goos:darwin goarch:arm64
ScalarBaseMult/P521-8 3.06MB ± 3% 0.00MB ± 0% -99.97% (p=0.000 n=10+10)
ScalarMult/P521-8 3.05MB ± 1% 0.00MB ± 0% -99.97% (p=0.000 n=9+10)
pkg:crypto/ecdsa goos:darwin goarch:arm64
Sign/P521-8 3.03MB ± 0% 0.01MB ± 0% -99.74% (p=0.000 n=10+8)
Verify/P521-8 6.06MB ± 1% 0.00MB ± 0% -99.93% (p=0.000 n=9+9)
GenerateKey/P521-8 3.02MB ± 0% 0.00MB ± 0% -99.96% (p=0.000 n=9+10)
name old allocs/op new allocs/op delta
pkg:crypto/elliptic goos:darwin goarch:arm64
ScalarBaseMult/P521-8 19.8k ± 3% 0.0k ± 0% -99.95% (p=0.000 n=10+10)
ScalarMult/P521-8 19.7k ± 1% 0.0k ± 0% -99.95% (p=0.000 n=9+10)
pkg:crypto/ecdsa goos:darwin goarch:arm64
Sign/P521-8 19.6k ± 0% 0.1k ± 0% -99.63% (p=0.000 n=10+10)
Verify/P521-8 39.2k ± 1% 0.1k ± 0% -99.84% (p=0.000 n=9+10)
GenerateKey/P521-8 19.5k ± 0% 0.0k ± 0% -99.91% (p=0.000 n=9+10)
Updates #40171
Change-Id: Ic898b09a2388382bf51ec007d9a79d72d44efe10
Reviewed-on: https://go-review.googlesource.com/c/go/+/315271
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Katie Hockman <katie@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
Unfortunately, we can't improve the function signature to refer to
crypto.PrivateKey and crypto.PublicKey, even if they are both
interface{}, because it would break assignments to function types.
Fixes#37845
Change-Id: I627f2ac1e1ba98b128dac5382f9cc2524eaef378
Reviewed-on: https://go-review.googlesource.com/c/go/+/224157
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
steals idea from CL 312093
further investigation revealed additional duplicate
slots (equivalent, but not equal), so delete those too.
Rearranged Func.Names to be addresses of slots,
create canonical addresses so that split slots
(which use those addresses to refer to their parent,
and split slots can be further split)
will preserve "equivalent slots are equal".
Removes duplicates, improves metrics for "args at entry".
Change-Id: I5bbdcb50bd33655abcab3d27ad8cdce25499faaf
Reviewed-on: https://go-review.googlesource.com/c/go/+/312292
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
