Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source listing handles missing binaries and addresses. #628

Merged
merged 2 commits into from
May 6, 2021

Conversation

ghemawat
Copy link
Contributor

@ghemawat ghemawat commented May 6, 2021

Some profiles can contain locations without an address, or reference
binaries/libraries that are not available. Treat these better when
producing source listings by using any source information present
directly in the profile.Location.

Details:

If a location does not have an address, create a synthetic address
that does not fall in a known mapping.

If a location references an object file that is not found, use any
file/line information baked directly in the location.

Tweaked generated html to clicking for source lines that have no
inlining/assembly to display.

Fixes #621.

Some profiles can contain locations without an address, or reference
binaries/libraries that are not available. Treat these better when
producing source listings by using any source information present
directly in the profile.Location.

Details:

If a location does not have an address, create a synthetic address
that does not fall in a known mapping.

If a location references an object file that is not found, use any
file/line information baked directly in the location.

Tweaked generated html to clicking for source lines that have no
inlining/assembly to display.
@google-cla google-cla bot added the cla: yes label May 6, 2021
@ghemawat ghemawat linked an issue May 6, 2021 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2021

Codecov Report

Merging #628 (3178125) into master (3a04a4d) will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
+ Coverage   67.05%   67.32%   +0.26%     
==========================================
  Files          39       40       +1     
  Lines        7246     7295      +49     
==========================================
+ Hits         4859     4911      +52     
+ Misses       1950     1948       -2     
+ Partials      437      436       -1     
Impacted Files Coverage Δ
...c/github.com/google/pprof/internal/report/synth.go 100.00% <0.00%> (ø)
.../github.com/google/pprof/internal/report/source.go 79.49% <0.00%> (+2.22%) ⬆️

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 3a04a4d...3178125. Read the comment docs.

addr = sp.synth.address(loc)
}

cum[addr] += value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pre-existing and doesn't have to be changed (if it needs to be changed) in this CL, but: do we handle recursion correctly here? It feels that we'll double count the cumulative time if the same address is present multiple times in the stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right. Let me look at that in a separate change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hate to butt in here, but is there a chance that this is related to #720?

I'm not familiar with this code, but a flavor of #720 manifested for me with the following conditions:

  • using weblist instead of list
  • providing the binary for disassembly

and had the following behavior:

  • in a sample function that had 4 caller locations, the count was 5x what the expected value (e.g. that returned by list or weblist without the binary) was.
  • the printed assembly was printed 5 times

image

As far as I can tell, that fits the bill for the code in question here: the call tree for this function is only invoked for web printing and not the CLI interactions

internal/report/synth.go Show resolved Hide resolved
internal/report/synth.go Show resolved Hide resolved
@aalexand aalexand merged commit 923b5ab into google:master May 6, 2021
@ghemawat ghemawat deleted the source2 branch May 6, 2021 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blank webui.source page without any error
4 participants