Than McIntosh [Tue, 7 Jan 2020 20:47:43 +0000 (15:47 -0500)]
[dev.link] cmd/link: add support for "not in symbol table" attribute
Add loader.Loader support for recording whether symbols should appear
in the symbol table (e.g. AttrNotInSymbolTable). Change the preloader
to apply this attribute to selected symbols (e.g. go.string.*,
runtime.gcbits.*) during the initial scan over object file defs, since
at that point the name is available.
Change-Id: I54be9a649107e90c0fd544a1c0ba87e2d98833ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/213778
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Than McIntosh [Mon, 30 Dec 2019 19:12:54 +0000 (14:12 -0500)]
[dev.link] cmd/link: convert Link.linksetup method to work with loader
Switch the linker's Link.linksetup method to work with loader.Sym
instead of sym.Symbols. Currently enabled when the new ELF host object
loader is turned on.
Change-Id: I336cc9f36166767baac574455531e195b6f1ac57
Reviewed-on: https://go-review.googlesource.com/c/go/+/213423
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Than McIntosh [Sat, 7 Dec 2019 01:11:36 +0000 (20:11 -0500)]
[dev.link] cmd/link: convert ELF host object loading to new loader
This is a rewrite of the ELF host object loader to use just the Loader
interfaces for symbol creation, without constructing sym.Symbols. At
the moment this is gated under the temporary linker command line
option "-newldelf". This version is able to get through all.bash
on linux/amd64.
Than McIntosh [Fri, 3 Jan 2020 20:25:28 +0000 (15:25 -0500)]
[dev.link] cmd/link: add support for setting symbol File property
Add loader methods to get/set the "file" symbol property.
In the legacy sym.Symbol struct, there is both a 'unit' field
(pointing to a CompilationUnit with package path, etc) and a 'file'
field. In the case of compiler-generated Go symbols, 'file' is
redundant (stores the package again), however for symbols read out of
a shared library (in the -linkshared case) it is important to be able
to record the file for a symbol.
With the loader, add a side table that can be used to store a file for
a symbol, and add methods for getting/setting file.
Change-Id: Iefceb8e7780f31457b658c099196de6e00be8aaf
Reviewed-on: https://go-review.googlesource.com/c/go/+/213421
Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
Than McIntosh [Fri, 3 Jan 2020 20:17:26 +0000 (15:17 -0500)]
[dev.link] cmd/link: add loader support for recording ElfType for symbols
Add hooks in the loader for recording the ElfType of a symbol, as a
replacement for the sym.Symbol ElfType() and SetElfType() methods.
This symbol property is set only for symbols read from shared
libraries by ldshlibsyms.
Change-Id: I3a3c193c3718dcfb73a2364d6e19e0419dab5b41
Reviewed-on: https://go-review.googlesource.com/c/go/+/213420
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>
Than McIntosh [Wed, 18 Dec 2019 20:14:46 +0000 (15:14 -0500)]
[dev.link] cmd/link: handle multiple levels of overwrite
Revamp the way that symbol overwrites are handled to deal with symbols
that are overwritten more than once (such as "_cgo_mmap"). The
scenario here is that a symbol can be overwritten twice, once during
preload, and then again when host objects are read during internal
linking. This can result in a situation where we have two entries in
the overwrite map, from X -> Y and then from Y -> Z. Rather than
search the overwrite map when adding new entries, add a helper routine
for querying the map that catches this situation and fixes it up.
Also with this patch is a couple of tweaks to the loader.Dump method
to insure that it can dump the entire global index space without
crashing due to odd overwrites (as in the scenario above).
Change-Id: Ib6c8a0e03e92fc2b57318001711b501eeaf12249
Reviewed-on: https://go-review.googlesource.com/c/go/+/212098
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org>
Than McIntosh [Tue, 7 Jan 2020 01:37:50 +0000 (20:37 -0500)]
[dev.link] cmd/link: propagate gotype symbol info in cloneToExternal
This patch fixes a problem with the loader's cloneToExternal method,
specifically that the new external clone created did not get
the proper Gotype value from its orginal symbol.
Than McIntosh [Mon, 16 Dec 2019 19:14:29 +0000 (14:14 -0500)]
[dev.link] cmd/link: support updates to contents of obj-based Syms
Add in the hooks to SymbolBuilder and to the loader to allow
the linker to make modifications to a non-external symbol (e.g.
a sym whose index is less than loader.extStart).
The basic idea is to manufacture a new external symbol with the same
name and version, then import the old symbol's content (type, data,
relocations, etc) into the payload struct for the new symbol, and
finally redirect the name lookup tables to target the new sym for the
specified name/version.
This change is needed in order to convert over the host object loaders
to avoid use of sym.Symbol.
Than McIntosh [Thu, 21 Nov 2019 21:03:58 +0000 (16:03 -0500)]
[dev.link] cmd/link: keep loader symbol info in sym.CompilationUnit
In sym.Library and sym.CompilationUnit there are slices of *sym.Symbol
pointer that hold text symbols contained in the unit lib. To support
DWARF generation with new loader, add equivalent slices that hold
loader.Sym values for functions in scope. This will be needed if at
some point we push the sym.Symbol creation "wavefront" beyond dwarf
gen.
This patch also insures that live host object symbols are added to the
context Textp2 slice, since they would not make it on otherwise.
[NB: not sure if this is the best way to do this.]
Change-Id: I4f440e12cebc525b1e37082ad39cf7338aeb6b99
Reviewed-on: https://go-review.googlesource.com/c/go/+/208231
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org>
Than McIntosh [Fri, 13 Dec 2019 19:21:13 +0000 (14:21 -0500)]
[dev.link] cmd/link: add some new loader decode type sym utilities
Add some new utility functions for decoding "type.*" symbol data
using loader.Sym instead of sym.Symbol. These are needed for DWARF
type DIE generation.
Than McIntosh [Fri, 13 Dec 2019 18:45:15 +0000 (13:45 -0500)]
[dev.link] cmd/link: move new decodesym utility routines to a separate file
Relocate the various new functions for decoding type.* symbol payloads
(using new loader interfaces) to a new file, as opposed to having them
tacked onto the end of deadcode2.go.
Than McIntosh [Wed, 11 Dec 2019 19:36:17 +0000 (14:36 -0500)]
[dev.link] cmd/link: add SymbolBuilder helper
Add SymbolBuilder helper type -- this type provides a set of methods
intended to make it easy to manipulate the content of a symbol (type,
relocations, data, etc).
Than McIntosh [Wed, 18 Dec 2019 15:57:15 +0000 (10:57 -0500)]
[dev.link] cmd/link: fix some bugs in loader
This patch fixes a couple of bugs introduced in CL 210778 and CL 207606:
- apply the same version selection scheme in loader.CreateExtSym that
we're currently using for loader.Create (since the two functions
will be used in the same way by the host object loader)\ 1
- add code to the loader's NewLoader function to create initial map
values for some of the map-based symbol attributes (somewhere along
the line the code to do this seems to have gotten lost, so this
patch adds it back).
- fix a coding error in growAttrBitmaps (wrong bitmap passed to
append when extending attrOnList)
Than McIntosh [Thu, 12 Dec 2019 13:08:40 +0000 (08:08 -0500)]
[dev.link] cmd/link: add dynimp, localentry attributes for loader.Sym
Add new methods to get/set the "dynimplib", "dynimpvers" and
"localentry" attributes for an external Sym in loader.Loader. These
attribute values are stored sparsely, since we expect that most
symbols will not need them; they are set when processing cgo
directives and when dealing with host object symbols.
Change-Id: If0b3c173307801d39cb576bb99c83b9081c42d9c
Reviewed-on: https://go-review.googlesource.com/c/go/+/211298 Reviewed-by: Jeremy Faller <jeremy@golang.org>
Than McIntosh [Wed, 11 Dec 2019 19:24:19 +0000 (14:24 -0500)]
[dev.link] cmd/link: record external symbol alignment
Add a mechanism for recording symbol alignment for external symbols
under the new loader scheme. Alignments is stored in a side table,
since most symbols don't wind up needing an alignment other than zero.
Than McIntosh [Wed, 11 Dec 2019 19:17:14 +0000 (14:17 -0500)]
[dev.link] cmd/link: support marking outer/sub for external loader.Sym
Add a loader mechanism for recording outer/sub relationships between
symbols without falling back on sym.Symbol. Also includes a new
"PrependSub" method that provides a way to chain a sub-symbol only the
list of an outer symbol (a common operation when manipulating
outer/sub relationships in the linker).
Change-Id: I70c72356945ceec2bacdcdc25bcc352bfb6765a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/210777
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>
Than McIntosh [Wed, 11 Dec 2019 19:05:14 +0000 (14:05 -0500)]
[dev.link] cmd/link: expand set of symbol attributes in loader
Add in a collection of new loader interfaces for getting/setting
symbol attributes, e.g. properties that would normally be part of the
sym.Symbol "Attr" field. This change also moves references to the
loaders 'reachable' bitmap behind a pair of loader methods, so that we
a consistent way of accessing symbol attributes overall. It is worth
noting that not every symbol attribute is backed by a bitmap; for some
infrequently used attributes, a map[Sym]struct{} is used instead.
Change-Id: I0010c9cd928d41b4bb6cdf45db4581e11c3c5db3
Reviewed-on: https://go-review.googlesource.com/c/go/+/210778
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>
Than McIntosh [Wed, 11 Dec 2019 18:39:39 +0000 (13:39 -0500)]
[dev.link] cmd/link: initial support for linker-materialized external symbols
Create loader infrastructure for constructing the payloads of external
symbols from scratch, as opposed to passing in a sym.Symbol object
containing the payload.
The general idea is that clients can use the loader to create new
external Sym's using loader.AddExtSym, and then can add
relocations/data to the new sym with symbol builder interfaces (to be
provided in an subsequent patch), as opposed to having to use
sym.Symbol.
This change preserves compatibility with the old way of doing things
(passing in sym.Symbol) via a new loader.InstallSym method. If a
client invokes this method for a specific Sym, then the loader keeps
track of this fact and uses the sym.Symbol as the backing store instead.
Also included is a small unit test for the new interfaces -- not clear
whether this really needs to be kept around long term... it was mainly
useful during initial bringup.
Change-Id: If8ab15df7b64636e56b317155dfe6d7cdfe23b71
Reviewed-on: https://go-review.googlesource.com/c/go/+/207606
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>
Jeremy Faller [Wed, 27 Nov 2019 17:33:35 +0000 (12:33 -0500)]
[dev.link] cmd/link: remove sym.Symbols from loader
Pushing the wavefront forward requires a single source of truth for
symbols. This CL removes sym.Symbols from the loader for host object
loaders, allowing us to have the single source of truth for symbols be
the loader, not some strange combination of sym.Symbols and the loader.
Change-Id: Ib8ff0117ebe6040fade346047535ff342d781081
Reviewed-on: https://go-review.googlesource.com/c/go/+/209217
Run-TryBot: Jeremy Faller <jeremy@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Than McIntosh [Wed, 4 Dec 2019 14:47:22 +0000 (09:47 -0500)]
[dev.link] cmd/link: avoid allsyms loop in initarray setup
In the linker's symtab() function, avoid looping over the context's
Syms.Allsyms array to locate the entry symbol when setting up the init
array section; do an explicit ABI0 symbol lookup instead. This is a
minor efficiency tweak / code cleanup.
Fixes #20205.
Change-Id: I2ebc17a3cb2cd63e9f5052bc80f1b0ac72c960e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/209838
Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Xiangdong Ji [Fri, 22 Nov 2019 17:02:06 +0000 (17:02 +0000)]
cmd/vet: honor analyzer flags when running vet outside $GOROOT/src
Additional vet flags specified by user are discarded if 'go vet'
is invoked outside $GOROOT/src to check a package under $GOROOT
(including those under "vendor" of $GOROOT), fix it by avoiding the
overwriting, the logic of detemining if the package under vetting
comes from $GOROOT remains untouched.
Also checked 'go tool vet <options> <cfg>' and 'go vet <options>
<user pkg>', both worked w./w.o this fix.
Fixes #35837.
Change-Id: I549af7964e40440afd35f2d1971f77eee6f8de34
Reviewed-on: https://go-review.googlesource.com/c/go/+/209498
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Ian Lance Taylor [Mon, 2 Dec 2019 20:07:22 +0000 (12:07 -0800)]
runtime: use current P's race context in timer code
We were using the race context of the P that held the timer,
but since we unlock the P's timers while executing a timer
that could lead to a race on the race context itself.
Updates #6239
Updates #27707
Fixes #35906
Change-Id: I5f9d5f52d8e28dffb88c3327301071b16ed1a913
Reviewed-on: https://go-review.googlesource.com/c/go/+/209580
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com>
Than McIntosh [Mon, 25 Nov 2019 19:07:59 +0000 (14:07 -0500)]
cmd/link: additional fixes for -newobj and "ld -r" ELF host objects
The previous fix for this issue (CL 208479) was not general enough;
this patch revises it to handle more cases.
The problem with the original fix was that once a sym.Symbol is
created for a given static symbol and given a bogus anonymous version
of -1, we hit problems if some other non-anonymous symbol (created by
host object loading) had relocations targeting the static symbol.
In this patch instead of assigning a fixed anonymous version of -1 to
such symbols, each time loader.Create is invoked we create a new
(unique) anonymous version for the sym.Symbol, then enter the result
into the loader's extStaticSyms map, permitting it to be found in
lookups when processing relocation targets.
NB: this code will hopefully get a lot simpler once we can move host
object loading away from early sym.Symbol creation.
Richard Miller [Tue, 19 Nov 2019 13:15:28 +0000 (13:15 +0000)]
runtime: on plan9 don't return substitute address for sysReserve
Plan 9 doesn't have a way to reserve virtual memory, so the
implementation of sysReserve allocates memory space (which won't
be backed with real pages until the virtual pages are referenced).
If the space is then freed with sysFree, it's not returned to
the OS (because Plan 9 doesn't allow shrinking a shared address
space), but it must be cleared to zeroes in case it's reallocated
subsequently.
This interacts badly with the way mallocinit on 64-bit machines
sets up the heap, calling sysReserve repeatedly for a very large
(64MB?) arena with a non-nil address hint, and then freeing the space
again because it doesn't have the expected alignment. The
repeated clearing of multiple megabytes adds significant startup
time to every go program.
We correct this by restricting sysReserve to allocate memory only
when the caller doesn't provide an address hint. If a hint is
provided, sysReserve will now return nil instead of allocating memory
at a different address.
Fixes #27744
Change-Id: Iae5a950adefe4274c4bc64dd9c740d19afe4ed1c
Reviewed-on: https://go-review.googlesource.com/c/go/+/207917
Run-TryBot: David du Colombier <0intro@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: David du Colombier <0intro@gmail.com>
Michael Anthony Knyszek [Fri, 22 Nov 2019 21:06:15 +0000 (21:06 +0000)]
runtime: ready scavenger without next
This change makes it so that waking up the scavenger readies its
goroutine without "next" set, so that it doesn't interfere with the
application's use of the runnext feature in the scheduler which helps
fairness.
As of CL 201763 the scavenger began waking up much more often, and in
TestPingPongHog this meant that it would sometimes supercede either a
hog or light goroutine in runnext, leading to a skew in the results and
ultimately a test flake.
This change thus re-enables the TestPingPongHog test on the builders.
Michael Anthony Knyszek [Tue, 26 Nov 2019 21:16:43 +0000 (21:16 +0000)]
runtime: reset scavenge address in scavengeAll
Currently scavengeAll (which is called by debug.FreeOSMemory) doesn't
reset the scavenge address before scavenging, meaning it could miss
large portions of the heap. Fix this by reseting the address before
scavenging, which will ensure it is able to walk over the entire heap.
Brad Fitzpatrick [Tue, 26 Nov 2019 23:57:35 +0000 (23:57 +0000)]
net/http: update bundled x/net/http2
Updates bundled http2 to x/net git rev ef20fe5d7 for:
http2: make Transport.IdleConnTimeout consider wall (not monotonic) time
https://golang.org/cl/208798 (#29308)
http2: make CipherSuites validation error more verbose
https://golang.org/cl/200317 (#34776)
http2: track unread bytes when the pipe is broken
https://golang.org/cl/187377 (#28634)
http2: split cookie pair into separate hpack header fields
https://golang.org/cl/155657 (#29386)
Fixes #29308
Fixes #28634
Change-Id: I71a03ca62ccb5ff35a5cfadd8dc705a4491ae7ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/209077 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Cherry Zhang [Fri, 15 Nov 2019 02:34:35 +0000 (21:34 -0500)]
runtime: print more information on stack overflow
Print the current SP and (old) stack bounds when the stack grows
too large. This helps to identify the problem: whether a large
stack is used, or something else goes wrong.
Cherry Zhang [Fri, 15 Nov 2019 01:23:17 +0000 (20:23 -0500)]
cmd/internal/obj: mark split-stack prologue nonpreemptible
When there are both a synchronous preemption request (by
clobbering the stack guard) and an asynchronous one (by signal),
the running goroutine may observe the synchronous request first
in stack bounds check, and go to the path of calling morestack.
If the preemption signal arrives at this point before the call to
morestack, the goroutine will be asynchronously preempted,
entering the scheduler. When it is resumed, the scheduler clears
the preemption request, unclobbers the stack guard. But the
resumed goroutine will still call morestack, as it is already on
its way. morestack will, as there is no preemption request,
double the stack unnecessarily. If this happens multiple times,
the stack may grow too big, although only a small amount is
actually used.
To fix this, we mark the stack bounds check and the call to
morestack async-nonpreemptible, starting after the memory
instruction (mostly a load, on x86 CMP with memory).
Not done for Wasm as it does not support async preemption.
Fixes #35470.
Change-Id: Ibd7f3d935a3649b80f47539116ec9b9556680cf2
Reviewed-on: https://go-review.googlesource.com/c/go/+/207350 Reviewed-by: David Chase <drchase@google.com>
Cherry Zhang [Thu, 14 Nov 2019 22:03:44 +0000 (17:03 -0500)]
cmd/internal/obj, runtime: use register map to mark unsafe points
Currently we use stack map index -2 to mark unsafe points, i.e.
PC ranges that is not safe for async preemption. This has a
problem: it cannot mark CALL instructions, because for stack scan
a valid stack map index is needed.
This CL switches to use register map index for marking unsafe
points instead, which does not conflict with stack scan and can
be applied on CALL instructions. This is necessary as next CL
will mark call to morestack nonpreemptible.
For #35470.
Change-Id: I357bf26c996e1fee1e7eebe4e6bb07d62930d3f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/207349 Reviewed-by: David Chase <drchase@google.com>
Cherry Zhang [Tue, 26 Nov 2019 04:29:02 +0000 (23:29 -0500)]
runtime: disable async preemption on darwin/arm(64) if no cgo
On darwin, we use libc calls, and cgo is required on ARM and
ARM64 so we have TLS set up to save/restore G during C calls. If
cgo is absent, we cannot save/restore G in TLS, and if a signal
is received during C execution we cannot get the G. Therefore
don't send signals (and hope that we won't receive any signal
during C execution).
This can only happen in the go_bootstrap program (otherwise cgo
is required).
Fixes #35800.
Change-Id: I6c02a9378af02c19d32749a42db45165b578188d
Reviewed-on: https://go-review.googlesource.com/c/go/+/208818
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Bryan C. Mills [Fri, 22 Nov 2019 21:06:11 +0000 (16:06 -0500)]
misc: remove use of relative directories in overlayDir functions
It turns out that the relative-path support never worked in the first
place.
It had been masked by the fact that we ~never invoke overlayDir with
an absolute path, which caused filepath.Rel to always return an error,
and overlayDir to always fall back to absolute paths.
Since the absolute paths seem to be working fine (and are simpler),
let's stick with those. As far as I can recall, the relative paths
were only a space optimization anyway.
Updates #28387
Updates #30316
Change-Id: Ie8cd28f3c41ca6497ace2799f4193d7f5dde7a37
Reviewed-on: https://go-review.googlesource.com/c/go/+/208481
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Dan Scales [Tue, 19 Nov 2019 20:41:51 +0000 (12:41 -0800)]
runtime: add go:nosplit to cgo_mmap.go:mmap() and sys_darwin.go:mmap()
cgo_mmap.go:mmap() is called by mem_linux.go:sysAlloc(), a low-level memory
allocation function. mmap() should be nosplit, since it is called in a lot of
low-level parts of the runtime and callers often assume it won't acquire any
locks.
As an example there is a potential deadlock involving two threads if mmap is not nosplit:
trace.bufLock acquired, then stackpool[order].item.mu, then mheap_.lock
- can happen for traceEvents that are not invoked on the system stack and cause
a traceFlush, which causes a sysAlloc, which calls mmap(), which may cause a
stack split. mheap_.lock
mheap_.lock acquired, then trace.bufLock
- can happen when doing a trace in reclaimChunk (which holds the mheap_ lock)
Also, sysAlloc() has a comment that it is nosplit because it may be invoked
without a valid G, in which case its callee mmap() should also be nosplit.
Similarly, sys_darwin.go:mmap() is called by mem_darwin.go:sysAlloc(), and should
be nosplit for the same reasons.
Extra gomote testing: linux/arm64, darwin/amd64
Change-Id: Ia4d10cec5cf1e186a0fe5aab2858c6e0e5b80fdc
Reviewed-on: https://go-review.googlesource.com/c/go/+/207844 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Than McIntosh [Sat, 23 Nov 2019 14:59:30 +0000 (09:59 -0500)]
cmd/link: disable new testpoint on mips pending investigation
Skip TestMinusRSymsWithSameName testpoint on MIPS for the time being
since it triggers failures on that arch. Will re-enable once the
problems are fixed.
Because of concurrent goroutines it is possible for multiple event
handlers to return at the same time. This was not properly supported
and caused the wrong goroutine to continue, which in turn caused
memory corruption.
This change adds a stack of events so it is always clear which is the
innermost event that needs to return next.
Than McIntosh [Fri, 22 Nov 2019 19:31:13 +0000 (14:31 -0500)]
cmd/link: add new linker testpoint for "ld -r" host object
This adds a new test that builds a small Go program with linked
against a *.syso file that is the result of an "ld -r" link. The
sysobj in question has multiple static symbols in the same section
with the same name, which triggered a bug in the loader in -newobj
mode.
Than McIntosh [Fri, 22 Nov 2019 20:27:01 +0000 (15:27 -0500)]
cmd/link: fix bug with -newobj and "ld -r" ELF host objects
When the ELF host object loader encounters a static/hidden symbol, it
creates a sym.Symbol for it but does not enter it into the sym.Symbols
lookup table. Under -newobj mode, this was not happening correctly; we
were adding the sym via loader.LookupOrCreate, which resulted in
collisions when it encountered symbols with the same name + version +
section (this can happen for "ld -r" objects).
Dmitri Shuralyov [Sat, 16 Nov 2019 23:19:33 +0000 (23:19 +0000)]
net/http: rename tests for Redirect and StripPrefix
Before May 2018, I mistakenly thought the _suffix naming convention¹
used by examples also applied to tests. Thanks to a code review comment²
from Ian Lance Taylor, I have since learned that is not true.
This trivial change fixes some collateral damage from my earlier
misunderstanding, resulting in improved test naming consistency.
Brad Fitzpatrick [Fri, 1 Nov 2019 18:23:05 +0000 (11:23 -0700)]
net/http: make Transport.IdleConnTimeout consider wall (not monotonic) time
Both laptops closing their lids and cloud container runtimes
suspending VMs both faced the problem where an idle HTTP connection
used by the Transport could be cached for later reuse before the
machine is frozen, only to wake up many minutes later to think that
their HTTP connection was still good (because only a second or two of
monotonic time passed), only to find out that the peer hung up on them
when they went to write.
HTTP/1 connection reuse is inherently racy like this, but no need for
us to step into a trap if we can avoid it. Also, not everybody sets
Request.GetBody to enable re-tryable POSTs. And we can only safely
retry requests in some cases.
So with this CL, before reusing an old connection, double check the walltime.
Testing was done both with a laptop (closing the lid for a bit) and
with QEMU, running "stop" and "cont" commands in the monitor and
sending QMP guest agent commands to update its wall clock after the
"cont":
Jay Conrod [Thu, 21 Nov 2019 21:55:11 +0000 (16:55 -0500)]
cmd/go: fix and re-enable build_trimpath test
The test was comparing a binary built from a list of files to a test
build from a named package. That should not (and did not) work. The
test now compares two binaries built the same way in different
directories.
Also add a portion of the test for GOPATH and fix the gccgo portion of
the test (verified manually).
Fixes #35435
Change-Id: I2535a0011c9d97d2274e5550ae277302dbb91e6f
Reviewed-on: https://go-review.googlesource.com/c/go/+/208234
Run-TryBot: Jay Conrod <jayconrod@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Bryan C. Mills [Thu, 21 Nov 2019 21:26:09 +0000 (16:26 -0500)]
cmd/go: do not panic when computing Shlib for a package with no Target
In module mode, a non-main package lacks an install target.
The location of the .shlib corresponding to a given target is stored
in a .shlibname file alongside its install target, so in module mode
a non-main package also lacks a .shlibname file.
This also implies that such a package cannot be installed with
'go install -buildmode=linkshared', but that is a problem
for another day.
Fixes #35759
Updates #34347
Change-Id: Id3e0e068266d5fb9b061a59e70f9a65985d4973b
Reviewed-on: https://go-review.googlesource.com/c/go/+/208233
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Bryan C. Mills [Fri, 22 Nov 2019 18:26:58 +0000 (13:26 -0500)]
cmd/go: add a 'buildmode' condition for script tests
In CL 208233 I am fixing a panic that occurs only with a specific
build mode. I want that test to run on all platforms that support that
build mode, but the logic for determining support is somewhat
involved.
For now, I am duplicating that logic into the cmd/internal/sys
package, which already reports platform support for other build flags.
We can refactor cmd/go/internal/work to use the extracted function in
a followup CL.
Updates #35759
Change-Id: Ibbaedde4d1e8f683c650beedd10849bc27e7a6e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/208457
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Bryan C. Mills [Thu, 21 Nov 2019 20:03:24 +0000 (15:03 -0500)]
misc/cgo/testshared: make -v output less verbose
Previously, 'go test -v' in this directory would result in a massive
dump of go command output, because the test plumbed -v to 'build -x'.
This change separates them into distinct flags, so that '-v' only
implies the display of default 'go' command output.
Updates #30316
Change-Id: Ifb125f35ec6a0bebe7e8286e7c546d132fb213df
Reviewed-on: https://go-review.googlesource.com/c/go/+/208232
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Michael Anthony Knyszek [Fri, 22 Nov 2019 16:34:16 +0000 (16:34 +0000)]
runtime: release worldsema before Gosched in STW GC mode
After CL 182657 we no longer hold worldsema across the GC, we hold
gcsema instead.
However in STW GC mode we don't release worldsema before calling Gosched
on the user goroutine (note that user goroutines are disabled during STW
GC) so that user goroutine holds onto it. When the GC is done and the
runtime inevitably wants to "stop the world" again (though there isn't
much to stop) it'll sit there waiting for worldsema which won't be
released until the aforementioned goroutine is scheduled, which it won't
be until the GC is done!
So, we have a deadlock.
The fix is easy: just release worldsema before calling Gosched.
TestPhysicalMemoryUtilization occasionally fails on some platforms by
only a small margin. The reason for this is that it assumes the
scavenger will always be able to scavenge all the memory that's released
by sweeping, but because of the page cache, there could be free and
unscavenged memory held onto by a P which the scavenger simply cannot
get to.
As a result, if the page cache gets filled completely (512 KiB of free
and unscavenged memory) this could skew a test which expects to
scavenge roughly 8 MiB of memory. More specifically, this is 512 KiB of
memory per P, and if a system is more inclined to bounce around
between Ps (even if there's only one goroutine), this memory can get
"stuck".
Through some experimentation, I found that failures correlated highly
with relatively large amounts of memory ending up in some page cache
(like 60 or 64 pages) on at least one P.
This change changes the test's threshold such that it accounts for the
page cache, and scales up with GOMAXPROCS. Because the test constants
themselves don't change, however, the test must now also bound
GOMAXPROCS such that the threshold doesn't get too high (at which point
the test becomes meaningless).
Fixes #35580.
Change-Id: I6bdb70706de991966a9d28347da830be4a19d3a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/208377
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Jay Conrod [Thu, 21 Nov 2019 23:54:38 +0000 (18:54 -0500)]
cmd/go: add 'go generate' commands to modfile_flag test
Verify that 'go generate' works with -modfile. Also check that
go commands starts with 'go generate' do not inherit -modfile, but
they should still work if -modfile is set in GOFLAGS.
Updates #34506
Change-Id: I5e1f897b4e38e4fdaccc0fbb7a71b8d0e9fc0660
Reviewed-on: https://go-review.googlesource.com/c/go/+/208236
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Bryan C. Mills [Wed, 20 Nov 2019 19:39:19 +0000 (14:39 -0500)]
misc/cgo/testcshared: avoid writing to GOROOT in tests
The tests in this package invoked 'go install -i -buildmode=c-shared'
in order to generate an archive as well as multiple C header files.
Unfortunately, the behavior of the '-i' flag is inappropriately broad
for this use-case: it not only generates the library and header files
(as desired), but also attempts to install a number of (unnecessary)
archive files for transitive dependencies to
GOROOT/pkg/$GOOS_$GOARCH_testcshared_shared, which may not be writable
— for example, if GOROOT is owned by the root user but the test is
being run by a non-root user.
Instead, for now we generate the header files for transitive dependencies
separately by running 'go tool cgo -exportheader'.
In the future, we should consider how to improve the ergonomics for
generating transitive header files without coupling that to
unnecessary library installation.
Updates #28387
Updates #30316
Updates #35715
Change-Id: I622426a860828020d98f7040636f374e5c766d28
Reviewed-on: https://go-review.googlesource.com/c/go/+/208119
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Bryan C. Mills [Wed, 20 Nov 2019 15:19:43 +0000 (10:19 -0500)]
misc/cgo/testcarchive: avoid writing to GOROOT in tests
Also add a -testwork flag to facilitate debugging the test itself.
Three of the tests of this package invoked 'go install -i
-buildmode=c-archive' in order to generate an archive as well as
multiple C header files.
Unfortunately, the behavior of the '-i' flag is inappropriately broad
for this use-case: it not only generates the library and header files
(as desired), but also attempts to install a number of (unnecessary)
archive files for transitive dependencies to
GOROOT/pkg/$GOOS_$GOARCH_shared, which may not be writable — for
example, if GOROOT is owned by the root user but the test is being run
by a non-root user.
Instead, for now we generate the header files for transitive dependencies
separately by running 'go tool cgo -exportheader'.
In the future, we should consider how to improve the ergonomics for
generating transitive header files without coupling that to
unnecessary library installation.
Updates #28387
Updates #30316
Updates #35715
Change-Id: I3d483f84e22058561efe740aa4885fc3f26137b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/208117
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Hana Kim [Mon, 18 Nov 2019 17:44:33 +0000 (12:44 -0500)]
runtime/pprof: avoid crash due to truncated stack traces
The profile proto message builder maintains a location entry cache
that maps a location (possibly involving multiple user frames
that represent inlined function calls) to the location id. We have
been using the first pc of the inlined call sequence as the key of
the cached location entry assuming that, for a given pc, the sequence
of frames representing the inlined call stack is deterministic and
stable. Then, when analyzing the new stack trace, we expected the
exact number of pcs to be present in the captured stack trace upon
the cache hit.
This assumption does not hold, however, in the presence of the stack
trace truncation in the runtime during profiling, and also with the
potential bugs in runtime.
A better fix is to use all the pcs of the inlined call sequece as
the key instead of the first pc. But that is a bigger code change.
This CL avoids the crash assuming the trace was truncated.
Fixes #35538
Change-Id: I8c6bae98bc8b178ee51523c7316f56b1cce6df16
Reviewed-on: https://go-review.googlesource.com/c/go/+/207609
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Keith Randall <khr@golang.org>
David Chase [Tue, 19 Nov 2019 18:23:35 +0000 (13:23 -0500)]
cmd/compile: try harder to not use an empty src.XPos for a bogus line
The fix for #35652 did not guarantee that it was using a non-empty
src position to replace an empty one. The new code checks again
and falls back to a more certain position. (The input in question
compiles to a single empty infinite loop, and none of the actual instructions
had any source position at all. That is a bug, but given the pathology
of this input, not one worth dealing with this late in the release cycle,
if ever.)
TODO: Add runtime.InfiniteLoop(), replace infinite loops with a call to
that, and use an eco-friendly runtime.gopark instead. (This was Cherry's
excellent idea.)
Roberto Clapis [Mon, 18 Nov 2019 09:05:07 +0000 (10:05 +0100)]
text/template: harden JSEscape to also escape ampersand and equal
Ampersand and equal are not dangerous in a JS/JSString context
but they might cause issues if interpolated in HTML attributes.
This change makes it harder to introduce XSS by misusing
escaping.
Thanks to t1ddl3r <t1ddl3r@gmail.com> for reporting this common
misuse scenario.
Fixes #35665
Change-Id: Ice6416477bba4cb2ba2fe2cfdc20e027957255c0
Reviewed-on: https://go-review.googlesource.com/c/go/+/207637 Reviewed-by: Filippo Valsorda <filippo@golang.org> Reviewed-by: Mike Samuel <mikesamuel@gmail.com> Reviewed-by: Andrew Bonventre <andybons@golang.org> Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Jay Conrod [Thu, 21 Nov 2019 17:50:14 +0000 (12:50 -0500)]
cmd/go: report an error for 'go list -m ...' outside a module
Previously, we just reported an error for "all". Now we report an
error for any pattern that matches modules in the build list. The
build list can only contain the module "command-line-arguments", so
these patterns are not meaningful.
Fixes #35728
Change-Id: Ibc736491ec9164588f9657c09d1b9683b33cf1de
Reviewed-on: https://go-review.googlesource.com/c/go/+/208222
Run-TryBot: Jay Conrod <jayconrod@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Cherry Zhang [Tue, 19 Nov 2019 21:04:32 +0000 (16:04 -0500)]
runtime: relax TestAsyncPreempt
In TestAsyncPreempt, the function being tested for preemption,
although still asynchronously preemptible, may have only samll
ranges of PCs that are preemtible. In an unlucky run, it may
take quite a while to have a signal that lands on a preemptible
instruction. The test case is kind of an extreme. Relax it to
make it more preemptible.
In the original version, the first closure has more work to do,
and it is not a leaf function, and the second test case is a
frameless leaf function. In the current version, the first one
is also a frameless leaf function (the atomic is intrinsified).
Add some calls to it. It is still not preemptible without async
preemption.
Andrew [Wed, 20 Nov 2019 17:06:51 +0000 (12:06 -0500)]
all: base64-encode binaries that will cause Apple notarization to fail
Starting with macOS 10.15 (Catalina), Apple now requires all software
distributed outside of the App Store to be notarized. Any binaries we
distribute must abide by a strict set of requirements like code-signing
and having a minimum target SDK of 10.9 (amongst others).
Apple’s notarization service will recursively inspect archives looking to
find notarization candidate binaries. If it finds a binary that does not
meet the requirements or is unable to decompress an archive, it will
reject the entire distribution. From cursory testing, it seems that the
service uses content sniffing to determine file types, so changing
the file extension will not work.
There are some binaries and archives included in our distribution that
are being detected by Apple’s service as potential candidates for
notarization or decompression. As these are files used by tests and some
are intentionally invalid, we don’t intend to ever make them compliant.
As a workaround for this, we base64-encode any binaries or archives that
Apple’s notarization service issues a warning for, as these warnings will
become errors in January 2020.
Bryan C. Mills [Wed, 20 Nov 2019 19:43:04 +0000 (14:43 -0500)]
misc/cgo/testplugin: avoid writing to GOROOT
One of the 'go build' commands executed by this test passed the '-i'
flag, which caused the 'go' command to attempt to install transitive
standard-library dependencies to GOROOT/pkg/$GOOS_$GOARCH_dynlink.
That failed if GOROOT/pkg was not writable (for example, if GOROOT was
owned by the root user, but the user running the test was not root).
As far as I can tell the '-i' flag is not necessary in this test.
Prior to the introduction of the build cache it may have been an
optimization, but now that the build cache is required the '-i' flag
only adds extra work.
Bryan C. Mills [Wed, 20 Nov 2019 22:04:35 +0000 (17:04 -0500)]
misc/cgo/fortran: avoid writing to $PWD
The bash script that drives this test needs to know whether the
fortran compiler works, but it doesn't actually care about the
generated binary. Write that binary to /dev/null.
Eric Rutherford [Tue, 19 Nov 2019 02:35:33 +0000 (20:35 -0600)]
path: minor changes to improve documentation for Join
Reworking the comments in path to call out how leading
empty elements are treated. Also updating filepath.Join
since it shared much of the wording from path.Join.
Updates #35655
Change-Id: I5b15c5d36e9d19831ed39e6bcc7f2fd6c1330033
Reviewed-on: https://go-review.googlesource.com/c/go/+/207797 Reviewed-by: Rob Pike <r@golang.org>
Austin Clements [Fri, 25 Oct 2019 20:17:41 +0000 (16:17 -0400)]
runtime: support preemption on windows/{386,amd64}
This implements preemptM on Windows using SuspendThead and
ResumeThread.
Unlike on POSIX platforms, preemptM on Windows happens synchronously.
This means we need a make a few other tweaks to suspendG:
1. We need to CAS the G back to _Grunning before doing the preemptM,
or there's a good chance we'll just catch the G spinning on its
status in the runtime, which won't be preemptible.
2. We need to rate-limit preemptM attempts. Otherwise, if the first
attempt catches the G at a non-preemptible point, the busy loop in
suspendG may hammer it so hard that it never makes it past that
non-preemptible point.
Austin Clements [Tue, 19 Nov 2019 01:21:38 +0000 (20:21 -0500)]
runtime: ensure thread handle is valid in profileloop1
On Windows, there is currently a race between unminit closing the
thread's handle and profileloop1 suspending the thread using its
handle. If another handle reuses the same handle value, this can lead
to unpredictable results.
To fix this, we protect the thread handle with a lock and duplicate it
under this lock in profileloop1 before using it.
This is going to become a much bigger problem with non-cooperative
preemption (#10958, #24543), which uses the same basic mechanism as
profileloop1.
Clément Chigot [Wed, 20 Nov 2019 12:57:24 +0000 (13:57 +0100)]
runtime: disable GDB tests on AIX with -short
Since the new page allocator, AIX's GDB has trouble running Go programs.
It does work but it can be really slow. Therefore, they are disable when
tests are run with -short.
Updates: #35710
Change-Id: Ibfc4bd2cd9714268f1fe172aaf32a73612e262d6
Reviewed-on: https://go-review.googlesource.com/c/go/+/207919
Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>