[PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

[Jakob Johnson via Phabricator via llvm-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

feedback-v1



================
Comment at: lldb/include/lldb/Target/Trace.h:520
+  /// core id -> data kind -> size
+  llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, size_t>>
+      m_live_core_data;
----------------
Would this work instead? I noticed that the several other maps around this code also use unordered_map and string, maybe we could update those too at some point.


================
Comment at: lldb/include/lldb/Target/Trace.h:538
+  /// core id -> data kind -> file
+  llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, FileSpec>>
+      m_postmortem_core_data;
----------------
same as comment above related to using LLVM types rather than std types wherever possible.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:193
 
-Expected<std::vector<uint8_t>>
-IntelPTMultiCoreTrace::GetBinaryData(const TraceGetBinaryDataRequest &request) {
-  return createStringError(inconvertibleErrorCode(), "Unimplemented");
+Expected<Optional<std::vector<uint8_t>>>
+IntelPTMultiCoreTrace::TryGetBinaryData(
----------------
`Expected<Optional<...>>` feels weirddddd. Are both of these "layers" needed?
I noticed this in a couple places but I'm just leaving a single comment here.


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:191
+   * with respect to their definition of head pointer. In the case
+   * of Aux data buffer the head always wraps around the aux buffer
+   * and we don't need to care about it, whereas the data_head keeps
----------------
Be careful with the "strong" language here. The AUX head only wraps around when the AUX buffer is mmaped with read-only, if it is mmapped with write perms, then the AUX head behaves like the data head (it doesn't auto wrap)

See this snipped of kernel code, the overwrite flag is set based on the mmap PROT and that overwrite flag is passed to the intelpt code.
https://github.com/torvalds/linux/blob/df0cc57e/kernel/events/ring_buffer.c#L670-L733


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126015/new/

https://reviews.llvm.org/D126015