[dev.link] cmd/link: add defined package symbols first, before any non-package symbols

Currently, the loader adds defined package symbols and
non-package symbols to the global index space object by object.
This CL changes it to add all the defined package symbols first,
then all the non-package symbols. The advantage of doing this is
that when adding package symbols, by definition they cannot be
dup to each other, so we don't need to do a name lookup when
adding them.

We still add them to the lookup table (for now), since they may
still be referenced by name (e.g. through linkname). This CL is
also a prerequisite if we want to move to not adding package
symbols to the lookup table entirely (e.g. by using
pre-generated in-file lookup table).

Also update some comments to reflect the current state.

Change-Id: Ib757e070b48a9ef6215e47dc3421fc5c055b746c
Reviewed-on: https://go-review.googlesource.com/c/go/+/219078
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>

src/cmd/link/internal/ld/lib.go

@@ -422,30 +422,33 @@ func (ctxt *Link) loadlib() {
 			loadobjfile(ctxt, lib)
 		}
 	}
+	// At this point, the Go objects are "preloaded". Not all the symbols are
+	// added to the symbol table (only defined package symbols are). Looking
+	// up symbol by name may not get expected result.
 
-	iscgo = ctxt.loader.Lookup("x_cgo_init", 0) != 0
-	ctxt.canUsePlugins = ctxt.loader.Lookup("plugin.Open", sym.SymVerABIInternal) != 0
+	iscgo = ctxt.LibraryByPkg["runtime/cgo"] != nil
+	ctxt.canUsePlugins = ctxt.LibraryByPkg["plugin"] != nil
 
 	// We now have enough information to determine the link mode.
 	determineLinkMode(ctxt)
 
