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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
