Split the musl cgo link path by architecture - #3932
Conversation
The glibc link path is already arch-split (link_glibclinux_aarch64.go and link_glibclinux_x86_64.go), but the musl path never was. link_muslc.go carried `linux && muslc && !sys_wasmvm` with no arch term, so a linux/arm64 muslc build selected it and emitted -lwasmvm_muslc, which resolves the x86_64 archive. The build then failed at link time with incompatible-archive errors. Constrain the existing files to amd64 and add aarch64 siblings pointing at the libwasmvm*_muslc.aarch64.a archives, which were already checked in but referenced by nothing. Verified per file rather than in aggregate. Removing any one of the three new files drops that package to zero link directives on linux/arm64 (undefined references at link), and dropping the amd64 term from any one of the three existing files takes it to two, putting both archives on one link line. For GOARCH=amd64 the selected files and the resulting cgo LDFLAGS are byte-identical before and after, so the shipping build is untouched. The new files are byte-identical to CosmWasm/wasmvm v2.1.0's equivalents. The vendored tree here predates that upstream fix. Note for reviewers: exotic linux architectures (386, riscv64, s390x, ppc64le) previously matched the amd64 muslc directive under the muslc tag and failed with an incompatible-archive error. They now match no link file and fail with undefined references instead. That matches what the glibc path has always done, and seid cannot build on those architectures regardless, since giga/executor/lib rejects them at compile time.
link_directives_test.go checks that every -l<name> resolves to an archive on disk, but it never parses //go:build, so it cannot see which directive applies to which platform. It passed even while the musl arm64 bug was present. Nothing else covers this either. No CI job, goreleaser build or Dockerfile stage builds linux/arm64 with the muslc tag, so the combination this repo just fixed is never exercised anywhere, and reverting the arch split would leave the whole tree green. Assert instead that every build configuration selects exactly one link_*.go per api package, which file it is, and that an aarch64 archive is used exactly when GOARCH is arm64. Selection is evaluated with go/build's own MatchFile rather than a reimplementation, so the answer is the toolchain's. Confirmed to fail, not merely to pass. Deleting any one of the three aarch64 link files, dropping the amd64 term from any one of the three amd64 files, and swapping the -l archive names between two files all turn it red. The swap case still passes the existing tests, since both names resolve to real archives.
The static-paths filter decides whether a PR runs the static build job. It covered the build scripts, Makefile, vendored libgcc, goreleaser config and this workflow, but not the cgo link files or the goreleaser shim, so a PR changing only the link directives ran with the static job skipped, which renders as green. Add the three api link_ paths, the two root link tests and goreleaser-shim.sh. Because this file is itself in the pattern, the change takes effect on the PR that introduces it. Backporting note: the release bot cannot push workflow files, so this commit has to be cherry-picked by hand when this lands on a release branch.
PR SummaryLow Risk Overview
Adds Reviewed by Cursor Bugbot for commit bca62aa. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arch-splitting the muslc cgo link path is correct: the three existing files gain amd64, the three new aarch64 siblings match the existing glibc naming/tag convention and point at archives already vendored and checksum-pinned, and the new link_constraints_test.go genuinely closes the gap that let this bug survive. Only one minor maintenance nit; nothing blocking.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Verified the tag arithmetic by hand across all three packages: for
linux/amd64the selected files and resultingLDFLAGSare unchanged,linux/arm64 muslcnow selects exactly the newlink_muslc_aarch64.go, and theriscv64row correctly selects nothing (all glibc files carry!muslc, both muslc files now carry an arch term). The archives the three new directives name are all present in-tree and already pinned bystatic_archive_checksums_test.go, including the aarch64 variants. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
.github/workflows/cross-arch-build.ymlstatic-pathsPATTERN gates on the link_*.go sources and onscripts/check-libwasmvm-static.sh, but not on the vendored archives themselves (sei-wasmvm/internal/api/libwasmvm*.a,sei-wasmd/x/wasm/artifacts/v*/api/libwasmvm*.a). A PR into main that regenerates or replaces an archive therefore skips the static build job. Since this PR is already extending that pattern, adding thelibwasmvmarchive paths would be a cheap way to close it.
|
|
||
| // linkPkgDirs are the vendored libwasmvm api packages whose link_*.go files carry the | ||
| // cgo directive naming a prebuilt archive. | ||
| var linkPkgDirs = []string{ |
There was a problem hiding this comment.
[nit] linkPkgDirs is byte-identical to linkDirs in link_directives_test.go, and both files are in package sei_test, so the second list is a copy that has to be kept in sync by convention. When a v160 artifacts package is vendored, updating only one list silently drops constraint coverage for the new package while TestLinkDirectivesResolve still looks green — exactly the shape of blind spot this test exists to remove. Reusing linkDirs here (or hoisting one list both tests read) makes it a single choke point instead.
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
nice, thanks @monty-sei
Can I ask you to add checksum tests too please? Take a look at existing checksum tests for other architectures I added. ty!
Left some comments otherwise no blockers 🙌
| } | ||
|
|
||
| // hasTag reports whether tag is present in tags. | ||
| func hasTag(tags []string, tag string) bool { |
|
|
||
| // linkPlatforms enumerates the build configurations this repo produces, plus the | ||
| // linux/arm64 muslc cell the static arm64 binary depends on. | ||
| var linkPlatforms = []linkPlatform{ |
There was a problem hiding this comment.
I would inline this inside the test function itself. That's the predominant Go convention and is the only place this slice is used.
| } | ||
|
|
||
| // linkPlatform is a build configuration and the link directive it must resolve to. | ||
| type linkPlatform struct { |
There was a problem hiding this comment.
You can also inline this inside the test function as an anonymous struct value.
| // Naming an archive that exists is already covered by | ||
| // TestLinkDirectivesResolve, and that holds even if two directives are | ||
| // swapped. Pin the architecture of the archive as well. | ||
| if p.goos == "linux" && !hasTag(p.tags, "sys_wasmvm") && len(selected) == 1 { |
There was a problem hiding this comment.
You can separate tests the same way we separate build using tags to only run on certain structures. That's the Go native way to do this, where you can write tests that only run on Linux amd64 for example
|
|
||
| // selectedLinkFiles returns the link_*.go files in dir that the Go toolchain compiles for | ||
| // the given platform. | ||
| func selectedLinkFiles(t *testing.T, dir string, p linkPlatform) []string { |
There was a problem hiding this comment.
This can also be made into architecture specific tests with constant values using build tags which is a Go Native way of doing it.
| ctx.GOOS = p.goos | ||
| ctx.GOARCH = p.goarch | ||
| ctx.BuildTags = p.tags | ||
| // The link files are pure `import "C"`; with cgo off the toolchain excludes them all. |
There was a problem hiding this comment.
Is this true? Change that value to false and see if the tests pass.
3 participants
Prerequisite for shipping a static
linux/arm64seid binary. It changes three words, adds three five-line files, and adds a test.The bug
The glibc link path is arch-split (
link_glibclinux_aarch64.goandlink_glibclinux_x86_64.go), but the musl path never was.link_muslc.gocarriedlinux && muslc && !sys_wasmvmwith no arch term, so alinux/arm64muslc build selected it, emitted-lwasmvm_muslc, resolved the x86_64 archive, and died at link time with incompatible-archive errors.The
libwasmvm*_muslc.aarch64.aarchives have been checked in for months, referenced by nothing.The change
Constrain the three existing files to
amd64, and add three aarch64 siblings pointing at the archives already in the tree. The new files are byte-identical to CosmWasm/wasmvm v2.1.0's equivalents, since the vendored tree here predates that upstream fix.Validation
Each of the six changes was tested in isolation rather than as one bundle:
linux/arm64 muslcselectionamd64from any one fileReal links in the pinned release image (Alpine 3.23.3, gcc 15.2.0, native arm64) confirm the model predicts linker behaviour rather than just build-tag arithmetic. The baseline fails with 3x
incompatible, the "new files but no constraints" variant fails with 6x (double, one per extra archive), "constraints but no new files" fails withundefined reference to version_str, and the full change producesELF 64-bit LSB executable, ARM aarch64, statically linked.amd64 is provably untouched. For
GOARCH=amd64the selected files and the resulting cgoLDFLAGSare byte-identical before and after, across all three packages. An actual amd64 static build via unmodifiedscripts/build-static.shsucceeds with the change applied.No consensus impact observed. A mixed fleet of three binaries built from this branch (arm64 static musl, amd64 static musl, arm64 dynamic glibc) on one chain agreed on every app hash compared, including the blocks carrying a wasm store/instantiate/execute cycle. Separately, the seven consensus-relevant packages that swap amd64 assembly for portable Go on arm64 all pass their upstream vector suites on both architectures.
Reviewer notes
linux/arm64with the muslc tag yet. It unblocks that work, it does not ship an arm64 binary.giga/executor/librejects them at compile time with a clearer message.Why the test
link_directives_test.gochecks each-l<name>resolves to a file on disk but never parses//go:build, so it passed even while this bug was present. Nothing else covers it either, because no job anywhere builds arm64 muslc. Without the new test, reverting this change is a green build.It asserts each platform selects exactly one link directive, which file it is, and that an aarch64 archive is used exactly when
GOARCH=arm64. Selection is evaluated withgo/build's ownMatchFile, so the answer is the toolchain's rather than a reimplementation.Confirmed to fail rather than merely to pass. All seven ways of breaking this change turn it red, including swapping the
-lnames between two files, which is a case the existing tests still pass.