runtime/debug: populate BuildInfo so ReadBuildInfo works - #5592
Conversation
|
This PR updates all submodules - unintentionally I guess? |
debug.ReadBuildInfo() returns ok=false under TinyGo, so anything that
reports its own version — a --version flag, a crash handler, a metric —
has nothing to read.
The information already exists: `go list` reports module paths and
versions for the loaded packages, and the standard toolchain stamps the
same data into runtime/debug.modinfo, a plain string global that
runtime/debug parses back into a *BuildInfo. This fills that global the
same way.
Four pieces, because the data has to travel:
- loader: keep the module Version that `go list` already returns and
the struct was discarding.
- builder: assemble the modinfo string and set the global, unless
-ldflags="-X runtime/debug.modinfo=..." already did.
- src/runtime/debug: parse it, which is where ReadBuildInfo reads from.
- go.mod: golang.org/x/mod, for module.Check and semver validation of
what goes into the string.
Skipped in GOPATH mode and when the main package is not in a module,
where there is nothing to report.
Verified: a module built with this prints its own path and version from
ReadBuildInfo, where it previously reported nothing available.
|
Correct, entirely unintentional — thanks for catching it. The branch had picked up submodule pointer bumps for All seven are gone; the diff is now only Also rebased on dev, which merged cleanly with c33682c apart from an import that both sides added to; |
86d1329 to
e053e7f
Compare
|
This needs a test. |
Adds testdata/buildinfo.go to the compiler test list. The output has to be identical on every machine, so nothing here prints a version, a module path or a checksum the build happens to have. What is checked is either derived — that ReadBuildInfo always succeeds, that it reports a toolchain version even with no module information embedded, and that the version names the compiler — or comes from a fixed module string parsed in the test. That string covers the four line kinds and a replacement, so the parse, the round trip back through BuildInfo.String, and the shapes that must be rejected are all exercised. The distinction the last group draws is the one worth having: a line whose prefix is not a known kind is skipped, which is what upstream does and what lets an older parser read a newer module string, while a known kind with the wrong number of columns is an error — ReadBuildInfo falls back to reporting just the toolchain version when that happens.
|
Added The constraint is that the golden output has to be identical on every machine, so nothing printed is a version, a module path or a checksum the build happens to have. What is asserted is either derived — ReadBuildInfo always succeeds, it reports a toolchain version even with no module info embedded, and that version names the compiler — or comes from a fixed module string the test parses, covering the four line kinds plus a replacement, the round trip back through One distinction in there is worth calling out, because writing the test is what settled it: a line whose prefix is not a known kind is skipped rather than rejected — matching upstream, and what lets an older parser read a newer module string — while a known kind with the wrong column count is an error, which is when I could not run the full |
It overflows the ATmega by about 4.8 KiB of flash and 5.3 KiB of RAM: the parser builds a BuildInfo and formats errors, which pulls in fmt and strings. The same reason json.go, stdlib.go and testing.go are skipped there. AVR was the only target that failed; every other one in the matrix ran it.
|
CI caught one thing on the new test and it is fixed: Added to the existing AVR skip alongside AVR was the only target that failed; every other one in the matrix ran the test and passed. |
|
Is this something we ought to have a flag to disable, or is the space cost "small enough" that it won't make a difference? Skipping the test on AVR seems to suggest we might want to be able to disable it. |
The parser and BuildInfo.String used fmt.Errorf and fmt.Fprintf, which links fmt into any binary that calls ReadBuildInfo. That is what made testdata/buildinfo.go overflow the ATmega and need an AVR skip. Replacing those with errors.New, strconv and strings.Builder writes cuts a wasip1 binary that calls ReadBuildInfo from 677,404 to 293,340 bytes, 56.7% smaller. The testdata program itself goes from 685,718 to 303,901. A binary that never calls ReadBuildInfo is unaffected either way -- the linker already drops the parser and the embedded string when nothing references them. Error messages are unchanged. The line-number wrap keeps %w semantics via a parseError type with an Unwrap method rather than fmt.Errorf, so errors.Is and errors.As behave as they did. %q on a byte formats a single-quoted rune, so that one site uses strconv.QuoteRune, not Quote.
Stamping runs six git subprocesses on every build, one of which is `git status --porcelain` over the whole work tree. That happens whether or not the program ever calls ReadBuildInfo, and the go toolchain has -buildvcs for exactly this reason, so mirror it rather than inventing something: "auto" (the default, current behaviour), "false" to skip it, "true" to require it. As in the go toolchain, "true" is an error when no stamp can be produced, rather than silently falling back to "(devel)". The wall-clock saving on a small module is inside the noise; what -buildvcs=false removes is the repository access itself (five git subprocesses), which matters on a large work tree, on a network filesystem, and for reproducible or sandboxed builds where the tree is not a git checkout at all.
|
Good question, and measuring it changed what I think the answer is. Two pushes: one removes most of the cost, one adds the flag. The size cost was The parser and
A binary that never calls Error messages are byte-identical. The line-number wrap keeps I could not build AVR locally to confirm the skip can be dropped — the device packages need regenerating on this machine. Given the test program is 56% smaller and it overflowed by about 4.8 KiB, I expect it now fits, but CI is what will actually say. Happy to remove the skip in this PR if it passes, or leave it if you would rather not risk the churn. The flag: While measuring the above I noticed the stamping path shells out to git six times per build — including Being straight about the benefit: on a small module the wall-clock difference is inside the noise. What Validation covered by table cases in If you would rather not have the flag, say so and I will drop that commit — it is self-contained, and after the One thing I deliberately did not do: a flag that disables embedding |
|
Here is a generated review using Simplified Technical English. Summary The PR has five commits. The loader keeps Module.Version from go list. The builder makes a modinfo string and puts it in the binary with the existing -X globals mechanism. The src/runtime/debug package gets upstream ParseBuildInfo, without fmt. The PR also adds a -buildvcs flag and a testdata test. The design is correct. It uses globalValues and the upstream text format, so the existing code does the work. The new block is before the StripVarInitializer loop. This is the correct position. Problems
const buildInfoMagic = "\xff Go buildinf:" "\xff Go buildinf:" is the 14-byte magic of the .go.buildinfo section. The debug/buildinfo package uses it. It is not the modinfo wrapper. The modinfo wrapper is a 16-byte value. See cmd/go/internal/modload/build.go:29, infoStart, _ = hex.DecodeString("3077af0c9274080241e1c107e6d618e6"). Result: the branch never runs for a true Go modinfo string. Thus the comment about tolerance of the delimiters is not correct. Also the constant has 14 bytes, but the comment says 16 bytes and the code removes 16 bytes. Therefore the code removes the wrong number of bytes, even if the prefix agrees. TinyGo controls the writer. Remove the magic code. If you keep it, use the correct 16 bytes and calculate the strip length from the constant.
go build adds a dirty marker. See cmd/go/internal/load/pkg.go:2650 to 2657, vers += "+dirty", or .dirty for +incompatible. This PR calculates modified for vcs.modified, but it does not use modified for the version. The purpose of the PR is a --version flag. Thus a report of v1.2.3 for a modified tree is wrong.
go build makes sure that the module directory and the work directory are in the same repository. It then finds the version against the module path of the repository root. See pkg.go:2603 to 2647. It gives an error with -buildvcs=true, and it omits the data with auto. In this PR, git describe --tags --match "v[0-9]*" and git tag --points-at HEAD run in the module directory. Therefore they can find the tags of a parent repository. For a monorepo with tags for each directory, such as sub/v1.2.3, or for a module at example.com/m/v2, the code puts a version in the binary that is not the version of the module. semver.Major(older) takes the major version from the tag that git found. It does not take the major version from the module path suffix. Related point: the PR description says that x/mod gives module.Check and semver validation. But the diff does not call module.Check. A call to module.Check(main.Module.Path, version) before you accept the stamp, with a fallback to (devel), finds most of these conditions. It is a small change.
Smaller points
One point to make sure of makeGlobalsModule, at builder/build.go:1248, makes runtime/debug.modinfo with default visibility and external linkage. Thus the loop at build.go:634 does not make it internal, and IR dead code removal keeps it. This is true also for a program that does not import runtime/debug. This is the first globalValues entry for a package that can be absent from the build. Commit 3ade2ea says that such binaries do not change. This is possible, because section removal at the final link can delete the data. But do a -size=short measurement before and after, on a small Cortex-M target. The string becomes larger when the number of dependencies increases. Also, commit 445d4fb added the AVR skip for buildinfo.go, because of fmt. Then commit 3ade2ea removed fmt and made the binary 56% smaller. The skip is still in the final diff. Do the AVR test again. If AVR is now satisfactory, remove the skip and the change to main_test.go. |
buildInfoMagic was wrong and is removed. The constant held the 14-byte ".go.buildinfo" section magic, not the 16-byte modinfo wrapper cmd/go uses (infoStart in modload/build.go), while the code stripped 16 bytes and the comment claimed the constant was 16 bytes. The branch could never fire on a real modinfo string. TinyGo controls the writer, so nothing needs stripping at all. gitVCSStamp: a modified work tree now yields "+dirty", as go build does; previously "modified" was recorded in vcs.modified but ignored for the version, so a dirty tree reported a clean tag. gitVCSStamp: refuse to stamp when the module is not at the repository root. The tag searches run in the module directory and so could pick up a parent repository's tags, versioning a nested module with something that is not its own. Also check the major version against any /vN suffix on the module path. That check is deliberately CheckPathMajor and not module.Check, because module.Check additionally rejects a path whose first element has no dot, which is normal for a module that is never published and would lose those their stamp. gitVCSStamp: an unparsable commit time is now a stamp failure rather than a zero time, which module.PseudoVersion would have encoded as 00010101000000. gitVCSStamp: keep the first git failure and report it, so -buildvcs=true distinguishes a refusal over safe.directory or permissions from "not a git work tree". vcs.time now uses RFC3339Nano, matching cmd/go. Documented what the writer deliberately omits. Dropped a single-case switch, and matched the surrounding return style.
|
Thank you — this was a good review, and four of the findings were real. All fixed in First, a correction to what I told you. I said I expected the AVR skip could now be dropped. It cannot. I could not build AVR locally before because this checkout had never run
So removing 1. 2. Dirty tree reporting a clean version — fixed, 3. Module root vs repository root — fixed. On 4. Zero commit time — fixed; an unparsable Smaller points — all taken: The one you asked me to make sure of — you were right to ask, and the answer is that it does get eliminated. On
Byte-identical when unused, so the global does not survive dead-code elimination despite the external linkage. A caller pays about 13.4 KB flash and 1.4 KB RAM on Cortex-M, which is the parser rather than the string. For reference the plain program is 188 bytes larger than stock 0.41.1, and that is 0.41→0.42 drift, not this change — the identical pair above rules the feature out. Behaviour verified by hand after the changes: clean tree at a tag gives |
4 participants
debug.ReadBuildInfo()returnsok=falseunder TinyGo, so anything that reports its own version — a--versionflag, a crash handler, a metric — has nothing to read.The information already exists.
go listreports module paths and versions for the loaded packages, and the standard toolchain stamps the same data intoruntime/debug.modinfo, a plain string global thatruntime/debugparses back into a*BuildInfo. This fills that global the same way, so the existing parsing path does the rest.Four pieces, because the data has to travel:
Versionthatgo listalready returns and the struct was discarding-ldflags="-X runtime/debug.modinfo=..."already didReadBuildInforeads fromgolang.org/x/mod, formodule.Checkand semver validation of what goes into the stringSkipped in GOPATH mode and when the main package isn't in a module, where there's nothing to report.
Verified: a module built with this prints its own path and version from
ReadBuildInfo(path: bitest,main: bitest (devel)), where it previously reported nothing available.