From c26734c668e97cf99640476c95cc9a674b78895d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sun, 4 Jun 2023 14:09:18 +0100 Subject: [PATCH] simplify our handling of "go list" errors 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. --- go_std_tables.go | 3 +- main.go | 7 +++ scripts/gen-go-std-tables.sh | 2 +- shared.go | 76 +++++++++----------------------- testdata/script/list_error.txtar | 6 +-- 5 files changed, 33 insertions(+), 61 deletions(-) diff --git a/go_std_tables.go b/go_std_tables.go index deaf9b2..8556eff 100644 --- a/go_std_tables.go +++ b/go_std_tables.go @@ -1,6 +1,6 @@ // Code generated by scripts/gen-go-std-tables.sh; DO NOT EDIT. -// Generated from Go version devel go1.21-0fd6ae548f Fri Apr 28 20:33:34 2023 +0000. +// Generated from Go version devel go1.21-3637132233 Sat Jun 3 01:13:08 2023 +0000. package main @@ -41,7 +41,6 @@ var runtimeLinknamed = []string{ "runtime/coverage", "runtime/debug", "runtime/metrics", - "runtime/metrics_test", "runtime/pprof", "runtime/trace", "sync", diff --git a/main.go b/main.go index eb71740..484a6aa 100644 --- a/main.go +++ b/main.go @@ -1102,6 +1102,13 @@ func (tf *transformer) transformLinkname(localName, newName string) (string, str pkgPath := newName[:pkgSplit] pkgSplit++ // skip over the dot + if strings.HasSuffix(pkgPath, "_test") { + // runtime uses a go:linkname to metrics_test; + // we don't need this to work for now on regular builds, + // though we might need to rethink this if we want "go test std" to work. + continue + } + var err error lpkg, err = listPackage(tf.curPkg, pkgPath) if err == nil { diff --git a/scripts/gen-go-std-tables.sh b/scripts/gen-go-std-tables.sh index 039bbda..1c9fa46 100755 --- a/scripts/gen-go-std-tables.sh +++ b/scripts/gen-go-std-tables.sh @@ -12,7 +12,7 @@ runtime_and_deps=$(go list -deps runtime) # This resulting list is what we need to "go list" when obfuscating the runtime, # as they are the packages that we may be missing. runtime_linknamed=$(comm -23 <( - sed -rn 's@//go:linkname .* ([^.]*)\.[^.]*@\1@p' "${goroot}"/src/runtime/*.go | grep -vE '^main|^runtime\.' | sort -u + sed -rn 's@//go:linkname .* ([^.]*)\.[^.]*@\1@p' "${goroot}"/src/runtime/*.go | grep -vE '^main|^runtime\.|_test$' | sort -u ) <( # Note that we assume this is constant across platforms. go list -deps runtime | sort -u diff --git a/shared.go b/shared.go index 271d8f9..526e918 100644 --- a/shared.go +++ b/shared.go @@ -18,6 +18,7 @@ import ( "time" "golang.org/x/mod/module" + "golang.org/x/mod/semver" ) //go:generate ./scripts/gen-go-std-tables.sh @@ -151,12 +152,10 @@ type listedPackage struct { Dir string CompiledGoFiles []string + IgnoredGoFiles []string Imports []string - Incomplete bool - // These two exist to report package loading errors to the user. - Error *packageError - DepsErrors []*packageError + Error *packageError // to report package loading errors to the user // The fields below are not part of 'go list', but are still reused // between garble processes. Use "Garble" as a prefix to ensure no @@ -174,6 +173,7 @@ type listedPackage struct { } type packageError struct { + Pos string Err string } @@ -254,58 +254,30 @@ func appendListedPackages(packages []string, mainBuild bool) error { if sharedCache.ListedPackages == nil { sharedCache.ListedPackages = make(map[string]*listedPackage) } - var pkgErrors []string + var pkgErrors strings.Builder for dec.More() { var pkg listedPackage if err := dec.Decode(&pkg); err != nil { return err } - // Sometimes cmd/go sets Error without setting Incomplete per the docs. - // TODO: remove the workaround once https://go.dev/issue/57724 is fixed. - if pkg.Error != nil || pkg.DepsErrors != nil { - pkg.Incomplete = true - } - if perr := pkg.Error; perr != nil { - switch { - // All errors in non-std packages are fatal, - // but only some errors in std packages are. - case strings.Contains(pkg.ImportPath, "."): - fallthrough - default: - // Error messages sometimes include a trailing newline. - pkgErrors = append(pkgErrors, strings.TrimSpace(perr.Err)) - - // Some packages in runtimeLinknamed are OS-specific, - // like crypto/internal/boring/fipstls, so "no Go files" - // for the current OS can be ignored safely as an error. - case pkg.Standard && strings.Contains(perr.Err, "build constraints exclude all Go files"): - // Some packages in runtimeLinknamed are recent, - // like "arena", so older Go versions that we support - // do not yet have them and that's OK. - // Note that pkg.Standard is false for them. - // Note that Go 1.21 is swapping "GOROOT" for "std". - // TODO(mvdan): We try to list test packages like runtime/metrics_test, which always fail. - case strings.Contains(perr.Err, "is not in GOROOT"): - case strings.Contains(perr.Err, "is not in std"): - case strings.Contains(perr.Err, "cannot find package"): - } - } - if len(pkg.DepsErrors) > 0 { - for i, derr := range pkg.DepsErrors { - // When an error in DepsErrors starts with a "# pkg/path" line, - // it's an error that we're already printing via that package's Error field. - // Otherwise, the error is that we couldn't find that package at all, - // so we do need to print it here as that package won't be listed. - if i == 0 { - if strings.HasPrefix(derr.Err, "# ") { - break - } - pkgErrors = append(pkgErrors, "# "+pkg.ImportPath) + if pkg.Standard && len(pkg.CompiledGoFiles) == 0 && len(pkg.IgnoredGoFiles) > 0 { + // Some packages in runtimeLinknamed need a build tag to be importable, + // like crypto/internal/boring/fipstls with boringcrypto, + // so any pkg.Error should be ignored when the build tag isn't set. + } else if pkg.ImportPath == "maps" && semver.Compare(sharedCache.GoVersionSemver, "v1.21") < 0 { + // "maps" was added in Go 1.21, so Go 1.20 runs into a "not found" error. + } else { + if pkgErrors.Len() > 0 { + pkgErrors.WriteString("\n") + } + if perr.Pos != "" { + pkgErrors.WriteString(perr.Pos) + pkgErrors.WriteString(": ") } // Error messages sometimes include a trailing newline. - pkgErrors = append(pkgErrors, strings.TrimSpace(derr.Err)) + pkgErrors.WriteString(strings.TrimRight(perr.Err, "\n")) } } @@ -329,8 +301,8 @@ func appendListedPackages(packages []string, mainBuild bool) error { if err := cmd.Wait(); err != nil { return fmt.Errorf("go list error: %v:\nargs: %q\n%s", err, args, stderr.Bytes()) } - if len(pkgErrors) > 0 { - return errors.New(strings.Join(pkgErrors, "\n")) + if pkgErrors.Len() > 0 { + return errors.New(pkgErrors.String()) } anyToObfuscate := false @@ -345,11 +317,7 @@ func appendListedPackages(packages []string, mainBuild bool) error { // "unknown pc" crashes on windows in the cgo test otherwise. path == "runtime/cgo": - // We can't obfuscate packages which weren't loaded. - // This can happen since we ignore some pkg.Error messages above. - case pkg.Incomplete: - - // No point in obfuscating empty packages. + // No point in obfuscating empty packages, like OS-specific ones that don't match. case len(pkg.CompiledGoFiles) == 0: // Test main packages like "foo/bar.test" are always obfuscated, diff --git a/testdata/script/list_error.txtar b/testdata/script/list_error.txtar index 228d189..43f730e 100644 --- a/testdata/script/list_error.txtar +++ b/testdata/script/list_error.txtar @@ -5,13 +5,11 @@ -- stderr-go1.20.golden -- # test/main/broken broken${/}broken.go:5:16: cannot use 123 (untyped int constant) as string value in variable declaration -# test/main/imports_missing -package test/main/missing is not in GOROOT (${GOROOT}${/}src${/}test${/}main${/}missing) +imports_missing${/}imports.go:5:8: package test/main/missing is not in GOROOT (${GOROOT}${/}src${/}test${/}main${/}missing) -- stderr-go1.21.golden -- # test/main/broken broken${/}broken.go:5:16: cannot use 123 (untyped int constant) as string value in variable declaration -# test/main/imports_missing -package test/main/missing is not in std (${GOROOT}${/}src${/}test${/}main${/}missing) +imports_missing${/}imports.go:5:8: package test/main/missing is not in std (${GOROOT}${/}src${/}test${/}main${/}missing) -- go.mod -- module test/main