-	if ctxt.LinkMode == LinkExternal && !iscgo && ctxt.LibraryByPkg["runtime/cgo"] == nil && !(objabi.GOOS == "darwin" && ctxt.BuildMode != BuildModePlugin && (ctxt.Arch.Family == sys.AMD64 || ctxt.Arch.Family == sys.I386)) {
+	if ctxt.LinkMode == LinkExternal && !iscgo && !(objabi.GOOS == "darwin" && ctxt.BuildMode != BuildModePlugin && (ctxt.Arch.Family == sys.AMD64 || ctxt.Arch.Family == sys.I386)) {
 		// This indicates a user requested -linkmode=external.
 		// The startup code uses an import of runtime/cgo to decide
 		// whether to initialize the TLS.  So give it one. This could
 		// be handled differently but it's an unusual case.
-		if lib := loadinternal(ctxt, "runtime/cgo"); lib != nil {
-			if lib.Shlib != "" {
-				ldshlibsyms(ctxt, lib.Shlib)
-			} else {
+		if lib := loadinternal(ctxt, "runtime/cgo"); lib != nil && lib.Shlib == "" {
 			if ctxt.BuildMode == BuildModeShared || ctxt.linkShared {
 				Exitf("cannot implicitly include runtime/cgo in a shared library")
 			}
 			loadobjfile(ctxt, lib)
 		}
 	}
-	}
 
+	// Add non-package symbols and references of externally defined symbols.
+	ctxt.loader.LoadNonpkgSyms(ctxt.Arch, ctxt.Syms)
+
+	// Load symbols from shared libraries, after all Go object symbols are loaded.
 	for _, lib := range ctxt.Library {
 		if lib.Shlib != "" {
 			if ctxt.Debugvlog > 1 {
@@ -455,9 +458,6 @@ func (ctxt *Link) loadlib() {
 		}
 	}
 
-	// Add references of externally defined symbols.
-	ctxt.loader.LoadRefs(ctxt.Arch, ctxt.Syms)
-
 	// Process cgo directives (has to be done before host object loading).
 	ctxt.loadcgodirectives()
 

src/cmd/link/internal/loader/loader.go

@@ -125,31 +125,20 @@ func growBitmap(reqLen int, b bitmap) bitmap {
 //
 // Notes on the layout of global symbol index space:
 //
-// TODO: rework index space reservation.
-//
 // - Go object files are read before host object files; each Go object
-//   read adds its defined (package + non-package) symbols to the global
-//   index space.
+//   read adds its defined package symbols to the global index space.
+//   Nonpackage symbols are not yet added.
 //
-// - In loader.LoadRefs(), the loader makes a sweep through all of the
-//   non-package references in each object file and allocates sym indices
-//   for any symbols that have not yet been defined (start of this space
-//   is marked by loader.extStart).
+// - In loader.LoadNonpkgSyms, add non-package defined symbols and
+//   references in all object files to the global index space.
 //
 // - Host object file loading happens; the host object loader does a
 //   name/version lookup for each symbol it finds; this can wind up
 //   extending the external symbol index space range. The host object
-//   loader currently stores symbol payloads in sym.Symbol objects,
-//   which get handed off to the loader.
+//   loader stores symbol payloads in loader.payloads using SymbolBuilder.
 //
-// - A given external symbol (Sym) either has a sym.Symbol acting as
-//   its backing store (this will continue to be the case until we
-//   finish rewriting the host object loader to work entirely with
-//   loader.Sym) or it has a "payload" backing store (represented by
-//   extSymPayload). Newly created external symbols (created by
-//   a call to AddExtSym or equivalent) start out in the "has payload"
-//   state, and continue until installSym is called for the sym
-//   index in question.
+// - For now, in loader.LoadFull we convert all symbols (Go + external)
+//   to sym.Symbols.
 //
 // - At some point (when the wayfront is pushed through all of the
 //   linker), all external symbols will be payload-based, and we can
@@ -237,6 +226,12 @@ type Loader struct {
 	elfsetstring elfsetstringFunc
 }
 
+const (
+	pkgDef    = iota
+	nonPkgDef
+	nonPkgRef
+)
+
 type elfsetstringFunc func(s *sym.Symbol, str string, off int)
 
 // extSymPayload holds the payload (data + relocations) for linker-synthesized
@@ -299,17 +294,15 @@ func (l *Loader) addObj(pkg string, r *oReader) Sym {
 	if _, ok := l.objByPkg[pkg]; !ok {
 		l.objByPkg[pkg] = r
 	}
-	n := r.NSym() + r.NNonpkgdef()
 	i := Sym(len(l.objSyms))
 	l.start[r] = i
 	l.objs = append(l.objs, objIdx{r, i})
-	l.growValues(int(i) + n - 1)
 	return i
 }
 
 // Add a symbol from an object file, return the global index and whether it is added.
 // If the symbol already exist, it returns the index of that symbol.
-func (l *Loader) AddSym(name string, ver int, r *oReader, li int, dupok bool, typ sym.SymKind) (Sym, bool) {
+func (l *Loader) AddSym(name string, ver int, r *oReader, li int, kind int, dupok bool, typ sym.SymKind) (Sym, bool) {
 	if l.extStart != 0 {
 		panic("AddSym called after AddExtSym is called")
 	}
@@ -328,6 +321,18 @@ func (l *Loader) AddSym(name string, ver int, r *oReader, li int, dupok bool, ty
 		addToGlobal()
 		return i, true
 	}
+	if kind == pkgDef {
+		// Defined package symbols cannot be dup to each other.
+		// We load all the package symbols first, so we don't need
+		// to check dup here.
+		// We still add it to the lookup table, as it may still be
+		// referenced by name (e.g. through linkname).
+		l.symsByName[ver][name] = i
+		addToGlobal()
+		return i, true
+	}
+
+	// Non-package (named) symbol. Check if it already exists.
 	oldi, existed := l.symsByName[ver][name]
 	if !existed {
 		l.symsByName[ver][name] = i
@@ -1468,8 +1473,9 @@ func (x RelocByOff) Len() int           { return len(x) }
 func (x RelocByOff) Swap(i, j int)      { x[i], x[j] = x[j], x[i] }
 func (x RelocByOff) Less(i, j int) bool { return x[i].Off < x[j].Off }
 
-// Preload a package: add autolibs, add symbols to the symbol table.
-// Does not read symbol data yet.
+// Preload a package: add autolibs, add defined package symbols to the symbol table.
+// Does not add non-package symbols yet, which will be done in LoadNonpkgSyms.
+// Does not read symbol data.
 func (l *Loader) Preload(arch *sys.Arch, syms *sym.Symbols, f *bio.Reader, lib *sym.Library, unit *sym.CompilationUnit, length int64, pn string, flags int) {
 	roObject, readonly, err := f.Slice(uint64(length))
 	if err != nil {
@@ -1495,16 +1501,38 @@ func (l *Loader) Preload(arch *sys.Arch, syms *sym.Symbols, f *bio.Reader, lib *
 		unit.DWARFFileTable[i] = r.DwarfFile(i)
 	}
 
-	istart := l.addObj(lib.Pkg, or)
-	l.growAttrBitmaps(int(istart) + ndef + nnonpkgdef)
-	for i, n := 0, ndef+nnonpkgdef; i < n; i++ {
+	l.addObj(lib.Pkg, or)
+	l.preloadSyms(or, pkgDef)
+
+	// The caller expects us consuming all the data
+	f.MustSeek(length, os.SEEK_CUR)
+}
+
+// Preload symbols of given kind from an object.
+func (l *Loader) preloadSyms(r *oReader, kind int) {
+	ndef := r.NSym()
+	nnonpkgdef := r.NNonpkgdef()
+	var start, end int
+	switch kind {
+	case pkgDef:
+		start = 0
+		end = ndef
+	case nonPkgDef:
+		start = ndef
+		end = ndef + nnonpkgdef
+	default:
+		panic("preloadSyms: bad kind")
+	}
+	l.growSyms(len(l.objSyms) + end - start)
+	l.growAttrBitmaps(len(l.objSyms) + end - start)
+	for i := start; i < end; i++ {
 		osym := goobj2.Sym{}
-		osym.Read(r, r.SymOff(i))
-		name := strings.Replace(osym.Name, "\"\".", pkgprefix, -1)
-		v := abiToVer(osym.ABI, localSymVersion)
+		osym.Read(r.Reader, r.SymOff(i))
+		name := strings.Replace(osym.Name, "\"\".", r.pkgprefix, -1)
+		v := abiToVer(osym.ABI, r.version)
 		dupok := osym.Dupok()
-		gi, added := l.AddSym(name, v, or, i, dupok, sym.AbiSymKindToSymKind[objabi.SymKind(osym.Type)])
-		or.syms[i] = gi
+		gi, added := l.AddSym(name, v, r, i, kind, dupok, sym.AbiSymKindToSymKind[objabi.SymKind(osym.Type)])
+		r.syms[i] = gi
 		if !added {
 			continue
 		}
@@ -1522,14 +1550,14 @@ func (l *Loader) Preload(arch *sys.Arch, syms *sym.Symbols, f *bio.Reader, lib *
 			l.SetAttrNotInSymbolTable(gi, true)
 		}
 	}
-
-	// The caller expects us consuming all the data
-	f.MustSeek(length, os.SEEK_CUR)
 }
 
-// Make sure referenced symbols are added. Most of them should already be added.
-// This should only be needed for referenced external symbols.
-func (l *Loader) LoadRefs(arch *sys.Arch, syms *sym.Symbols) {
+// Add non-package symbols and references to external symbols (which are always
+// named).
+func (l *Loader) LoadNonpkgSyms(arch *sys.Arch, syms *sym.Symbols) {
+	for _, o := range l.objs[1:] {
+		l.preloadSyms(o.r, nonPkgDef)
+	}
 	for _, o := range l.objs[1:] {
 		loadObjRefs(l, o.r, arch, syms)
 	}

src/cmd/link/internal/loader/loader_test.go

@@ -20,7 +20,7 @@ import (
 // data or relocations).
 func addDummyObjSym(t *testing.T, ldr *Loader, or *oReader, name string) Sym {
 	idx := len(ldr.objSyms)
-	s, ok := ldr.AddSym(name, 0, or, idx, false, sym.SRODATA)
+	s, ok := ldr.AddSym(name, 0, or, idx, nonPkgDef, false, sym.SRODATA)
 	if !ok {
 		t.Errorf("AddrSym failed for '" + name + "'")
 	}