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

*: remove instances of go:linkname throughout repository #128922

Open
rickystewart opened this issue Aug 13, 2024 · 5 comments
Open

*: remove instances of go:linkname throughout repository #128922

rickystewart opened this issue Aug 13, 2024 · 5 comments
Assignees
Labels
A-build-system C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-dev-inf

Comments

@rickystewart
Copy link
Collaborator

rickystewart commented Aug 13, 2024

With Go 1.23, the Go linker is much more restrictive about uses of go:linkname and will specifically disallow linking to internal standard library symbols. For now it seems we are safe to upgrade to Go 1.23 as they have surveyed major open-source repositories including cockroach to find instances of go:linkname and allow-listed already-linkname'd symbols. However, things are liable to change in the future and symbols may be removed from the allow-list. We recognize this usage of go:linkname is an anti-pattern that should be addressed.

Additionally, ~each of these is something that is liable to break in an upgrade and requires extra scrutiny on upgrades or else the code will behave unpredictably or unsafely. Removing these instances and replacing them with public, supported API's will minimize the amount of work that must be done with Go upgrades.

Some of these usages would be easiest to remove by up-streaming the relevant functionality into the Go toolchain (e.g. goschedstats)

pkg/sql/sem/builtins/pgcrypto_builtins.go
25:	_ "unsafe" // required to use go:linkname
36:	_ "golang.org/x/crypto/bcrypt" // linked to by go:linkname
423:// bcryptLinked accesses private method bcrypt.bcrypt by using go:linkname.
425://go:linkname bcryptLinked golang.org/x/crypto/bcrypt.bcrypt

pkg/testutils/bazelcodecover/code_cover_on.go
62://go:linkname runtime_addExitHook runtime.addExitHook

pkg/storage/slice_go1.9.go
18:// The go:linkname directives provides backdoor access to private functions in
23://go:linkname mallocgc runtime.mallocgc

pkg/util/goschedstats/runtime_go1.23.go
29:	_ "unsafe" // required by go:linkname
168://go:linkname allp runtime.allp
171://go:linkname sched runtime.sched
174://go:linkname lock runtime.lock
177://go:linkname unlock runtime.unlock

pkg/util/goschedstats/runtime_go1.20_21_22.go
21:	_ "unsafe" // required by go:linkname
160://go:linkname allp runtime.allp
163://go:linkname sched runtime.sched
166://go:linkname lock runtime.lock
169://go:linkname unlock runtime.unlock

pkg/util/goschedstats/runtime_go1.19.go
21:	_ "unsafe" // required by go:linkname
126://go:linkname allp runtime.allp
129://go:linkname sched runtime.sched
132://go:linkname lock runtime.lock
135://go:linkname unlock runtime.unlock

pkg/util/grunning/enabled.go
18:import _ "unsafe" // for go:linkname
23://go:linkname grunningnanos runtime.grunningnanos

pkg/util/randutil/rand.go
22:	_ "unsafe" // required by go:linkname
201://go:linkname FastUint32 runtime.fastrand

pkg/util/ctxutil/context.go
83://go:linkname context_cancelCtxKey context.cancelCtxKey

pkg/util/ctxutil/canceler_1_20.go
47://go:linkname context_removeChild context.removeChild
50://go:linkname context_propagateCancel context.propagateCancel

For now I'm assigning this to dev-inf for lack of a better owner.

Jira issue: CRDB-41282

@petermattis
Copy link
Collaborator

We’re already patching the Go runtime. Did we consider making a patch to add //go:linkname annotations on the symbol definitions in the Go runtime to allow our continued access. Some of these go:linkname uses are purely for performance (mallocgc), but others (e.g. goschedstats) seem to be very difficult to replace with another technique. The only other technique that immediately comes to mind is to add a Go runtime patch to export the symbols which is pretty much equivalent to adding //go:linkname annotations.

@rickystewart
Copy link
Collaborator Author

rickystewart commented Aug 30, 2024

@petermattis Yes, see the comment here.

The main problem with depending on our patched Go to circumvent the issue is that developers do not always use Bazel for builds. As an example, people frequently run builds and tests in GoLand for access to the debugger. Those builds will be broken as people generally use the upstream Go instead of our patched version. (This is not hypothetical and has already occurred, for example, here [internal link].)

