-
Notifications
You must be signed in to change notification settings - Fork 129
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
support kernel annotations for versioned vmlinux and kernel modules in collapse-perf #182
Conversation
It'd be great to include the examples from the upstream PR as tests! |
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
==========================================
+ Coverage 90.25% 90.53% +0.27%
==========================================
Files 16 17 +1
Lines 2238 2293 +55
==========================================
+ Hits 2020 2076 +56
+ Misses 218 217 -1
Continue to review full report at Codecov.
|
@jonhoo, can we add regex lib? The draft implementation uses |
I'd still prefer not to bring in regex, since I think these are pretty easy parsing cases: fn is_vmlinux(s: &str) -> bool {
if let Some(vm) = s.rfind("vmlinux") {
s[vm..].chars().all(|c| c.is_ascii_alphanumeric() || c == '-')
} else {
false
}
} |
d5eac75
to
efe233c
Compare
@jonhoo, I put the functions into separate module |
Yes, that looks great, thanks! |
efe233c
to
a979c92
Compare
I'm surprised that this doesn't affect any of the tests.. We don't have any with versioned kernel symbols I suppose? Maybe it'd be good to add a short one? Not sure if you feel like it might be worth it? |
@jonhoo, It would be good to add this case to test. Unfortunately I was not able to get the example and I do not want to create an artificial one instead. |
8c027d8
to
da4b09e
Compare
@AnderEnder Can't we use the ones in the original issue? Namely
and
|
Released in 0.10.1. |
Support kernel annotations for versioned vmlinux and kernel modules
Backport of brendangregg/FlameGraph#232
Example of stack trace: