builder: rescan C dependencies for cache keys

Stop caching C dependency lists as reusable inputs to object cache
lookups. Instead, ask Clang for the current dependency list before each
lookup and key the object by the current dependencies, compiler flags,
and clang/LLVM identity.

This avoids stale object hits when include path resolution changes, such
as when a new header is added earlier in an include path. It also avoids
using dependency data produced by a different clang binary.

Author: jakebailey

builder/cc.go

@@ -10,7 +10,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"os"
 	"path/filepath"
 	"sort"
@@ -25,37 +24,21 @@ import (
 // Compiling the same file again (if nothing changed, including included header
 // files) the output is loaded from the build cache instead.
 //
-// Its operation is a bit complex (more complex than Go package build caching)
-// because the list of file dependencies is only known after the file is
-// compiled. However, luckily compilers have a flag to write a list of file
-// dependencies in Makefile syntax which can be used for caching.
+// Its operation is a bit complex (more complex than Go package build caching),
+// because the list of file dependencies depends on C include path resolution.
+// TinyGo asks Clang for the current dependency list before looking for an object
+// cache hit, then uses the hashes of those dependencies in the object key.
 //
-// Because of this complexity, every file has in fact two cached build outputs:
-// the file itself, and the list of dependencies. Its operation is as follows:
-//
-//	depfile = hash(path, compiler, cflags, ...)
-//	if depfile exists:
-//	  outfile = hash of all files and depfile name
-//	  if outfile exists:
-//	    # cache hit
-//	    return outfile
-//	# cache miss
+//	dependencies = clang -M source
+//	outfile = hash(path, compiler, cflags, dependencies, ...)
+//	if outfile exists:
+//	  # cache hit
+//	  return outfile
 //	tmpfile = compile file
-//	read dependencies (side effect of compile)
-//	write depfile
-//	outfile = hash of all files and depfile name
 //	rename tmpfile to outfile
 //
-// There are a few edge cases that are not handled:
-//   - If a file is added to an include path, that file may be included instead of
-//     some other file. This would be fixed by also including lookup failures in the
-//     dependencies file, but I'm not aware of a compiler which does that.
-//   - The Makefile syntax that compilers output has issues, see readDepFile for
-//     details.
-//   - A header file may be changed to add/remove an include. This invalidates the
-//     depfile but without invalidating its name. For this reason, the depfile is
-//     written on each new compilation (even when it seems unnecessary). However, it
-//     could in rare cases lead to a stale file fetched from the cache.
+// The Makefile syntax that compilers output has issues, see readDepFile for
+// details.
 func compileAndCacheCFile(abspath, tmpdir string, cflags []string, printCommands func(string, ...string)) (string, error) {
 	// Hash input file.
 	fileHash, err := hashFile(abspath)
@@ -67,65 +50,47 @@ func compileAndCacheCFile(abspath, tmpdir string, cflags []string, printCommands
 	unlock := lock(filepath.Join(goenv.Get("GOCACHE"), fileHash+".c.lock"))
 	defer unlock()
 
-	// Create cache key for the dependencies file.
-	buf, err := json.Marshal(struct {
-		Path        string
-		Hash        string
-		Flags       []string
-		LLVMVersion string
-	}{
-		Path:        abspath,
-		Hash:        fileHash,
-		Flags:       cflags,
-		LLVMVersion: llvm.Version,
-	})
+	compilerID, err := clangCompilerIdentity()
 	if err != nil {
-		panic(err) // shouldn't happen
+		return "", err
 	}
-	depfileNameHashBuf := sha512.Sum512_224(buf)
-	depfileNameHash := hex.EncodeToString(depfileNameHashBuf[:])
-
-	// Load dependencies file, if possible.
-	depfileName := "dep-" + depfileNameHash + ".json"
-	depfileCachePath := filepath.Join(goenv.Get("GOCACHE"), depfileName)
-	depfileBuf, err := os.ReadFile(depfileCachePath)
-	var dependencies []string // sorted list of dependency paths
-	if err == nil {
-		// There is a dependency file, that's great!
-		// Parse it first.
-		err := json.Unmarshal(depfileBuf, &dependencies)
-		if err != nil {
-			return "", fmt.Errorf("could not parse dependencies JSON: %w", err)
-		}
 
-		// Obtain hashes of all the files listed as a dependency.
-		outpath, err := makeCFileCachePath(dependencies, depfileNameHash)
-		if err == nil {
-			if _, err := os.Stat(outpath); err == nil {
-				return outpath, nil
-			} else if !errors.Is(err, fs.ErrNotExist) {
-				return "", err
-			}
-		}
-	} else if !errors.Is(err, fs.ErrNotExist) {
-		// expected either nil or IsNotExist
+	dependencies, err := scanCFileDependencies(abspath, tmpdir, cflags, printCommands)
+	if err != nil {
+		return "", err
+	}
+	outpath, err := makeCFileCachePath(abspath, cFileCompileArgs(abspath, "$OBJ", cflags), compilerID, dependencies)
+	if err != nil {
+		return "", err
+	}
+	if _, err := os.Stat(outpath); err == nil {
+		return outpath, nil
+	} else if !errors.Is(err, os.ErrNotExist) {
 		return "", err
 	}
 
-	objTmpFile, err := os.CreateTemp(goenv.Get("GOCACHE"), "tmp-*.bc")
+	objTmpFile, err := compileCFile(goenv.Get("GOCACHE"), abspath, cflags, printCommands)
 	if err != nil {
 		return "", err
 	}
-	objTmpFile.Close()
-	depTmpFile, err := os.CreateTemp(tmpdir, "dep-*.d")
+	err = os.Rename(objTmpFile, outpath)
 	if err != nil {
 		return "", err
 	}
+	return outpath, nil
+}
+
+func scanCFileDependencies(abspath, tmpdir string, cflags []string, printCommands func(string, ...string)) ([]string, error) {
+	depTmpFile, err := os.CreateTemp(tmpdir, "dep-*.d")
+	if err != nil {
+		return nil, err
+	}
 	depTmpFile.Close()
-	flags := append([]string{}, cflags...)                                                 // copy cflags
-	flags = append(flags, "-MD", "-MV", "-MTdeps", "-MF", depTmpFile.Name(), "-flto=thin") // autogenerate dependencies
-	flags = append(flags, "-c", "-o", objTmpFile.Name(), abspath)
-	if strings.ToLower(filepath.Ext(abspath)) == ".s" {
+	defer os.Remove(depTmpFile.Name())
+
+	flags := append([]string{}, cflags...)
+	flags = append(flags, "-M", "-MV", "-MTdeps", "-MF", depTmpFile.Name(), abspath)
+	if isAssemblyFile(abspath) {
 		// If this is an assembly file (.s or .S, lowercase or uppercase), then
 		// we'll need to add -Qunused-arguments because many parameters are
 		// relevant to C, not assembly. And with -Werror, having meaningless
@@ -137,13 +102,12 @@ func compileAndCacheCFile(abspath, tmpdir string, cflags []string, printCommands
 	}
 	err = runCCompiler(flags...)
 	if err != nil {
-		return "", &commandError{"failed to build", abspath, err}
+		return nil, &commandError{"failed to scan dependencies", abspath, err}
 	}
 
-	// Create sorted and uniqued slice of dependencies.
 	dependencyPaths, err := readDepFile(depTmpFile.Name())
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 	dependencyPaths = append(dependencyPaths, abspath) // necessary for .s files
 	dependencySet := make(map[string]struct{}, len(dependencyPaths))
@@ -156,50 +120,44 @@ func compileAndCacheCFile(abspath, tmpdir string, cflags []string, printCommands
 		dependencySlice = append(dependencySlice, path)
 	}
 	sort.Strings(dependencySlice)
+	return dependencySlice, nil
+}
 
-	// Write dependencies file.
-	f, err := os.CreateTemp(filepath.Dir(depfileCachePath), depfileName)
-	if err != nil {
-		return "", err
+func cFileCompileArgs(abspath, objpath string, cflags []string) []string {
+	flags := append([]string{}, cflags...)
+	flags = append(flags, "-flto=thin")
+	flags = append(flags, "-c", "-o", objpath, abspath)
+	if isAssemblyFile(abspath) {
+		flags = append(flags, "-Qunused-arguments")
 	}
+	return flags
+}
 
-	buf, err = json.MarshalIndent(dependencySlice, "", "\t")
-	if err != nil {
-		panic(err) // shouldn't happen
-	}
-	_, err = f.Write(buf)
-	if err != nil {
-		return "", err
-	}
-	err = f.Close()
-	if err != nil {
-		return "", err
-	}
-	err = os.Rename(f.Name(), depfileCachePath)
+func compileCFile(cacheDir, abspath string, cflags []string, printCommands func(string, ...string)) (string, error) {
+	objTmpFile, err := os.CreateTemp(cacheDir, "tmp-*.bc")
 	if err != nil {
 		return "", err
 	}
+	objTmpFile.Close()
 
-	// Move temporary object file to final location.
-	outpath, err := makeCFileCachePath(dependencySlice, depfileNameHash)
-	if err != nil {
-		return "", err
+	flags := cFileCompileArgs(abspath, objTmpFile.Name(), cflags)
+	if printCommands != nil {
+		printCommands("clang", flags...)
 	}
-	err = os.Rename(objTmpFile.Name(), outpath)
+	err = runCCompiler(flags...)
 	if err != nil {
-		return "", err
+		return "", &commandError{"failed to build", abspath, err}
 	}
-
-	return outpath, nil
+	return objTmpFile.Name(), nil
 }
 
 // Create a cache path (a path in GOCACHE) to store the output of a compiler
-// job. This path is based on the dep file name (which is a hash of metadata
-// including compiler flags) and the hash of all input files in the paths slice.
-func makeCFileCachePath(paths []string, depfileNameHash string) (string, error) {
+// job. This path is based on the compiler identity, compiler flags, and the
+// hash of all dependency files.
+func makeCFileCachePath(path string, flags []string, compilerID string, dependencies []string) (string, error) {
 	// Hash all input files.
-	fileHashes := make(map[string]string, len(paths))
-	for _, path := range paths {
+	fileHashes := make(map[string]string, len(dependencies))
+	for _, path := range dependencies {
 		hash, err := hashFile(path)
 		if err != nil {
 			return "", err
@@ -209,11 +167,17 @@ func makeCFileCachePath(paths []string, depfileNameHash string) (string, error)
 
 	// Calculate a cache key based on the above hashes.
 	buf, err := json.Marshal(struct {
-		DepfileHash string
-		FileHashes  map[string]string
+		Path             string
+		Flags            []string
+		LLVMVersion      string
+		CompilerIdentity string
+		FileHashes       map[string]string
 	}{
-		DepfileHash: depfileNameHash,
-		FileHashes:  fileHashes,
+		Path:             path,
+		Flags:            flags,
+		LLVMVersion:      llvm.Version,
+		CompilerIdentity: compilerID,
+		FileHashes:       fileHashes,
 	})
 	if err != nil {
 		panic(err) // shouldn't happen
@@ -225,6 +189,10 @@ func makeCFileCachePath(paths []string, depfileNameHash string) (string, error)
 	return outpath, nil
 }
 
+func isAssemblyFile(path string) bool {
+	return strings.ToLower(filepath.Ext(path)) == ".s"
+}
+
 // hashFile hashes the given file path and returns the hash as a hex string.
 func hashFile(path string) (string, error) {
 	f, err := os.Open(path)

builder/cc_test.go

@@ -1,6 +1,8 @@
 package builder
 
 import (
+	"os"
+	"path/filepath"
 	"reflect"
 	"testing"
 )
@@ -31,3 +33,53 @@ func TestSplitDepFile(t *testing.T) {
 		}
 	}
 }
+
+func TestCFileCacheIncludePathShadowing(t *testing.T) {
+	t.Setenv("GOCACHEPROG", "")
+
+	dir := t.TempDir()
+
+	include1 := filepath.Join(dir, "include1")
+	include2 := filepath.Join(dir, "include2")
+	if err := os.Mkdir(include1, 0o777); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Mkdir(include2, 0o777); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(include2, "value.h"), []byte("#define VALUE 1\n"), 0o666); err != nil {
+		t.Fatal(err)
+	}
+	source := filepath.Join(dir, "test.c")
+	if err := os.WriteFile(source, []byte("#include \"value.h\"\nint value(void) { return VALUE; }\n"), 0o666); err != nil {
+		t.Fatal(err)
+	}
+
+	flags := []string{
+		"-I", include1,
+		"-I", include2,
+		"--target=x86_64-unknown-linux-gnu",
+	}
+	first, err := compileAndCacheCFile(source, dir, flags, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	second, err := compileAndCacheCFile(source, dir, flags, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if first != second {
+		t.Fatalf("unchanged compile did not hit cache: %s != %s", first, second)
+	}
+
+	if err := os.WriteFile(filepath.Join(include1, "value.h"), []byte("#define VALUE 2\n"), 0o666); err != nil {
+		t.Fatal(err)
+	}
+	shadowed, err := compileAndCacheCFile(source, dir, flags, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if shadowed == first {
+		t.Fatal("include path shadowing reused stale cached object")
+	}
+}