weblist output disassembly now contains inlined source. #235

ghemawat · 2017-09-28T21:07:31Z

In addition, fixed the line number assigned to instructions from inlined calls.

As an illustration, see the before and after output for a "pprof -weblist" command for a simple function:

Note that in the "before" page, the 7.26 second line cannot be expanded, but in the "after" page clicking on it reveals a wealth of information.

codecov-io · 2017-09-28T21:55:55Z

Codecov Report

Merging #235 into master will increase coverage by 3.32%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
+ Coverage   61.71%   65.04%   +3.32%     
==========================================
  Files          34       34              
  Lines        7081     7134      +53     
==========================================
+ Hits         4370     4640     +270     
+ Misses       2326     2093     -233     
- Partials      385      401      +16

Impacted Files	Coverage Δ
internal/report/source_html.go	`0% <ø> (ø)`	⬆️
internal/report/report.go	`31.49% <100%> (+9.57%)`	⬆️
internal/report/source.go	`80.77% <81.05%> (+52.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d78d1ea...cfa9ea2. Read the comment docs.

ghemawat · 2017-09-28T22:01:11Z

I am confused by these code coverage reports.

E.g., clicking through to https://codecov.io/gh/google/pprof/pull/235/src/internal/report/report.go, it says that coverage has disappeared for printCallgrind, and showed up for getDisambiguatedNames. Neither of those two functions should have been affected by this PR.

Is the code coverage report actually reliable?

aalexand · 2017-09-28T23:18:14Z

@ghemawat It does look confusing sometimes. E.g. on #236 where I just fixed the test.sh script to strip some redundant tags from the coverage output the coverage changed which I wouldn't expect. I will look more into this.

That said, looking at your report.go link, where does it say the coverage for printCallgrind disappeared? I can see it all read which I think means it's just not covered at all.

aalexand · 2017-09-28T23:27:08Z

Similarly for getDisambiguatedNames, I don't think it shows that the function is now covered, it just shows that it used to be covered and covered now. Codecov has a Chrome extension which is useful to overlay the current coverage data over the source in Github:

aalexand · 2017-09-29T19:21:43Z

internal/report/source.go

+		// So find the outer-most linenumber in the source file.
+		frames, err := o.file.SourceLine(an.address + o.base)
+		found := false
+		if err == nil {


Nit: Go style is to limit the scope of variables when possible by putting the assignment within if:

found := false if frames, err := o.file.SourceLine(an.address + o.base); err == nil { ...

Also, should something be done with the error?

Done w.r.t. scoping.

Error handling: The one other call to SourceLine (in symbolizer.go) treats an error as missing information. I think we should follow the same approach here for graceful degradation in case of missing data.

aalexand · 2017-09-29T19:27:52Z

internal/report/source.go

+		lines = []string{""} // Skip 0th line
+		f, err := openSourceFile(path, reader.searchPath)
+		if err != nil {
+			reader.errors[path] = err


Cover the error path?

It is covered. The error gets remembered in the sourceReader, and then dealt with at the bottom of getSourceFromLine. Added comment to sourceReader struct to explain this better.

Sorry, I meant cover in tests. The coverage told the path of remembering the error in reader.errors is not covered. It now says that it's covered (I couldn't see why though).

I think the new test code I added is what caused coverage for this code (getSourceFromFile uses the line method).

I was not sure what caused "reader.errors[path] = err" line under the "err != nil" condition to be covered, which is what the coverage says now. The test case provides the source files so my feeling was it shouldn't get into the error handler here. Maybe the coverage info is off.

aalexand · 2017-09-29T19:28:08Z

internal/report/source.go

@@ -529,3 +609,20 @@ func trimPath(path string) string {
 	}
 	return path
 }
+
+func indentation(line string) int {


Add a test.

aalexand · 2017-09-29T19:35:29Z

internal/report/source.go

+		// number from F, not from H (which is what Disasm gives us).
+		//
+		// So find the outer-most linenumber in the source file.
+		frames, err := o.file.SourceLine(an.address + o.base)


From the diff view in the coverage UI looks like this new code is not covered, apparently because the enclosing function is not covered. Consider adding a test, though feel free to decline because the lack of test seems pre-existing.

Done: had to add a sample program, its binary, and a generated profile.

aalexand · 2017-09-29T19:37:58Z

The change is pretty cool, I forgot to say.

* Added a sample program, its binary and profile. * Added weblist test based on sample program. * Added test for indentation.

aalexand

Just a couple more notes.

aalexand · 2017-09-29T21:30:58Z

internal/report/source.go

+		lines = []string{""} // Skip 0th line
+		f, err := openSourceFile(path, reader.searchPath)
+		if err != nil {
+			reader.errors[path] = err


Sorry, I meant cover in tests. The coverage told the path of remembering the error in reader.errors is not covered. It now says that it's covered (I couldn't see why though).

aalexand · 2017-09-29T21:36:47Z

internal/report/source_test.go

+)
+
+func TestWebList(t *testing.T) {
+	if runtime.GOOS != "linux" {


Should this check for runtime.GOARCH == "amd64" or is objdump cross-arch enough?

Good point. Extended the conditional.

aalexand · 2017-09-29T21:39:41Z

internal/report/source_test.go

+		{"        \tfoo", 16},
+	} {
+		if n := indentation(c.str); n != c.indent {
+			t.Errorf("indentation for %v: expect %d, got %d\n", c.str, c.indent, n)


t.Errorf("indentation(%s): got %d, want %d", c.str, n, c.indent), see https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures

aalexand · 2017-09-29T21:40:41Z

internal/report/source_test.go

+func TestIndentation(t *testing.T) {
+	for _, c := range []struct {
+		str    string
+		indent int


Nit: wantIndent

aalexand · 2017-09-29T22:43:50Z

internal/report/testdata/sample/sample.go

+	busyLoop()
+}
+
+func busyLoop() {


FWIW, I am not sure Golang includes the DWARF inline debug information, at least I don't see the inline expansion working with this test case - https://aalexand.github.io/scratch/busyloop.html. We can improve it later I guess.

Yes, I could not cause inlining to show up properly in the intermediate data produced by this test. We can add in a C++ binary later that relies on inlining and test its coverage as well.

An alternative would be to use faked out ObjTool, but I think using the real ObjTool gives better confidence that things are working.

Agree, it's better to avoid fakes as much as possible.

weblist output disassembly now contains inlined source. In addition, fixed the line number assigned to instructions from inlined calls. As an illustration, see the before and after output for a "pprof -weblist" command for a simple function: [before](https://ghemawat.github.io/scratch/tmp/list1.html) [after](https://ghemawat.github.io/scratch/tmp/list2.html) Note that in the "before" page, the 7.26 second line cannot be expanded, but in the "after" page clicking on it reveals a wealth of information.

googlebot added the cla: yes label Sep 28, 2017

ghemawat closed this Sep 28, 2017

ghemawat reopened this Sep 28, 2017

aalexand reviewed Sep 29, 2017

View reviewed changes

ghemawat added 4 commits September 29, 2017 12:56

weblist output disassembly now contains inlined source.

3fef6c6

Handle review comments.

78d6b10

* Added a sample program, its binary and profile. * Added weblist test based on sample program. * Added test for indentation.

Add missing %s to Fatalf call.

e75739b

Relativize profile file names so test works on other machines.

cfa9ea2

aalexand reviewed Sep 29, 2017

View reviewed changes

aalexand approved these changes Oct 3, 2017

View reviewed changes

aalexand merged commit 8494129 into google:master Oct 3, 2017

nolanmar511 mentioned this pull request Nov 1, 2017

Skip TestWebInterface on nacl #252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

weblist output disassembly now contains inlined source. #235

weblist output disassembly now contains inlined source. #235

ghemawat commented Sep 28, 2017

codecov-io commented Sep 28, 2017 •

edited

Loading

ghemawat commented Sep 28, 2017

aalexand commented Sep 28, 2017

aalexand commented Sep 28, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Oct 3, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand commented Sep 29, 2017

aalexand left a comment

aalexand Sep 29, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Sep 29, 2017

ghemawat Sep 29, 2017

aalexand Oct 3, 2017

weblist output disassembly now contains inlined source. #235

weblist output disassembly now contains inlined source. #235

Conversation

ghemawat commented Sep 28, 2017

codecov-io commented Sep 28, 2017 • edited Loading

Codecov Report

ghemawat commented Sep 28, 2017

aalexand commented Sep 28, 2017

aalexand commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand commented Sep 29, 2017

aalexand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 28, 2017 •

edited

Loading