[Lldb-commits] [PATCH] D123281: [trace][intel pt] Create a common accessor for live and postmortem data

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/Trace.h:265
+      lldb::tid_t tid, llvm::StringRef kind,
+      std::function<llvm::Error(llvm::ArrayRef<uint8_t> data)> callback);
+
----------------
jj10306 wrote:
> typedef the callback to be cleaner and make the intention more clear?
good idea


================
Comment at: lldb/include/lldb/Target/Trace.h:385
+  /// tid -> data kind -> file
+  std::map<lldb::tid_t, std::unordered_map<std::string, FileSpec>>
+      m_postmortem_thread_data;
----------------
jj10306 wrote:
> Can you explain what "kind" represents and why we need the nested map? Also, I see that for live tracing we have a map for both processes and threads, why is this not the case for post mortem?
those "kinds" are just arbitrarily tagged data. If you see the jLLDBTraceGetState, there's a mention to those data kinds. This is the client side of those data kinds. The difference in usage is that lldb-server will return it as a data blob in memory, and in the post-mortem case the data will be in files.

I should make those maps DenseMaps, now that I think of.

We still don't have a map for post mortem process because we haven't needed it yet, but we might soon.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:271-274
+  // The libipt library does not modify the trace buffer, hence the
+  // following casts are safe.
+  config.begin = const_cast<uint8_t *>(buffer.data());
+  config.end = const_cast<uint8_t *>(buffer.data() + buffer.size());
----------------
jj10306 wrote:
> The couple minor changes to Libiptdecoder aren't related to this diff, maybe move these to D123106.
these are needed because ArrayRef and MutableArrayRef have different const modifiers for their pointers. I like the fact that we are now passing an ArrayRef here instead of a mutable one for safety


================
Comment at: lldb/source/Target/Trace.cpp:265
+
+  MemoryBuffer &data = **trace_or_error;
+  ArrayRef<uint8_t> array_ref(
----------------
jj10306 wrote:
> is guaranteed that the UP will be non-null? 
yes, otherwise there would have been an error in the previous if


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123281