pkg/bisect: detect wrong bisections

Detect bisection to merge commits and to commits that don't affect
kernel binary (comments, other arches, whitespaces, etc).
Such bisections are not reported in emails (but shown on web).

Update #1271

2 files changed, 283 insertions, 238 deletions

diff --git a/pkg/bisect/bisect.go b/pkg/bisect/bisect.go
index 1458e141e..5a6197c32 100644
--- a/pkg/bisect/bisect.go
+++ b/pkg/bisect/bisect.go
@@ -56,7 +56,7 @@ type env struct {
 	bisecter  vcs.Bisecter
 	commit    *vcs.Commit
 	head      *vcs.Commit
-	inst      instance.BuilderTester
+	inst      instance.Env
 	numTests  int
 	buildTime time.Duration
 	testTime  time.Duration
@@ -64,44 +64,53 @@ type env struct {
 
 const NumTests = 10 // number of tests we do per commit
 
-// Run does the bisection and returns:
-//  - if bisection is conclusive, the single cause/fix commit
+// Result describes bisection result:
+//  - if bisection is conclusive, the single cause/fix commit in Commits
 //    - for cause bisection report is the crash on the cause commit
 //    - for fix bisection report is nil
-//    - *vcs.Commit will be nil
-//  - if bisection is inconclusive, range of potential cause/fix commits
+//    - Commit is nil
+//    - NoopChange is set if the commit did not cause any change in the kernel binary
+//      (bisection result it most likely wrong)
+//  - if bisection is inconclusive, range of potential cause/fix commits in Commits
 //    - report is nil in such case
-//    - *vcs.Commit will be nil
-//  - if the crash still happens on the oldest release/HEAD (for cause/fix bisection correspondingly),
-//    no commits and the crash report on the oldest release/HEAD; and *vcs.Commit will point to
-//    the oldest/latest commit where crash happens.
-//  - if the crash is not reproduced on the start commit, an error, *vcs.Commit will be nil
-func Run(cfg *Config) ([]*vcs.Commit, *report.Report, *vcs.Commit, error) {
+//    - Commit is nil
+//  - if the crash still happens on the oldest release/HEAD (for cause/fix bisection correspondingly)
+//    - no commits in Commits
+//    - the crash report on the oldest release/HEAD;
+//    - Commit points to the oldest/latest commit where crash happens.
+type Result struct {
+	Commits    []*vcs.Commit
+	Report     *report.Report
+	Commit     *vcs.Commit
+	NoopChange bool
+}
+
+// Run does the bisection and returns either the Result,
+// or, if the crash is not reproduced on the start commit, an error.
+func Run(cfg *Config) (*Result, error) {
 	if err := checkConfig(cfg); err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 	cfg.Manager.Cover = false // it's not supported somewhere back in time
 	repo, err := vcs.NewRepo(cfg.Manager.TargetOS, cfg.Manager.Type, cfg.Manager.KernelSrc)
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 	bisecter, ok := repo.(vcs.Bisecter)
 	if !ok {
-		return nil, nil, nil, fmt.Errorf("bisection is not implemented for %v", cfg.Manager.TargetOS)
+		return nil, fmt.Errorf("bisection is not implemented for %v", cfg.Manager.TargetOS)
 	}
 	inst, err := instance.NewEnv(&cfg.Manager)
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 	if _, err = repo.CheckoutBranch(cfg.Kernel.Repo, cfg.Kernel.Branch); err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
-
 	return runImpl(cfg, repo, bisecter, inst)
 }
 
-func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.BuilderTester) (
-	[]*vcs.Commit, *report.Report, *vcs.Commit, error) {
+func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.Env) (*Result, error) {
 	env := &env{
 		cfg:      cfg,
 		repo:     repo,
@@ -110,7 +119,7 @@ func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.Bu
 	}
 	head, err := repo.HeadCommit()
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 	env.head = head
 	if cfg.Fix {
@@ -119,91 +128,106 @@ func runImpl(cfg *Config, repo vcs.Repo, bisecter vcs.Bisecter, inst instance.Bu
 		env.log("bisecting cause commit starting from %v", cfg.Kernel.Commit)
 	}
 	start := time.Now()
-	commits, rep, bad, err := env.bisect()
+	res, err := env.bisect()
 	env.log("revisions tested: %v, total time: %v (build: %v, test: %v)",
 		env.numTests, time.Since(start), env.buildTime, env.testTime)
 	if err != nil {
 		env.log("error: %v", err)
-		return nil, nil, nil, err
+		return nil, err
 	}
-	if len(commits) == 0 {
+	if len(res.Commits) == 0 {
 		if cfg.Fix {
 			env.log("the crash still happens on HEAD")
 		} else {
 			env.log("the crash already happened on the oldest tested release")
 		}
-		env.log("commit msg: %v", bad.Title)
-		env.log("crash: %v\n%s", rep.Title, rep.Report)
-		return nil, rep, bad, nil
+		env.log("commit msg: %v", res.Commit.Title)
+		env.log("crash: %v\n%s", res.Report.Title, res.Report.Report)
+		return res, nil
 	}
 	what := "bad"
 	if cfg.Fix {
 		what = "good"
 	}
-	if len(commits) > 1 {
+	if len(res.Commits) > 1 {
 		env.log("bisection is inconclusive, the first %v commit could be any of:", what)
-		for _, com := range commits {
+		for _, com := range res.Commits {
 			env.log("%v", com.Hash)
 		}
-		return commits, nil, nil, nil
+		return res, nil
 	}
-	com := commits[0]
+	com := res.Commits[0]
 	env.log("first %v commit: %v %v", what, com.Hash, com.Title)
 	env.log("cc: %q", com.CC)
-	if rep != nil {
-		env.log("crash: %v\n%s", rep.Title, rep.Report)
+	if res.Report != nil {
+		env.log("crash: %v\n%s", res.Report.Title, res.Report.Report)
 	}
-	return commits, rep, nil, nil
+	return res, nil
 }
 
-func (env *env) bisect() ([]*vcs.Commit, *report.Report, *vcs.Commit, error) {
+func (env *env) bisect() (*Result, error) {
 	cfg := env.cfg
 	var err error
 	if err := build.Clean(cfg.Manager.TargetOS, cfg.Manager.TargetVMArch,
 		cfg.Manager.Type, cfg.Manager.KernelSrc); err != nil {
-		return nil, nil, nil, fmt.Errorf("kernel clean failed: %v", err)
+		return nil, fmt.Errorf("kernel clean failed: %v", err)
 	}
 	env.log("building syzkaller on %v", cfg.Syzkaller.Commit)
 	if err := env.inst.BuildSyzkaller(cfg.Syzkaller.Repo, cfg.Syzkaller.Commit); err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 	com, err := env.repo.CheckoutCommit(cfg.Kernel.Repo, cfg.Kernel.Commit)
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 	env.commit = com
-	res, _, rep0, err := env.test()
+	testRes, err := env.test()
 	if err != nil {
-		return nil, nil, nil, err
-	} else if res != vcs.BisectBad {
-		return nil, nil, nil, fmt.Errorf("the crash wasn't reproduced on the original commit")
+		return nil, err
+	} else if testRes.verdict != vcs.BisectBad {
+		return nil, fmt.Errorf("the crash wasn't reproduced on the original commit")
 	}
 	bad, good, rep1, err := env.commitRange()
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 	if rep1 != nil {
-		return nil, rep1, bad, nil // still not fixed/happens on the oldest release
+		return &Result{Report: rep1, Commit: bad}, nil // still not fixed/happens on the oldest release
 	}
-	reports := make(map[string]*report.Report)
-	reports[cfg.Kernel.Commit] = rep0
+	results := map[string]*testResult{cfg.Kernel.Commit: testRes}
 	commits, err := env.bisecter.Bisect(bad.Hash, good.Hash, cfg.Trace, func() (vcs.BisectResult, error) {
-		res, com, rep, err := env.test()
-		reports[com.Hash] = rep
+		testRes1, err := env.test()
+		if err != nil {
+			return 0, err
+		}
+		results[testRes1.com.Hash] = testRes1
 		if cfg.Fix {
-			if res == vcs.BisectBad {
-				res = vcs.BisectGood
-			} else if res == vcs.BisectGood {
-				res = vcs.BisectBad
+			if testRes1.verdict == vcs.BisectBad {
+				testRes1.verdict = vcs.BisectGood
+			} else if testRes1.verdict == vcs.BisectGood {
+				testRes1.verdict = vcs.BisectBad
 			}
 		}
-		return res, err
+		return testRes1.verdict, err
 	})
-	var rep *report.Report
+	if err != nil {
+		return nil, err
+	}
+	res := &Result{
+		Commits: commits,
+	}
 	if len(commits) == 1 {
-		rep = reports[commits[0].Hash]
+		com := commits[0]
+		if testRes := results[com.Hash]; testRes != nil {
+			res.Report = testRes.rep
+			if testRes.kernelSign != "" && len(com.Parents) == 1 {
+				if prevRes := results[com.Parents[0]]; prevRes != nil {
+					res.NoopChange = testRes.kernelSign == prevRes.kernelSign
+				}
+			}
+		}
 	}
-	return commits, rep, nil, err
+	return res, nil
 }
 
 func (env *env) commitRange() (*vcs.Commit, *vcs.Commit, *report.Report, error) {
@@ -218,12 +242,12 @@ func (env *env) commitRangeForFix() (*vcs.Commit, *vcs.Commit, *report.Report, e
 	if _, err := env.repo.SwitchCommit(env.head.Hash); err != nil {
 		return nil, nil, nil, err
 	}
-	res, _, rep, err := env.test()
+	res, err := env.test()
 	if err != nil {
 		return nil, nil, nil, err
 	}
-	if res != vcs.BisectGood {
-		return env.head, nil, rep, nil
+	if res.verdict != vcs.BisectGood {
+		return env.head, nil, res.rep, nil
 	}
 	return env.head, env.commit, nil, nil
 }
@@ -245,45 +269,57 @@ func (env *env) commitRangeForBug() (*vcs.Commit, *vcs.Commit, *report.Report, e
 		if err != nil {
 			return nil, nil, nil, err
 		}
-		res, _, rep, err := env.test()
+		res, err := env.test()
 		if err != nil {
 			return nil, nil, nil, err
 		}
-		if res == vcs.BisectGood {
+		if res.verdict == vcs.BisectGood {
 			return lastBad, com, nil, nil
 		}
-		if res == vcs.BisectBad {
+		if res.verdict == vcs.BisectBad {
 			lastBad = com
-			lastRep = rep
+			lastRep = res.rep
 		}
 	}
 	return lastBad, nil, lastRep, nil
 }
 
-func (env *env) test() (vcs.BisectResult, *vcs.Commit, *report.Report, error) {
+type testResult struct {
+	verdict    vcs.BisectResult
+	com        *vcs.Commit
+	rep        *report.Report
+	kernelSign string
+}
+
+func (env *env) test() (*testResult, error) {
 	cfg := env.cfg
 	env.numTests++
 	current, err := env.repo.HeadCommit()
 	if err != nil {
-		return 0, nil, nil, err
+		return nil, err
 	}
 	bisectEnv, err := env.bisecter.EnvForCommit(cfg.BinDir, current.Hash, cfg.Kernel.Config)
 	if err != nil {
-		return 0, nil, nil, err
+		return nil, err
 	}
 	compilerID, err := build.CompilerIdentity(bisectEnv.Compiler)
 	if err != nil {
-		return 0, nil, nil, err
+		return nil, err
 	}
 	env.log("testing commit %v with %v", current.Hash, compilerID)
 	buildStart := time.Now()
 	if err := build.Clean(cfg.Manager.TargetOS, cfg.Manager.TargetVMArch,
 		cfg.Manager.Type, cfg.Manager.KernelSrc); err != nil {
-		return 0, nil, nil, fmt.Errorf("kernel clean failed: %v", err)
+		return nil, fmt.Errorf("kernel clean failed: %v", err)
 	}
-	_, err = env.inst.BuildKernel(bisectEnv.Compiler, cfg.Kernel.Userspace,
+	_, kernelSign, err := env.inst.BuildKernel(bisectEnv.Compiler, cfg.Kernel.Userspace,
 		cfg.Kernel.Cmdline, cfg.Kernel.Sysctl, bisectEnv.KernelConfig)
 	env.buildTime += time.Since(buildStart)
+	res := &testResult{
+		verdict:    vcs.BisectSkip,
+		com:        current,
+		kernelSign: kernelSign,
+	}
 	if err != nil {
 		if verr, ok := err.(*osutil.VerboseError); ok {
 			env.log("%v", verr.Title)
@@ -294,27 +330,28 @@ func (env *env) test() (vcs.BisectResult, *vcs.Commit, *report.Report, error) {
 		} else {
 			env.log("%v", err)
 		}
-		return vcs.BisectSkip, current, nil, nil
+		return res, nil
 	}
 	testStart := time.Now()
 	results, err := env.inst.Test(NumTests, cfg.Repro.Syz, cfg.Repro.Opts, cfg.Repro.C)
 	env.testTime += time.Since(testStart)
 	if err != nil {
 		env.log("failed: %v", err)
-		return vcs.BisectSkip, current, nil, nil
+		return res, nil
 	}
 	bad, good, rep := env.processResults(current, results)
-	res := vcs.BisectSkip
+	res.rep = rep
+	res.verdict = vcs.BisectSkip
 	if bad != 0 {
-		res = vcs.BisectBad
+		res.verdict = vcs.BisectBad
 	} else if NumTests-good-bad > NumTests/3*2 {
 		// More than 2/3 of instances failed with infrastructure error,
 		// can't reliably tell that the commit is good.
-		res = vcs.BisectSkip
+		res.verdict = vcs.BisectSkip
 	} else if good != 0 {
-		res = vcs.BisectGood
+		res.verdict = vcs.BisectGood
 	}
-	return res, current, rep, nil
+	return res, nil
 }
 
 func (env *env) processResults(current *vcs.Commit, results []error) (bad, good int, rep *report.Report) {
diff --git a/pkg/bisect/bisect_test.go b/pkg/bisect/bisect_test.go
index 72e12fbe3..04609e701 100644
--- a/pkg/bisect/bisect_test.go
+++ b/pkg/bisect/bisect_test.go
@@ -7,7 +7,6 @@ import (
 	"bytes"
 	"fmt"
 	"io/ioutil"
-	"math"
 	"os"
 	"strconv"
 	"testing"
@@ -21,13 +20,9 @@ import (
 // testEnv will implement instance.BuilderTester. This allows us to
 // set bisect.env.inst to a testEnv object.
 type testEnv struct {
-	repo        *vcs.TestRepo
-	r           vcs.Repo
-	t           *testing.T
-	fix         bool
-	brokenStart float64
-	brokenEnd   float64
-	culprit     float64
+	t    *testing.T
+	r    vcs.Repo
+	test BisectionTest
 }
 
 func (env *testEnv) BuildSyzkaller(repo, commit string) error {
@@ -35,90 +30,68 @@ func (env *testEnv) BuildSyzkaller(repo, commit string) error {
 }
 
 func (env *testEnv) BuildKernel(compilerBin, userspaceDir, cmdlineFile, sysctlFile string,
-	kernelConfig []byte) (string, error) {
-	return "", nil
-}
-
-func crashErrors(num int, title string) []error {
-	var errors []error
-	for i := 0; i < num; i++ {
-		errors = append(errors, &instance.CrashError{
-			Report: &report.Report{
-				Title: fmt.Sprintf("crashes at %v", title),
-			},
-		})
+	kernelConfig []byte) (string, string, error) {
+	commit := env.headCommit()
+	kernelSign := fmt.Sprintf("sign-%v", commit)
+	if commit >= env.test.sameBinaryStart && commit <= env.test.sameBinaryEnd {
+		kernelSign = "same-sign"
 	}
-	return errors
+	return "", kernelSign, nil
 }
 
-func nilErrors(num int) []error {
-	var errors []error
-	for i := 0; i < num; i++ {
-		errors = append(errors, nil)
+func (env *testEnv) Test(numVMs int, reproSyz, reproOpts, reproC []byte) ([]error, error) {
+	commit := env.headCommit()
+	if commit >= env.test.brokenStart && commit <= env.test.brokenEnd {
+		return nil, fmt.Errorf("broken build")
 	}
-	return errors
+	if !env.test.fix && commit >= env.test.culprit || env.test.fix && commit < env.test.culprit {
+		return crashErrors(numVMs, "crash occurs"), nil
+	}
+	return make([]error, numVMs), nil
 }
 
-func (env *testEnv) Test(numVMs int, reproSyz, reproOpts, reproC []byte) ([]error, error) {
-	hc, err := env.r.HeadCommit()
+func (env *testEnv) headCommit() int {
+	com, err := env.r.HeadCommit()
 	if err != nil {
 		env.t.Fatal(err)
 	}
-	commit, err := strconv.ParseFloat(hc.Title, 64)
+	commit, err := strconv.ParseUint(com.Title, 10, 64)
 	if err != nil {
-		env.t.Fatalf("invalid commit title: %v", hc.Title)
-	}
-	var e error
-	var res []error
-	if commit >= env.brokenStart && commit <= env.brokenEnd {
-		e = fmt.Errorf("broken build")
-	} else if commit < env.culprit && !env.fix || commit >= env.culprit && env.fix {
-		res = nilErrors(numVMs)
-	} else {
-		res = crashErrors(numVMs, "crash occurs")
+		env.t.Fatalf("invalid commit title: %v", com.Title)
 	}
-	return res, e
+	return int(commit)
 }
 
-type Ctx struct {
-	t          *testing.T
-	baseDir    string
-	repo       *vcs.TestRepo
-	r          vcs.Repo
-	cfg        *Config
-	inst       *testEnv
-	originRepo *vcs.TestRepo
-}
-
-func NewCtx(t *testing.T, fix bool, brokenStart, brokenEnd, culprit float64, commit string) *Ctx {
-	baseDir, err := ioutil.TempDir("", "syz-git-test")
+func runBisection(t *testing.T, test BisectionTest) (*Result, error) {
+	baseDir, err := ioutil.TempDir("", "syz-bisect-test")
 	if err != nil {
 		t.Fatal(err)
 	}
-	originRepo := vcs.CreateTestRepo(t, baseDir, "originRepo")
+	defer os.RemoveAll(baseDir)
+	repo := vcs.CreateTestRepo(t, baseDir, "repo")
+	if !repo.SupportsBisection() {
+		t.Skip("bisection is unsupported by git (probably too old version)")
+	}
 	for rv := 4; rv < 10; rv++ {
 		for i := 0; i < 6; i++ {
-			originRepo.CommitChange(fmt.Sprintf("%v", rv*100+i))
+			repo.CommitChange(fmt.Sprintf("%v", rv*100+i))
 			if i == 0 {
-				originRepo.SetTag(fmt.Sprintf("v%v.0", rv))
+				repo.SetTag(fmt.Sprintf("v%v.0", rv))
 			}
 		}
 	}
-	if !originRepo.SupportsBisection() {
-		t.Skip("bisection is unsupported by git (probably too old version)")
-	}
-	repo := vcs.CloneTestRepo(t, baseDir, "repo", originRepo)
 	r, err := vcs.NewRepo("test", "64", repo.Dir)
 	if err != nil {
 		t.Fatal(err)
 	}
-	sc, err := r.GetCommitByTitle(commit)
+	sc, err := r.GetCommitByTitle(fmt.Sprint(test.startCommit))
 	if err != nil {
 		t.Fatal(err)
 	}
+	trace := new(bytes.Buffer)
 	cfg := &Config{
-		Fix:   fix,
-		Trace: new(bytes.Buffer),
+		Fix:   test.fix,
+		Trace: trace,
 		Manager: mgrconfig.Config{
 			TargetOS:     "test",
 			TargetVMArch: "64",
@@ -126,177 +99,212 @@ func NewCtx(t *testing.T, fix bool, brokenStart, brokenEnd, culprit float64, com
 			KernelSrc:    repo.Dir,
 		},
 		Kernel: KernelConfig{
-			Repo:   originRepo.Dir,
+			Repo:   repo.Dir,
 			Commit: sc.Hash,
 		},
 	}
 	inst := &testEnv{
-		repo:        repo,
-		r:           r,
-		t:           t,
-		fix:         fix,
-		brokenStart: brokenStart,
-		brokenEnd:   brokenEnd,
-		culprit:     culprit,
-	}
-	c := &Ctx{
-		t:          t,
-		baseDir:    baseDir,
-		repo:       repo,
-		r:          r,
-		cfg:        cfg,
-		inst:       inst,
-		originRepo: originRepo,
+		t:    t,
+		r:    r,
+		test: test,
 	}
-	return c
+	res, err := runImpl(cfg, r, r.(vcs.Bisecter), inst)
+	t.Log(trace.String())
+	return res, err
 }
 
-type BisectionTests struct {
+type BisectionTest struct {
 	// input environment
 	name        string
 	fix         bool
-	startCommit string
-	brokenStart float64
-	brokenEnd   float64
+	startCommit int
+	brokenStart int
+	brokenEnd   int
+	// Range of commits that result in the same kernel binary signature.
+	sameBinaryStart int
+	sameBinaryEnd   int
 	// expected output
-	errIsNil     bool
+	expectErr    bool
+	expectRep    bool
+	noopChange   bool
 	commitLen    int
-	repIsNil     bool
-	oldestLatest string
+	oldestLatest int
 	// input and output
-	culprit float64
+	culprit int
 }
 
 func TestBisectionResults(t *testing.T) {
 	t.Parallel()
-	var tests = []BisectionTests{
+	tests := []BisectionTest{
 		// Tests that bisection returns the correct cause commit.
 		{
-			name:        "bisect cause finds cause",
-			fix:         false,
-			startCommit: "905",
-			brokenStart: math.Inf(0),
-			brokenEnd:   0,
-			errIsNil:    true,
+			name:        "cause-finds-cause",
+			startCommit: 905,
 			commitLen:   1,
-			repIsNil:    false,
+			expectRep:   true,
 			culprit:     602,
 		},
 		// Tests that cause bisection returns error when crash does not reproduce
 		// on the original commit.
 		{
-			name:        "bisect cause does not repro",
-			fix:         false,
-			startCommit: "400",
-			brokenStart: math.Inf(0),
-			brokenEnd:   0,
-			errIsNil:    false,
-			commitLen:   0,
-			repIsNil:    true,
-			culprit:     math.Inf(0),
+			name:        "cause-does-not-repro",
+			startCommit: 400,
+			expectErr:   true,
 		},
 		// Tests that no commits are returned when crash occurs on oldest commit
 		// for cause bisection.
 		{
-			name:         "bisect cause crashes oldest",
-			fix:          false,
-			startCommit:  "905",
-			brokenStart:  math.Inf(0),
-			brokenEnd:    0,
-			errIsNil:     true,
+			name:         "cause-crashes-oldest",
+			startCommit:  905,
 			commitLen:    0,
-			repIsNil:     false,
+			expectRep:    true,
 			culprit:      0,
-			oldestLatest: "400",
+			oldestLatest: 400,
 		},
-		// Tests that more than 1 commit is returned when cause bisection is
-		// inconclusive.
+		// Tests that more than 1 commit is returned when cause bisection is inconclusive.
 		{
-			name:        "bisect cause inconclusive",
-			fix:         false,
-			startCommit: "802",
+			name:        "cause-inconclusive",
+			startCommit: 802,
 			brokenStart: 500,
 			brokenEnd:   700,
-			errIsNil:    true,
 			commitLen:   14,
-			repIsNil:    true,
 			culprit:     605,
 		},
 		// Tests that bisection returns the correct fix commit.
 		{
-			name:        "bisect fix finds fix",
+			name:        "fix-finds-fix",
 			fix:         true,
-			startCommit: "400",
-			brokenStart: math.Inf(0),
-			brokenEnd:   0,
-			errIsNil:    true,
+			startCommit: 400,
 			commitLen:   1,
-			repIsNil:    true,
 			culprit:     500,
 		},
 		// Tests that fix bisection returns error when crash does not reproduce
 		// on the original commit.
 		{
-			name:        "bisect fix does not repro",
+			name:        "fix-does-not-repro",
 			fix:         true,
-			startCommit: "905",
-			brokenStart: math.Inf(0),
-			brokenEnd:   0,
-			errIsNil:    false,
-			commitLen:   0,
-			repIsNil:    true,
-			culprit:     0,
+			startCommit: 905,
+			expectErr:   true,
 		},
 		// Tests that no commits are returned when crash occurs on HEAD
 		// for fix bisection.
 		{
-			name:         "bisect fix crashes HEAD",
+			name:         "fix-crashes-HEAD",
 			fix:          true,
-			startCommit:  "400",
-			brokenStart:  math.Inf(0),
-			brokenEnd:    0,
-			errIsNil:     true,
+			startCommit:  400,
 			commitLen:    0,
-			repIsNil:     false,
+			expectRep:    true,
 			culprit:      1000,
-			oldestLatest: "905",
+			oldestLatest: 905,
 		},
-		// Tests that more than 1 commit is returned when fix bisection is
-		// inconclusive.
+		// Tests that more than 1 commit is returned when fix bisection is inconclusive.
 		{
-			name:        "bisect fix inconclusive",
+			name:        "fix-inconclusive",
 			fix:         true,
-			startCommit: "400",
+			startCommit: 400,
 			brokenStart: 500,
 			brokenEnd:   600,
-			errIsNil:    true,
 			commitLen:   8,
-			repIsNil:    true,
 			culprit:     501,
 		},
+		{
+			name:            "cause-same-binary",
+			startCommit:     905,
+			commitLen:       1,
+			expectRep:       true,
+			culprit:         503,
+			sameBinaryStart: 502,
+			sameBinaryEnd:   503,
+			noopChange:      true,
+		},
+		{
+			name:            "cause-same-binary-off-by-one",
+			startCommit:     905,
+			commitLen:       1,
+			expectRep:       true,
+			culprit:         503,
+			sameBinaryStart: 400,
+			sameBinaryEnd:   502,
+		},
+		{
+			name:            "cause-same-binary-off-by-one-2",
+			startCommit:     905,
+			commitLen:       1,
+			expectRep:       true,
+			culprit:         503,
+			sameBinaryStart: 503,
+			sameBinaryEnd:   905,
+		},
+		{
+			name:            "fix-same-binary",
+			fix:             true,
+			startCommit:     400,
+			commitLen:       1,
+			culprit:         503,
+			sameBinaryStart: 502,
+			sameBinaryEnd:   504,
+			noopChange:      true,
+		},
 	}
 	for _, test := range tests {
-		t.Run(fmt.Sprintf("%v", test.name), func(t *testing.T) {
-			c := NewCtx(t, test.fix, test.brokenStart, test.brokenEnd, test.culprit, test.startCommit)
-			defer os.RemoveAll(c.baseDir)
-			commits, rep, com, err := runImpl(c.cfg, c.r, c.r.(vcs.Bisecter), c.inst)
-			if test.errIsNil && err != nil || !test.errIsNil && err == nil {
-				t.Fatalf("returned error: '%v'", err)
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			if test.expectErr &&
+				(test.commitLen != 0 ||
+					test.expectRep ||
+					test.oldestLatest != 0 ||
+					test.culprit != 0) {
+				t.Fatalf("expecting non-default values on error")
+			}
+			if test.brokenStart > test.brokenEnd {
+				t.Fatalf("bad broken start/end: %v/%v",
+					test.brokenStart, test.brokenEnd)
+			}
+			if test.sameBinaryStart > test.sameBinaryEnd {
+				t.Fatalf("bad same binary start/end: %v/%v",
+					test.sameBinaryStart, test.sameBinaryEnd)
+			}
+			res, err := runBisection(t, test)
+			if test.expectErr != (err != nil) {
+				t.Fatalf("returned error: %v", err)
+			}
+			if err != nil {
+				if res != nil {
+					t.Fatalf("got both result and error: '%v' %+v", err, *res)
+				}
+				return
+			}
+			if len(res.Commits) != test.commitLen {
+				t.Fatalf("expected %d commits got %d commits", test.commitLen, len(res.Commits))
 			}
-			if len(commits) != test.commitLen {
-				t.Fatalf("expected %d commits got %d commits", test.commitLen, len(commits))
+			expectedTitle := fmt.Sprint(test.culprit)
+			if len(res.Commits) == 1 && expectedTitle != res.Commits[0].Title {
+				t.Fatalf("expected commit '%v' got '%v'", expectedTitle, res.Commits[0].Title)
 			}
-			expectedTitle := fmt.Sprintf("%v", test.culprit)
-			if len(commits) == 1 && expectedTitle != commits[0].Title {
-				t.Fatalf("expected commit '%v' got '%v'", expectedTitle, commits[0].Title)
+			if test.expectRep != (res.Report != nil) {
+				t.Fatalf("got rep: %v, want: %v", res.Report, test.expectRep)
 			}
-			if test.repIsNil && rep != nil || !test.repIsNil && rep == nil {
-				t.Fatalf("returned rep: '%v'", err)
+			if res.NoopChange != test.noopChange {
+				t.Fatalf("got noop change: %v, want: %v", res.NoopChange, test.noopChange)
 			}
-			if test.oldestLatest != "" && test.oldestLatest != com.Title ||
-				test.oldestLatest == "" && com != nil {
-				t.Fatalf("expected latest/oldest: '%v' got '%v'", test.oldestLatest, com.Title)
+			if test.oldestLatest != 0 && fmt.Sprint(test.oldestLatest) != res.Commit.Title ||
+				test.oldestLatest == 0 && res.Commit != nil {
+				t.Fatalf("expected latest/oldest: %v got '%v'",
+					test.oldestLatest, res.Commit.Title)
 			}
 		})
 	}
 }
+
+func crashErrors(num int, title string) []error {
+	var errors []error
+	for i := 0; i < num; i++ {
+		errors = append(errors, &instance.CrashError{
+			Report: &report.Report{
+				Title: fmt.Sprintf("crashes at %v", title),
+			},
+		})
+	}
+	return errors
+}