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

[Jakob Johnson via Phabricator via lldb-commits]

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

looks good overall, just a couple minor suggestions and questions



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288
 
+    decoded_thread.NotifyCPU(execution.thread_execution.cpu_id);
+
----------------
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?


================
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 {
----------------
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?


================
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");
+      }
----------------
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


================
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);
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129340