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

Add pc to help identify unknown addresses. #124

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

gdbentley
Copy link
Contributor

Add the function's pc to the module name or the "unknown" string in order to identify where the unknown lies (useful for managed runtimes that are missing some reporting of JIT or other symbols) and to differentiate between unknowns.

@brendangregg
Copy link
Owner

Where does the "{PC}" style come from? If another Linux tool uses curly brackets to signify a PC, then good, we're following a convention. If instead we're making it up, then I might double check whether a convention already exists. Just trying to follow principle of least surprise.

@gdbentley
Copy link
Contributor Author

It's made up. The tools that I know of (gdb, perf report, VTune, etc.) will either print a symbol or the address, not both. I didn't want to give up printing the module name if that was known though. I chose curly braces as a delimiter since they are not valid characters in identifiers in any language I'm aware of (e.g. C, C++, Java, PHP, Perl, or Python) and thus would not be found in a module or function name.

@brendangregg
Copy link
Owner

Ok, I've looked around. Things like oops messages, ftrace hist triggers, and lockstat will print raw addresses. The formats I've seen include address, 0xaddress, [address], and [<address>].

We're already using []'s. I'm tempted to go with <>, so that it's:

[unknown <ffffffff8104ba65>]

Which looks similar to a lockstat and oops message call stack format.

I tried it out with some production traces, and it increases the SVG size by between 15% and about 300%, for data that I think a lot of us won't use. I'm make this an optional parameter, like --addrs, so we can do it when desired.

@gdbentley
Copy link
Contributor Author

I've made the suggested changes and rebased. Please have another look.

@brendangregg
Copy link
Owner

Thanks, works!

@brendangregg brendangregg merged commit 2aa2153 into brendangregg:master Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants