x/sys/cpu: respect CPU feature overrides specified in GODEBUG

[Lekensteyn]

golang.org/x/sys/cpu does not allow CPU feature detection to be overridden, unlike internal/cpu. This results in surprising behavior while debugging software that do not use internal/cpu but the external golang.org/x/sys/cpu package.

@martisch already mentioned this difference in #32102 (comment).

The use case was forcing qtls in https://github.com/lucas-clemente/quic-go to use ChaCha20-Poly1305 instead of AES128-GCM in TLS 1.3 for debugging purposes.

What version of Go are you using (go version)?

$ go version
go version go1.12.9 linux/amd64

Does this issue reproduce with the latest release?

Yes, reproduced with golang/sys@5fe476d.

What did you do?

Create detect.go:

package main

import (
	"golang.org/x/sys/cpu"
)

func main() {
	print("cpu.X86.HasAES: ", cpu.X86.HasAES, "\n")
}

Run with:

./detect
GODEBUG=cpu.all=off ./detect
# demonstrate that GODEBUG is interpreted
GODEBUG=cpu.allx=off ./detect

What did you expect to see?

cpu.X86.HasAES: true
cpu.X86.HasAES: false
GODEBUG: unknown cpu feature "allx"
cpu.X86.HasAES: true

What did you see instead?

cpu.X86.HasAES: true
cpu.X86.HasAES: true
GODEBUG: unknown cpu feature "allx"
cpu.X86.HasAES: true

[martisch]

