vendor: review inclusion of x/sys/cpu

[mundaym]

@rsc said in #24843 (comment):

• for code vendored from x/crypto into standard library, change it during import to use internal/cpu, not x/sys/cpu
• x/sys must not be vendored into the main tree

However it looks like it has now been vendored into std as of CL 164623.

My only real concern with this is that AFAICT x/sys/cpu doesn't contain the same options as internal/cpu to manually enable/disable CPU features using an environment variable.

Thoughts?

[mundaym]

@bcmills

[bcmills]

Yeah, it was vendored in CL 164623 because the decision about not vendoring it wasn't recorded anywhere visible in the source tree.

I don't see much discussion on #24843 about the underlying reasoning for that prohibition. The only comments that seem to even mention a concern are #24843 (comment) and #24843 (comment).

[bcmills]

@martisch's concrete concern there was:

[…] the runtime was recently changed to use internal/cpu and init is not used anymore for initialization but runtime calls a custom init function early before even map code is initialized to init internal/cpu. So there would need to be some new code around initializing if triggered by runtime and some triggered through normal init chain if in x/*/cpu. Would seem better to me then to keep runtime cpu feature flag detection separate again and just initialize internal/cpu through normal init when used in std lib or x/*/cpu such that there is one common clean initialization path.

If the decision is made to vendor in x/sys/cpu then initializing it right before alginit in runtime like it currently does is afaik the right place as initializing the map code is the first place where it permanently matters and still before any other more complex std lib code. There could be a init variable that is also read by the normal init to test if the package already had been initialized by the runtime.

Some architectures (arm) need to query information from the OS to initialize the flags and not having the init too early and when env variables can be already read will allow for masking flags like cl/91737 e.g. for testing different code paths. […]

[martisch]

I dont think we want to vendor in x/sys/cpu (which also pulls in x/sys/unix).
As noted above the decision was to replace x/sys/cpu with internal/cpu when adding/updating code in std/lib and therefore not much work has done to keep them in sync in all aspects.

My concerns would for keeping x/sys/cpu would be:

1. x/sys/cpu does not support GODEBUG (and therefore we can not reliably turn off usage of cpu features anymore in std lib e.g. for debugging and testing) if we keep x/sys/cpu in :(
2. x/sys/cpu does not support all the archs/OS combinations (e.g. freebsd/arm and freebsd/arm64) that internal/cpu supports through having access to auxv information from the runtime. Which AFAIK means that if both are in use we will have parts of the std lib assume a cpu feature is present while other parts assume it is not. There are other cases were runtime set cpu features in a way which x/sys/cpu doesnt. Not sure if relevant yet (https://go-review.googlesource.com/c/go/+/147377).
3. bloat - we have two implementations in the go binaries and the x/sys/cpu supports flags that are not used anywhere in the std lib. For internal/cpu we explicitly kept the set small to used cpu features or soon to be used cpu features. Apart from binary size this keeps needed cache lines small when checking feature flags.

Im happy to put in the work to rewrite all uses of x/sys/cpu to internal/cpu so we dont need to ship x/sys/cpu with go1.13.

For go1.14 we might want to have rewrite tooling or decide to make internal/cpu be a public cpu package in the std/lib so it can just be used by the vendored in packages even outside std/lib.

[gopherbot]

Change https://golang.org/cl/177897 mentions this issue: all: remove x/sys/cpu and replace with internal/cpu

[martisch]

With modules in place it does not seem we can just modify the copies in the vendor directories.

What options do we have within the freeze to not include x/sys/unix and x/sys/cpu and run two different cpu packages with differing detection capabilities and features (GODEBUG) in go1.13 std lib?

@randall77 @rsc

[bradfitz]

Can we instead drop internal/cpu and just use golang.org/x/sys/cpu?

[martisch]

If the vendoring in process as originally planed and done pre go1.13 can not be adjusted anymore: I would prefer exposing a cpu package which carefully exposes values from internal/cpu instead of using golang.org/x/sys/cpu.

Its technically possible to use golang.org/x/sys/cpu with the following work likely needed:

• some build tag magic such that x/sys/cpu
  -- does not have dependencies like x/sys/unix or io/util
  -- does not initialize through runtime and then again (even with different values) through init
• support GODEBUG in x/sys/cpu (both when getting env from runtime and getting env without runtime support)
• replace uses in go code and std lib assembly from internal/cpu to vendor/golang.org/x/sys/cpu

This will leave some not so nice properties:

• x/sys/cpu supports more features than is needed by std lib (could possibly be mitigated again by build tags)
• there will be 3 different code parts of x/sys/cpu. One thats only exercised when in std lib, one thats common to both and one thats only used outside std lib. Which seems to make it really two or three packages with a common interface.
• for such a tightly coupled code with runtime/std lib the development will be rather independent. Until x/sys/cpu is vendored in and tested with std lib trybots the runtime integration is likely not often tested.
• x/sys/cpu will support detecting one set of os/arch combination while outside std lib and another while inside std lib making testing even more complicated
• binaries importing x/sys/cpu directly will contain two different versions of x/sys/cpu (one directly and one from vendoring in). This could get confusing with e.g. GODEBUG and one reporting the feature to be not detected or known while the other does.

[martisch]

Possible "low risk" alternative for go1.13. Expose a new cpu package from std/lib that only exposes the cpu options needed for vendored in code. These are:

S390X.HasVX
S390X.HasVXE
X86.HasSSSE3
X86.HasAVX2
X86.HasBMI2

internal/cpu can stay as is for go1.13 and no other runtime/std lib changes needed.
The cpu package through normal init can copy the values from internal/cpu that were already initialized through runtime. This isolates runtime impact if values in cpu are accidentally written too. That leaves x/crypto needing to be changed to use a build tag and switch to using std lib cpu from go1.13 on. If x/crypto wants to use a new cpu feature detection than first x/sys/cpu can be used and then for the next possible go release can be switched over to use cpu with one more exposed variable. Wether internal/cpu will be completely exposed as package cpu is left as an option for the future.

[bcmills]

(CC @FiloSottile)

[bcmills]

An even-lower-risk option for Go 1.13 is to leave x/sys/cpu and internal/cpu as separate packages (and accept the redundancy between them), and have a plan to unify them (or at least factor out the common subset) for 1.14.

[rsc]

We need to make the standard library NOT depend on x/sys/cpu. Whatever link exists needs to be removed.

[rsc]

It looks like the deps are through poly1305 and chacha20poly1305. At least for Go 1.13, we can have them do init-time hw feature checks on their own, like everyone did before x/sys/cpu existed.

[rsc]

FWIW my main objection to x/sys/cpu is that it drags in x/sys/unix. That's the real red line from my point of view. If we can make x/sys/cpu NOT depend on x/sys/unix, then it might be OK to live with x/sys/cpu being vendored.

[bradfitz]

Russ, is there a technical reason std shouldn't depend on x/sys?

[bradfitz]

(Replies crossed in flight.)

[bradfitz]

What if std stopped using the syscall package and only used x/sys/unix?

[gopherbot]

Change https://golang.org/cl/179178 mentions this issue: cpu: don't depend on the golang.org/x/sys/unix package for AIX

[gopherbot]

Change https://golang.org/cl/179180 mentions this issue: src/vendor: update golang.org/x/sys to remove x/sys/unix dep