From e8fcf811ec8e1678e173f2c0549b21f88a5ccb66 Mon Sep 17 00:00:00 2001
From: Dmitry Vyukov <dvyukov@google.com>
Date: Thu, 25 Jun 2020 20:03:10 +0200
Subject: pkg/bisect: don't ignore errors during config minimization

Ignoring errors is bad. It leads to silent failures.
If there is some particular error condition that is expected
to happen and we want to ignore, we need to ignore that one only
rather than all potenital errors in the whole process.
---
 pkg/bisect/bisect.go      | 56 ++++++++++++++++++++++++++---------------------
 pkg/bisect/bisect_test.go |  8 +++----
 2 files changed, 34 insertions(+), 30 deletions(-)

(limited to 'pkg/bisect')

diff --git a/pkg/bisect/bisect.go b/pkg/bisect/bisect.go
index 7d49b63bb..ac43bae7e 100644
--- a/pkg/bisect/bisect.go
+++ b/pkg/bisect/bisect.go
@@ -123,8 +123,10 @@ func runImpl(cfg *Config, repo vcs.Repo, inst instance.Env) (*Result, error) {
 	if !ok {
 		return nil, fmt.Errorf("bisection is not implemented for %v", cfg.Manager.TargetOS)
 	}
-	// Minimizer may or may not be supported.
-	minimizer, _ := repo.(vcs.ConfigMinimizer)
+	minimizer, ok := repo.(vcs.ConfigMinimizer)
+	if !ok && len(cfg.Kernel.BaselineConfig) != 0 {
+		return nil, fmt.Errorf("config minimization is not implemented for %v", cfg.Manager.TargetOS)
+	}
 	env := &env{
 		cfg:       cfg,
 		repo:      repo,
@@ -205,7 +207,9 @@ func (env *env) bisect() (*Result, error) {
 	}
 
 	if len(cfg.Kernel.BaselineConfig) != 0 {
-		env.minimizeConfig()
+		if err := env.minimizeConfig(); err != nil {
+			return nil, err
+		}
 	}
 
 	bad, good, rep1, results1, err := env.commitRange()
@@ -270,7 +274,7 @@ func (env *env) bisect() (*Result, error) {
 	return res, nil
 }
 
-func (env *env) minimizeConfig() {
+func (env *env) minimizeConfig() error {
 	cfg := env.cfg
 	// Check if crash reproduces with baseline config.
 	originalConfig := cfg.Kernel.Config
@@ -278,29 +282,34 @@ func (env *env) minimizeConfig() {
 	testRes, err := env.test()
 	cfg.Kernel.Config = originalConfig
 	if err != nil {
-		env.log("testing baseline config failed")
-	} else if testRes.verdict == vcs.BisectBad {
+		env.log("testing baseline config failed: %v", err)
+		return err
+	}
+	if testRes.verdict == vcs.BisectBad {
 		env.log("crash reproduces with baseline config")
 		cfg.Kernel.Config = cfg.Kernel.BaselineConfig
-	} else if testRes.verdict == vcs.BisectGood && env.minimizer != nil {
-		predMinimize := func(test []byte) (vcs.BisectResult, error) {
-			cfg.Kernel.Config = test
-			testRes, err := env.test()
-			if err != nil {
-				return 0, err
-			}
-			return testRes.verdict, err
-		}
-		// Find minimal configuration based on baseline to reproduce the crash
-		cfg.Kernel.Config, err = env.minimizer.Minimize(cfg.Kernel.Config,
-			cfg.Kernel.BaselineConfig, cfg.Trace, predMinimize)
+		return nil
+	}
+	if testRes.verdict == vcs.BisectSkip {
+		env.log("unable to test using baseline config, keep original config")
+		return nil
+	}
+	predMinimize := func(test []byte) (vcs.BisectResult, error) {
+		cfg.Kernel.Config = test
+		testRes, err := env.test()
 		if err != nil {
-			env.log("Minimizing config failed, using original config")
-			cfg.Kernel.Config = originalConfig
+			return 0, err
 		}
-	} else {
-		env.log("unable to test using baseline config, keep original config")
+		return testRes.verdict, err
+	}
+	// Find minimal configuration based on baseline to reproduce the crash.
+	cfg.Kernel.Config, err = env.minimizer.Minimize(cfg.Kernel.Config,
+		cfg.Kernel.BaselineConfig, cfg.Trace, predMinimize)
+	if err != nil {
+		env.log("minimizing config failed: %v", err)
+		return err
 	}
+	return nil
 }
 
 func (env *env) detectNoopChange(results map[string]*testResult, com *vcs.Commit) (bool, error) {
@@ -429,9 +438,6 @@ func (env *env) test() (*testResult, error) {
 	cfg := env.cfg
 	env.numTests++
 	current, kernelSign, err := env.build()
-	if err != nil {
-		return nil, err
-	}
 	res := &testResult{
 		verdict:    vcs.BisectSkip,
 		com:        current,
diff --git a/pkg/bisect/bisect_test.go b/pkg/bisect/bisect_test.go
index 8c8b75991..a8fc8d51d 100644
--- a/pkg/bisect/bisect_test.go
+++ b/pkg/bisect/bisect_test.go
@@ -205,7 +205,7 @@ var bisectionTests = []BisectionTest{
 		baselineConfig: "baseline-skip",
 	},
 	{
-		name:           "cause-finds-cause-minimize_succeeds",
+		name:           "cause-finds-cause-minimize-succeeds",
 		startCommit:    905,
 		commitLen:      1,
 		expectRep:      true,
@@ -213,12 +213,10 @@ var bisectionTests = []BisectionTest{
 		baselineConfig: "minimize-succeeds",
 	},
 	{
-		name:           "cause-finds-cause-minimize_fails",
+		name:           "cause-finds-cause-minimize-fails",
 		startCommit:    905,
-		commitLen:      1,
-		expectRep:      true,
-		culprit:        602,
 		baselineConfig: "minimize-fails",
+		expectErr:      true,
 	},
 	// Tests that cause bisection returns error when crash does not reproduce
 	// on the original commit.
-- 
cgit mrf-deployment