I think we should port over all CPU feature masking options that internal/cpu supports. Individual feature settings and masking all features. This will also help packages imported into the standard library that use x/sys/cpu (see #32102) to be aligned with what internal/cpu will report.

Adding others to discuss if it is ok for x/ packages to interpret GODEBUG.
/cc @randall77 @bradfitz @bcmills @aclements

[bradfitz]

Remind me why we have both internal/cpu and golang.org/x/sys/cpu? It'd sure be nice if we only had one (x/sys/cpu) and the Go runtime could just use it.

[bcmills]

To the extent that x/sys/cpu and internal/cpu remain separate, I agree that x/sys/cpu should support all of the same GODEBUG options as internal/cpu. Perhaps there should be a GODEBUG parser somewhere in one of the core x/ repos?

I also agree with @bradfitz that ideally in the long term we should find some way to consolidate on only x/sys/cpu.

[randall77]

runtime needs to import internal/cpu, so it's not trivial to use x/sys/cpu as a drop-in replacement. For example, x/sys/cpu imports encoding/binary, which imports reflect, which imports runtime.

I think there is a lot of scope to share an implementation (or large parts thereof), but it isn't trivial.

[bradfitz]

The encoding/binary dependency is not necessary.

And the only use of runtime is to look at runtime.GOARCH which we could instead inject during init and hide that file using build tags when vendoring the module into std.

It seems like we could make golang.org/x/sys/cpu a leaf package and drop internal/cpu.

[martisch]

Other complications for why runtime does not use x/sys/cpu:

• some constructs like maps can not be used anywhere as those parts of the runtime are not initialised yet when internal/cpu runs code.
• internal/cpu supports more architectures/platforms as it is has access to non public runtime internals like auxv information to derive hwcap bits which x/sys/cpu can not use directly unless we expose a runtime API or update unsafe coupling with any related runtime change.
• historical: x/sys/cpu was forked from internal/cpu because it was not public.

Let me ask the other way around when we already think about sharing a common implementation:
Why cant we just make internal/cpu public as package cpu in standard lib?

[bradfitz]

Why cant we just make internal/cpu public as package cpu in standard lib?

We've tried to stop adding new packages to std, preferring everything to be public where it can have its own release cadence. (e.g. fix bugs today, don't wait 6 months.... https://golang.org/doc/faq#x_in_std)

[martisch]

We've tried to stop adding new packages to std, preferring everything to be public where it can have its own release cadence. (e.g. fix bugs today, don't wait 6 months.... https://golang.org/doc/faq#x_in_std)

I am aware of that but I think that point will lose some of its appeal when we hide the coupling with runtime internals under the hood with build tags and unsafe. We also have exposed other std lib/runtime functions in new std lib packages lately like bytes/hash that are coupled to the runtime.

If we will have a serious bug in CPU feature detection I would want to fix that for the runtime in a point release too instead of waiting 6 months.

[bradfitz]

I sent https://go-review.googlesource.com/c/sys/+/194646 to remove the encoding/binary dependency at least.

We don't use maps as-is. Maybe that complicates the GODEBUG parsing, but we have the code already without it, and it's not particularly performance sensitive.

For hwcap, perhaps a new //go: directive before the variables? Today we have:

// These are initialized by archauxv in runtime/os_linux_arm64.go. 
var HWCap uint
var HWCap2 uint

Those could be:

//go:hwcap1
var HWCap uint
//go:hwcap2
var HWCap2 uint

And any package (or one package under different versions) could all get it initialized.

[ianlancetaylor]

Probably many people know this, but some background.

Right now the HWCap variables in internal/cpu are initialized by rt0_go => args => sysargs => (on GNU/Linux and FreeBSD) sysauxv => archauxv (on AIX they are initialized by rt0_go => osinit => setupSystemConf). The additional variables in internal/cpu are initialized by cpu.Initialize, which is called by rt0_go => schedinit => cpuinit. It is cpuinit that fetches the GODEBUG environment variable,and cpu.Initialize that parses it. This means, and this is a key point, that the internal/cpu variables are initialized before package initialization is run, and in particular before alginit is done, and therefore before any maps are created.

Any attempt to replace internal/cpu with x/sys/cpu needs to preserve that property.

If we vendor x/sys/cpu, and update the initialization so that it works with the vendored x/sys/cpu replacing internal/cpu, then the initialization will only affect the vendored copy. It won't affect user imports of golang.org/x/sys/cpu.

Using //go:hwcap comments is an interesting idea but we also need to run cpu.Initialize before package initialization, and that does not seem appropriate for a magic comment.

One approach might be

1. Change internal/cpu to not rely on variables being initialized by the runtime package. Instead, pass those variables to cpu.Initialize. The values are CPU-specific but the types are not.
2. Move internal/cpu to golang.org/x/sys/cpu/internal/details.
3. Vendor golang.org/x/sys/cpu/internal/details into the standard library, and use it instead of internal/cpu.
4. Change x/sys/cpu to refer to x/sys/cpu/internal/details with declarations like var X86 = details.X86.
5. Change x/sys/cpu to add an init function that reads the hwcap values however it likes, and the GODEBUG variable, and calls details.Initialize with them.

At first glance I think that handles everything except that the godoc for x/sys/cpu might be bad.

[tmthrgd]

Just a drive by comment @ianlancetaylor.

4. Change x/sys/cpu to refer to x/sys/cpu/internal/details with declarations like var X86 = details.X86.

Instead of this, which as you note would make the documentation confusing, what if x/sys/cpu just redeclared all the var X86 struct{ ... }, as it does now, and copied them from x/sys/cpu/internal/details in init. That could be done field by field or perhaps using unsafe.

Something like:

package cpu

import "golang.org/x/sys/cpu/internal/details"

var X86 struct{
	HasAES bool // AES hardware implementation (AES NI)
	// ...
}

func init() {
	X86.HasAES = details.X86.HasAES
	// ...
}

That way the godoc is exactly the same as it is now. The struct declarations and the copying could even be automated with go generate to keep them in sync

[ianlancetaylor]

Sure, we could do that, but it's less than ideal because then we have two copies of all the types. Let's see if we can figure out a better way.

[bcmills]

If we vendor x/sys/cpu, and update the initialization so that it works with the vendored x/sys/cpu replacing internal/cpu, then the initialization will only affect the vendored copy. It won't affect user imports of golang.org/x/sys/cpu.

There is an interesting interplay with #30241.

If x/sys/cpu (or x/sys/cpu/internal/details) is closely tied to the internals of the runtime, then we either cannot allow users to upgrade it arbitrarily — and therefore cannot unify the user-upgraded copy with the std-vendored copy — or will need to maintain build-constrained variants matching each different runtime hook.

[bcmills]

Using //go:hwcap comments is an interesting idea but we also need to run cpu.Initialize before package initialization, and that does not seem appropriate for a magic comment.

I'm probably missing something, but couldn't we use a //go:linkname directive to patch in the Initialize function?

[bcmills]

Vendor golang.org/x/sys/cpu/internal/details into the standard library, and use it instead of internal/cpu.

Note that the standard library does not in general have access to internal packages within vendored modules. It would be possible to give it such access, but that would make it more likely that an otherwise-non-breaking change within a module would break the standard library.

[bcmills]

So here is one possible alternative.

1. Add the //go:hwcap directives as described in x/sys/cpu: respect CPU feature overrides specified in GODEBUG #33963 (comment), or use a //go:linkname directive to deduplicate the HWCap variables within the program.
2. Use the same technique — //go:linkname or another directive — to unify a var godebug string within golang.org/x/sys/cpu with a variable populated by the runtime.
3. Add a func Initialize() to golang.org/x/sys/cpu. Have it read from the godebug variable, and ensure that it does not use maps or otherwise rely on the usual package initialization. Make Initialize() a no-op if cpu.Initialized is already true.
4. Make golang.org/x/sys/cpu.init invoke its own Initialize function.
5. Make cpuinit also invoke golang.org/x/sys/cpu.Initialize. Since cpuinit is within the standard library, for now that will resolve to vendor/golang.org/x/sys/cpu.Initialize.

Then the startup sequence becomes, roughly:

1. sysargs or archauxv initializes the HWCap variables.
2. cpuinit initializes the godebug variable, which is aliased between the golang.org/x/sys/cpu and vendor/golang.org/x/sys/cpu packages.
3. cpuinit invokes vendor/golang.org/x/sys/cpu.Initialize.
4. The runtime finishes initializing and begins package initialization.
5. Package initialization runs vendor/golang.org/x/sys/cpu.Initialize again, but it is a no-op (due to step 3).
6. If golang.org/x/sys/cpu is imported in user code, package initialization runs golang.org/x/sys/cpu.Initialize, which re-parses the shared godebug variable.

If we eventually unify vendor/golang.org/x/sys/cpu with golang.org/x/sys/cpu as proposed in #30241, the only change is that step (6) no longer occurs.

[martisch]

Would there be a problem later with the outlined procedure when user code or std lib needs to initialize two different major versions of x/sys/cpu?

When HWcap3 or similar comes along that needs to be sourced from runtime I assume we would need build tags on the initialization or variables linknamed in x/sys/cpu?

[bcmills]

Would there be a problem later with the outlined procedure when user code or std lib needs to initialize two different major versions of x/sys/cpu?

I don't think so? If I understand linking correctly(‽), the //go:hwcap and/or //go:linkname directives should deduplicate all of the copies of each variable across all major versions to the same underlying variable, so initializing each variable once within the runtime would cover all applicable versions.

For simplicity, presumably the standard library should depend on only one major version of x/sys/cpu, so it would still only need to call the Initialize function for the one version it cares about.

When HWcap3 or similar comes along that needs to be sourced from runtime I assume we would need build tags on the initialization or variables linknamed in x/sys/cpu?

Yes, that seems correct.

[martisch]

I was thinking about initialization function problems with different major versions. I guess there is no problem because runtime only needs to initialize the major version that it itself uses.

[ianlancetaylor]

I'm nervous about trying to use go:linkname to unify variables in a vendored package.

I think we clearly want a user program to be able to import its own specific version of golang.org/x/sys/cpu without anything breaking if the standard library is using a different (vendored) version of golang.org/x/sys/cpu. It's not immediately obvious to me how to do that if we are using special directives.

If we're willing to use a build tag to build the vendored version of golang.org/x/sys/cpu, then I think we can adopt my earlier suggestion without the cpu/internal/details packages, while retaining a usable godoc and without relying on any special linker directives for vendored packages.

[bcmills]

I think we clearly want a user program to be able to import its own specific version of golang.org/x/sys/cpu without anything breaking if the standard library is using a different (vendored) version of golang.org/x/sys/cpu.

I don't think that's incompatible with the go:linkname approach, provided that the two copies of golang.org/x/sys/cpu don't attempt to mutate the linkname-aliased variables.

(That may imply that the aliased variables need to be unexported, but if need be we can use an ordinary init function to copy unexported variables to the exported ones at package-init time.)

[bcmills]

If we're willing to use a build tag to build the vendored version of golang.org/x/sys/cpu,

I would really rather not do that if we can avoid it — a dependency on path-specific build tags would add a lot of complexity if we eventually allow vendor/golang.org/x/sys/cpu to be merged with golang.org/x/sys/cpu in a later Go release.

[bcmills]

Actually, I don't think we need //go:linkname at all. We could add a minimal package to the standard library — perhaps runtime/cpu? — that exports only the runtime-initialized HWCap and GODEBUG variables, using accessor functions (which could be made intrinsics if need be) to ensure that they are read-only.

Then we vendor in golang.org/x/sys/cpu, and have golang.org/x/sys/cpu.Initialize read from those variables and initialize the rest of the package-level state as described in #33963 (comment).

[ianlancetaylor]

I don't see how we can usefully merge vendor/golang.org/x/sys/cpu with golang.org/x/sys/cpu, given that in the normal case they will be different versions of the package. And it's a fairly small package. Is it worth doing the work to merge the two packages in the special case where they are exactly the same version?

I do think that a runtime/cpu package would work, if we decide that it's OK to add that to the standard library API.

[bradfitz]

Given how special this package is (effectively part of the runtime already), I'd be fine with exporting it as runtime/cpu. I'm not super excited about it, but I recognize it as the easiest thing to do, and it's not terrible. New CPU bits shouldn't need to be added as often as new Linux system calls, so locking ourselves down to twice-a-year updates seems acceptable.

[bcmills]

I don't see how we can usefully merge vendor/golang.org/x/sys/cpu with golang.org/x/sys/cpu, given that in the normal case they will be different versions of the package.

If we end up doing #30241, then the version of golang.org/x/sys/cpu used by the runtime would be upgraded to the version implied by the user's go.mod file (or vice-versa). So we would have only one copy, but it would potentially be newer than what the toolchain shipped with.

[ianlancetaylor]

@bcmills Given that the package is tightly tied to the runtime, I don't see how that could work in general. It would become impossible for us to significantly change the package in any way; if we did we would break people using different versions of the external package.

[bcmills]

@ianlancetaylor, as long as the runtime ABI only changes at major releases, x/sys/cpu could use build constraints of the form +build go1.(X+1),!go1.X to select the code for the corresponding runtime.

[ianlancetaylor]

It seems to me that the likelihood of subtle problems from doing this outweighs any possible benefit.

[martisch]

@PolliOs has a prototype to support GODEBUG for x/sys/cpu which we will send for review after a bit more testing and refinement.

[gopherbot]

Change https://golang.org/cl/245237 mentions this issue: cpu: add GODEBUG options to disable use of instruction set extensions