test: cover arm64 stdlib sysreg cross-target paths#12
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
=======================================
Coverage 69.48% 69.48%
=======================================
Files 57 57
Lines 16380 16380
=======================================
Hits 11382 11382
Misses 3683 3683
Partials 1315 1315
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the arm64 stdlib tests for runtime/sys and cpu to support cross-target compilation, which is a great improvement for testing. It also adds a new regression test for TranslateGoModule. The changes are well-aligned with the goal of improving test coverage for cross-compilation paths. My main feedback is to refactor duplicated code in the test functions to improve maintainability.
| tmp := t.TempDir() | ||
| llPath := filepath.Join(tmp, "dit-gomod.ll") | ||
| objPath := filepath.Join(tmp, "dit-gomod.o") | ||
| if err := os.WriteFile(llPath, []byte(tr.Module.String()), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| cmd := exec.Command(llc, "-mtriple="+triple, "-filetype=obj", llPath, "-o", objPath) | ||
| out, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| s := string(out) | ||
| if llcUnsupportedTarget(s) { | ||
| t.Skipf("llc does not support triple %q: %s", triple, strings.TrimSpace(s)) | ||
| } | ||
| t.Fatalf("llc failed: %v\n%s", err, s) | ||
| } |
There was a problem hiding this comment.
There's significant code duplication for compiling the generated LLVM IR. The logic for creating a temporary directory, writing the .ll file, executing llc, and handling errors is repeated in TestStdlibInternalRuntimeSys_ARM64_Compile, TestTranslateGoModule_StdlibInternalRuntimeSys_ARM64_Compile, and also in TestStdlibInternalCPU_ARM64_Compile in stdlib_cpu_arm64_test.go.
To improve maintainability and reduce redundancy, you could extract this logic into a shared helper function. For example:
func compileAndCheck(t *testing.T, llc, triple, llContent, name string) {
t.Helper()
tmp := t.TempDir()
llPath := filepath.Join(tmp, name+".ll")
objPath := filepath.Join(tmp, name+".o")
if err := os.WriteFile(llPath, []byte(llContent), 0644); err != nil {
t.Fatal(err)
}
cmd := exec.Command(llc, "-mtriple="+triple, "-filetype=obj", llPath, "-o", objPath)
out, err := cmd.CombinedOutput()
if err != nil {
s := string(out)
if llcUnsupportedTarget(s) {
t.Skipf("llc does not support triple %q: %s", triple, strings.TrimSpace(s))
}
t.Fatalf("llc failed: %v\n%s", err, s)
}
}You could add this helper to stdlib_runtime_sys_arm64_test.go and then call it from the test functions like this:
In TestStdlibInternalRuntimeSys_ARM64_Compile:
compileAndCheck(t, llc, triple, ll, "dit")
In TestTranslateGoModule_StdlibInternalRuntimeSys_ARM64_Compile:
compileAndCheck(t, llc, triple, tr.Module.String(), "dit-gomod")
This would make the tests cleaner and easier to maintain.
There was a problem hiding this comment.
Addressed in a7e4d58. I factored the repeated llc/object-build path into compileLLVMToObject in stdlib_arm64_test_helpers_test.go, and both runtime/sys tests plus TestStdlibInternalCPU_ARM64_Compile now call the shared helper. Re-ran the targeted arm64 tests and go test ./... after the refactor.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| goroot := testGOROOT(t) | ||
| src, err := os.ReadFile(filepath.Join(goroot, "src", "internal", "cpu", "cpu_arm64.s")) | ||
| if err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
Both TestStdlibInternalRuntimeSys_ARM64_Compile and TestTranslateGoModule_StdlibInternalRuntimeSys_ARM64_Compile (added in this same PR) guard os.ReadFile errors with an os.IsNotExist check and skip gracefully when the file is absent from the GOROOT. This test does not — a stripped or non-standard GOROOT that lacks cpu_arm64.s will hard-fail here rather than skip.
| t.Fatal(err) | |
| src, err := os.ReadFile(filepath.Join(goroot, "src", "internal", "cpu", "cpu_arm64.s")) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| t.Skip("internal/cpu/cpu_arm64.s not present in this GOROOT") | |
| } | |
| t.Fatal(err) | |
| } |
There was a problem hiding this comment.
Addressed in a7e4d58. TestStdlibInternalCPU_ARM64_Compile now mirrors the runtime/sys tests and skips cleanly on os.IsNotExist(err) for internal/cpu/cpu_arm64.s.
| } | ||
| } | ||
|
|
||
| func testGOROOT(t *testing.T) string { |
There was a problem hiding this comment.
testGOROOT and testGoEnv are defined here but are also called from stdlib_cpu_arm64_test.go. Every other cross-file test helper in this repo (mustGoPackage, testResolveSym, findLlcAndClang, etc.) lives in a dedicated helper file. Placing these in stdlib_runtime_sys_arm64_test.go creates an implicit dependency: if this file is ever renamed or its build tag changes, the CPU test silently breaks. Consider moving them to a shared file like stdlib_helpers_test.go.
There was a problem hiding this comment.
Addressed in a7e4d58. I moved testGOROOT / testGoEnv out of stdlib_runtime_sys_arm64_test.go into the new shared helper file stdlib_arm64_test_helpers_test.go, alongside the shared llc compile helper.
| return goroot | ||
| } | ||
|
|
||
| func testGoEnv(t *testing.T, key string) string { |
There was a problem hiding this comment.
If the go binary is not on PATH (e.g. a minimal cross-compile container), exec.Command will error and t.Fatalf fires — the test hard-fails rather than skips. Meanwhile, the t.Skip("GOROOT not available") guard in testGOROOT (line 143) becomes unreachable, because testGoEnv either returns a non-empty string or stops the test via Fatalf before returning "".
Consider propagating the error to testGOROOT and converting it to a t.Skip there, so the intent of "skip when toolchain discovery fails" is actually reachable:
func testGoEnv(t *testing.T, key string) (string, error) {
t.Helper()
out, err := exec.Command("go", "env", key).CombinedOutput()
if err != nil {
return "", fmt.Errorf("go env %s: %w\n%s", key, err, out)
}
return strings.TrimSpace(string(out)), nil
}There was a problem hiding this comment.
Addressed in a7e4d58. testGoEnv now returns (string, error) instead of calling t.Fatalf, and testGOROOT turns toolchain-discovery failure into a skip. That makes the GOROOT not available path reachable on minimal environments where go is missing from PATH.
|
Good change overall — removing the host-arch guard and hardcoding the cross-compile triple is the right approach, and the graceful |
|
Addressed the review feedback in
Re-verified with:
|
1 participant
Summary
runtime/sysandcpucompile tests as true cross-targetaarch64checks instead of skipping on non-arm64 hostsTranslateGoModuleregression forinternal/runtime/sys/dit_arm64.s, which is the public entry llgo actually callsllclacks the requested targetWhy previous tests missed this
TestStdlibInternalRuntimeSys_ARM64_CompileandTestStdlibInternalCPU_ARM64_Compileonly ran whenruntime.GOARCH == "arm64"runtime/syscoverage only exercised low-levelTranslate, while llgo hitsTranslateGoModuledit_arm64.sregressionValidation
go test -run 'TestStdlibInternal(RuntimeSys|CPU)_ARM64_Compile|TestTranslateGoModule_StdlibInternalRuntimeSys_ARM64_Compile' ./...go test ./...