`go:linkname` (de)stabilization

[vmg]

Fresh off the press, the upcoming Go 1.23 release will ship with some annoying changes to the linker.

Here's the full issue: https://github.com/golang/go/issues/67401

As a summary, the Go authors are concerned by the prevalent use of go:linkname throughout the Go ecosystem to access the internals of the runtime. They intend to hamper the usage of this feature in the upcoming release by adding a strict check, so that only runtime symbols that have been explicitly exported inside the runtime can be accessed externally via go:linkname. To prevent further breakage of the ecosystem, they've scanned the top N most popular Go packages and whitelisted their go:linkname symbol dependencies so they can continue to be linked externally for the foreseeable future.

Unfortunately for us, Vitess makes some mild use of go:linkname and it's not in the top N packages that have been scanned: although Vitess is plenty popular as an OSS project, their scan measures popularity by number of dependant packages. As a "top level" application, there's very few projects that have vitessio/vitess as a go.mod dependency. So we're screwed this way.

On the other hand, as a top level package, we have access to a new flag in ldflags that allows us (and will always allow, as it'll become part of the Go compatibility guarantees) to disable the whitelisted export checks and access the Go internals indiscriminately. We can always choose to build Vitess with this flag, and we may need to do so in the short term as soon as we upgrade to Go 1.23.

Regardless, I think it'd be a good idea to create a tracking issue to enumerate all our go:linkname dependencies and eventually remove all of them until we can compile Vitess with the strict linker checks in Go 1.23.

• [ ] //go:linkname strhash runtime.strhash (hack/runtime.go): this function is now exported in the new hash packages in the stdlib so there’s no reason to continue to link to it this way.
• [ ] //go:linkname roundupsize runtime.roundupsize (hack/runtime.go): this function is kinda important and has not been whitelisted in Go 1.23 for external usage. We use it as part of our caching infrastructure to calculate very accurate measurements for how much memory do cached objects occupy. The sizeof of a Go struct is not the actual amount of memory it occupies, because all structs are rounded up to the GC’s allocation sizes; by accessing this internal method, we can calculate the exact memory usage of each allocation in the cache. There are two options here:
  • copy this code into Vitess and vendor it; it’s not that much code, and it hasn’t changed since Go 1.0, so that’s always an option
  • just stop trying to calculate memory usage accurately and assume Sizeof(OUR STRUCT) is the size it’ll occupy in memory. This will under-report memory usage, but how much of a big deal is it?
• [ ] //go:linkname Atof64 strconv.atof64, //go:linkname Atof32 strconv.atof32 (hack/runtime.go): these are strconv methods that parse a floating point value and return how many characters were parsed. The public methods exported in the strconv package will error out if there’s any trailing data after the floating point value, but this is not MySQL’s behavior, so we need access to this internal method. This doesn’t look like it’ll be whitelisted, but it’s very little code: we should vendor it in the parse package. Upon further review, these methods implicitly support parsing hexadecimal floating point numbers, something which MySQL does not, so we might as well vendor them and remove this extra functionality that doesn’t match MySQL’s.
• [ ] //go:linkname ParseFloatPrefix strconv.parseFloatPrefix (go/hack/compat.go): this is not used anywhere, we get to just remove it :sparkles:
• [ ] //go:linkname writeBarrier runtime.writeBarrier, //go:linkname atomicwb runtime.atomicwb (atomic2/atomic128.go): these methods are used to implement our own 128-bit atomic integers. They’ve actually been whitelisted already, so we can depend on them indefinitely. Furthermore, we’ll only need to keep this implementation temporarily, as 128 bit atomics have been approved for an upcoming Go release, they’re just missing an implementation. As soon as they’re in the standard library we can remove our implementation.
• [ ] //go:linkname sync_runtime_Semacquire sync.runtime_Semacquire, //go:linkname sync_runtime_Semrelease sync.runtime_Semrelease (smartconnpool/sema_norace.go): these are an important but not crucial performance optimization for our connection pool implementation. The methods appear to be whitelisted in the Go standard library, but they use a specific linkage name, so it’s not clear to me whether they can be accessed by Vitess or only by the explicit packages where they’re exported. Regardless, the usage of semaphores can be replaced with sync.WaitGroup, which is slightly less efficient.
• [ ] //go:linkname DisableProtoBufRandomness google.golang.org/protobuf/internal/detrand.Disable (hack/detrand.go): Oh boy. You can see the story behind this linkname in the file where’s it’s created:
  • https://github.com/vitessio/vitess/blob/3773563eab7f82b6af6c59c15530494bb900bb95/go/hack/detrand.go#L23-L41
  • This will never be whitelisted because it’s not part of the standard library. There are two options here:
    • [ ] Have one of our interns finish https://github.com/vitessio/vitess/issues/8162, which is a lot of busywork but it’s not hard at all to do.
    • [ ] Have a soft fork of the protobuf package that changes one line of code, which is annoying but also very little work.

[dbussink]

• Have a soft fork of the protobuf package that changes one line of code, which is annoying but also very little work.

This usually ends up being a maintenance nightmare though. We'd have to update manually on each protobuf release and not forget about it (but that will then definitely happen). So I'd be strongly in favor of not having to do this tbh.

[stefanb]

Tnx, this also surfaced with the testing of recently released Go 1.23 rc1 in:

• https://github.com/Homebrew/homebrew-core/pull/175310

[dbussink]

Tnx, this also surfaced with the testing of recently released Go 1.23 rc1 in:

Fwiw, this is not going to be in a Vitess release soon yet (v21 is still a bit out, v20 is due very shortly). So Homebrew would either use our pre-built release binaries, build with Go 1.22 (since that's also what is supported to build with atm) or carry their own patches for this.

[stefanb]

Those homebrew formulae that will still be failing at the time of Go 1.23 release will likely be pinned to Go 1.22 for building until they are made compatible with the current (then) stable Go release.

[dbussink]

Those homebrew formulae that will still be failing at the time of Go 1.23 release will likely be pinned to Go 1.22 for building until they are made compatible with the current (then) stable Go release.

Fwiw, I fully expect that we will build Vitess with -ldflags=-checklinkname=0 in the future as there's still remaining usage that we don't have any way yet of resolving in any way.

• Have one of our interns finish Remove Golden Tests from the test suite #8162, which is a lot of busywork but it’s not hard at all to do.

@vmg I'm realizing that this is not actually enough. The issue is not just about testing. We disable the randomization also for JSON marshaling. I think that randomized field ordering and random spaces injected into vschemas that end-users interact with is not really an acceptable solution.

• Have a soft fork of the protobuf package that changes one line of code, which is annoying but also very little work.

It's either this, or we keep basically -ldflags=-checklinkname=0 forever.

[rsc]

FWIW, for now at least the linkname restriction only applies to names in the standard library. You can linkname DisableProtoBufRandomness without any special flags. Whether you should is a different question, of course. I recently hit the same problem and while I did use a linkname for about five minutes, I decided in the end to run the JSON through json.Compact in my tests instead.

[lamados]

fwiw, at work we're currently using vitess.io/vitess/go/vt/sqlparser which depends on that usage of runtime.roundupsize in hack/runtime.go, so we're having to pin our builds to go1.22 until that issue's fixed too. so much for the go 1 promise of compatibility i guess

[dbussink]

wiw, at work we're currently using vitess.io/vitess/go/vt/sqlparser which depends on that usage of runtime.roundupsize in hack/runtime.go, so we're having to pin our builds to go1.22 until that issue's fixed too. so much for the go 1 promise of compatibility i guess

This usage is removed in https://github.com/vitessio/vitess/pull/16016. You can update to latest main for the parser which removes this usage. You can also build with -ldflags=-checklinkname=0 to allow it still.

[seveas]

Will https://github.com/vitessio/vitess/pull/16016 be backported to the current release branch, or do we need to wait until the next major release?

[dbussink]

Will #16016 be backported to the current release branch, or do we need to wait until the next major release?

@seveas It won't be backported. We also don't backport Go major upgrades normally, so it's not necessary for that. It will be in the next major release though which also will use Go 1.23. Existing releases will keep using the Go version that they are on.

[seveas]

Ack, we temporarily switched one of our projects' go.mod to use the main branch for now and will change back to released versions when the next major version is out. Other projects are using the ldflags workaround, both approaches allow us to upgrade our projects to go 1.23