internal/gcimporter: require binary export data header

FindExportData now returns an error if the header indicating
the beginning of the export data is not "$$B\n". (Historically
"$$\n" was also used for non-binary export data.) The
signature of FindExportData no longer returns hdr as it is
redundant.

This also simplifies internal/gcimporter.Import using this
new assumption.

Updates golang/go#70651

Change-Id: I0d136d6c474ae6a7bfc2baeb33c22fd412711452
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633279
Reviewed-by: Robert Griesemer <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Commit-Queue: Tim King <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: timothy-king

go/gcexportdata/gcexportdata.go

@@ -106,7 +106,7 @@ func Find(importPath, srcDir string) (filename, path string) {
 // additional trailing data beyond the end of the export data.
 func NewReader(r io.Reader) (io.Reader, error) {
 	buf := bufio.NewReader(r)
-	_, size, err := gcimporter.FindExportData(buf)
+	size, err := gcimporter.FindExportData(buf)
 	if err != nil {
 		return nil, err
 	}

internal/gcimporter/exportdata.go

@@ -41,14 +41,13 @@ func readGopackHeader(r *bufio.Reader) (name string, size int64, err error) {
 // FindExportData positions the reader r at the beginning of the
 // export data section of an underlying cmd/compile created archive
 // file by reading from it. The reader must be positioned at the
-// start of the file before calling this function. The hdr result
-// is the string before the export data, either "$$" or "$$B".
+// start of the file before calling this function.
 // The size result is the length of the export data in bytes.
 //
 // This function is needed by [gcexportdata.Read], which must
 // accept inputs produced by the last two releases of cmd/compile,
 // plus tip.
-func FindExportData(r *bufio.Reader) (hdr string, size int64, err error) {
+func FindExportData(r *bufio.Reader) (size int64, err error) {
 	// Read first line to make sure this is an object file.
 	line, err := r.ReadSlice('\n')
 	if err != nil {
@@ -90,21 +89,27 @@ func FindExportData(r *bufio.Reader) (hdr string, size int64, err error) {
 		return
 	}
 
-	// Skip over object header to export data.
-	// Begins after first line starting with $$.
+	// Skip over object headers to get to the export data section header "$$B\n".
+	// Object headers are lines that do not start with '$'.
 	for line[0] != '$' {
 		if line, err = r.ReadSlice('\n'); err != nil {
 			err = fmt.Errorf("can't find export data (%v)", err)
 			return
 		}
 		size -= int64(len(line))
 	}
-	hdr = string(line)
-	// TODO(taking): Return an error when hdr != "$$B\n".
+
+	// Check for the binary export data section header "$$B\n".
+	hdr := string(line)
+	if hdr != "$$B\n" {
+		err = fmt.Errorf("unknown export data header: %q", hdr)
+		return
+	}
 	// TODO(taking): Remove end-of-section marker "\n$$\n" from size.
 
 	if size < 0 {
 		err = fmt.Errorf("invalid size (%d) in the archive file: %d bytes remain without section headers (recompile package)", arsize, size)
+		return
 	}
 
 	return

internal/gcimporter/gcimporter.go

@@ -161,6 +161,8 @@ func FindPkg(path, srcDir string) (filename, id string) {
 // Import imports a gc-generated package given its import path and srcDir, adds
 // the corresponding package object to the packages map, and returns the object.
 // The packages map must contain all packages already imported.
+//
+// TODO(taking): Import is only used in tests. Move to gcimporter_test.
 func Import(packages map[string]*types.Package, path, srcDir string, lookup func(path string) (io.ReadCloser, error)) (pkg *types.Package, err error) {
 	var rc io.ReadCloser
 	var filename, id string
@@ -210,58 +212,50 @@ func Import(packages map[string]*types.Package, path, srcDir string, lookup func
 	}
 	defer rc.Close()
 
-	var hdr string
 	var size int64
 	buf := bufio.NewReader(rc)
-	if hdr, size, err = FindExportData(buf); err != nil {
+	if size, err = FindExportData(buf); err != nil {
 		return
 	}
 
-	switch hdr {
-	case "$$B\n":
-		var data []byte
-		data, err = io.ReadAll(buf)
-		if err != nil {
-			break
-		}
+	var data []byte
+	data, err = io.ReadAll(buf)
+	if err != nil {
+		return
+	}
+	if len(data) == 0 {
+		return nil, fmt.Errorf("no data to load a package from for path %s", id)
+	}
 
-		// TODO(gri): allow clients of go/importer to provide a FileSet.
-		// Or, define a new standard go/types/gcexportdata package.
-		fset := token.NewFileSet()
-
-		// Select appropriate importer.
-		if len(data) > 0 {
-			switch data[0] {
-			case 'v', 'c', 'd':
-				// binary: emitted by cmd/compile till go1.10; obsolete.
-				return nil, fmt.Errorf("binary (%c) import format is no longer supported", data[0])
-
-			case 'i':
-				// indexed: emitted by cmd/compile till go1.19;
-				// now used only for serializing go/types.
-				// See https://github.com/golang/go/issues/69491.
-				_, pkg, err := IImportData(fset, packages, data[1:], id)
-				return pkg, err
-
-			case 'u':
-				// unified: emitted by cmd/compile since go1.20.
-				_, pkg, err := UImportData(fset, packages, data[1:size], id)
-				return pkg, err
-
-			default:
-				l := len(data)
-				if l > 10 {
-					l = 10
-				}
-				return nil, fmt.Errorf("unexpected export data with prefix %q for path %s", string(data[:l]), id)
-			}
-		}
+	// TODO(gri): allow clients of go/importer to provide a FileSet.
+	// Or, define a new standard go/types/gcexportdata package.
+	fset := token.NewFileSet()
+
+	// Select appropriate importer.
+	switch data[0] {
+	case 'v', 'c', 'd':
+		// binary: emitted by cmd/compile till go1.10; obsolete.
+		return nil, fmt.Errorf("binary (%c) import format is no longer supported", data[0])
+
+	case 'i':
+		// indexed: emitted by cmd/compile till go1.19;
+		// now used only for serializing go/types.
+		// See https://github.com/golang/go/issues/69491.
+		_, pkg, err := IImportData(fset, packages, data[1:], id)
+		return pkg, err
+
+	case 'u':
+		// unified: emitted by cmd/compile since go1.20.
+		_, pkg, err := UImportData(fset, packages, data[1:size], id)
+		return pkg, err
 
 	default:
-		err = fmt.Errorf("unknown export data header: %q", hdr)
+		l := len(data)
+		if l > 10 {
+			l = 10
+		}
+		return nil, fmt.Errorf("unexpected export data with prefix %q for path %s", string(data[:l]), id)
 	}
-
-	return
 }
 
 type byPath []*types.Package