cmd/go: separate compile and link steps in action graph

To the extent that invoking the compiler and invoking the linker
have different dependency requirements, representing both steps
by a single action node leads to confusion.

If we move to having separate .a and .x (import metadata) files
in the standard builds, then the .a is a link dependency but not
a compile dependency, and vice versa for .x.
Today, in shared library builds, the .a is a compile dependency
and a link dependency, while the .so is only a link dependency.

Also in this CL: change the gccgo link step to extract _cgo_flags
into root.Objdir, which is private to the link step, instead of into
b.WorkDir, which is shared by all the link steps that could possibly
be running in parallel. And attempt to handle the -n and -x flags
when loading _cgo_flags, instead of dying attempting to read
an archive that wasn't written.

Also in this CL: create a.Objdir before running a.Func, to avoid
duplicating the Mkdir(a.Objdir) in every a.Func.

A future CL will update the link action's Deps to be accurate.
(Right now the link steps search out the true Deps by walking
the entire action graph.)

Change-Id: I15128ce2bd064887f98abc3a4cf204241f518631
Reviewed-on: https://go-review.googlesource.com/69830
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index 53361bb5d77fb9c32c936599f9133e465bb2c581..ae8fd67df6acb04edf4cda87af57272fc70067c5 100644 (file)
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -729,10 +729,10 @@ func TestBuildComplex(t *testing.T) {
        defer tg.cleanup()
        tg.parallel()
        tg.setenv("GOPATH", filepath.Join(tg.pwd(), "testdata"))
-       tg.run("build", "-o", os.DevNull, "complex")
+       tg.run("build", "-x", "-o", os.DevNull, "complex")
 
        if _, err := exec.LookPath("gccgo"); err == nil {
-               tg.run("build", "-o", os.DevNull, "-compiler=gccgo", "complex")
+               tg.run("build", "-x", "-o", os.DevNull, "-compiler=gccgo", "complex")
        }
 }
 

diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go
index 0d21194287dbeccc5b48bf518d69e10d22c273f2..719a155fe8306207dc6a0c66f8b8b35fe5400f24 100644 (file)
--- a/src/cmd/go/internal/test/test.go
+++ b/src/cmd/go/internal/test/test.go
@@ -896,11 +896,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin
 
        load.ComputeStale(pmain)
 
