Add extra checks to select the right mapping and filter samples within it. #266
Conversation
* Pass pointers to the perf profile proto and the main memory mapping instead of storing them as class members. * The `PerfParser` also does not need to be held on to - it is used only once. * The `PerfReader` must be held on to since it owns all of the perf related data.
…bject. * A new check of the sample PID against the main mapping PID. * Remove the check for whether the block's code was within the bounds of the executable segment in terms of file addresses, this is redundant as long as the mapping is valid.
|
|
||
| namespace { | ||
|
|
||
| std::string GetFileNameFromPath(const std::string &path) { |
There was a problem hiding this comment.
This could work with llvm::StringRef and save some copying. This would also save the llvm::StringRef -> std::string conversion on line 92.
| event.mmap_event().prot() & 0b001 /* PROT_READ */ && | ||
| event.mmap_event().prot() & 0b100 /* PROT_EXEC */) { |
There was a problem hiding this comment.
Do PROT_READ and PROT_EXEC exist as symbolic constants somewhere? If not, let's define them here in the file.
| GetFileNameFromPath(event.mmap_event().filename()) == file_name && | ||
| event.mmap_event().prot() & 0b001 /* PROT_READ */ && | ||
| event.mmap_event().prot() & 0b100 /* PROT_EXEC */) { | ||
| return &event.mmap_event(); | ||
| } | ||
| } |
There was a problem hiding this comment.
If there are multiple mmap events, we might want to use all of them (and then match by the mapped range/virtual address of the branch).
Not in this PR, but perhaps let's keep this as a TODO?
| } | ||
| } | ||
|
|
||
| return absl::OkStatus(); | ||
| return absl::InvalidArgumentError(absl::StrFormat( | ||
| "The given `perf.data`-like file does not have a mapping corresponding " |
There was a problem hiding this comment.
Very nit: In these multi-line string literals, I'd recommend putting the space between words at the beginning of the following line rather than at the end of the current line.
This gives a nice visual cue that this is a longer text, and it's easier to check that there are spaces everywhere.
| // Filter out non-sample events. | ||
| if (!event.has_sample_event()) { | ||
| continue; | ||
| } | ||
|
|
||
| // Filter out sample events from outside the profiled binary. | ||
| if (!event.sample_event().has_pid() || | ||
| !(event.sample_event().pid() == mapping->pid())) | ||
| continue; |
There was a problem hiding this comment.
Very nit: Please put braces around this statement, for consistency with the rest of the code.
| @@ -73,17 +73,26 @@ absl::StatusOr<const quipper::PerfDataProto *> AnnotatingImporter::LoadPerfData( | |||
| return &perf_reader_.proto(); | |||
| } | |||
|
|
|||
| namespace { | |||
|
|
|||
| std::string GetFileNameFromPath(const std::string &path) { | |||
There was a problem hiding this comment.
Nit: This is usually called "Basename" in the unix world.
of the executable segment in terms of file addresses, this is
redundant as long as the mapping is valid.