log error message when negative samples appear in non-diff_base profiles

internal/driver/driver.go

@@ -114,7 +114,7 @@ func generateReport(p *profile.Profile, cmd []string, vars variables, o *plugin.
 
 	// Generate the report.
 	dst := new(bytes.Buffer)
-	if err := report.Generate(dst, rpt, o.Obj); err != nil {
+	if err := report.Generate(dst, rpt, o.Obj, o.UI); err != nil {
 		return err
 	}
 	src := dst

internal/driver/webui.go

@@ -398,7 +398,7 @@ func (ui *webInterface) peek(w http.ResponseWriter, req *http.Request) {
 	}
 
 	out := &bytes.Buffer{}
-	if err := report.Generate(out, rpt, ui.options.Obj); err != nil {
+	if err := report.Generate(out, rpt, ui.options.Obj, ui.options.UI); err != nil {
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		ui.options.UI.PrintErr(err)
 		return

internal/report/report.go

@@ -82,7 +82,11 @@ type Options struct {
 }
 
 // Generate generates a report as directed by the Report.
-func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool) error {
+func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool, ui plugin.UI) error {
+	if rpt.unexpNegSamples {
+		ui.PrintErr("Profile has negative values, so percentage values may be incorrect. If using -base, consider using -diff_base instead.")
+	}
+
 	o := rpt.options
 
 	switch o.OutputFormat {
@@ -1202,8 +1206,8 @@ func New(prof *profile.Profile, o *Options) *Report {
 		}
 		return measurement.ScaledLabel(v, o.SampleUnit, o.OutputUnit)
 	}
-	return &Report{prof, computeTotal(prof, o.SampleValue, o.SampleMeanDivisor),
-		o, format}
+	total, unexpNegSamples := computeTotal(prof, o.SampleValue, o.SampleMeanDivisor)
+	return &Report{prof, total, unexpNegSamples, o, format}
 }
 
 // NewDefault builds a new report indexing the last sample value
@@ -1225,15 +1229,19 @@ func NewDefault(prof *profile.Profile, options Options) *Report {
 // computeTotal computes the sum of the absolute value of all sample values.
 // If any samples have label indicating they belong to the diff base, then the
 // total will only include samples with that label.
-func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) int64 {
+// Returns the profile total, and a boolean which is true if the profile was not
+// a diff base profile, but did have negative samples.
+func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) (int64, bool) {
 	var div, total, diffDiv, diffTotal int64
+	var negSamples bool
 	for _, sample := range prof.Sample {
 		var d, v int64
 		v = value(sample.Value)
 		if meanDiv != nil {
 			d = meanDiv(sample.Value)
 		}
 		if v < 0 {
+			negSamples = true
 			v = -v
 		}
 		total += v
@@ -1246,20 +1254,23 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i
 	if diffTotal > 0 {
 		total = diffTotal
 		div = diffDiv
+		// Negative samples are expected in diff base profiles.
+		negSamples = false
 	}
 	if div != 0 {
-		return total / div
+		return total / div, negSamples
 	}
-	return total
+	return total, negSamples
 }
 
 // Report contains the data and associated routines to extract a
 // report from a profile.
 type Report struct {
-	prof        *profile.Profile
-	total       int64
-	options     *Options
-	formatValue func(int64) string
+	prof            *profile.Profile
+	total           int64
+	unexpNegSamples bool
+	options         *Options
+	formatValue     func(int64) string
 }
 
 // Total returns the total number of samples in a report.

internal/report/report_test.go

@@ -28,8 +28,9 @@ import (
 )
 
 type testcase struct {
-	rpt  *Report
-	want string
+	rpt              *Report
+	want             string
+	wantNegSampleErr bool
 }
 
 func TestSource(t *testing.T) {
@@ -38,7 +39,13 @@ func TestSource(t *testing.T) {
 	sampleValue1 := func(v []int64) int64 {
 		return v[1]
 	}
-
+	negProfile := testProfile.Copy()
+	negProfile.Sample = negProfile.Sample[:2]
+	for _, sample := range negProfile.Sample {
+		for i, v := range sample.Value {
+			sample.Value[i] = -v
+		}
+	}
 	for _, tc := range []testcase{
 		{
 			rpt: New(
@@ -69,12 +76,33 @@ func TestSource(t *testing.T) {
 			),
 			want: path + "source.dot",
 		},
+		{
+			rpt: New(
+				negProfile.Copy(),
+				&Options{
+					OutputFormat: List,
+					Symbol:       regexp.MustCompile(`.`),
+					SampleValue:  sampleValue1,
+					SampleUnit:   negProfile.SampleType[1].Unit,
+				},
+			),
+			wantNegSampleErr: true,
+			want:             path + "source_neg_samples.rpt",
+		},
 	} {
 		var b bytes.Buffer
-		if err := Generate(&b, tc.rpt, &binutils.Binutils{}); err != nil {
+		ui := &proftest.TestUI{T: t}
+		if tc.wantNegSampleErr {
+			ui.AllowRx = "Profile has negative values, so percentage values may be incorrect. If using -base, consider using -diff_base instead."
+		}
+		if err := Generate(&b, tc.rpt, &binutils.Binutils{}, ui); err != nil {
 			t.Fatalf("%s: %v", tc.want, err)
 		}
 
+		if tc.wantNegSampleErr && ui.NumAllowRxMatches != 1 {
+			t.Errorf("want %q logged to UI once, got this message logged %d time(s)", ui.AllowRx, ui.NumAllowRxMatches)
+		}
+
 		gold, err := ioutil.ReadFile(tc.want)
 		if err != nil {
 			t.Fatalf("%s: %v", tc.want, err)
@@ -365,10 +393,11 @@ func TestComputeTotal(t *testing.T) {
 	}
 
 	testcases := []struct {
-		desc           string
-		prof           *profile.Profile
-		value, meanDiv func(v []int64) int64
-		wantTotal      int64
+		desc                string
+		prof                *profile.Profile
+		value, meanDiv      func(v []int64) int64
+		wantTotal           int64
+		wantUnexpNegSamples bool
 	}{
 		{
 			desc: "no diff base, all positive values, index 1",
@@ -392,7 +421,8 @@ func TestComputeTotal(t *testing.T) {
 			value: func(v []int64) int64 {
 				return v[1]
 			},
-			wantTotal: 111,
+			wantTotal:           111,
+			wantUnexpNegSamples: true,
 		},
 		{
 			desc: "diff base, some negative values",
@@ -406,9 +436,13 @@ func TestComputeTotal(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
-			if gotTotal := computeTotal(tc.prof, tc.value, tc.meanDiv); gotTotal != tc.wantTotal {
+			gotTotal, gotUnexpNegSamples := computeTotal(tc.prof, tc.value, tc.meanDiv)
+			if gotTotal != tc.wantTotal {
 				t.Errorf("got total %d, want %v", gotTotal, tc.wantTotal)
 			}
+			if gotUnexpNegSamples != tc.wantUnexpNegSamples {
+				t.Errorf("got unexpected negative samples %v, want %v", gotUnexpNegSamples, tc.wantUnexpNegSamples)
+			}
 		})
 	}
 }

internal/report/source_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/google/pprof/internal/binutils"
+	"github.com/google/pprof/internal/proftest"
 	"github.com/google/pprof/profile"
 )
 
@@ -27,7 +28,7 @@ func TestWebList(t *testing.T) {
 		SampleUnit:   cpu.SampleType[1].Unit,
 	})
 	var buf bytes.Buffer
-	if err := Generate(&buf, rpt, &binutils.Binutils{}); err != nil {
+	if err := Generate(&buf, rpt, &binutils.Binutils{}, &proftest.TestUI{T: t}); err != nil {
 		t.Fatalf("could not generate weblist: %v", err)
 	}
 	output := buf.String()

internal/report/testdata/source_neg_samples.rpt

@@ -0,0 +1,34 @@
+Total: 11
+ROUTINE ======================== bar in testdata/source1
+       -10        -10 (flat, cum) 90.91% of Total
+         .          .      5:source1 line 5;
+         .          .      6:source1 line 6;
+         .          .      7:source1 line 7;
+         .          .      8:source1 line 8;
+         .          .      9:source1 line 9;
+       -10        -10     10:source1 line 10;
+         .          .     11:source1 line 11;
+         .          .     12:source1 line 12;
+         .          .     13:source1 line 13;
+         .          .     14:source1 line 14;
+         .          .     15:source1 line 15;
+ROUTINE ======================== foo in testdata/source1
+         0        -10 (flat, cum) 90.91% of Total
+         .          .      1:source1 line 1;
+         .          .      2:source1 line 2;
+         .          .      3:source1 line 3;
+         .        -10      4:source1 line 4;
+         .          .      5:source1 line 5;
+         .          .      6:source1 line 6;
+         .          .      7:source1 line 7;
+         .          .      8:source1 line 8;
+         .          .      9:source1 line 9;
+ROUTINE ======================== main in testdata/source1
+        -1        -11 (flat, cum)   100% of Total
+         .          .      1:source1 line 1;
+        -1        -11      2:source1 line 2;
+         .          .      3:source1 line 3;
+         .          .      4:source1 line 4;
+         .          .      5:source1 line 5;
+         .          .      6:source1 line 6;
+         .          .      7:source1 line 7;