all: review fixes

pkg/covermerger/covermerger.go

@@ -20,7 +20,6 @@ const (
 	KeyFilePath     = "file_path"
 	KeyStartLine    = "sl"
 	KeyHitCount     = "hit_count"
-	KeyArch         = "arch"
 )
 
 type FileRecord map[string]string
@@ -46,35 +45,31 @@ type Frame struct {
 	EndCol    int
 }
 
-func (fr FileRecord) Frame() Frame {
-	f := Frame{}
+func (fr FileRecord) Frame() (*Frame, error) {
+	f := &Frame{}
 	var err error
 	if f.StartLine, err = strconv.Atoi(fr[KeyStartLine]); err != nil {
-		panic(fmt.Sprintf("failed to Atoi(%s)", fr[KeyStartLine]))
+		return nil, fmt.Errorf("failed to Atoi(%s): %w", fr[KeyStartLine], err)
 	}
-	return f
+	return f, nil
 }
 
-func (fr FileRecord) HitCount() int {
+func (fr FileRecord) HitCount() (int, error) {
 	if hitCount, err := strconv.Atoi(fr[KeyHitCount]); err != nil {
-		panic(fmt.Sprintf("failed to Atoi(%s)", fr[KeyHitCount]))
+		return 0, fmt.Errorf("failed to Atoi(%s): %w", fr[KeyHitCount], err)
 	} else {
-		return hitCount
+		return hitCount, nil
 	}
 }
 
-func (fr FileRecord) Arch() string {
-	return fr[KeyArch]
-}
-
 type MergeResult struct {
 	HitCounts  map[int]int
 	FileExists bool
 	LostFrames map[RepoBranchCommit]int64
 }
 
 type FileCoverageMerger interface {
-	AddRecord(rbc RepoBranchCommit, arch string, f Frame, hitCount int)
+	AddRecord(rbc RepoBranchCommit, f *Frame, hitCount int)
 	Result() *MergeResult
 }
 
@@ -85,9 +80,7 @@ func batchFileData(c *Config, targetFilePath string, records FileRecords,
 	for _, record := range records {
 		repoBranchCommitsMap[record.RepoBranchCommit()] = true
 	}
-	if c.BaseType == BaseManual {
-		repoBranchCommitsMap[c.Base] = true
-	}
+	repoBranchCommitsMap[c.Base] = true
 	repoBranchCommits := maps.Keys(repoBranchCommitsMap)
 	getFiles := getFileVersions
 	if c.getFileVersionsMock != nil {
@@ -97,35 +90,24 @@ func batchFileData(c *Config, targetFilePath string, records FileRecords,
 	if err != nil {
 		return nil, fmt.Errorf("failed to getFileVersions: %w", err)
 	}
-	base := getBaseRBC(c, targetFilePath, fvs)
-	merger := makeFileLineCoverMerger(fvs, base)
+	merger := makeFileLineCoverMerger(fvs, c.Base)
 	for _, record := range records {
+		var f *Frame
+		if f, err = record.Frame(); err != nil {
+			return nil, fmt.Errorf("error parsing %s records: %w", targetFilePath, err)
+		}
+		var hitCount int
+		if hitCount, err = record.HitCount(); err != nil {
+			return nil, fmt.Errorf("error parsing %s records: %w", targetFilePath, err)
+		}
 		merger.AddRecord(
 			record.RepoBranchCommit(),
-			record.Arch(),
-			record.Frame(),
-			record.HitCount())
+			f,
+			hitCount)
 	}
 	return merger.Result(), nil
 }
 
-// getBaseRBC is a base(target) file version selector.
-// The easiest strategy is to use some specified commit.
-// For the namespace level signals merging we'll select target dynamically.
-func getBaseRBC(c *Config, targetFilePath string, fvs fileVersions) RepoBranchCommit {
-	switch c.BaseType {
-	case BaseManual:
-		return c.Base
-	case BaseLastUpdated:
-		// If repo is not specifies use the much more expensive approach.
-		// The base commit is the commit where non-empty target file was last modified.
-		if res := freshestRBC(fvs); res != nil {
-			return *res
-		}
-	}
-	panic(fmt.Sprintf("failed searching best RBC for file %s", targetFilePath))
-}
-
 func makeRecord(fields, schema []string) FileRecord {
 	record := make(FileRecord)
 	if len(fields) != len(schema) {
@@ -138,15 +120,9 @@ func makeRecord(fields, schema []string) FileRecord {
 	return record
 }
 
-const (
-	BaseManual = iota
-	BaseLastUpdated
-)
-
 type Config struct {
 	Workdir             string
 	skipRepoClone       bool
-	BaseType            int              // BaseManual, BaseLastUpdated.
 	Base                RepoBranchCommit // used by BaseManual
 	getFileVersionsMock func(*Config, string, []RepoBranchCommit) (fileVersions, error)
 }

pkg/covermerger/covermerger_test.go

@@ -123,7 +123,6 @@ samp_time,1,360,arch,b1,ci-mock,git://repo,master,commit2,not_changed.c,func1,4,
 				&Config{
 					Workdir:       test.workdir,
 					skipRepoClone: true,
-					BaseType:      BaseManual,
 					Base: RepoBranchCommit{
 						Repo:   test.baseRepo,
 						Branch: test.baseBranch,

pkg/covermerger/deleted_file_merger.go

@@ -6,7 +6,7 @@ package covermerger
 type DeletedFileLineMerger struct {
 }
 
-func (a *DeletedFileLineMerger) AddRecord(RepoBranchCommit, string, Frame, int) {
+func (a *DeletedFileLineMerger) AddRecord(RepoBranchCommit, *Frame, int) {
 }
 
 func (a *DeletedFileLineMerger) Result() *MergeResult {

pkg/covermerger/file_line_merger.go

@@ -3,10 +3,6 @@
 
 package covermerger
 
-import (
-	"time"
-)
-
 func makeFileLineCoverMerger(
 	fvs fileVersions, base RepoBranchCommit) FileCoverageMerger {
 	baseFile := ""
@@ -34,22 +30,6 @@ func makeFileLineCoverMerger(
 	return a
 }
 
-// freshestRBC returns RepoBranchCommit with the last modified non-empty file.
-func freshestRBC(fvs fileVersions) *RepoBranchCommit {
-	var res *RepoBranchCommit
-	var resLastUpdated time.Time
-	for rbc, fv := range fvs {
-		if fv.content == "" {
-			continue
-		}
-		if res == nil || resLastUpdated.Before(fv.lastUpdated) {
-			res = &rbc
-			resLastUpdated = fv.lastUpdated
-		}
-	}
-	return res
-}
-
 type FileLineCoverMerger struct {
 	rbcToFile  fileVersions
 	baseFile   string
@@ -58,7 +38,7 @@ type FileLineCoverMerger struct {
 	lostFrames map[RepoBranchCommit]int64
 }
 
-func (a *FileLineCoverMerger) AddRecord(rbc RepoBranchCommit, arch string, f Frame, hitCount int) {
+func (a *FileLineCoverMerger) AddRecord(rbc RepoBranchCommit, f *Frame, hitCount int) {
 	if a.matchers[rbc] == nil {
 		if hitCount > 0 {
 			a.lostFrames[rbc]++

pkg/covermerger/lines_matcher.go

@@ -4,7 +4,6 @@
 package covermerger
 
 import (
-	"log"
 	"strings"
 
 	dmp "github.com/sergi/go-diff/diffmatchpatch"
@@ -44,8 +43,5 @@ type LineToLineMatcher struct {
 }
 
 func (lm *LineToLineMatcher) SameLinePos(line int) int {
-	if line < 0 || line > len(lm.lineToLine) {
-		log.Printf("wrong line number %d", line)
-	}
 	return lm.lineToLine[line]
 }

pkg/covermerger/repos.go

@@ -14,8 +14,7 @@ import (
 )
 
 type fileVersion struct {
-	content     string
-	lastUpdated time.Time
+	content string
 }
 
 type fileVersions map[RepoBranchCommit]fileVersion
@@ -29,21 +28,13 @@ func getFileVersions(c *Config, targetFilePath string, rbcs []RepoBranchCommit,
 
 	res := make(fileVersions)
 	for _, rbc := range rbcs {
-		fileBytes, err := repos[rbc].FileVersion(targetFilePath, rbc.Commit)
+		fileBytes, err := repos[rbc].Object(targetFilePath, rbc.Commit)
 		// It is ok if some file doesn't exist. It means we have repo FS diff.
 		if err != nil {
 			continue
 		}
-		var lastUpdated time.Time
-		if c.BaseType == BaseLastUpdated {
-			if lastUpdated, err = repos[rbc].FileEditTime(targetFilePath); err != nil {
-				return nil, fmt.Errorf("failed to get file %s modification date: %w",
-					targetFilePath, err)
-			}
-		}
 		res[rbc] = fileVersion{
-			content:     string(fileBytes),
-			lastUpdated: lastUpdated,
+			content: string(fileBytes),
 		}
 	}
 	return res, nil

pkg/vcs/fuchsia.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"time"
 
 	"github.com/google/syzkaller/pkg/osutil"
 )
@@ -104,11 +103,3 @@ func (ctx *fuchsia) Object(name, commit string) ([]byte, error) {
 func (ctx *fuchsia) MergeBases(firstCommit, secondCommit string) ([]*Commit, error) {
 	return ctx.repo.MergeBases(firstCommit, secondCommit)
 }
-
-func (ctx *fuchsia) FileEditTime(path string) (time.Time, error) {
-	return time.Time{}, fmt.Errorf("not implemented for fuchsia")
-}
-
-func (ctx *fuchsia) FileVersion(path, commit string) ([]byte, error) {
-	return nil, fmt.Errorf("not implemented for fuchsia")
-}

pkg/vcs/git.go

@@ -619,24 +619,3 @@ func (git *git) MergeBases(firstCommit, secondCommit string) ([]*Commit, error)
 	}
 	return ret, nil
 }
-
-func (git *git) FileEditTime(path string) (time.Time, error) {
-	output, err := git.git("log", "-1", "--pretty=format:%cI", path)
-	if err != nil {
-		return time.Time{}, fmt.Errorf("failed to get time for %s: %w", path, err)
-	}
-	t, err := time.Parse(time.RFC3339, string(output))
-	if err != nil {
-		return time.Time{}, fmt.Errorf("failed to parse datatime %s: %w", output, err)
-	}
-	return t, nil
-}
-
-func (git *git) FileVersion(path, commit string) ([]byte, error) {
-	target := fmt.Sprintf("%s:%s", commit, path)
-	output, err := git.git("show", target)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get %s: %w", target, err)
-	}
-	return output, nil
-}

pkg/vcs/vcs.go

@@ -69,12 +69,6 @@ type Repo interface {
 
 	// MergeBases returns good common ancestors of the two commits.
 	MergeBases(firstCommit, secondCommit string) ([]*Commit, error)
-
-	// FileEditTime returns the last file modification date.
-	FileEditTime(path string) (time.Time, error)
-
-	// FileVersion "git show" file. It is apx. 3 times slower than cat.
-	FileVersion(path, commit string) ([]byte, error)
 }
 
 // Bisecter may be optionally implemented by Repo.

tools/syz-bq.sh

@@ -5,12 +5,12 @@
 set -e # exit on any problem
 set -o pipefail
 
-while getopts w:f:t:n:r: option
+while getopts w:d:t:n:r: option
 do
     case "${option}"
         in
         w)workdir=${OPTARG};;
-        f)from_date=${OPTARG};;
+        d)duration=${OPTARG};;
         t)to_date=${OPTARG};;
         n)namespace=${OPTARG};;
         r)repo=${OPTARG};;
@@ -27,9 +27,9 @@ then
   echo "-t is required to specify to_date"
   exit
 fi
-if [ -z "$from_date" ]
+if [ -z "$duration" ]
 then
-  echo "-f is required to specify from_date"
+  echo "-d is required to specify duration"
   exit
 fi
 if [ -z "$namespace" ]
@@ -52,12 +52,12 @@ CREATE TABLE IF NOT EXISTS
     "repo" text,
     "commit" text,
     "filepath" text,
-    "datefrom" date,
+    "duration" bigint,
     "dateto" date,
     "instrumented" bigint,
     "covered" bigint,
   PRIMARY KEY
-    (datefrom, dateto, commit, filepath) );')
+    (duration, dateto, commit, filepath) );')
 gcloud spanner databases ddl update coverage --instance=syzbot --project=syzkaller \
  --ddl="$create_table"
 
@@ -82,6 +82,7 @@ echo The latest commit as of $to_date is $base_commit.
 # rm -rf $base_dir
 # echo Temp dir $base_dir deleted.
 
+from_date=$(date -d "$to_date - $duration days" +%Y-%m-%d)
 sessionID=$(uuidgen)
 gsURI=$(echo gs://syzbot-temp/bq-exports/${sessionID}/*.csv.gz)
 echo fetching data from bigquery
@@ -98,8 +99,8 @@ AS (
     kernel_repo, kernel_branch, kernel_commit, file_path, sl, SUM(hit_count) as hit_count
   FROM syzkaller.syzbot_coverage.'$namespace'
   WHERE
-    TIMESTAMP_TRUNC(timestamp, DAY) >= TIMESTAMP("'$from_date'") AND
-    TIMESTAMP_TRUNC(timestamp, DAY) <= TIMESTAMP("'$to_date'") AND
+    TIMESTAMP_TRUNC(timestamp, DAY) >= "'$from_date'" AND
+    TIMESTAMP_TRUNC(timestamp, DAY) <= "'$to_date'" AND
     version = 1
   GROUP BY file_path, kernel_commit, kernel_repo, kernel_branch, sl
   ORDER BY file_path
@@ -117,7 +118,7 @@ go run ./tools/syz-covermerger/ -workdir $workdir \
   -commit $base_commit \
   -save-to-spanner true \
   -namespace $namespace \
-  -date-from $from_date \
+  -duration $duration \
   -date-to $to_date
 
 echo Cleanup