When updating Garble to support Go 1.22.0, CI on MacOS spotted
that the syscall package was failing to build given that it uses
assembly code which is only allowed in some std packages.
That allowlist is based on import paths, and we were obfuscating
the syscall package's import path, so that was breaking GOOS=darwin.
As a fix, I added syscall to runtimeAndDeps to not obfuscate it.
That wasn't a great fix; it's not part of runtime and its dependencies,
and there's no reason we should avoid obfuscating the package contents.
Not obfuscating the contents in fact broke x/sys/unix,
as it contains a copy of syscall.Rlimit which it type converted with.
Undo that fix and reinstate the gogarble.txtar syscall test.
Implement the fix where we only leave syscall's import path alone.
Add a regression test, and add a note about adding x/net and x/sys
to check-third-party.sh so that we can catch these bugs earlier.
Fixes#830.
It seems like building with Go 1.22.0 for GOOS=darwin started
running into some issues with the syscall package's use of ABIInternal
in assembly source code:
> exec garble build
[stderr]
# syscall
[...].s:16: ABI selector only permitted when compiling runtime, reference was to "runtime.entersyscall"
The error can be reproduced from another platform like GOOS=linux
as long as we have any test that cross-compiles std to GOOS=darwin.
We had crossbuild.txtar which only ensured we covered GOOS=windows
and GOOS=linux, so add a third case to ensure MacOS is covered too.
This will slow down the tests a bit, but is important for the sake
of ensuring that we catch these bugs early, even without MacOS on CI.
In fact, we hadn't caught this earlier for Go 1.22 precisely because
on CI we only tested on Go tip with GOOS=linux, for the sake of speed.
Adding the rest of the package import paths from objabi.allowAsmABIPkgs
to our runtimeAndDeps generated map solves this error.
In early December, a new internal package linknamed from runtime
was introduced, internal/chacha8rand. Re-generate the tables.
Note that due to the same group of CLs and refactors,
math/rand and net are no longer linknamed from runtime in Go 1.22.
They are still in Go 1.21, so keep those entries around for now.
We can remove math/rand/v2, as it doesn't yet exist in 1.21.
Fixes#820.
The Go 1.21 linker patches luckily rebased on master as of
de5b418bea70aaf27de1f47e9b5813940d1e15a4 just fine.
The addition of the strings import in the second patch was removed,
since the file in Go 1.22 now has this package import.
We can remove the Go 1.20 linker patches too, since we no longer support
that Go version in the upcoming release.
Start treating runtime/internal/startlinetest as part of the runtime,
since otherwise its test-only trickery breaks "garble build std":
# runtime/internal/startlinetest
[...]/XS7r7lPHkTG.s:23: ABI selector only permitted when compiling runtime, reference was to "HGoWHDsKwh.AlfA2or7Nnb"
asm: assembly of $WORK/.tmp/garble-shared1535203339/HGoWHDsKwh/XS7r7lPHkTG.s failed
While here, update actions/checkout and staticcheck in CI.
Two new packages linknamed with the runtime package,
one new intrinsic function, and one that is being removed in Go 1.22
but we want to keep around as long as we support Go 1.21.
Also note that, since math/rand/v2 simply does not exist until Go 1.22,
we need to adjust appendListedPackages to not fail on older versions.
First, teach scripts/gen-go-std-tables.sh to omit test packages,
since runtime/metrics_test would always result in an error.
Instead, make transformLinkname explicitly skip that package,
leaving a comment about a potential improvement if needed.
Second, the only remaining "not found" error we had was "maps" on 1.20,
so rewrite that check based on ImportPath and GoVersionSemver.
Third, detect packages with the "exclude all Go files" error
by looking at CompiledGoFiles and IgnoredGoFiles, which is less brittle.
This means that we are no longer doing any filtering on pkg.Error.Err,
which means we are less likely to break with Go error message changes.
Fourth, the check on pkg.Incomplete is now obsolete given the above,
meaning that the CompiledGoFiles length check is plenty.
Finally, stop trying to be clever about how we print errors.
Now that we're no longer skipping packages based on pkg.Error values,
printing pkg.DepsErrors was causing duplicate messages in the output.
Simply print pkg.Error values with only minimal tweaks:
including the position if there is any, and avoiding double newlines.
Overall, this makes our logic a lot less complicated,
and garble still works the way we want it to.
A couple of new packages in runtimeAndDeps,
and go list's Package.DepsErrors may now include package build errors
which we want to ignore as we would print those as duplicates.
We obfuscate import paths as well as their declared names.
The compiler treats some packages and APIs in special ways,
and the way it detects those is by looking at import paths and names.
In the past, we have avoided obfuscating some names like embed.FS or
reflect.Value.MethodByName for this reason. Otherwise,
go:embed or the linker's deadcode elimination might be broken.
This matching by path and name also happens with compiler intrinsics.
Intrinsics allow the compiler to rewrite some standard library calls
with small and efficient assembly, depending on the target GOARCH.
For example, math/bits.TrailingZeros32 gets replaced with ssa.OpCtz32,
which on amd64 may result in using the TZCNTL instruction.
We never noticed that we were breaking many of these intrinsics.
The intrinsics for funcs declared in the runtime and its dependencies
still worked properly, as we do not obfuscate those packages yet.
However, for other packages like math/bits and sync/atomic,
the intrinsics were being entirely disabled due to obfuscated names.
Skipping intrinsics is particularly bad for performance,
and it also leads to slightly larger binaries:
│ old │ new │
│ bin-B │ bin-B vs base │
Build-16 5.450Mi ± ∞ ¹ 5.333Mi ± ∞ ¹ -2.15% (p=0.029 n=4)
Finally, the main reason we noticed that intrinsics were broken
is that apparently GOARCH=mips fails to link without them,
as some symbols end up being not defined at all.
This patch fixes builds for the MIPS family of architectures.
Rather than building and linking all of std for every GOARCH,
test that intrinsics work by asking the compiler to print which
intrinsics are being applied, and checking that math/bits gets them.
This fix is relatively unfortunate, as it means we stop obfuscating
about 120 function names and a handful of package paths.
However, fixing builds and intrinsics is much more important.
We can figure out better ways to deal with intrinsics in the future.
Fixes#646.
Daniel Martí
lu4p