fail on user packages with build errors early

The added test case would panic, because we would try to hash a name
with a broken package's GarbleActionID, which was empty.

We skipped over all package errors in appendListedPackages because two
kinds of errors were OK in the standard library.
However, this also meant we ignored real errors we should stop at,
because obfuscating those user packages is pointless.

Add more assertions, check for the OK errors explicitly,
and fail on any other error immediately.
Note that, in the process, I also found a bug in cmd/go.

Uncovered by github.com/bytedance/sonic,
whose internal/loader package fails to build on Go 1.20.

shared.go

@@ -144,7 +144,11 @@ type listedPackage struct {
 	Dir             string
 	CompiledGoFiles []string
 	Imports         []string
-	Incomplete      bool
+
+	Incomplete bool
+	// These two exist only to fill Incomplete.
+	Error      *packageError
+	DepsErrors []*packageError
 
 	// The fields below are not part of 'go list', but are still reused
 	// between garble processes. Use "Garble" as a prefix to ensure no
@@ -157,9 +161,14 @@ type listedPackage struct {
 	GarbleActionID []byte `json:"-"`
 
 	// ToObfuscate records whether the package should be obfuscated.
+	// When true, GarbleActionID must not be empty.
 	ToObfuscate bool `json:"-"`
 }
 
+type packageError struct {
+	Err string
+}
+
 func (p *listedPackage) obfuscatedImportPath() string {
 	// We can't obfuscate these standard library import paths,
 	// as the toolchain expects to recognize the packages by them:
@@ -214,12 +223,42 @@ func appendListedPackages(packages []string, withDeps bool) error {
 	if cache.ListedPackages == nil {
 		cache.ListedPackages = make(map[string]*listedPackage)
 	}
+	var pkgErrors []string
 	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 {
+			pkg.Incomplete = true
+		}
+
+		if pkg.Incomplete {
+			switch {
+			// All errors in non-std packages are fatal,
+			// but only some errors in std packages are.
+			case strings.Contains(pkg.ImportPath, "."):
+				fallthrough
+			default:
+				pkgErrors = append(pkgErrors, pkg.Error.Err)
+				continue
+
+			// 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(pkg.Error.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.
+			case strings.Contains(pkg.Error.Err, "is not in GOROOT"):
+			case strings.Contains(pkg.Error.Err, "cannot find package"):
+			}
+		}
+
 		// Note that we use the `-e` flag above with `go list`.
 		// If a package fails to load, the Incomplete and Error fields will be set.
 		// We still record failed packages in the ListedPackages map,
@@ -237,7 +276,7 @@ func appendListedPackages(packages []string, withDeps bool) error {
 		// Work around https://go.dev/issue/28749:
 		// cmd/go puts assembly, C, and C++ files in CompiledGoFiles.
 		//
-		// TODO: remove when upstream has fixed the bug.
+		// TODO: Fixed in Go 1.20; remove once we drop support for Go 1.19.
 		out := pkg.CompiledGoFiles[:0]
 		for _, path := range pkg.CompiledGoFiles {
 			switch filepath.Ext(path) {
@@ -256,6 +295,9 @@ func appendListedPackages(packages []string, withDeps bool) error {
 	if err := cmd.Wait(); err != nil {
 		return fmt.Errorf("go list error: %v: %s", err, stderr.Bytes())
 	}
+	if len(pkgErrors) > 0 {
+		return errors.New(strings.Join(pkgErrors, "\n"))
+	}
 
 	anyToObfuscate := false
 	for path, pkg := range cache.ListedPackages {
@@ -263,15 +305,18 @@ func appendListedPackages(packages []string, withDeps bool) error {
 		if pkg.ForTest != "" {
 			path = pkg.ForTest
 		}
-		// Test main packages like "foo/bar.test" are always obfuscated,
-		// just like main packages.
 		switch {
+		// We don't support obfuscating these yet.
 		case cannotObfuscate[path]:
-			// We don't support obfuscating these yet.
 
+		// We can't obfuscate packages which weren't loaded.
 		case pkg.Incomplete:
-			// We can't obfuscate packages which weren't loaded.
 
+		// No point in obfuscating empty packages.
+		case len(pkg.CompiledGoFiles) == 0:
+
+		// Test main packages like "foo/bar.test" are always obfuscated,
+		// just like unnamed and plugin main packages.
 		case pkg.Name == "main" && strings.HasSuffix(path, ".test"),
 			path == "command-line-arguments",
 			strings.HasPrefix(path, "plugin/unnamed"),
@@ -279,6 +324,9 @@ func appendListedPackages(packages []string, withDeps bool) error {
 
 			pkg.ToObfuscate = true
 			anyToObfuscate = true
+			if len(pkg.GarbleActionID) == 0 {
+				return fmt.Errorf("package %q to be obfuscated lacks build id?", pkg.ImportPath)
+			}
 		}
 	}
 

testdata/script/asm.txtar

@@ -8,6 +8,11 @@ exec ./main
 cmp stderr main.stderr
 ! binsubstr main$exe 'test/main' 'privateAdd' 'PublicAdd' 'garble_'
 
+# We used to panic on broken packages with assembly.
+! garble build ./broken/...
+stderr -count=1 'broken[/\\]asm[/\\].*MissingType'
+stderr -count=1 'broken[/\\]syntax[/\\].*unexpected newline'
+
 [short] stop # no need to verify this with -short
 
 # Ensure that reversing doesn't error with assembly files.
@@ -91,6 +96,30 @@ package imported
 func PublicAdd(x, y int32) int32
 
 -- imported/imported_amd64.s --
+TEXT ·PublicAdd(SB),$0-16
+	MOVL x+0(FP), BX
+	MOVL y+4(FP), BP
+	ADDL BP, BX
+	MOVL BX, ret+8(FP)
+	RET
+-- broken/syntax/broken.go --
+package broken
+
+func PublicAdd
+
+-- broken/asm/imported_amd64.s --
+TEXT ·PublicAdd(SB),$0-16
+	MOVL x+0(FP), BX
+	MOVL y+4(FP), BP
+	ADDL BP, BX
+	MOVL BX, ret+8(FP)
+	RET
+-- broken/asm/broken.go --
+package broken
+
+func PublicAdd(x, y int32) MissingType
+
+-- broken/imported_amd64.s --
 TEXT ·PublicAdd(SB),$0-16
 	MOVL x+0(FP), BX
 	MOVL y+4(FP), BP