Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: clean up system call code #51087

Open
prattmic opened this issue Feb 8, 2022 · 23 comments
Open

runtime: clean up system call code #51087

prattmic opened this issue Feb 8, 2022 · 23 comments
Assignees
Labels
NeedsInvestigation
Milestone

Comments

@prattmic
Copy link
Member

@prattmic prattmic commented Feb 8, 2022

The code used to make system calls from the runtime is a mess and a big pain point when making OS-level changes in the runtime.

Quick summary of some of the pain points:

  • Completely independent implementation from package syscall.
  • System call stubs exist in a variety of OS- and arch-specific sys* files (57, to be precise, mostly assembly), totaling ~23k lines of code [1], which is 100% hand-written. Adding a new syscall stub often requires adding new assembly to 10+ files.
  • There is no generic Syscall function that can be used to make an arbitrary system call. You must add a new stub to make a new call.

Some things we could do to improve the situation:

  • Add generic Syscall functions, used by other stubs. And/or use code generation to generate stubs rather than doing so manually.
  • Move syscall code to a new package (like runtime/internal/syscall or perhaps runtime/internal/linux?) to avoid littering the runtime package itself with so many files.
  • Make package syscall depend on the runtime's Syscall stub so we only have a single implementation.

[1] Some of this code is things other than syscall stubs, like thread entrypoints.

cc @aclements @mknyszek @cherrymui

@prattmic prattmic added this to the Backlog milestone Feb 8, 2022
@aclements
Copy link
Member

@aclements aclements commented Feb 8, 2022

And/or use code generation to generate stubs rather than doing so manually.

All or most of the information we would need to generate the stubs is already in the syscall package. The needs of the syscall package are of course a bit different, but the metadata should be effectively the same.

Orthogonal to code generation, given a generic runtime Syscall function (or a few of them for different numbers of arguments), would there even be any overhead to having the stubs be in Go rather than assembly? In addition to significantly reducing the amount of assembly, I think this would encourage us to do better error handling.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Feb 8, 2022

Just to add another pain point: the stubs handle errors differently. Some returns errno (negated or not?), some just crash hard (writing to an invalid address), which is difficult to debug sometimes.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 8, 2022

Change https://go.dev/cl/383999 mentions this issue: runtime/internal/syscall: new package for linux

@cherrymui cherrymui added the NeedsInvestigation label Feb 8, 2022
gopherbot pushed a commit that referenced this issue Feb 15, 2022
Add a generic syscall package for use by the runtime. Eventually we'd
like to clean up system calls in the runtime to use more code generation
and be moved out of the main runtime package.

The implementations of the assembly functions are based on copies of
syscall.RawSyscall6, modified slightly for more consistency between
arches. e.g., renamed trap to num, always set syscall num register
first.

For now, this package is just the bare minimum needed for
doAllThreadsSyscall to make an arbitrary syscall.

For #51087.
For #50113.

Change-Id: Ibecb5e6303279ce15286759e1cd6a2ddc52f7c72
Reviewed-on: https://go-review.googlesource.com/c/go/+/383999
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@ii64
Copy link

@ii64 ii64 commented Feb 23, 2022

Seems this introduce build error, i got:
gofrontend commit 45fd14ab8baf5e86012a808426f8ef52c1d77943
image

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Feb 23, 2022

@ii64 this issue is about a plan for the future, not what has happened. Your comment is unrelated to this issue.

Looks like you are using gollvm. What version of gollvm are you using? Try updating to the latest. If that still fail, please file a new issue. Thanks.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388094 mentions this issue: test: rename live_syscall.go to live_uintptrkeepalive.go

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388477 mentions this issue: syscall: define Syscall in terms of RawSyscall

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388475 mentions this issue: runtime/internal/syscall, syscall: replace RawSyscall6 with runtime implementation

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388478 mentions this issue: syscall: define Syscall6 in terms of RawSyscall6

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388095 mentions this issue: cmd/compile: add //go:uintptrkeepalive

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388476 mentions this issue: syscall: define RawSyscall in terms of RawSyscall6

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388479 mentions this issue: syscall: implement rawSyscallNoError in terms of RawSyscall

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2022

Change https://go.dev/cl/388474 mentions this issue: syscall: move Syscall declarations to OS files

@prattmic prattmic self-assigned this Feb 28, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2022

Change https://go.dev/cl/401096 mentions this issue: runtime/internal/syscall: use ABIInternal for Syscall6 on amd64

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 20, 2022

Change https://go.dev/cl/401337 mentions this issue: DO NOT SUBMIT: WIP: support inlining with //go:uintptrkeepalive

gopherbot pushed a commit that referenced this issue Apr 21, 2022
CL 388095 will change this file significantly. Move it preemptively to
ensure git tracks the move properly.

For #51087

Change-Id: I1408aecf8675723041b64e54cf44cdec38cc655c
Reviewed-on: https://go-review.googlesource.com/c/go/+/388094
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
This CL exports the existing ir.UintptrKeepAlive via the new directive
//go:uintptrkeepalive. This makes the compiler insert KeepAlives for
pointers converted to uintptr in calls, keeping them alive for the
duration of the call.

//go:uintptrkeepalive requires //go:nosplit, as stack growth can't
handle these arguments (it cannot know which are pointers). We currently
check this on the immediate function, but the actual restriction applies
to all transitive calls.

The existing //go:uintptrescapes is an extension of
//go:uintptrkeepalive which forces pointers to escape to the heap, thus
eliminating the stack growth issue.

This pragma is limited to the standard library.

For #51087

