etw-stackwalk: track modules in tracing service [chromium/src : main]
Sean Maher (Gerrit)
Sean Maher added 4 comments![]( "Open in Gerrit")
static constexpr char kNoDebugId[] = "fffffffffffffffffffffffffffffff";you should make it something cooler than _this_
if (!interned_module.was_emitted) {to avoid nesting you could do the same early return thing as in the place i commented 'naisu'
EVENT_TRACE_FLAG_IMAGE_LOAD; // ⭐⭐⭐ TODO: is this needed?⭐⭐⭐
Related details
- Jesse McKenna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jesse McKenna (Gerrit)
Jesse McKenna voted and added 5 comments![]( "Open in Gerrit")
Votes added by Jesse McKenna
| Commit-Queue | +1 |
5 comments
Thank you for the comments! I've polished this up a lot and I think it's a lot more ready now. : )
// ⭐⭐⭐ TODO: filenames or full paths?Jesse McKennahmmm
The latest patchset uses full filenames to track the modules (in ActiveProcesses) in case the same DLL is loaded from multiple places (for some reason...?) and trims to just filename when uploading, since the full path leaks user info and I don't see a need for it. It seems like terasymbol just needs the debug ID. Feel free to comment back if any of that sounds wrong, though!
static constexpr char kNoDebugId[] = "fffffffffffffffffffffffffffffff";you should make it something cooler than _this_
Haha fair enough! I put it back to the dummy string from your draft. I was worried that it might not be clear to viewers if it's surfaced in the Perfetto UI, but the "cool code" string is probably well-known enough.
to avoid nesting you could do the same early return thing as in the place i commented 'naisu'
Good idea, I reduced nesting throughout this block and I think that does make it way easier to read.
EVENT_TRACE_FLAG_IMAGE_LOAD; // ⭐⭐⭐ TODO: is this needed?Jesse McKenna⭐⭐⭐
Done (removed) - it's just my "don't forget to fix this" placeholder : )
Related details
- Etienne Pierre-Doray
- Sean Maher
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 6 comments![]( "Open in Gerrit")
Cool dude!
absl::flat_hash_map<base::FilePath, std::string> known_debug_ids_;TODO: fill known_debug_ids_ from browser?
absl::flat_hash_map<base::FilePath, Image> loaded_images;Can we make this a (sorted) map based on base_address? This would allow binary search in GetImageForAddress.
And still support lookup in RemoveLoadedImage.
if (!inclusion_policy_.ShouldRecordImageEvents(process_id)) {This could probably just be ShouldRecordCallStacks().
InterningIndexEntry interned_frame = interned_frames_.LookupOrAdd(
std::make_pair(relative_address, debug_id.value_or(kNoDebugId)));I think we could keep interned_frames_ keyed on instruction_pointer, this would make hashing cheaper (and code slightly simpler); we don't actually emit interned value.
Could this also collide (same debug id across different processes)?
reinterpret_cast<const uint8_t*>(debug_id_str.data()),Any idea why other calls don't need reinterpret_cast?
https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/src/profiling/common/interning_output.cc;l=110?q=InternedString%20-file:proto&ss=chromium%2Fchromium%2Fsrc
Related details
- Jesse McKenna
- Sean Maher
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
auto process_id = *iterator.CopyObject<uint32_t>();This will crash if packet is invalid?
We should probably check kMinimumSize similar to other functions.
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
uintptr_t base_address_ = 0;This should be uint64_t for correctness (x86 target can trace a x64 target)
Jesse McKenna (Gerrit)
Jesse McKenna added 8 comments![]( "Open in Gerrit")
absl::flat_hash_map<base::FilePath, std::string> known_debug_ids_;TODO: fill known_debug_ids_ from browser?
Done
absl::flat_hash_map<base::FilePath, Image> loaded_images;Can we make this a (sorted) map based on base_address? This would allow binary search in GetImageForAddress.
And still support lookup in RemoveLoadedImage.
Done
This should be uint64_t for correctness (x86 target can trace a x64 target)
Done
auto process_id = *iterator.CopyObject<uint32_t>();This will crash if packet is invalid?
We should probably check kMinimumSize similar to other functions.
Oops, done - thanks!
if (!inclusion_policy_.ShouldRecordImageEvents(process_id)) {This could probably just be ShouldRecordCallStacks().
That makes sense, done
InterningIndexEntry interned_frame = interned_frames_.LookupOrAdd(
std::make_pair(relative_address, debug_id.value_or(kNoDebugId)));I think we could keep interned_frames_ keyed on instruction_pointer, this would make hashing cheaper (and code slightly simpler); we don't actually emit interned value.
Could this also collide (same debug id across different processes)?
I'm open to this, but re: collision: I think the same [rel_pc, debug ID] could plausibly appear in more than one process, but I think that would also be true of a given instruction pointer. Also, if the same [rel_pc, debug ID] appeared in more than one process, the info associated with the frame would still be the same (rel_pc, debug ID, and module file path). On the other hand, if the same instruction pointer appeared in multiple processes, that info would also be different. That makes it seem like instruction pointer alone isn't suitable to identify a frame.
Please feel free to let me know if I'm just misunderstanding. : )
reinterpret_cast<const uint8_t*>(debug_id_str.data()),Any idea why other calls don't need reinterpret_cast?
https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/src/profiling/common/interning_output.cc;l=110?q=InternedString%20-file:proto&ss=chromium%2Fchromium%2Fsrc
Done (not needed) - I was using the `set_str(data, length)` overload, but the `set_str(string)` one is shorter and doesn't require casting. Thanks for noticing this.
Related details
- Etienne Pierre-Doray
- Sean Maher
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 5 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
5 comments
iter->second.loaded_images.insert(To make this more robust against missing Unload, this could use insert_or_assign.
const auto file_name = *CopyWString(iterator);If CopyWString can't find null character, it returns nullopt. It might be a good idea to guard this against invalid data:
```
auto file_name = CopyWString(iterator);
if (!file_name) {
return; // Safety exit if not null-terminated or truncated
}
```
InterningIndexEntry interned_frame = interned_frames_.LookupOrAdd(
std::make_pair(relative_address, debug_id.value_or(kNoDebugId)));Jesse McKennaI think we could keep interned_frames_ keyed on instruction_pointer, this would make hashing cheaper (and code slightly simpler); we don't actually emit interned value.
Could this also collide (same debug id across different processes)?
I'm open to this, but re: collision: I think the same [rel_pc, debug ID] could plausibly appear in more than one process, but I think that would also be true of a given instruction pointer. Also, if the same [rel_pc, debug ID] appeared in more than one process, the info associated with the frame would still be the same (rel_pc, debug ID, and module file path). On the other hand, if the same instruction pointer appeared in multiple processes, that info would also be different. That makes it seem like instruction pointer alone isn't suitable to identify a frame.
Please feel free to let me know if I'm just misunderstanding. : )
Ah makes sense, let's keep debug_id then.
InterningIndexEntry interned_module = interned_modules_.LookupOrAdd(
std::make_pair(stack_process, module->base_address_));I guess this could also be keyed by `debug_id`?
Related details
- Jesse McKenna
- Sean Maher
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |