[Lldb-commits] [PATCH] D129340: [trace][intel pt] Create a CPU change event and expose it in the dumper

[walter erquinigo via Phabricator via lldb-commits]

wallace requested review of this revision.
wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288
 
+    decoded_thread.NotifyCPU(execution.thread_execution.cpu_id);
+
----------------
jj10306 wrote:
> Does this mean our trace will always start with the CPU change event or would it be possible that we won't know the CPU id until the first context switch occurs?
for per-cpu tracing, we'll always have the cpu change event as the first trace item. For per-thread tracing, we won't have this


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:81-88
       return m_tsc_range->GetTsc();
     else
       return llvm::None;
   }
 }
 
+Optional<lldb::cpu_id_t> TraceCursorIntelPT::GetCPU() const {
----------------
jj10306 wrote:
> afaiu, we use pretty much the same data structures to keep track of TSCs and CPU Ids. Is there a reason that the TSC structures live on the trace cursor whereas the CPU id structures live on the decoded thread itself?
> Wondering if it would make more sense to move the TSC structures off of the trace cursor and be consistent with how it's done for CPU Ids.
> wdyt?
I've been trying different ways of storing the data and exposing them, so that's why you see some inconsistencies. And yes, we should things the same way we we've doing it for CPUs, but I'll leave it for a future diff, in which we also decide how we are going to expose TSCs and nanoseconds in the trace cursor.


================
Comment at: lldb/source/Target/TraceDumper.cpp:137-141
       m_s << "(event) " << TraceCursor::EventKindToString(*item.event);
+      if (*item.event == eTraceEventCPUChanged) {
+        m_s.Format(" [new CPU={0}]",
+                   item.cpu_id ? std::to_string(*item.cpu_id) : "unavailable");
+      }
----------------
jj10306 wrote:
> What about making an `EventToString` that abstracts away the details of how to print an event? Basically the same as `EventKindToString` but have it take in an item instead of event kind.
> Otherwise, we will have to continue adding logic here for each new event/any modifications to existing events
i think that won't be needed now because i don't see any other part of LLDB dumping this information in a string format. Other external consumers would probably have their own format for this data


================
Comment at: lldb/source/Target/TraceDumper.cpp:204-206
+    m_j.attribute("event", TraceCursor::EventKindToString(*item.event));
+    if (item.event == eTraceEventCPUChanged)
+      m_j.attribute("cpuId", item.cpu_id);
----------------
jj10306 wrote:
> similar suggestions as above about potentially moving all of this logic into a single function, maybe in this case it could be `EventToJson` or something similar and it takes in a ref to the json as well as the item
again, i don't see this being reused in other parts of LLDB, so I don't think it's worth doing the abstractions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129340