[Lldb-commits] [PATCH] D137614: [trace] Add a new call graph reconstructor

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:296
   std::vector<TraceItemStorage> m_item_data;
+  std::vector<TraceInstructionExtraInfo> m_insn_extra_info;
   /// The TraceItemKind for each trace item encoded as uint8_t. We don't include
----------------
jj10306 wrote:
> do we need to store this or can this information be calculated on demand? just thinking about massive traces and want to make sure we're intentional about only storing what we absolutely need to for each instruction
I thought about it and it will help us have more correctness for now and a little bit more speed with little memory cost. The total size per instruction went from 9 bytes to 11, which is very decent. 

Now some details: the Disassembler fails with some instructions. I was tracing dynolog and found many instances of the disassembler failing, but libipt doesn't fail. In other words, we can't get the control flow kind from LLDB but we can from libipt. Same for the byte size. That means that we need to fix the disassembler, but that will require a lot of work that I can hardly prioritize, so I'd rather use libipt for now.



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:317
              events_stats.total_count);
-    for (const auto &event_to_count : events_stats.events_counts) {
-      s.Format("      {0}: {1}\n",
-               TraceCursor::EventKindToString(event_to_count.first),
-               event_to_count.second);
+    for (auto it = events_stats.events_counts.begin();
+         it != events_stats.events_counts.end(); ++it) {
----------------
jj10306 wrote:
> what's the reason for this change?
wtf, i think this is from a previous commit i never intended to publish. I was playing with some stuff. I need to revert this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137614