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