Since switching to checkconst style static analysis for constants imported from
Go internal pkgs, we've seen a proliferation of duplicate files for various Go
versions. This is not an ideal state of affairs, and results in needless time
spent keeping Go and gVisor in sync WRT to these constants.
To fix this problem, this CL restores template rendering. Instead of going back
to the original implementation where we had separate .tmpl files, this CL uses
Adin's unused implementation of integrating template generation into Go
comments. This allows these files to be parsed as normal by other tools, and
results in not needing separate nogo_facts blaze targets just for template
files.
For simplicity and versatility I have decided to retain the checkconst
annotations. While there don't seem to be that many cases where having
+checkconst annotations over rendering is an improvement (one such case is in
runtime_go12\d_unsafe.go) there may be other such cases in the future. The
template rendering is done via the checkconst analyzer, so there is no redundant
information.
This CL mainly focuses on removing duplicate files per Go version, so other uses
of checkconst annotations that reference internal gvisor pkgs are kept as-is
(e.g. bluepill_.*.s, entry_.*.s, etc.).
PiperOrigin-RevId: 861305642
Adapted from cl/630063475.
This causes FUSE tests involving submounts to fail, because fuse.inode.Valid()
just returns false (fails revalidation) after the entry time expires, causing
all submounts to be unmounted; change it to perform revalidation instead, a la
Linux's fs/fuse/dir.c:fuse_dentry_revalidate(). This in turn requires that we
plumb the dentry's parent and name through kernfs.Inode.Valid().
PiperOrigin-RevId: 630476483
This allows for external information to be passed to restore code.
Similar to a9af2d309b ("Plumb restore context to afterLoad()").
Updates #1956.
PiperOrigin-RevId: 614125262
Currently, the gvisor nogo checks are failing with this error:
gvisor/pkg/sync/runtime_exectracer2.go:21:2: only the
first constant in this group has an explicit type
I don’t know why the constants in runtime_exectracer1.go do not
seem to trigger this error, but either way, the fix is simple enough.
PiperOrigin-RevId: 588412325
It is an idea of running codespell as part of our presubmit checks.
Before enabling it for new changes, let's fix what it has found.
Signed-off-by: Andrei Vagin <[email protected]>
sync.SeqCount relies on the following memory orderings:
- All stores following BeginWrite() in program order happen after the atomic
read-modify-write (RMW) of SeqCount.epoch. In the Go 1.19 memory model, this
is implied by atomic loads being acquire-seqcst.
- All stores preceding EndWrite() in program order happen before the RMW of
SeqCount.epoch. In the Go 1.19 memory model, this is implied by atomic stores
being release-seqcst.
- All loads following BeginRead() in program order happen after the load of
SeqCount.epoch. In the Go 1.19 memory model, this is implied by atomic loads
being acquire-seqcst.
- All loads preceding ReadOk() in program order happen before the load of
SeqCount.epoch. The Go 1.19 memory model does not imply this property.
The x86 memory model *does* imply this final property, and in practice the
current Go compiler does not reorder memory accesses around the load of
SeqCount.epoch, so sync.SeqCount behaves correctly on x86.
However, on ARM64, the instruction that is actually emitted for the atomic load
from SeqCount.epoch is LDAR:
```
gvisor/pkg/sentry/kernel/kernel.SeqAtomicTryLoadTaskGoroutineSchedInfo():
gvisor/pkg/sentry/kernel/seqatomic_taskgoroutineschedinfo_unsafe.go:34
56371c: f9400025 ldr x5, [x1]
563720: f9400426 ldr x6, [x1, #8]
563724: f9400822 ldr x2, [x1, #16]
563728: f9400c23 ldr x3, [x1, #24]
gvisor/pkg/sentry/kernel/seqatomic_taskgoroutineschedinfo_unsafe.go:36
56372c: d503201f nop
gvisor/pkg/sync/sync.(*SeqCount).ReadOk():
gvisor/pkg/sync/seqcount.go:107
563730: 88dffc07 ldar w7, [x0]
563734: 6b0400ff cmp w7, w4
```
LDAR is explicitly documented as not implying the required memory ordering:
https://developer.arm.com/documentation/den0024/latest/Memory-Ordering/Barriers/One-way-barriers
Consequently, SeqCount.ReadOk() is incorrectly memory-ordered on weakly-ordered
architectures. To fix this, we need to introduce an explicit memory fence.
On ARM64, there is no way to implement the memory fence in question without
resorting to assembly, so the implementation is straightforward. On x86, we
introduce a compiler fence, since future compilers might otherwise reorder
memory accesses to after atomic loads; the only apparent way to do so is also
by using assembly, which unfortunately introduces overhead:
- After the call to sync.MemoryFenceReads(), callers zero XMM15 and reload the
runtime.g pointer from %fs:-8, reflecting the switch from ABI0 to
ABIInternal. This is a relatively small cost.
- Before the call to sync.MemoryFenceReads(), callers spill all registers to
the stack, since ABI0 function calls clobber all registers. The cost of this
depends on the state of the caller before the call, and is not reflected in
BenchmarkSeqCountReadUncontended (which does not read any protected state
between the calls to BeginRead() and ReadOk()).
Both of these problems are caused by Go assembly functions being restricted to
ABI0. Go provides a way to mark assembly functions as using ABIInternal
instead, but restricts its use to functions in package runtime
(https://github.com/golang/go/issues/44065). runtime.publicationBarrier(),
which is semantically "sync.MemoryFenceWrites()", is implemented as a compiler
fence on x86; defining sync.MemoryFenceReads() as an alias for that function
(using go:linkname) would mitigate the former problem, but not the latter.
Thus, for simplicity, we define sync.MemoryFenceReads() in (ABI0) assembly, and
have no choice but to eat the overhead.
("Fence" and "barrier" are often used interchangeably in this context; Linux
uses "barrier" (e.g. `smp_rmb()`), while C++ uses "fence" (e.g.
`std::atomic_memory_fence()`). We choose "fence" to reduce ambiguity with
"write barriers", since Go is a GC'd language.)
PiperOrigin-RevId: 573861378
The last remaining !go1.22 build is protecting the definition of
pkg/sync.maptype, which is a copy of runtime.maptype. We need to ensure these
definitions match so we can safely access the hasher field.
At its core, this CL achieves this check by ensuring that
unsafe.Offsetof(maptype{}.Hasher) matches the offset in the runtime version of
the type.
Several things happen along the way to achieve this:
* As of May 2023, runtime.maptype is actually a type alias for
internal/abi.MapType. checkoffset was failing to record the offsets because it
skipped type aliases for no good reason. Simply removing the type alias check
is sufficient to make type aliases work. (This part of the CL is technically
unnecessary because this CL ultimately references internal/abi.MapType
directly in anticipation of removal of the type alias. But there is no reason
not to allow type aliases).
* The checkconst / checkoffset regexp unintentionally does not allow / in
package paths, even though the rest of the package supports /. Fix this.
* checkconst was comparing the literal AST expression string against the
runtime value (i.e., "unsafe.Offsetof(maptype{}.Hasher)" vs "72", which fails
comparison. Switch to getting the resolved constant value from the type
checker.
* nogo/check.importer only loads package facts on direct import (stored in
importer.cache). If a package is not directly imported ImportPackageFact will
not find the facts. Typically packages need to ensure they directly depend on
packages they want facts from (e.g., pkg/sync has a dummy import of runtime in
runtime.go). This doesn't work for internal/abi because we cannot directly
import an internal package. Work around this as a hack by unconditionally
"importing" internal/abi when analyzing any package.
With regard to the last point, not that the nogo/defs.bzl nogo integration only
provides facts from the direct dependencies and the entire stdlib (since the
stdlib is analyzed as one bundle). So this trick only works for a stdlib
package. A bazel package indirect dependency would be missing facts altogether.
PiperOrigin-RevId: 549999084
FDTable.descriptorTable is a slice of unsafe.Pointer-s and its maximum length
is MaxInt32. It requires up to 16GB of memory. A process can use just a few
descriptors but sets one or more of them to high numbers. In this case,
FDTable.descriptorTable is extended to the maximum size.
The problem here is that go-runtime zeros memory regions when they are reused.
In the case of fdtable, the memory region is 16GB, so it is a time consuming
operation. Second, it forces the kernel to allocate physical pages to
the entire region.
This change adds another level to descriptorTable, so the first level is
a slice of buckets where each bucket is a slice of descriptors. The bucket
size is fixed to 512 entries to fit one page.
Before:
BenchmarkFDLookupAndDecRef-12 50834290 23.70 ns/op
BenchmarkCreateWithMaxFD-12 2 7194873988 ns/op
BenchmarkFDLookupAndDecRefConcurrent-12 23775555 49.68 ns/op
BenchmarkTableLookup-12 412888780 2.835 ns/op
BenchmarkTableMapLookup-12 87944782 12.84 ns/op
After:
BenchmarkFDLookupAndDecRef-12 46229940 25.03 ns/op
BenchmarkCreateWithMaxFD-12 13 82573899 ns/op
BenchmarkFDLookupAndDecRefConcurrent-12 21889380 54.13 ns/op
BenchmarkTableLookup-12 415851230 2.821 ns/op
BenchmarkTableMapLookup-12 97236267 11.89 ns/op
Reported-by: [email protected]
PiperOrigin-RevId: 539138632
For all trivial and zero frame-sized functions, we now require an explicit
NOFRAME annotation as the heuristic has changed. See go.dev/cl/466316.
Most of these functions do not *require* NOFRAME, but this change encodes
the existing behavior in order to avoid accidental bugs or regressions.
PiperOrigin-RevId: 513946210
The problem is that it checkes circular dependencies and one then adds the
target lock to the graph.
Reported-by: [email protected]
PiperOrigin-RevId: 512637945
This allows fact information to be validated in the underlying source files,
but requires us to explicitly maintain this in appropriate version-tagged, and
architecture-tagged files. This is more explicit and safer.
This mechanism uses a special regular expression for matching a +checkconst
stanza to validate constant values, sizes and offsets. This applies to both Go
source files and assembly files.
PiperOrigin-RevId: 511867507
cl/504661697 removes this build tag once and for all, but it is blocked on
additional work needed to get it working with OSS bazel builds.
Until that is ready, bump the build tag to unblock testing Go tip (1.21).
PiperOrigin-RevId: 505923005
This removes the need to check the offset every release.
I've also removed the negative build tag from runtime_amd64.go, which less
obviously correct. In theory, we should check that this package's use of
nmspinning is still valid in each release, but there is no way to automate that
and I don't realistically seeing checking happening beyond verifying tests
work. Additionally, the existing use is already suspect. :)
The good news is the misuse is likely to cause scheduling issues, not memory
corruption.
PiperOrigin-RevId: 505114683
Accessing a pointer to the data in a slice can be achieved with
`unsafe.Pointer(&slice[0])`.
347e062d3b motivated using gohacks.SliceHeader in
this way with "we often use SliceHeader to extract pointers from slices in a
way that avoids bounds checking and/or handles nil slices correctly", but this
no longer seems to be the case. None of the remaining uses are obviously
performance sensitive or necessarily include bounds checks, nor get used with
nil slices.
This brings us one step closer to removing gohacks.SliceHeader, which is one
less internal detail to keep in sync with Go.
For #8422
PiperOrigin-RevId: 504408578
This CL does the following:
- Add the ability for nested locks to have names.
- Give names to all current uses of nested locks in the codebase.
- Truncate `lockdep` debug stack traces to avoid the clutter from the
`lockdep` code itself
- Simplify `lockdep` to not longer require `classMap`.
PiperOrigin-RevId: 491486620
Also adds the "// +checkalignedignore" escape hatch for packages to opt out of
checking.
See cl/439349432 for justification (https://github.com/google/gvisor/pull/7376).
PiperOrigin-RevId: 444918125
gVisor bot