-       if ptest != p {
-               a := b.Action(work.ModeBuild, work.ModeBuild, ptest)
-               a.Link = false
-       }
-
        a := b.Action(work.ModeBuild, work.ModeBuild, pmain)
        a.Target = testDir + testBinary + cfg.ExeSuffix
        if cfg.Goos == "windows" {

diff --git a/src/cmd/go/internal/work/build.go b/src/cmd/go/internal/work/build.go
index 13bbabf65bdfa22af4d2e076f169331e12da947d..ead79f381cff7aa502ef67a1c91ab3d90bc3ca7f 100644 (file)
--- a/src/cmd/go/internal/work/build.go
+++ b/src/cmd/go/internal/work/build.go
@@ -691,7 +691,6 @@ type Action struct {
        triggers []*Action // inverse of deps
 
        // Generated files, directories.
-       Link   bool   // target is executable, not just package
        Objdir string // directory for intermediate objects
        Target string // goal of the action: the created package or executable
 
@@ -749,7 +748,6 @@ func actionGraphJSON(a *Action) string {
                        ID:         id,
                        IgnoreFail: a.IgnoreFail,
                        Args:       a.Args,
-                       Link:       a.Link,
                        Objdir:     a.Objdir,
                        Target:     a.Target,
                        Failed:     a.Failed,
@@ -955,7 +953,7 @@ func (b *Builder) action1(mode BuildMode, depMode BuildMode, p *load.Package, lo
                mode = ModeBuild
        }
        a.Objdir = b.NewObjdir()
-       a.Link = p.Name == "main" && !p.Internal.ForceLibrary
+       link := p.Name == "main" && !p.Internal.ForceLibrary
 
        switch mode {
        case ModeInstall:
@@ -989,7 +987,16 @@ func (b *Builder) action1(mode BuildMode, depMode BuildMode, p *load.Package, lo
                a.Func = (*Builder).build
                a.Target = a.Objdir + "_pkg_.a"
                a.Package.Internal.Pkgfile = a.Target
-               if a.Link {
+
+               if link {
+                       a = &Action{
+                               Mode:    "link",
+                               Func:    (*Builder).link,
+                               Package: a.Package,
+                               Objdir:  a.Objdir,
+                               Deps:    []*Action{a},
+                       }
+
                        // An executable file. (This is the name of a temporary file.)
                        // Because we run the temporary file in 'go run' and 'go test',
                        // the name will show up in ps listings. If the caller has specified
@@ -1018,7 +1025,10 @@ func (b *Builder) action1(mode BuildMode, depMode BuildMode, p *load.Package, lo
 }
 
 func (b *Builder) libaction(libname string, pkgs []*load.Package, mode, depMode BuildMode) *Action {
-       a := &Action{Mode: "libaction???"}
+       a := &Action{
+               Mode:   "libaction", // should be overwritten below
+               Objdir: b.NewObjdir(),
+       }
        switch mode {
        default:
                base.Fatalf("unrecognized mode %v", mode)
@@ -1201,8 +1211,14 @@ func (b *Builder) Do(root *Action) {
        // any actions that are runnable as a result.
        handle := func(a *Action) {
                var err error
+
                if a.Func != nil && (!a.Failed || a.IgnoreFail) {
-                       err = a.Func(b, a)
+                       if a.Objdir != "" {
+                               err = b.Mkdir(a.Objdir)
+                       }
+                       if err == nil {
+                               err = a.Func(b, a)
+                       }
                }
 
                // The actions run in parallel but all the updates to the
@@ -1316,11 +1332,7 @@ func (b *Builder) build(a *Action) (err error) {
                b.Print(a.Package.ImportPath + "\n")
        }
 
-       // Make build directory.
        objdir := a.Objdir
-       if err := b.Mkdir(objdir); err != nil {
-               return err
-       }
 
        // make target directory
        dir, _ := filepath.Split(a.Target)
@@ -1554,22 +1566,32 @@ func (b *Builder) build(a *Action) (err error) {
                }
        }
 
-       // Link if needed.
-       if a.Link {
-               importcfg := a.Objdir + "importcfg.link"
-               if err := b.writeLinkImportcfg(a, importcfg); err != nil {
-                       return err
-               }
+       return nil
+}
 
-               // The compiler only cares about direct imports, but the
-               // linker needs the whole dependency tree.
-               all := ActionList(a)
-               all = all[:len(all)-1] // drop a
-               if err := BuildToolchain.ld(b, a, a.Target, importcfg, all, objpkg, objects); err != nil {
+func (b *Builder) link(a *Action) (err error) {
+       importcfg := a.Objdir + "importcfg.link"
+       if err := b.writeLinkImportcfg(a, importcfg); err != nil {
+               return err
+       }
+
+       // make target directory
+       dir, _ := filepath.Split(a.Target)
+       if dir != "" {
+               if err := b.Mkdir(dir); err != nil {
                        return err
                }
        }
 
+       // The compiler only cares about direct imports, but the
+       // linker needs the whole dependency tree.
+       all := ActionList(a)
+       all = all[:len(all)-1] // drop a
+       objpkg := a.Objdir + "_pkg_.a"
+       if err := BuildToolchain.ld(b, a, a.Target, importcfg, all, objpkg, nil); err != nil { // TODO: ofiles
+               return err
+       }
+
        return nil
 }
 
@@ -1698,7 +1720,7 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) {
        }()
        a1 := a.Deps[0]
        perm := os.FileMode(0666)
-       if a1.Link {
+       if a1.Mode == "link" {
                switch cfg.BuildBuildmode {
                case "c-archive", "c-shared", "plugin":
                default:
@@ -2859,7 +2881,7 @@ func (gccgoToolchain) pack(b *Builder, a *Action, afile string, ofiles []string)
        return b.run(p.Dir, p.ImportPath, nil, "ar", "rc", mkAbs(objdir, afile), absOfiles)
 }
 
-func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string, allactions []*Action, mainpkg string, ofiles []string, buildmode, desc string) error {
+func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string, allactions []*Action, buildmode, desc string) error {
        // gccgo needs explicit linking with all package dependencies,
        // and all LDFLAGS from cgo dependencies.
        apackagePathsSeen := make(map[string]bool)
@@ -2899,42 +2921,36 @@ func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string
                return nil
        }
 
+       newID := 0
        readAndRemoveCgoFlags := func(archive string) (string, error) {
-               newa, err := ioutil.TempFile(b.WorkDir, filepath.Base(archive))
-               if err != nil {
-                       return "", err
-               }
-               olda, err := os.Open(archive)
-               if err != nil {
-                       return "", err
-               }
-               _, err = io.Copy(newa, olda)
-               if err != nil {
+               newID++
+               newArchive := root.Objdir + fmt.Sprintf("_pkg%d_.a", newID)
+               if err := b.copyFile(root, newArchive, archive, 0666, false); err != nil {
                        return "", err
                }
-               err = olda.Close()
-               if err != nil {
-                       return "", err
-               }
-               err = newa.Close()
-               if err != nil {
-                       return "", err
+               if cfg.BuildN || cfg.BuildX {
+                       b.Showcmd("", "ar d %s _cgo_flags", newArchive)
+                       if cfg.BuildN {
+                               // TODO(rsc): We could do better about showing the right _cgo_flags even in -n mode.
+                               // Either the archive is already built and we can read them out,
+                               // or we're printing commands to build the archive and can
+                               // forward the _cgo_flags directly to this step.
+                               return "", nil
+                       }
                }
-
-               newarchive := newa.Name()
-               err = b.run(b.WorkDir, desc, nil, "ar", "x", newarchive, "_cgo_flags")
+               err := b.run(root.Objdir, desc, nil, "ar", "x", newArchive, "_cgo_flags")
                if err != nil {
                        return "", err
                }
-               err = b.run(".", desc, nil, "ar", "d", newarchive, "_cgo_flags")
+               err = b.run(".", desc, nil, "ar", "d", newArchive, "_cgo_flags")
                if err != nil {
                        return "", err
                }
-               err = readCgoFlags(filepath.Join(b.WorkDir, "_cgo_flags"))
+               err = readCgoFlags(filepath.Join(root.Objdir, "_cgo_flags"))
                if err != nil {
                        return "", err
                }
-               return newarchive, nil
+               return newArchive, nil
        }
 
        actionsSeen := make(map[*Action]bool)
@@ -3018,14 +3034,6 @@ func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string
                }
        }
 
-       for i, o := range ofiles {
-               if filepath.Base(o) == "_cgo_flags" {
-                       readCgoFlags(o)
-                       ofiles = append(ofiles[:i], ofiles[i+1:]...)
-                       break
-               }
-       }
-
        ldflags = append(ldflags, "-Wl,--whole-archive")
        ldflags = append(ldflags, afiles...)
        ldflags = append(ldflags, "-Wl,--no-whole-archive")
@@ -3112,7 +3120,7 @@ func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string
                }
        }
 
-       if err := b.run(".", desc, nil, tools.linker(), "-o", out, ofiles, ldflags, buildGccgoflags); err != nil {
+       if err := b.run(".", desc, nil, tools.linker(), "-o", out, ldflags, buildGccgoflags); err != nil {
                return err
        }
 
@@ -3126,13 +3134,13 @@ func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string
 }
 
 func (tools gccgoToolchain) ld(b *Builder, root *Action, out, importcfg string, allactions []*Action, mainpkg string, ofiles []string) error {
-       return tools.link(b, root, out, importcfg, allactions, mainpkg, ofiles, ldBuildmode, root.Package.ImportPath)
+       return tools.link(b, root, out, importcfg, allactions, ldBuildmode, root.Package.ImportPath)
 }
 
 func (tools gccgoToolchain) ldShared(b *Builder, toplevelactions []*Action, out, importcfg string, allactions []*Action) error {
        fakeRoot := &Action{Mode: "gccgo ldshared"}
        fakeRoot.Deps = toplevelactions
-       return tools.link(b, fakeRoot, out, importcfg, allactions, "", nil, "shared", out)
+       return tools.link(b, fakeRoot, out, importcfg, allactions, "shared", out)
 }
 
 func (tools gccgoToolchain) cc(b *Builder, a *Action, ofile, cfile string) error {