Add support for reading symbols from /tmp/perf-pid.map #573
Conversation
This adds basic support for /tmp/perf-pid.map. To cope with processes in containers, it supports: * mapping from BCC's PID namespace to the target process's PID namespace using /proc/pid/status * resolving a target process's root filesystem using /proc/pid/root
| @@ -0,0 +1,38 @@ | |||
| /* | |||
| * Copyright (c) 2016 GitHub, Inc. | |||
There was a problem hiding this comment.
Whoops, yes. Fixed now.
|
LGTM |
|
|
||
| size_t size; | ||
| char *line = NULL; | ||
| while (getline(&line, &size, status) != -1) |
There was a problem hiding this comment.
Not quite right here. According to POSIX.1-200, getline will only allocate a new buffer to store the line when line == NULL and size == 0. Since size is uninitialized here, this can break in really unexpected ways.
On top of that, getline allocates a buffer for each call (even in failure cases), and you're calling free only once outside the loop, hence leaking significant amounts of memory.
Please fix the initialization and deallocation, and add some curly braces to prevent further bugs like this in the future. Thanks! :)
There was a problem hiding this comment.
Thanks for the feedback!
Fixing size is fine. My getline man page actually says it's ignored if *lineptr == NULL, but http://man7.org/linux/man-pages/man3/getline.3.html says it must be zero. Regardless, there's clearly there's no downside to intializing to zero so I'll do that.
Re: getline allocating on every call: are you sure about that? http://man7.org/linux/man-pages/man3/getline.3.html says getline will use the buffer passed and realloc it if necessary. It doesn't say it'll allocate every time around and the example code in the man page just has a single free outside the loop. Additionally, my own simple test with valgrind appears to confirm this is safe:
https://gist.github.com/markdrayton/1ef00109ff2092898ab3b908d0598c41
There was a problem hiding this comment.
Ah, you're totally right, I misread the manpage. It will indeed realloc if the original pointer is not NULL... That is a much more friendly API to use! So yes, the only frisky thing is initializing the value of size. Sorry for wasting your time!
|
Sorry I couldn't get to review this earlier. @markdrayton: I quite dig the tests, but you're not using |
| return -1; | ||
|
|
||
| char *line = NULL; | ||
| size_t size; |
There was a problem hiding this comment.
Same here. This needs zero initialization.
|
Everything else looks good. Thanks for adding this feature! |
For #548, this adds basic support for reading symbols from
/tmp/perf-pid.map(e.g., from a JIT or other code relocation). To cope with processes in containers, it supports:using
/proc/pid/status/proc/pid/rootNote that mapping to a target's PID in a separate namespace requires a
NSpid:line in/proc/pid/status. IIRC this was added sometime in 4.0 but I can't remember exactly when.