Change-Id: If9a19d484d3561b4219e5539b70c11a3cc09391e
Reviewed-on: https://go-review.googlesource.com/c/go/+/388095
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
Future CLs will be changing the provenance of these functions. Move the
declarations to the individual OS files now so that future CLs can
change only 1 OS at a time rather than changing all at once.

For #51087

Change-Id: I5e1bca71e670263d8c0faa586c1b6b4de1a114b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/388474
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
…mplementation on linux

For #51087

Change-Id: I75a1bdeb5089454595f5ca04765a9c6e45cf9bd5
Reviewed-on: https://go-review.googlesource.com/c/go/+/388475
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
For #51087

Change-Id: I63e07638507328efe33dbf7dd5f8a8b78890e037
Reviewed-on: https://go-review.googlesource.com/c/go/+/388476
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
For #51087

Change-Id: I9de7e85ccf137ae73662759382334bcbe7208150
Reviewed-on: https://go-review.googlesource.com/c/go/+/388477
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
For #51087

Change-Id: I4a5b5cb74f12db8999c6ff0e98c3034b58af3959
Reviewed-on: https://go-review.googlesource.com/c/go/+/388478
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
For #51087.

Change-Id: I25971760b63ec0d23d0f011521dd197d81a38976
Reviewed-on: https://go-review.googlesource.com/c/go/+/401096
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2022

Change https://go.dev/cl/401636 mentions this issue: Revert "syscall: define Syscall in terms of RawSyscall on linux"

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2022

Change https://go.dev/cl/401634 mentions this issue: Revert "runtime/internal/syscall: use ABIInternal for Syscall6 on amd64"

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2022

Change https://go.dev/cl/401635 mentions this issue: Revert "syscall: define Syscall6 in terms of RawSyscall6 on linux"

gopherbot pushed a commit that referenced this issue Apr 21, 2022
This reverts CL 401096. Grandparent CL 388477 breaks cmd/go
TestScript/cover_pkgall_runtime.

For #51087.
For #52472.

Change-Id: Ie82fe5f50975f66eb91fb0d01cd8bbbd0265eb4e
Reviewed-on: https://go-review.googlesource.com/c/go/+/401634
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
This reverts CL 388478. Parent CL 388477 breaks cmd/go
TestScript/cover_pkgall_runtime.

For #51087.
For #52472.

Change-Id: Id5d5a4e138792cf130ecdcc6b996c8102d142a7e
Reviewed-on: https://go-review.googlesource.com/c/go/+/401635
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2022
This reverts CL 388477, which breaks cmd/go
TestScript/cover_pkgall_runtime.

For #51087.
For #52472.

Change-Id: Id58af419a889281f15df2471c58fece011fcffbc
Reviewed-on: https://go-review.googlesource.com/c/go/+/401636
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2022

Change https://go.dev/cl/401654 mentions this issue: syscall: define Syscall in terms of RawSyscall on linux

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2022

Change https://go.dev/cl/401656 mentions this issue: runtime/internal/syscall: use ABIInternal for Syscall6 on amd64

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2022

Change https://go.dev/cl/401655 mentions this issue: syscall: define Syscall6 in terms of RawSyscall6 on linux

gopherbot pushed a commit that referenced this issue Apr 21, 2022
This is a re-do of CL 388477, fixing #52472.

It is unsafe to call syscall.RawSyscall from syscall.Syscall with
-coverpkg=all and -race. This is because:

1. Coverage adds a sync/atomic call in RawSyscall to increment the
   coverage counter.
2. Race mode instruments sync/atomic calls with TSAN runtime calls. TSAN
   eventually calls runtime.racecallbackfunc, which expects
   getg().m.p != 0, which is no longer true after entersyscall().

cmd/go actually avoids adding coverage instrumention to package runtime
in race mode entirely to avoid these kinds of problems. Rather than also
excluding all of syscall for this one function, work around by calling
RawSyscall6 instead, which avoids coverage instrumention both by being
written in assembly and in package runtime/*.

For #51087
Fixes #52472

Change-Id: Iaffd27df03753020c4716059a455d6ca7b62f347
Reviewed-on: https://go-review.googlesource.com/c/go/+/401654
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Apr 22, 2022
This is an exact copy of CL 388478 after fixing #52472 in CL 401654.

For #51087
For #52472

Change-Id: I6c6bd7ddcab1512c682e6b44f61c7bcde97f5c58
Reviewed-on: https://go-review.googlesource.com/c/go/+/401655
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@prattmic
Copy link
Member Author

@prattmic prattmic commented Apr 22, 2022

Everything I expect to get into 1.19 is in. Moving future work to 1.20.

@prattmic prattmic removed this from the Go1.19 milestone Apr 22, 2022
@prattmic prattmic added this to the Go1.20 milestone Apr 22, 2022
gopherbot pushed a commit that referenced this issue Apr 22, 2022
This is an exact copy of CL 401096 after fixing #52472 in CL 401654.

For #51087
For #52472

Change-Id: Ib3c914949a15517aacdca2d72e69e1c7b217e430
Reviewed-on: https://go-review.googlesource.com/c/go/+/401656
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 28, 2022

Change https://go.dev/cl/402914 mentions this issue: syscall: add //go:norace to RawSyscall

gopherbot pushed a commit that referenced this issue Apr 28, 2022
RawSyscall is used in a variety of rather unsafe conditions, such as
after fork in forkAndExecInChild1. Disable race instrumentation to avoid
calling TSAN in unsafe conditions.

For #51087

Change-Id: I47c35e6f0768c77ddab99010ea0404c45ad2f1da
Reviewed-on: https://go-review.googlesource.com/c/go/+/402914
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
Status: In Progress
Development

No branches or pull requests

6 participants
@prattmic @aclements @gopherbot @cherrymui @ii64 and others