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

all: more thorough testing of GOAMD64=v1 instruction limits #63872

Open
rsc opened this issue Nov 1, 2023 · 2 comments
Open

all: more thorough testing of GOAMD64=v1 instruction limits #63872

rsc opened this issue Nov 1, 2023 · 2 comments
Labels
Builders x/build issues (builders, bots, dashboards) compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Nov 1, 2023

#63871, which found a PSHUFB (v2+) not guarded by any instruction set checks in a crypto/tls dependency, shows that we are not testing GOAMD64=v1 terribly well. That is, it is possible to use newer instructions in Go packages accidentally, without breaking any tests. If we are going to provide GOAMD64=v1 it seems like we should have a test that it works as advertised.

(To be clear, it is OK to emit these instructions when GOAMD64=v1, which is our default build mode, but only if they are guarded by an instruction set check before use. I don't see any such check in that case.)

I don't believe we have any v1 hardware to run on, and keeping access to that hardware would become progressively difficult in the future. I've heard a suggestion of using QEMU, but that's fraught with all sorts of problems. Instead I wonder if we should set up a stronger, builder version of cmd/compile/internal/amd64's TestGoAMD64v1. That test does the following:

  1. Replace any non-v1 instruction with a breakpoint instruction, found by disassembling the binary.
  2. Set GODEBUG environment variables to instruct code not to use post-v1 features.
  3. Run itself (its own test binary).

The stronger version would set some environment variable on a v1 builder that would cause the assembler to do two things:

  1. Replace all non-v1 instructions with breakpoints.
  2. Replace CPUID with a call to a simulation function that sets the registers to look like only v1 features are present.

This would apply during all of all.bash, not just during cmd/compile/internal/amd64.test. And it wouldn't depend on our getting the GODEBUG variables correct. We'd need a table of instruction->feature set for the assembler. We could set up the same for v2, v3.

Might be a good fixit week activity.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 1, 2023
@rsc rsc added this to the Backlog milestone Nov 1, 2023
@bcmills bcmills added compiler/runtime Issues related to the Go compiler and/or runtime. Builders x/build issues (builders, bots, dashboards) labels Nov 1, 2023
@randall77
Copy link
Contributor

We already have a test like that, in cmd/compile/internal/amd64/versions_test.go. It builds a binary with v1, clobbers all the non-v1 instructions in the binary with breakpoints, and then runs it.

To catch the example you found, the test binary would just need to invoke some crypto/tls operations. Currently the test binary doesn't exercise much other than the math and math/bits packages (and the test package).

@rsc
Copy link
Contributor Author

rsc commented Nov 1, 2023

Right, this would be a more complete version of that test (I mentioned it above). Instead of having to add new things to the specific test over time, it would just test everything. Being a separate builder would also end up testing all the x repos too - we have assembly in x/crypto and maybe others that can't be imported by cmd/compile/internal/amd64's test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests

4 participants