Superseded: validate deadcode-drop test binaries - #40
Conversation
There was a problem hiding this comment.
Review summary
This is a focused, well-structured change. The new validateTestBinary smoke-check is cleanly guarded (opt-in via ValidateTestBinary, skipped for BuildMode="build" and cross-GOOS/GOARCH targets), timeout handling is correct (DeadlineExceeded checked before the generic error branch), and it has good test coverage exercising both the clean-startup and startup-failure paths plus the build-mode skip. The -build-only non-zero exit path makes validation useful in size CI. Documentation and toml/README comments were cross-checked against suites.toml and the code and are accurate.
A few points worth considering below.
Not placed inline (no diff line):
-
Missing README documentation for
ValidateTestBinary—cmd/bent/README.md. Every other configuration field (Compiler,OmitVetFlag,UseBuildCache, etc.) is documented in the sample configuration block (~lines 113-138), but the newValidateTestBinaryoption is not. Since its behavior differs by build mode, adding it there (and noting it is ignored forBuildMode="build") would help users. -
Validation env/dir differs from the real run —
validateTestBinaryruns inbench.BuildDir()withcmdEnv(build-time env), whereasbenchOneruns inRunDir()with a different env (addsBENT_DIR/BENT_PROFILES/RunEnv, omitsGcEnv). This is acceptable for a "does the pruned binary start" smoke check with empty selections, but a one-line comment noting the divergence would help future readers who might assume it mirrors the real run.
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
| cmd := exec.CommandContext(ctx, binary, "-test.run=^$", "-test.bench=^$") |
There was a problem hiding this comment.
[P2] Validation bypasses the Docker sandbox for sandboxed benchmarks
For benchmarks with NotSandboxed == false (the default), the normal run path executes the built test binary inside docker run --net=none ... (bent.go:1277) precisely because these are third-party benchmark binaries fetched and built by the harness.
validateTestBinary executes that same freshly-built binary directly on the host with no container and no network isolation. The GOOS/GOARCH guard above only blocks cross-arch execution; on a Linux host it passes, so the binary's package init(), global initializers, and any TestMain run on the host with full network/filesystem access, even with -test.run=^$ -test.bench=^$.
Consider skipping validation when a sandbox would otherwise be used (i.e. only validate NotSandboxed benchmarks), or running validation through the same Docker sandbox. At minimum, document this trade-off on the ValidateTestBinary field.
| bench.Disabled = true // if it won't compile, it won't run, either. | ||
| return s + "(" + bench.Name + ")\n" | ||
| } | ||
| if err := config.validateTestBinary(bench, compileTo, cmdEnv); err != nil { |
There was a problem hiding this comment.
[P3] Validation re-runs on every build repeat, not once per target
compileOne (and therefore validateTestBinary) is invoked buildCount times per benchmark/config pair. With -a N / randomized repeats, the same target is compiled to distinctly-named binaries and re-validated on every repeat. Since a pruned binary's startup behavior doesn't change between identical builds, validation is redundant across repeats. Consider gating validation on count == 0, mirroring how runOtherBenchmarks is already gated at configuration.go:317.
(Skip if per-repeat re-validation is intentional, e.g. to catch nondeterministic build flakiness.)
|
Superseded after root-cause review: the size change originates from LLGo PR #2370 restoring the test-main reachability root. Please close this PR; the follow-up will be handled in xgo-dev/llgo. |
|
The LLGo-side follow-up is xgo-dev/llgo#2388. It guards the test-main DCE root and adds an executable regression test; it intentionally does not restore the invalid pre-#2370 size. |
Summary
LLGoDeadcodeDrop, beforebenchsizeaccepts the result-build-onlywhen compilation or validation failsMotivation
The LLGo size history previously accepted a much smaller
-deadcodedroptest binary that failed during test-main initialization withunreachable method called. Bent used-build-only, so the invalid output was measured without ever being started. Empty selections exercise generated test-main initialization without running the benchmark workload or adding runtime cost to the size metric.BuildMode = "build"workloads remain compile-only and are not executed.Testing
go test ./cmd/bentgo test ./...