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