You cannot select more than 25 topics Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
garble/.github/workflows/test.yml

68 lines
1.6 KiB
YAML

on:
push:
branches:
- master
pull_request:
branches:
- master
name: Test
jobs:
test:
strategy:
matrix:
go-version: [1.17.x]
fix a data race with the global cachedBinary mechanism (#413) Spotted by our friend "go test -race": WARNING: DATA RACE Write at 0x0000010522d8 by goroutine 69: mvdan.cc/garble.readFile() garble/main_test.go:124 +0x23a mvdan.cc/garble.binsubstr() garble/main_test.go:141 +0xc4 github.com/rogpeppe/go-internal/testscript.(*TestScript).run() github.com/rogpeppe/go-internal@v1.8.1-0.20211023094830-115ce09fd6b4/testscript/testscript.go:496 +0x9e8 [...] Previous write at 0x0000010522d8 by goroutine 60: mvdan.cc/garble.readFile() garble/main_test.go:124 +0x23a mvdan.cc/garble.binsubstr() garble/main_test.go:141 +0xc4 github.com/rogpeppe/go-internal/testscript.(*TestScript).run() github.com/rogpeppe/go-internal@v1.8.1-0.20211023094830-115ce09fd6b4/testscript/testscript.go:496 +0x9e8 [...] This wasn't a data race that we spotted via failures in practice, as it only affected test code since July. The race is due to the fact that each test script runs as a parallel sub-test within the same Go program, sharing all globals. As such, a single "cached binary" global is read and written with races. Moreover, note that the caching always missed. I briefly rewrote the code to avoid the race via a sync.Map keyed by absolute filenames, and while that removed the data race, the caching never actually hit. To have a cache hit, we need an absolute path to already be in the cache and for it to not have been modified since it was last cached. That is: modify-bin-1 foo binsubstr foo 'abc' # miss binsubstr foo 'def' # hit; use the cached "/tmp/[...]/foo" entry modify-bin-2 foo binsubstr foo 'abc' # miss However, the test scripts don't do contiguous binsubstr calls like these. Instead, they join repeated binsubstr calls: modify-bin-1 foo binsubstr foo 'abc' 'def' # miss modify-bin-2 foo binsubstr foo 'abc' # miss For that reason, remove the extra code entirely. I didn't notice any change to the performance of "go test -short" with a warm build cache, with: go test -c ./garble.test -test.short #warm cache benchcmd -n 5 TestShort ./garble.test -test.short name old time/op new time/op delta TestShort 4.62s ±12% 4.35s ±12% ~ (p=0.310 n=5+5) name old user-time/op new user-time/op delta TestShort 16.8s ± 3% 16.7s ± 3% ~ (p=0.690 n=5+5) name old sys-time/op new sys-time/op delta TestShort 7.28s ± 1% 7.26s ± 2% ~ (p=0.841 n=5+5) name old peak-RSS-bytes new peak-RSS-bytes delta TestShort 305MB ± 0% 306MB ± 0% ~ (p=0.421 n=5+5) Finally, start using "go test -race" on Linux on CI, which should have made the PR back in July red before merging.
3 years ago
os: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- name: Install Go
uses: actions/setup-go@v2
with:
go-version: ${{ matrix.go-version }}
- name: Checkout code
4 years ago
uses: actions/checkout@v2
- name: Test
run: |
go version
go test ./...
fix a data race with the global cachedBinary mechanism (#413) Spotted by our friend "go test -race": WARNING: DATA RACE Write at 0x0000010522d8 by goroutine 69: mvdan.cc/garble.readFile() garble/main_test.go:124 +0x23a mvdan.cc/garble.binsubstr() garble/main_test.go:141 +0xc4 github.com/rogpeppe/go-internal/testscript.(*TestScript).run() github.com/rogpeppe/go-internal@v1.8.1-0.20211023094830-115ce09fd6b4/testscript/testscript.go:496 +0x9e8 [...] Previous write at 0x0000010522d8 by goroutine 60: mvdan.cc/garble.readFile() garble/main_test.go:124 +0x23a mvdan.cc/garble.binsubstr() garble/main_test.go:141 +0xc4 github.com/rogpeppe/go-internal/testscript.(*TestScript).run() github.com/rogpeppe/go-internal@v1.8.1-0.20211023094830-115ce09fd6b4/testscript/testscript.go:496 +0x9e8 [...] This wasn't a data race that we spotted via failures in practice, as it only affected test code since July. The race is due to the fact that each test script runs as a parallel sub-test within the same Go program, sharing all globals. As such, a single "cached binary" global is read and written with races. Moreover, note that the caching always missed. I briefly rewrote the code to avoid the race via a sync.Map keyed by absolute filenames, and while that removed the data race, the caching never actually hit. To have a cache hit, we need an absolute path to already be in the cache and for it to not have been modified since it was last cached. That is: modify-bin-1 foo binsubstr foo 'abc' # miss binsubstr foo 'def' # hit; use the cached "/tmp/[...]/foo" entry modify-bin-2 foo binsubstr foo 'abc' # miss However, the test scripts don't do contiguous binsubstr calls like these. Instead, they join repeated binsubstr calls: modify-bin-1 foo binsubstr foo 'abc' 'def' # miss modify-bin-2 foo binsubstr foo 'abc' # miss For that reason, remove the extra code entirely. I didn't notice any change to the performance of "go test -short" with a warm build cache, with: go test -c ./garble.test -test.short #warm cache benchcmd -n 5 TestShort ./garble.test -test.short name old time/op new time/op delta TestShort 4.62s ±12% 4.35s ±12% ~ (p=0.310 n=5+5) name old user-time/op new user-time/op delta TestShort 16.8s ± 3% 16.7s ± 3% ~ (p=0.690 n=5+5) name old sys-time/op new sys-time/op delta TestShort 7.28s ± 1% 7.26s ± 2% ~ (p=0.841 n=5+5) name old peak-RSS-bytes new peak-RSS-bytes delta TestShort 305MB ± 0% 306MB ± 0% ~ (p=0.421 n=5+5) Finally, start using "go test -race" on Linux on CI, which should have made the PR back in July red before merging.
3 years ago
- name: Test with -race
# macos and windows tend to be a bit slower,
# and it's rare that a race in garble would be OS-specific.
if: matrix.os == 'ubuntu-latest'
run: |
go test -race ./...
test-gotip:
runs-on: ubuntu-latest
continue-on-error: true # master may not be as stable
steps:
- name: Install Go
env:
ensure the runtime is built in a reproducible way We went to great lengths to ensure garble builds are reproducible. This includes how the tool itself works, as its behavior should be the same given the same inputs. However, we made one crucial mistake with the runtime package. It has go:linkname directives pointing at other packages, and some of those pointed packages aren't its dependencies. Imagine two scenarios where garble builds the runtime package: 1) We run "garble build runtime". The way we handle linkname directives calls listPackage on the target package, to obfuscate the target's import path and object name. However, since we only obtained build info of runtime and its deps, calls for some linknames such as listPackage("sync/atomic") will fail. The linkname directive will leave its target untouched. 2) We run "garble build std". Unlike the first scenario, all listPackage calls issued by runtime's linkname directives will succeed, so its linkname directive targets will be obfuscated. At best, this can result in inconsistent builds, depending on how the runtime package was built. At worst, the mismatching object names can result in errors at link time, if the target packages are actually used. The modified test reproduces the worst case scenario reliably, when the fix is reverted: > env GOCACHE=${WORK}/gocache-empty > garble build -a runtime > garble build -o=out_rebuild ./stdimporter [stderr] # test/main/stdimporter JZzQivnl.NtQJu0H3: relocation target JZzQivnl.iioHinYT not defined JZzQivnl.NtQJu0H3.func9: relocation target JZzQivnl.yz5z0NaH not defined JZzQivnl.(*ypvqhKiQ).String: relocation target JZzQivnl.eVciBQeI not defined JZzQivnl.(*ypvqhKiQ).PkgPath: relocation target JZzQivnl.eVciBQeI not defined [...] The fix consists of two steps. First, if we're building the runtime and listPackage fails on a package, that means we ran into scenario 1 above. To avoid the inconsistency, we fill ListedPackages with "go list [...] std". This means we'll always build runtime as described in scenario 2 above. Second, when building packages other than the runtime, we only allow listPackage to succeed if we're listing a dependency of the current package. This ensures we won't run into similar reproducibility bugs in the future. Finally, re-enable test-gotip on CI since this was the last test flake.
3 years ago
GO_COMMIT: b357b05b70d2b8c4988ac2a27f2af176e7a09e1b # 2021-12-21
run: |
cd $HOME
mkdir $HOME/gotip
cd $HOME/gotip
wget -O gotip.tar.gz https://go.googlesource.com/go/+archive/${GO_COMMIT}.tar.gz
tar -xf gotip.tar.gz
echo "devel go1.18-${GO_COMMIT}" >VERSION
cd src
./make.bash
echo "GOROOT=$HOME/gotip" >>$GITHUB_ENV
echo "$HOME/gotip/bin" >>$GITHUB_PATH
- name: Checkout code
uses: actions/checkout@v2
- name: Test
run: |
go version
go test ./...
code-checks:
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v2
- name: Test that only LF line endings are used
run: ./scripts/crlf-test.sh