Here are some options:

  1. Use our patched Go runtime to allow-list or otherwise sanction the use of //go:linkname for the use cases where we need it, and just let the build be broken for anyone using the upstream go. Provide installation instructions for our patched SDK so people use it instead of the version from upstream, brew, etc. (Kind of an undesirable option: at that point we are no longer really a Go project, but are a project that uses a bespoke Go fork/dialect.)
  2. Depend on -checklinkname=0 to disable the checks. Disabling these checks outright is not really in the spirit of the feature, which is supposed to be for debugging/experimentation. Theoretically there is nothing stopping us from just applying it globally to all our builds and asking everyone to do the same on their machines as well. However, I wouldn't be surprised if support for -checklinkname were dropped in some future release, at which point we'll be back where we started and will need to apply some other solution. A potential issue is that disabling all these checks allows more usages of go:linkname to creep in, although we could compensate for this with extra lint checks that will disallow new usages that haven't already been grandfathererd in.
  3. (Variation of (1)) Use our patched Go for this functionality, but if you're not building with bazel, then you get a "stub" version of the code that does not have all the functionality. This is similar to what we currently do with grunning. This is probably the most future-proof of these options, although we'll be dealing with the maintenance cost as our patched version of the code deviates from the upstream. (Changes to upstream seem likely as the Go team has indicated they are making these changes to allow them to refactor, rename, or otherwise change these internal symbols; if I recall correctly, at least one such refactor was already attempted but had to be reverted due to //go:linkname dependencies on the symbol in some library.)

We should consider that the //go:linkname annotations to allow-list symbols is probably also temporary and that allow-list functionality will be dropped at a certain point. At that point our fork and upstream will be diverging even further.

These options could work for some period of time. The lowest maintenance, most preferable, most future-proof way to do it would of course be to remove the usages of go:linkname. If certain uses are impossible to replace with another technique, then the main option we have would be to propose and get landed a public, sanctioned API that exposes the information we need from the runtime in a supported manner. We could apply one of the techniques described above as a temporary stopgap while we're waiting for that to shake out.

@rickystewart
Copy link
Collaborator Author

Option (3) as described above is fine and seems to me to be the best overall solution, but ideally we would not do only that and would concurrently explore landing a sanctioned API for doing what we need to do with the Go runtime. In that way we can focus on bringing our tech debt down over time instead of committing to it and allowing it to accumulate indefinitely.

Much of what's in goschedstats is a maintenance burden, requires extra scrutiny on Go upgrades, and forgoes a lot of Go's advantages with regard to memory safety. It would be a huge win to not have to worry about it any longer.

@petermattis
Copy link
Collaborator

Option (3) as described above is fine and seems to me to be the best overall solution, but ideally we would not do only that and would concurrently explore landing a sanctioned API for doing what we need to do with the Go runtime. In that way we can focus on bringing our tech debt down over time instead of committing to it and allowing it to accumulate indefinitely.

I won't stop you from trying to "land a sanctioned API for what we need in the Go runtime", but the time frame for getting such an API into the Go runtime is not the same time frame we operate on. I think we've taken the right approach with patches to the Go runtime which are unpalatable upstream. I don't consider this tech debt. I think it was a practical path forward.

Much of what's in goschedstats is a maintenance burden, requires extra scrutiny on Go upgrades, and forgoes a lot of Go's advantages with regard to memory safety. It would be a huge win to not have to worry about it any longer.

This introspection into the Go runtime internals is critical to the functionality of that package and I'm not aware of any other way we can get the necessary info. Ack that there is a maintenance burden, but I think it is relatively small. There is also a maintenance burden in every Go major version upgrade for https:/petermattis/goid. I'd classify that burden as small as well.

@rickystewart
Copy link
Collaborator Author

I won't stop you from trying to "land a sanctioned API for what we need in the Go runtime", but the time frame for getting such an API into the Go runtime is not the same time frame we operate on. I think we've taken the right approach with patches to the Go runtime which are unpalatable upstream. I don't consider this tech debt. I think it was a practical path forward.

Yes, that is a longer-term suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-system C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-dev-inf
Projects
None yet
Development

No branches or pull requests

2 participants