[trace][intelpt] Support system-wide tracing [18] - some more improvements

[wallace]

Summary

This applies the changes requested for diff 12.

• use DenseMap<ConstString, _> instead of std::unordered_map<ConstString, _>, which is more idiomatic and possibly performant.
• deduplicate some code in Trace.cpp by using helper functions for fetching in maps
• stop using size and offset when fetching binary data, because we in fact read the entire buffers all the time. If we ever need streaming, we can implement it then. Now, the size is used only to check that we are getting the correct amount of data. This is useful because in some cases determining the size doesn't involve fetching the actual data.
• added back the x86_64 macro to the perf tests
• added more documentation
• simplified some file handling
• fixed some comments

[jj10306]

looks great overall, just a couple minor things.

lldb/source/Plugins/Process/Linux/Perf.cpp

205–217

can't this now be simplified to something like:

data_head %= data_size;
// Copy from the head to the end of the buffer.
output.insert(output.end(), data_head, data_size);
// Copy from the start of the buffer to the head.
output.insert(data.end(), data_start, data_head);

you will need to make the args to insert ptrs but the rough idea should be clear that you can unconditionally modulo the head no need to manually iterate any longer, just let insert handle that for you. you might need a check to see if the buffer is empty as well.

257–261

same comment as above

lldb/source/Plugins/Process/Linux/Perf.h

203–210

this is great, thanks for adding this

lldb/source/Target/Trace.cpp

45

nit: maybe put these in a separate namespace, up to you though.

67–75

nit: potentially better names on these?

[wallace]

Fixed in https://reviews.llvm.org/D127881

lldb/source/Plugins/Process/Linux/Perf.cpp

205–217

yes! They will be fixed in https://reviews.llvm.org/D127881

257–261

+1

lldb/source/Target/Trace.cpp

45

they are static, so it's equivalent to having them in an anonymous namespace. LLVM prefers this approach over creating a specific namespace

67–75

I'm just renaming them to Lookup. The 2 is confusing

lldb/docs/lldb-gdb-remote.txt

 //   "type": <string>,
 //       Tracing technology name, e.g. intel-pt, arm-etm.
 //   "kind": <string>,
 //       Identifier for the data.
 //   "coreId": <Optional decimal>,
 //       Core id in decimal if the data belongs to a CPU core.
 //   "tid"?: <Optional decimal>,
 //       Tid in decimal if the data belongs to a thread.
-//   "offset": <decimal>,
-//       Offset of the data in bytes.
-//   "size": <decimal>,
-//      Number of bytes in to read starting from the offset.
 //  }
 //
 // INTEL PT
 //
 //  Binary data kinds:
 //    - traceBuffer: trace buffer for a thread or a core.
 //    - procfsCpuInfo: contents of the /proc/cpuinfo file.
 //----------------------------------------------------------------------

lldb/include/lldb/Target/Trace.h

protected:
   Process *GetLiveProcess();
 
   /// Get the currently traced postmortem processes.
   ///
   /// \return
   ///     If it's not a live process session, return an empty list.
   llvm::ArrayRef<Process *> GetPostMortemProcesses();
 
+  /// Dispatcher for live trace data requests with some additional error
+  /// checking.
+  llvm::Expected<std::vector<uint8_t>>
+  GetLiveTraceBinaryData(const TraceGetBinaryDataRequest &request,
+                         uint64_t expected_size);
+
   /// Implementation of \a OnThreadBinaryDataRead() for live threads.
   llvm::Error OnLiveThreadBinaryDataRead(lldb::tid_t tid, llvm::StringRef kind,
                                          OnBinaryDataReadCallback callback);
 
   /// Implementation of \a OnLiveBinaryDataRead() for live cores.
   llvm::Error OnLiveCoreBinaryDataRead(lldb::core_id_t core,
                                        llvm::StringRef kind,
                                        OnBinaryDataReadCallback callback);
struct Storage {
     std::vector<Process *> postmortem_processes;
 
     /// These data kinds are returned by lldb-server when fetching the state of
     /// the tracing session. The size in bytes can be used later for fetching
     /// the data in batches.
     /// \{
 
     /// tid -> data kind -> size
-    llvm::DenseMap<lldb::tid_t, std::unordered_map<std::string, uint64_t>>
+    llvm::DenseMap<lldb::tid_t, llvm::DenseMap<ConstString, uint64_t>>
         live_thread_data;
 
     /// core id -> data kind -> size
-    llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, uint64_t>>
+    llvm::DenseMap<lldb::core_id_t, llvm::DenseMap<ConstString, uint64_t>>
         live_core_data_sizes;
     /// core id -> data kind -> bytes
     llvm::DenseMap<lldb::core_id_t,
-                   std::unordered_map<std::string, std::vector<uint8_t>>>
+                   llvm::DenseMap<ConstString, std::vector<uint8_t>>>
         live_core_data;
 
     /// data kind -> size
-    std::unordered_map<std::string, uint64_t> live_process_data;
+    llvm::DenseMap<ConstString, uint64_t> live_process_data;
     /// \}
 
     /// The list of cores being traced. Might be \b None depending on the
     /// plug-in.
     llvm::Optional<std::vector<lldb::core_id_t>> cores;
 
     /// Postmortem traces can specific additional data files, which are
     /// represented in this variable using a data kind identifier for each file.
     /// \{
 
     /// tid -> data kind -> file
-    llvm::DenseMap<lldb::tid_t, std::unordered_map<std::string, FileSpec>>
+    llvm::DenseMap<lldb::tid_t, llvm::DenseMap<ConstString, FileSpec>>
         postmortem_thread_data;
 
     /// core id -> data kind -> file
-    llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, FileSpec>>
+    llvm::DenseMap<lldb::core_id_t, llvm::DenseMap<ConstString, FileSpec>>
         postmortem_core_data;
 
     /// \}
 
     llvm::Optional<std::string> live_refresh_error;
   } m_storage;
 
   /// Get the storage after refreshing the data in the case of a live process.
   Storage &GetUpdatedStorage();
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TRACE_H

lldb/include/lldb/Utility/TraceGDBRemotePackets.h

struct TraceGetBinaryDataRequest {
   /// Tracing technology name, e.g. intel-pt, arm-coresight.
   std::string type;
   /// Identifier for the data.
   std::string kind;
   /// Optional tid if the data is related to a thread.
   llvm::Optional<lldb::tid_t> tid;
   /// Optional core id if the data is related to a cpu core.
   llvm::Optional<lldb::core_id_t> core_id;
-  /// Offset in bytes from where to start reading the data.
-  uint64_t offset;
-  /// Number of bytes to read.
-  uint64_t size;
 };
 
 bool fromJSON(const llvm::json::Value &value,
               lldb_private::TraceGetBinaryDataRequest &packet,
               llvm::json::Path path);
 
 llvm::json::Value toJSON(const lldb_private::TraceGetBinaryDataRequest &packet);
 /// \}
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_TRACEGDBREMOTEPACKETS_H

lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp

if (!request.core_id)
     return None;
   auto it = m_traces_per_core.find(*request.core_id);
   if (it == m_traces_per_core.end())
     return createStringError(
         inconvertibleErrorCode(),
         formatv("Core {0} is not being traced", *request.core_id));
 
   if (request.kind == IntelPTDataKinds::kTraceBuffer)
-    return it->second.first.GetTraceBuffer(request.offset, request.size);
+    return it->second.first.GetTraceBuffer();
   if (request.kind == IntelPTDataKinds::kPerfContextSwitchTrace)
-    return it->second.second.ReadFlushedOutDataCyclicBuffer(request.offset,
-                                                            request.size);
+    return it->second.second.GetReadOnlyDataBuffer();
   return None;
 }

lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h

public:
   llvm::Expected<std::vector<uint8_t>>
   GetBinaryData(const TraceGetBinaryDataRequest &request) const;
 
   /// Read the trace buffer managed by this trace instance. To ensure that the
   /// data is up-to-date and is not corrupted by read-write race conditions, the
   /// underlying perf_event is paused during read, and later it's returned to
   /// its initial state.
   ///
-  /// \param[in] offset
-  ///     Offset of the data to read.
-  ///
-  /// \param[in] size
-  ///     Number of bytes to read.
-  ///
   /// \return
-  ///     A vector with the requested binary data. The vector will have the
-  ///     size of the requested \a size. Non-available positions will be
-  ///     filled with zeroes.
-  llvm::Expected<std::vector<uint8_t>> GetTraceBuffer(size_t offset,
-                                                      size_t size);
+  ///     A vector with the requested binary data.
+  llvm::Expected<std::vector<uint8_t>> GetTraceBuffer();
 
   /// \return
   ///     The total the size in bytes used by the trace buffer managed by this
   ///     trace instance.
   size_t GetTraceBufferSize() const;
 
   /// Resume the collection of this trace.
   ///

lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp

 Error IntelPTSingleBufferTrace::Pause() {
   return m_perf_event.DisableWithIoctl();
 }
 
 Error IntelPTSingleBufferTrace::Resume() {
   return m_perf_event.EnableWithIoctl();
 }
 
-Expected<std::vector<uint8_t>>
-IntelPTSingleBufferTrace::GetTraceBuffer(size_t offset, size_t size) {
+Expected<std::vector<uint8_t>> IntelPTSingleBufferTrace::GetTraceBuffer() {
   // Disable the perf event to force a flush out of the CPU's internal buffer.
   // Besides, we can guarantee that the CPU won't override any data as we are
   // reading the buffer.
   // The Intel documentation says:
   //
   // Packets are first buffered internally and then written out
   // asynchronously. To collect packet output for postprocessing, a collector
   // needs first to ensure that all packet data has been flushed from internal
   // buffers. Software can ensure this by stopping packet generation by
   // clearing IA32_RTIT_CTL.TraceEn (see “Disabling Packet Generation” in
   // Section 35.2.7.2).
   //
   // This is achieved by the PERF_EVENT_IOC_DISABLE ioctl request, as
   // mentioned in the man page of perf_event_open.
-  return m_perf_event.ReadFlushedOutAuxCyclicBuffer(offset, size);
+  return m_perf_event.GetReadOnlyAuxBuffer();
 }
 
 Expected<IntelPTSingleBufferTrace>
 IntelPTSingleBufferTrace::Start(const TraceIntelPTStartRequest &request,
                                 Optional<lldb::tid_t> tid,
                                 Optional<core_id_t> core_id, bool disabled) {
 #ifndef PERF_ATTR_SIZE_VER5
   return createStringError(inconvertibleErrorCode(),

lldb/source/Plugins/Process/Linux/IntelPTThreadTraceCollection.cpp

IntelPTThreadTraceCollection::TryGetBinaryData(
   if (request.kind != IntelPTDataKinds::kTraceBuffer)
     return None;
 
   if (!TracesThread(*request.tid))
     return None;
 
   if (Expected<IntelPTSingleBufferTrace &> trace =
           GetTracedThread(*request.tid))
-    return trace->GetTraceBuffer(request.offset, request.size);
+    return trace->GetTraceBuffer();
   else
     return trace.takeError();
 }

lldb/source/Plugins/Process/Linux/Perf.h

public:
   ///
   /// This should be called only after \a PerfEvent::MmapMetadataAndBuffers,
   /// otherwise a failure might happen.
   ///
   /// \return
   ///   \a ArrayRef<uint8_t> extending \a aux_size bytes from \a aux_offset.
   llvm::ArrayRef<uint8_t> GetAuxBuffer() const;
 
-  /// Read the aux buffer managed by this perf event. To ensure that the
-  /// data is up-to-date and is not corrupted by read-write race conditions, the
-  /// underlying perf_event is paused during read, and later it's returned to
-  /// its initial state. The returned data will be linear, i.e. it will fix the
-  /// circular wrapping the might exist int he buffer.
-  ///
-  /// \param[in] offset
-  ///     Offset of the data to read.
-  ///
-  /// \param[in] size
-  ///     Number of bytes to read.
-  ///
-  /// \return
-  ///     A vector with the requested binary data. The vector will have the
-  ///     size of the requested \a size. Non-available positions will be
-  ///     filled with zeroes.
-  llvm::Expected<std::vector<uint8_t>>
-  ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size);
-
-  /// Read the data buffer managed by this perf event. To ensure that the
-  /// data is up-to-date and is not corrupted by read-write race conditions, the
-  /// underlying perf_event is paused during read, and later it's returned to
-  /// its initial state. The returned data will be linear, i.e. it will fix the
-  /// circular wrapping the might exist int he buffer.
-  ///
-  /// \param[in] offset
-  ///     Offset of the data to read.
-  ///
-  /// \param[in] size
-  ///     Number of bytes to read.
-  ///
-  /// \return
-  ///     A vector with the requested binary data. The vector will have the
-  ///     size of the requested \a size. Non-available positions will be
-  ///     filled with zeroes.
-  llvm::Expected<std::vector<uint8_t>>
-  ReadFlushedOutDataCyclicBuffer(size_t offset, size_t size);
+  /// Read the aux buffer managed by this perf event assuming it was configured
+  /// with PROT_READ permissions only, which indicates that the buffer is
+  /// automatically wrapped and overwritten by the kernel or hardware. To ensure
+  /// that the data is up-to-date and is not corrupted by read-write race
+  /// conditions, the underlying perf_event is paused during read, and later
+  /// it's returned to its initial state. The returned data will be linear, i.e.
+  /// it will fix the circular wrapping the might exist in the buffer.
+  ///
+  /// \return
+  ///     A vector with the requested binary data.
+  llvm::Expected<std::vector<uint8_t>> GetReadOnlyAuxBuffer();
+
+  /// Read the data buffer managed by this perf even assuming it was configured
+  /// with PROT_READ permissions only, which indicates that the buffer is
+  /// automatically wrapped and overwritten by the kernel or hardware. To ensure
+  /// that the data is up-to-date and is not corrupted by read-write race
+  /// conditions, the underlying perf_event is paused during read, and later
+  /// it's returned to its initial state. The returned data will be linear, i.e.
+  /// it will fix the circular wrapping the might exist int he buffer.
+  ///
+  /// \return
+  ///     A vector with the requested binary data.
+  llvm::Expected<std::vector<uint8_t>> GetReadOnlyDataBuffer();
 
   /// Use the ioctl API to disable the perf event and all the events in its
   /// group. This doesn't terminate the perf event.
   ///
   /// This is no-op if the perf event is already disabled.
   ///
   /// \return
   ///   An Error if the perf event couldn't be disabled.

lldb/source/Plugins/Process/Linux/Perf.cpp

 }
 
 ArrayRef<uint8_t> PerfEvent::GetAuxBuffer() const {
   perf_event_mmap_page &mmap_metadata = GetMetadataPage();
   return {reinterpret_cast<uint8_t *>(m_aux_base.get()),
            static_cast<size_t>(mmap_metadata.aux_size)};
 }
 
-Expected<std::vector<uint8_t>>
-PerfEvent::ReadFlushedOutDataCyclicBuffer(size_t offset, size_t size) {
+Expected<std::vector<uint8_t>> PerfEvent::GetReadOnlyDataBuffer() {
   // The following code assumes that the protection level of the DATA page
   // is PROT_READ. If PROT_WRITE is used, then reading would require that
   // this piece of code updates some pointers. See more about data_tail
   // in https://man7.org/linux/man-pages/man2/perf_event_open.2.html.
 
   bool was_enabled = m_enabled;
   if (Error err = DisableWithIoctl())
     return std::move(err);
 
   /**
    * The data buffer and aux buffer have different implementations
-   * with respect to their definition of head pointer. In the case
-   * of Aux data buffer the head always wraps around the aux buffer
+   * with respect to their definition of head pointer when using PROD_READ only.
+   * In the case of Aux data buffer the head always wraps around the aux buffer
    * and we don't need to care about it, whereas the data_head keeps
    * increasing and needs to be wrapped by modulus operator
    */
   perf_event_mmap_page &mmap_metadata = GetMetadataPage();
 
   ArrayRef<uint8_t> data = GetDataBuffer();
   uint64_t data_head = mmap_metadata.data_head;
   uint64_t data_size = mmap_metadata.data_size;
   std::vector<uint8_t> output;
-  output.reserve(size);
+  output.reserve(data.size());
 
   if (data_head > data_size) {
     uint64_t actual_data_head = data_head % data_size;
     // The buffer has wrapped
-    for (uint64_t i = actual_data_head + offset;
-         i < data_size && output.size() < size; i++)
+    for (uint64_t i = actual_data_head; i < data_size; i++)
       output.push_back(data[i]);
 
-    // We need to find the starting position for the left part if the offset was
-    // too big
-    uint64_t left_part_start = actual_data_head + offset >= data_size
-                                   ? actual_data_head + offset - data_size
-                                   : 0;
-    for (uint64_t i = left_part_start;
-         i < actual_data_head && output.size() < size; i++)
+    for (uint64_t i = 0; i < actual_data_head; i++)
       output.push_back(data[i]);
   } else {
-    for (uint64_t i = offset; i < data_head && output.size() < size; i++)
+    for (uint64_t i = 0; i < data_head; i++)
       output.push_back(data[i]);
   }
 
   if (was_enabled) {
     if (Error err = EnableWithIoctl())
       return std::move(err);
   }
 
-  if (output.size() != size)
-    return createStringError(inconvertibleErrorCode(),
-                             formatv("Requested {0} bytes of perf_event data "
-                                     "buffer but only {1} are available",
-                                     size, output.size()));
-
   return output;
 }
 
-Expected<std::vector<uint8_t>>
-PerfEvent::ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size) {
+Expected<std::vector<uint8_t>> PerfEvent::GetReadOnlyAuxBuffer() {
   // The following code assumes that the protection level of the AUX page
   // is PROT_READ. If PROT_WRITE is used, then reading would require that
   // this piece of code updates some pointers. See more about aux_tail
   // in https://man7.org/linux/man-pages/man2/perf_event_open.2.html.
 
   bool was_enabled = m_enabled;
   if (Error err = DisableWithIoctl())
     return std::move(err);
 
   perf_event_mmap_page &mmap_metadata = GetMetadataPage();
 
   ArrayRef<uint8_t> data = GetAuxBuffer();
   uint64_t aux_head = mmap_metadata.aux_head;
   uint64_t aux_size = mmap_metadata.aux_size;
   std::vector<uint8_t> output;
-  output.reserve(size);
+  output.reserve(data.size());
 
   /**
    * When configured as ring buffer, the aux buffer keeps wrapping around
    * the buffer and its not possible to detect how many times the buffer
    * wrapped. Initially the buffer is filled with zeros,as shown below
    * so in order to get complete buffer we first copy firstpartsize, followed
    * by any left over part from beginning to aux_head
    *
    * aux_offset [d,d,d,d,d,d,d,d,0,0,0,0,0,0,0,0,0,0,0] aux_size
    *                 aux_head->||<- firstpartsize  ->|
    *
    * */
 
-  for (uint64_t i = aux_head + offset; i < aux_size && output.size() < size;
-       i++)
+  for (uint64_t i = aux_head; i < aux_size; i++)
     output.push_back(data[i]);
 
-  // We need to find the starting position for the left part if the offset was
-  // too big
-  uint64_t left_part_start =
-      aux_head + offset >= aux_size ? aux_head + offset - aux_size : 0;
-  for (uint64_t i = left_part_start; i < aux_head && output.size() < size; i++)
+  for (uint64_t i = 0; i < aux_head; i++)
     output.push_back(data[i]);
 
   if (was_enabled) {
     if (Error err = EnableWithIoctl())
       return std::move(err);
   }
 
-  if (output.size() != size)
-    return createStringError(inconvertibleErrorCode(),
-                             formatv("Requested {0} bytes of perf_event aux "
-                                     "buffer but only {1} are available",
-                                     size, output.size()));
-
   return output;
 }
 
 Error PerfEvent::DisableWithIoctl() {
   if (!m_enabled)
     return Error::success();
 
   if (ioctl(*m_fd, PERF_EVENT_IOC_DISABLE, PERF_IOC_FLAG_GROUP) < 0)

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h

private:
 
   llvm::Expected<pt_cpu> GetCPUInfoForLiveProcess();
 
   /// Postmortem trace constructor
   ///
   /// \param[in] session
   ///     The definition file for the postmortem session.
   ///
-  /// \param[in] traces_proceses
+  /// \param[in] traced_processes
   ///     The processes traced in the live session.
   ///
   /// \param[in] trace_threads
   ///     The threads traced in the live session. They must belong to the
   ///     processes mentioned above.
   TraceIntelPT(JSONTraceSession &session,
                llvm::ArrayRef<lldb::ProcessSP> traced_processes,
                llvm::ArrayRef<lldb::ThreadPostMortemTraceSP> traced_threads);
private:
 
   /// We package all the data that can change upon process stops to make sure
   /// this contract is very visible.
   /// This variable should only be accessed directly by constructores or live
   /// process data refreshers.
   struct Storage {
     llvm::Optional<TraceIntelPTMultiCoreDecoder> multicore_decoder;
     /// These decoders are used for the non-per-core case
-    std::map<lldb::tid_t, std::unique_ptr<ThreadDecoder>> thread_decoders;
+    llvm::DenseMap<lldb::tid_t, std::unique_ptr<ThreadDecoder>> thread_decoders;
     /// Helper variable used to track long running operations for telemetry.
     TaskTimer task_timer;
     /// It is provided by either a session file or a live process to convert TSC
     /// counters to and from nanos. It might not be available on all hosts.
     llvm::Optional<LinuxPerfZeroTscConversion> tsc_conversion;
   } m_storage;
 
   /// It is provided by either a session file or a live process' "cpuInfo"

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

if (session.cores) {
     std::vector<tid_t> tids;
     for (const JSONProcess &process : session.processes)
       for (const JSONThread &thread : process.threads)
         tids.push_back(thread.tid);
 
     m_storage.multicore_decoder.emplace(*this);
   } else {
     for (const ThreadPostMortemTraceSP &thread : traced_threads) {
-      m_storage.thread_decoders.emplace(
+      m_storage.thread_decoders.try_emplace(
           thread->GetID(), std::make_unique<ThreadDecoder>(thread, *this));
       if (const Optional<FileSpec> &trace_file = thread->GetTraceFile()) {
         SetPostMortemThreadDataFile(
             thread->GetID(), IntelPTDataKinds::kTraceBuffer, *trace_file);
       }
     }
   }
 }

 
 TraceIntelPT::Storage &TraceIntelPT::GetUpdatedStorage() {
   RefreshLiveProcessState();
   return m_storage;
 }
 
 Error TraceIntelPT::DoRefreshLiveProcessState(TraceGetStateResponse state,
                                               StringRef json_response) {
-  m_storage = {};
+  m_storage = Storage();
 
   Expected<TraceIntelPTGetStateResponse> intelpt_state =
       json::parse<TraceIntelPTGetStateResponse>(json_response,
                                                 "TraceIntelPTGetStateResponse");
   if (!intelpt_state)
     return intelpt_state.takeError();
 
   m_storage.tsc_conversion = intelpt_state->tsc_perf_zero_conversion;
 
   if (!intelpt_state->cores) {
     for (const TraceThreadState &thread_state : state.traced_threads) {
       ThreadSP thread_sp =
           GetLiveProcess()->GetThreadList().FindThreadByID(thread_state.tid);
-      m_storage.thread_decoders.emplace(
+      m_storage.thread_decoders.try_emplace(
           thread_state.tid, std::make_unique<ThreadDecoder>(thread_sp, *this));
     }
   } else {
     std::vector<core_id_t> cores;
     for (const TraceCoreState &core : *intelpt_state->cores)
       cores.push_back(core.core_id);
 
     std::vector<tid_t> tids;

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp

return Object{
       {"traceBuffer", core.trace_buffer},
       {"contextSwitchTrace", core.context_switch_trace},
   };
 }
 
 bool fromJSON(const json::Value &value, JSONCore &core, Path path) {
   ObjectMapper o(value, path);
   uint64_t core_id;
-  if (!o || !o.map("coreId", core_id) ||
-      !o.map("traceBuffer", core.trace_buffer) ||
-      !o.map("contextSwitchTrace", core.context_switch_trace))
+  if (!(o && o.map("coreId", core_id) &&
+        o.map("traceBuffer", core.trace_buffer) &&
+        o.map("contextSwitchTrace", core.context_switch_trace)))
     return false;
   core.core_id = core_id;
   return true;
 }
 
 json::Value toJSON(const pt_cpu &cpu_info) {
   return Object{
       {"vendor", cpu_info.vendor == pcv_intel ? "GenuineIntel" : "Unknown"},
       {"family", cpu_info.family},
       {"model", cpu_info.model},
       {"stepping", cpu_info.stepping},
   };
 }
 
 bool fromJSON(const json::Value &value, pt_cpu &cpu_info, Path path) {
   ObjectMapper o(value, path);
   std::string vendor;
   uint64_t family, model, stepping;
-  if (!o || !o.map("vendor", vendor) || !o.map("family", family) ||
-      !o.map("model", model) || !o.map("stepping", stepping))
+  if (!(o && o.map("vendor", vendor) && o.map("family", family) &&
+        o.map("model", model) && o.map("stepping", stepping)))
     return false;
   cpu_info.vendor = vendor == "GenuineIntel" ? pcv_intel : pcv_unknown;
   cpu_info.family = family;
   cpu_info.model = model;
   cpu_info.stepping = stepping;
   return true;
 }
 
 json::Value toJSON(const JSONTraceSession &session) {
   return Object{{"type", session.type},
                 {"processes", session.processes},
                 // We have to do this because the compiler fails at doing it
                 // automatically because pt_cpu is not in a namespace
                 {"cpuInfo", toJSON(session.cpu_info)},
                 {"cores", session.cores},
                 {"tscPerfZeroConversion", session.tsc_perf_zero_conversion}};
 }
 
 bool fromJSON(const json::Value &value, JSONTraceSession &session, Path path) {
   ObjectMapper o(value, path);
-  if (!o || !o.map("processes", session.processes) ||
-      !o.map("type", session.type) || !o.map("cores", session.cores) ||
-      !o.map("tscPerfZeroConversion", session.tsc_perf_zero_conversion))
+  if (!(o && o.map("processes", session.processes) &&
+        o.map("type", session.type) && o.map("cores", session.cores) &&
+        o.map("tscPerfZeroConversion", session.tsc_perf_zero_conversion)))
     return false;
   if (session.cores && !session.tsc_perf_zero_conversion) {
     path.report(
         "\"tscPerfZeroConversion\" is required when \"cores\" is provided");
     return false;
   }
   // We have to do this because the compiler fails at doing it automatically
   // because pt_cpu is not in a namespace
   if (!fromJSON(*value.getAsObject()->get("cpuInfo"), session.cpu_info,
                 path.field("cpuInfo")))
     return false;
   return true;
 }
 
 } // namespace trace_intel_pt
 } // namespace lldb_private

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h

public:
   /// Parse the structured data trace session and create the corresponding \a
   /// Target objects. In case of an error, no targets are created.
   ///
   /// \return
   ///   A \a lldb::TraceSP instance with the trace session data. In case of
   ///   errors, return a null pointer.
   llvm::Expected<lldb::TraceSP> Parse();
 
-  llvm::Expected<lldb::TraceSP>
-  CreateTraceIntelPTInstance(JSONTraceSession &session,
-                             std::vector<ParsedProcess> &parsed_processes);
-
 private:
   /// Resolve non-absolute paths relative to the session file folder.
   FileSpec NormalizePath(const std::string &path);
 
-  lldb::ThreadPostMortemTraceSP ParseThread(lldb::ProcessSP &process_sp,
+  /// Create a post-mortem thread associated with the given \p process
+  /// using the definition from \p thread.
+  lldb::ThreadPostMortemTraceSP ParseThread(Process &process,
                                             const JSONThread &thread);
 
+  /// Given a session description and a list of fully parsed processes,
+  /// create an actual Trace instance that "traces" these processes.
+  llvm::Expected<lldb::TraceSP>
+  CreateTraceIntelPTInstance(JSONTraceSession &session,
+                             std::vector<ParsedProcess> &parsed_processes);
+
   /// Create the corresponding Threads and Process objects given the JSON
   /// process definition.
   ///
   /// \param[in] process
   ///   The JSON process definition
   llvm::Expected<ParsedProcess> ParseProcess(const JSONProcess &process);
 
-  llvm::Error ParseModule(lldb::TargetSP &target_sp, const JSONModule &module);
+  /// Create a moddule associated with the given \p target
+  /// using the definition from \p module.
+  llvm::Error ParseModule(Target &target, const JSONModule &module);
 
   /// Create a user-friendly error message upon a JSON-parsing failure using the
   /// \a json::ObjectMapper functionality.
   ///
   /// \param[in] root
   ///   The \a llvm::json::Path::Root used to parse the JSON \a value.
   ///
   /// \param[in] value
private:
   llvm::Error AugmentThreadsFromContextSwitches(JSONTraceSession &session);
 
   /// Modifiy the session file by normalizing all the paths relative to the
   /// session file directory.
   void NormalizeAllPaths(JSONTraceSession &session);
 
   Debugger &m_debugger;
   const llvm::json::Value &m_trace_session_file;
-  std::string m_session_file_dir;
+  const std::string m_session_file_dir;
 };
 
 } // namespace trace_intel_pt
 } // namespace lldb_private
 
 
 #endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_TRACEINTELPTSESSIONFILEPARSER_H

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp

 
 FileSpec TraceIntelPTSessionFileParser::NormalizePath(const std::string &path) {
   FileSpec file_spec(path);
   if (file_spec.IsRelative())
     file_spec.PrependPathComponent(m_session_file_dir);
   return file_spec;
 }
 
-Error TraceIntelPTSessionFileParser::ParseModule(lldb::TargetSP &target_sp,
+Error TraceIntelPTSessionFileParser::ParseModule(Target &target,
                                                  const JSONModule &module) {
   auto do_parse = [&]() -> Error {
     FileSpec system_file_spec(module.system_path);
 
     FileSpec local_file_spec(module.file.hasValue() ? *module.file
                                                     : module.system_path);
 
     ModuleSpec module_spec;
     module_spec.GetFileSpec() = local_file_spec;
     module_spec.GetPlatformFileSpec() = system_file_spec;
 
     if (module.uuid.hasValue())
       module_spec.GetUUID().SetFromStringRef(*module.uuid);
 
     Status error;
     ModuleSP module_sp =
-        target_sp->GetOrCreateModule(module_spec, /*notify*/ false, &error);
+        target.GetOrCreateModule(module_spec, /*notify*/ false, &error);
 
     if (error.Fail())
       return error.ToError();
 
     bool load_addr_changed = false;
-    module_sp->SetLoadAddress(*target_sp, module.load_address, false,
+    module_sp->SetLoadAddress(target, module.load_address, false,
                               load_addr_changed);
     return Error::success();
   };
   if (Error err = do_parse())
     return createStringError(
         inconvertibleErrorCode(), "Error when parsing module %s. %s",
         module.system_path.c_str(), toString(std::move(err)).c_str());
   return Error::success();
 }
 
 Error TraceIntelPTSessionFileParser::CreateJSONError(json::Path::Root &root,
                                                      const json::Value &value) {
   std::string err;
   raw_string_ostream os(err);
   root.printErrorContext(value, os);
   return createStringError(
       std::errc::invalid_argument, "%s\n\nContext:\n%s\n\nSchema:\n%s",
       toString(root.getError()).c_str(), os.str().c_str(), GetSchema().data());
 }
 
 ThreadPostMortemTraceSP
-TraceIntelPTSessionFileParser::ParseThread(ProcessSP &process_sp,
+TraceIntelPTSessionFileParser::ParseThread(Process &process,
                                            const JSONThread &thread) {
   lldb::tid_t tid = static_cast<lldb::tid_t>(thread.tid);
 
   Optional<FileSpec> trace_file;
   if (thread.trace_buffer)
     trace_file = FileSpec(*thread.trace_buffer);
 
   ThreadPostMortemTraceSP thread_sp =
-      std::make_shared<ThreadPostMortemTrace>(*process_sp, tid, trace_file);
-  process_sp->GetThreadList().AddThread(thread_sp);
+      std::make_shared<ThreadPostMortemTrace>(process, tid, trace_file);
+  process.GetThreadList().AddThread(thread_sp);
   return thread_sp;
 }
 
 Expected<TraceIntelPTSessionFileParser::ParsedProcess>
 TraceIntelPTSessionFileParser::ParseProcess(const JSONProcess &process) {
   TargetSP target_sp;
   Status error = m_debugger.GetTargetList().CreateTarget(
       m_debugger, /*user_exe_path*/ StringRef(), process.triple.getValueOr(""),
TraceIntelPTSessionFileParser::ParseProcess(const JSONProcess &process) {
   ProcessSP process_sp = target_sp->CreateProcess(
       /*listener*/ nullptr, "trace",
       /*crash_file*/ nullptr,
       /*can_connect*/ false);
 
   process_sp->SetID(static_cast<lldb::pid_t>(process.pid));
 
   for (const JSONThread &thread : process.threads)
-    parsed_process.threads.push_back(ParseThread(process_sp, thread));
+    parsed_process.threads.push_back(ParseThread(*process_sp, thread));
 
   for (const JSONModule &module : process.modules)
-    if (Error err = ParseModule(target_sp, module))
+    if (Error err = ParseModule(*target_sp, module))
       return std::move(err);
 
   if (!process.threads.empty())
     process_sp->GetThreadList().SetSelectedThreadByIndexID(0);
 
   // We invoke DidAttach to create a correct stopped state for the process and
   // its threads.
   ArchSpec process_arch;

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp

 #include <sstream>
 #include <string>
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::trace_intel_pt;
 using namespace llvm;
 
+/// Write a stream of bytes from \p data to the given output file.
+/// It creates or overwrites the output file, but not append.
 static llvm::Error WriteBytesToDisk(FileSpec &output_file,
                                     ArrayRef<uint8_t> data) {
   std::basic_fstream<char> out_fs = std::fstream(
       output_file.GetPath().c_str(), std::ios::out | std::ios::binary);
   if (!data.empty())
     out_fs.write(reinterpret_cast<const char *>(&data[0]), data.size());
 
   out_fs.close();

 ///     \a llvm::Success if the operation was successful, or an \a llvm::Error
 ///     otherwise.
 static llvm::Error
 WriteSessionToFile(const llvm::json::Value &trace_session_json,
                    const FileSpec &directory) {
   FileSpec trace_path = directory;
   trace_path.AppendPathComponent("trace.json");
   std::ofstream os(trace_path.GetPath());
-  os << std::string(formatv("{0:2}", trace_session_json));
+  os << formatv("{0:2}", trace_session_json).str();
   os.close();
   if (!os)
     return createStringError(inconvertibleErrorCode(),
                              formatv("couldn't write to the file {0}",
                                      trace_path.GetPath().c_str()));
   return Error::success();
 }
 

 ///     The threads section or \a llvm::Error in case of failures.
 static llvm::Expected<std::vector<JSONThread>>
 BuildThreadsSection(Process &process, FileSpec directory) {
   std::vector<JSONThread> json_threads;
   TraceSP trace_sp = process.GetTarget().GetTrace();
 
   FileSpec threads_dir = directory;
   threads_dir.AppendPathComponent("threads");
-  FileSystem::Instance().Resolve(threads_dir);
   sys::fs::create_directories(threads_dir.GetCString());
 
   for (ThreadSP thread_sp : process.Threads()) {
     lldb::tid_t tid = thread_sp->GetID();
     if (!trace_sp->IsTraced(tid))
       continue;
 
     JSONThread json_thread;

 static llvm::Expected<llvm::Optional<std::vector<JSONCore>>>
 BuildCoresSection(TraceIntelPT &trace_ipt, FileSpec directory) {
   if (trace_ipt.GetTracedCores().empty())
     return None;
 
   std::vector<JSONCore> json_cores;
   FileSpec cores_dir = directory;
   cores_dir.AppendPathComponent("cores");
-  FileSystem::Instance().Resolve(cores_dir);
   sys::fs::create_directories(cores_dir.GetCString());
 
   for (lldb::core_id_t core_id : trace_ipt.GetTracedCores()) {
     JSONCore json_core;
     json_core.core_id = core_id;
 
     {
       FileSpec output_trace = cores_dir;
for (size_t i = 0; i < module_list.GetSize(); ++i) {
     Address base_addr(objfile->GetBaseAddress());
     if (base_addr.IsValid() &&
         !process.GetTarget().GetSectionLoadList().IsEmpty())
       load_addr = base_addr.GetLoadAddress(&process.GetTarget());
 
     if (load_addr == LLDB_INVALID_ADDRESS)
       continue;
 
-    FileSystem::Instance().Resolve(directory);
     FileSpec path_to_copy_module = directory;
     path_to_copy_module.AppendPathComponent("modules");
     path_to_copy_module.AppendPathComponent(system_path);
     sys::fs::create_directories(path_to_copy_module.GetDirectory().AsCString());
 
     if (std::error_code ec = llvm::sys::fs::copy_file(
             system_path, path_to_copy_module.GetPath()))
       return createStringError(
Error TraceIntelPTSessionSaver::SaveToDisk(TraceIntelPT &trace_ipt,
   if (std::error_code ec =
           sys::fs::create_directories(directory.GetPath().c_str()))
     return llvm::errorCodeToError(ec);
 
   Expected<pt_cpu> cpu_info = trace_ipt.GetCPUInfo();
   if (!cpu_info)
     return cpu_info.takeError();
 
+  FileSystem::Instance().Resolve(directory);
+
   Expected<std::vector<JSONProcess>> json_processes =
       BuildProcessesSection(trace_ipt, directory);
 
   if (!json_processes)
     return json_processes.takeError();
 
   Expected<Optional<std::vector<JSONCore>>> json_cores =
       BuildCoresSection(trace_ipt, directory);

lldb/source/Target/Trace.cpp

 bool fromJSON(const Value &value, JSONSimpleTraceSession &session, Path path) {
   json::ObjectMapper o(value, path);
   return o && o.map("type", session.type);
 }
 
 } // namespace json
 } // namespace llvm
 
+/// Helper functions for fetching data in maps and returning Optionals or
+/// pointers instead of iterators for simplicity. It's worth mentioning that the
+/// Optionals version can't return the inner data by reference because of
+/// limitations in move constructors.
+/// \{
+template <typename K, typename V>
+static Optional<V> Lookup(DenseMap<K, V> &map, K k) {
+  auto it = map.find(k);
+  if (it == map.end())
+    return None;
+  return it->second;
+}
+
+template <typename K, typename V>
+static V *LookupAsPtr(DenseMap<K, V> &map, K k) {
+  auto it = map.find(k);
+  if (it == map.end())
+    return nullptr;
+  return &it->second;
+}
+
+template <typename K1, typename K2, typename V>
+static Optional<V> Lookup2(DenseMap<K1, DenseMap<K2, V>> &map, K1 k1, K2 k2) {
+  auto it = map.find(k1);
+  if (it == map.end())
+    return None;
+  return Lookup(it->second, k2);
+}
+
+template <typename K1, typename K2, typename V>
+static V *Lookup2AsPtr(DenseMap<K1, DenseMap<K2, V>> &map, K1 k1, K2 k2) {
+  auto it = map.find(k1);
+  if (it == map.end())
+    return nullptr;
+  return LookupAsPtr(it->second, k2);
+}
+/// \}
+
 static Error createInvalidPlugInError(StringRef plugin_name) {
   return createStringError(
       std::errc::invalid_argument,
       "no trace plug-in matches the specified type: \"%s\"",
       plugin_name.data());
 }
 
 Expected<lldb::TraceSP>
Expected<StringRef> Trace::FindPluginSchema(StringRef name) {
   if (!schema.empty())
     return schema;
 
   return createInvalidPlugInError(name);
 }
 
 Error Trace::Start(const llvm::json::Value &request) {
   if (!m_live_process)
-    return createStringError(inconvertibleErrorCode(),
-                             "Tracing requires a live process.");
+    return createStringError(
+        inconvertibleErrorCode(),
+        "Attempted to start tracing without a live process.");
   return m_live_process->TraceStart(request);
 }
 
 Error Trace::Stop() {
   if (!m_live_process)
-    return createStringError(inconvertibleErrorCode(),
-                             "Tracing requires a live process.");
+    return createStringError(
+        inconvertibleErrorCode(),
+        "Attempted to stop tracing without a live process.");
   return m_live_process->TraceStop(TraceStopRequest(GetPluginName()));
 }
 
 Error Trace::Stop(llvm::ArrayRef<lldb::tid_t> tids) {
   if (!m_live_process)
-    return createStringError(inconvertibleErrorCode(),
-                             "Tracing requires a live process.");
+    return createStringError(
+        inconvertibleErrorCode(),
+        "Attempted to stop tracing without a live process.");
   return m_live_process->TraceStop(TraceStopRequest(GetPluginName(), tids));
 }
 
 Expected<std::string> Trace::GetLiveProcessState() {
   if (!m_live_process)
-    return createStringError(inconvertibleErrorCode(),
-                             "Tracing requires a live process.");
+    return createStringError(
+        inconvertibleErrorCode(),
+        "Attempted to fetch live trace information without a live process.");
   return m_live_process->TraceGetState(GetPluginName());
 }
 
 Optional<uint64_t> Trace::GetLiveThreadBinaryDataSize(lldb::tid_t tid,
                                                       llvm::StringRef kind) {
   Storage &storage = GetUpdatedStorage();
-  auto it = storage.live_thread_data.find(tid);
-  if (it == storage.live_thread_data.end())
-    return None;
-  std::unordered_map<std::string, uint64_t> &single_thread_data = it->second;
-  auto single_thread_data_it = single_thread_data.find(kind.str());
-  if (single_thread_data_it == single_thread_data.end())
-    return None;
-  return single_thread_data_it->second;
+  return Lookup2(storage.live_thread_data, tid, ConstString(kind));
 }
 
 Optional<uint64_t> Trace::GetLiveCoreBinaryDataSize(lldb::core_id_t core_id,
                                                     llvm::StringRef kind) {
   Storage &storage = GetUpdatedStorage();
-  auto it = storage.live_core_data_sizes.find(core_id);
-  if (it == storage.live_core_data_sizes.end())
-    return None;
-  std::unordered_map<std::string, uint64_t> &single_core_data = it->second;
-  auto single_thread_data_it = single_core_data.find(kind.str());
-  if (single_thread_data_it == single_core_data.end())
-    return None;
-  return single_thread_data_it->second;
+  return Lookup2(storage.live_core_data_sizes, core_id, ConstString(kind));
 }
 
 Optional<uint64_t> Trace::GetLiveProcessBinaryDataSize(llvm::StringRef kind) {
   Storage &storage = GetUpdatedStorage();
-  auto data_it = storage.live_process_data.find(kind.str());
-  if (data_it == storage.live_process_data.end())
-    return None;
-  return data_it->second;
+  return Lookup(storage.live_process_data, ConstString(kind));
 }
 
 Expected<std::vector<uint8_t>>
-Trace::GetLiveThreadBinaryData(lldb::tid_t tid, llvm::StringRef kind) {
+Trace::GetLiveTraceBinaryData(const TraceGetBinaryDataRequest &request,
+                              uint64_t expected_size) {
   if (!m_live_process)
-    return createStringError(inconvertibleErrorCode(),
-                             "Tracing requires a live process.");
+    return createStringError(
+        inconvertibleErrorCode(),
+        formatv("Attempted to fetch live trace data without a live process. "
+                "Data kind = {0}, tid = {1}, core id = {2}.",
+                request.kind, request.tid, request.core_id));
+
+  Expected<std::vector<uint8_t>> data =
+      m_live_process->TraceGetBinaryData(request);
+
+  if (!data)
+    return data.takeError();
+
+  if (data->size() != expected_size)
+    return createStringError(
+        inconvertibleErrorCode(),
+        formatv("Got incomplete live trace data. Data kind = {0}, expected "
+                "size = {1}, actual size = {2}, tid = {3}, core id = {4}",
+                request.kind, expected_size, data->size(), request.tid,
+                request.core_id));
+
+  return data;
+}
+
+Expected<std::vector<uint8_t>>
+Trace::GetLiveThreadBinaryData(lldb::tid_t tid, llvm::StringRef kind) {
   llvm::Optional<uint64_t> size = GetLiveThreadBinaryDataSize(tid, kind);
   if (!size)
     return createStringError(
         inconvertibleErrorCode(),
         "Tracing data \"%s\" is not available for thread %" PRIu64 ".",
         kind.data(), tid);
 
-  TraceGetBinaryDataRequest request{GetPluginName().str(), kind.str(),   tid,
-                                    /*core_id=*/None,      /*offset=*/0, *size};
-  return m_live_process->TraceGetBinaryData(request);
+  TraceGetBinaryDataRequest request{GetPluginName().str(), kind.str(), tid,
+                                    /*core_id=*/None};
+  return GetLiveTraceBinaryData(request, *size);
 }
 
 Expected<std::vector<uint8_t>>
 Trace::GetLiveCoreBinaryData(lldb::core_id_t core_id, llvm::StringRef kind) {
   if (!m_live_process)
-    return createStringError(inconvertibleErrorCode(),
-                             "Tracing requires a live process.");
+    return createStringError(
+        inconvertibleErrorCode(),
+        "Attempted to fetch live cpu data without a live process.");
   llvm::Optional<uint64_t> size = GetLiveCoreBinaryDataSize(core_id, kind);
   if (!size)
     return createStringError(
         inconvertibleErrorCode(),
         "Tracing data \"%s\" is not available for core_id %" PRIu64 ".",
         kind.data(), core_id);
 
   TraceGetBinaryDataRequest request{GetPluginName().str(), kind.str(),
-                                    /*tid=*/None,          core_id,
-                                    /*offset=*/0,          *size};
+                                    /*tid=*/None, core_id};
   return m_live_process->TraceGetBinaryData(request);
 }
 
 Expected<std::vector<uint8_t>>
 Trace::GetLiveProcessBinaryData(llvm::StringRef kind) {
-  if (!m_live_process)
-    return createStringError(inconvertibleErrorCode(),
-                             "Tracing requires a live process.");
   llvm::Optional<uint64_t> size = GetLiveProcessBinaryDataSize(kind);
   if (!size)
     return createStringError(
         inconvertibleErrorCode(),
         "Tracing data \"%s\" is not available for the process.", kind.data());
 
   TraceGetBinaryDataRequest request{GetPluginName().str(), kind.str(),
-                                    /*tid=*/None,          /*core_id*/ None,
-                                    /*offset=*/0,          *size};
-  return m_live_process->TraceGetBinaryData(request);
+                                    /*tid=*/None, /*core_id*/ None};
+  return GetLiveTraceBinaryData(request, *size);
 }
 
 Trace::Storage &Trace::GetUpdatedStorage() {
   RefreshLiveProcessState();
   return m_storage;
 }
 
 const char *Trace::RefreshLiveProcessState() {
   if (!m_live_process)
     return nullptr;
 
   uint32_t new_stop_id = m_live_process->GetStopID();
   if (new_stop_id == m_stop_id)
     return nullptr;
 
   Log *log = GetLog(LLDBLog::Target);
   LLDB_LOG(log, "Trace::RefreshLiveProcessState invoked");
 
   m_stop_id = new_stop_id;
   m_storage = Trace::Storage();
 
-  auto HandleError = [&](Error &&err) -> const char * {
-    m_storage.live_refresh_error = toString(std::move(err));
-    return m_storage.live_refresh_error->c_str();
-  };
-
+  auto do_refresh = [&]() -> Error {
     Expected<std::string> json_string = GetLiveProcessState();
     if (!json_string)
-    return HandleError(json_string.takeError());
+      return json_string.takeError();
 
     Expected<TraceGetStateResponse> live_process_state =
-      json::parse<TraceGetStateResponse>(*json_string, "TraceGetStateResponse");
+        json::parse<TraceGetStateResponse>(*json_string,
+                                           "TraceGetStateResponse");
     if (!live_process_state)
-    return HandleError(live_process_state.takeError());
+      return live_process_state.takeError();
 
     if (live_process_state->warnings) {
       for (std::string &warning : *live_process_state->warnings)
         LLDB_LOG(log, "== Warning when fetching the trace state: {0}", warning);
     }
 
     for (const TraceThreadState &thread_state :
          live_process_state->traced_threads) {
       for (const TraceBinaryData &item : thread_state.binary_data)
-      m_storage.live_thread_data[thread_state.tid][item.kind] = item.size;
+        m_storage.live_thread_data[thread_state.tid].insert(
+            {ConstString(item.kind), item.size});
     }
 
     LLDB_LOG(log, "== Found {0} threads being traced",
              live_process_state->traced_threads.size());
 
     if (live_process_state->cores) {
       m_storage.cores.emplace();
       for (const TraceCoreState &core_state : *live_process_state->cores) {
         m_storage.cores->push_back(core_state.core_id);
         for (const TraceBinaryData &item : core_state.binary_data)
-        m_storage.live_core_data_sizes[core_state.core_id][item.kind] =
-            item.size;
+          m_storage.live_core_data_sizes[core_state.core_id].insert(
+              {ConstString(item.kind), item.size});
       }
       LLDB_LOG(log, "== Found {0} cpu cores being traced",
                live_process_state->cores->size());
     }
 
-
     for (const TraceBinaryData &item : live_process_state->process_binary_data)
-    m_storage.live_process_data[item.kind] = item.size;
+      m_storage.live_process_data.insert({ConstString(item.kind), item.size});
 
-  if (Error err = DoRefreshLiveProcessState(std::move(*live_process_state),
-                                            *json_string))
-    return HandleError(std::move(err));
+    return DoRefreshLiveProcessState(std::move(*live_process_state),
+                                     *json_string);
+  };
+
+  if (Error err = do_refresh()) {
+    m_storage.live_refresh_error = toString(std::move(err));
+    return m_storage.live_refresh_error->c_str();
+  }
 
   return nullptr;
 }
 
 Trace::Trace(ArrayRef<ProcessSP> postmortem_processes,
              Optional<std::vector<lldb::core_id_t>> postmortem_cores) {
   for (ProcessSP process_sp : postmortem_processes)
     m_storage.postmortem_processes.push_back(process_sp.get());

 
 uint32_t Trace::GetStopID() {
   RefreshLiveProcessState();
   return m_stop_id;
 }
 
 llvm::Expected<FileSpec>
 Trace::GetPostMortemThreadDataFile(lldb::tid_t tid, llvm::StringRef kind) {
-  auto NotFoundError = [&]() {
+  Storage &storage = GetUpdatedStorage();
+  if (Optional<FileSpec> file =
+          Lookup2(storage.postmortem_thread_data, tid, ConstString(kind)))
+    return *file;
+  else
     return createStringError(
         inconvertibleErrorCode(),
         formatv("The thread with tid={0} doesn't have the tracing data {1}",
                 tid, kind));
-  };
-
-  Storage &storage = GetUpdatedStorage();
-  auto it = storage.postmortem_thread_data.find(tid);
-  if (it == storage.postmortem_thread_data.end())
-    return NotFoundError();
-
-  std::unordered_map<std::string, FileSpec> &data_kind_to_file_spec_map =
-      it->second;
-  auto it2 = data_kind_to_file_spec_map.find(kind.str());
-  if (it2 == data_kind_to_file_spec_map.end())
-    return NotFoundError();
-  return it2->second;
 }
 
 llvm::Expected<FileSpec>
 Trace::GetPostMortemCoreDataFile(lldb::core_id_t core_id,
                                  llvm::StringRef kind) {
-  auto NotFoundError = [&]() {
+  Storage &storage = GetUpdatedStorage();
+  if (Optional<FileSpec> file =
+          Lookup2(storage.postmortem_core_data, core_id, ConstString(kind)))
+    return *file;
+  else
     return createStringError(
         inconvertibleErrorCode(),
         formatv("The core with id={0} doesn't have the tracing data {1}",
                 core_id, kind));
-  };
-
-  Storage &storage = GetUpdatedStorage();
-  auto it = storage.postmortem_core_data.find(core_id);
-  if (it == storage.postmortem_core_data.end())
-    return NotFoundError();
-
-  std::unordered_map<std::string, FileSpec> &data_kind_to_file_spec_map =
-      it->second;
-  auto it2 = data_kind_to_file_spec_map.find(kind.str());
-  if (it2 == data_kind_to_file_spec_map.end())
-    return NotFoundError();
-  return it2->second;
 }
 
 void Trace::SetPostMortemThreadDataFile(lldb::tid_t tid, llvm::StringRef kind,
                                         FileSpec file_spec) {
   Storage &storage = GetUpdatedStorage();
-  storage.postmortem_thread_data[tid][kind.str()] = file_spec;
+  storage.postmortem_thread_data[tid].insert({ConstString(kind), file_spec});
 }
 
 void Trace::SetPostMortemCoreDataFile(lldb::core_id_t core_id,
                                       llvm::StringRef kind,
                                       FileSpec file_spec) {
   Storage &storage = GetUpdatedStorage();
-  storage.postmortem_core_data[core_id][kind.str()] = file_spec;
+  storage.postmortem_core_data[core_id].insert({ConstString(kind), file_spec});
 }
 
 llvm::Error
 Trace::OnLiveThreadBinaryDataRead(lldb::tid_t tid, llvm::StringRef kind,
                                   OnBinaryDataReadCallback callback) {
   Expected<std::vector<uint8_t>> data = GetLiveThreadBinaryData(tid, kind);
   if (!data)
     return data.takeError();
   return callback(*data);
 }
 
 llvm::Error Trace::OnLiveCoreBinaryDataRead(lldb::core_id_t core_id,
                                             llvm::StringRef kind,
                                             OnBinaryDataReadCallback callback) {
   Storage &storage = GetUpdatedStorage();
-  auto core_data_entries = storage.live_core_data.find(core_id);
-  if (core_data_entries != storage.live_core_data.end()) {
-    auto core_data = core_data_entries->second.find(kind.str());
-    if (core_data != core_data_entries->second.end())
-      return callback(core_data->second);
-  }
+  if (std::vector<uint8_t> *core_data =
+          Lookup2AsPtr(storage.live_core_data, core_id, ConstString(kind)))
+    return callback(*core_data);
 
   Expected<std::vector<uint8_t>> data = GetLiveCoreBinaryData(core_id, kind);
   if (!data)
     return data.takeError();
-  auto it =
-      storage.live_core_data[core_id].insert({kind.str(), std::move(*data)});
+  auto it = storage.live_core_data[core_id].insert(
+      {ConstString(kind), std::move(*data)});
   return callback(it.first->second);
 }
 
 llvm::Error Trace::OnDataFileRead(FileSpec file,
                                   OnBinaryDataReadCallback callback) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> trace_or_error =
       MemoryBuffer::getFile(file.GetPath());
   if (std::error_code err = trace_or_error.getError())
     return createStringError(
         inconvertibleErrorCode(), "Failed fetching trace-related file %s. %s",
         file.GetCString(), toString(errorCodeToError(err)).c_str());
 
   MemoryBuffer &data = **trace_or_error;
   ArrayRef<uint8_t> array_ref(
       reinterpret_cast<const uint8_t *>(data.getBufferStart()),
       data.getBufferSize());
   return callback(array_ref);
 }
 
 llvm::Error
 Trace::OnPostMortemThreadBinaryDataRead(lldb::tid_t tid, llvm::StringRef kind,
                                         OnBinaryDataReadCallback callback) {
-  Expected<FileSpec> file = GetPostMortemThreadDataFile(tid, kind);
-  if (!file)
-    return file.takeError();
+  if (Expected<FileSpec> file = GetPostMortemThreadDataFile(tid, kind))
     return OnDataFileRead(*file, callback);
+  else
+    return file.takeError();
 }
 
 llvm::Error
 Trace::OnPostMortemCoreBinaryDataRead(lldb::core_id_t core_id,
                                       llvm::StringRef kind,
                                       OnBinaryDataReadCallback callback) {
-  Expected<FileSpec> file = GetPostMortemCoreDataFile(core_id, kind);
-  if (!file)
-    return file.takeError();
+  if (Expected<FileSpec> file = GetPostMortemCoreDataFile(core_id, kind))
     return OnDataFileRead(*file, callback);
+  else
+    return file.takeError();
 }
 
 llvm::Error Trace::OnThreadBinaryDataRead(lldb::tid_t tid, llvm::StringRef kind,
                                           OnBinaryDataReadCallback callback) {
   if (m_live_process)
     return OnLiveThreadBinaryDataRead(tid, kind, callback);
   else
     return OnPostMortemThreadBinaryDataRead(tid, kind, callback);

lldb/source/Utility/TraceGDBRemotePackets.cpp

if (!warnings)
     warnings.emplace();
   warnings->push_back(warning.data());
 }
 
 bool fromJSON(const json::Value &value, TraceCoreState &packet,
               json::Path path) {
   ObjectMapper o(value, path);
   uint64_t core_id;
-  if (!o || !o.map("coreId", core_id) ||
-      !o.map("binaryData", packet.binary_data))
+  if (!(o && o.map("coreId", core_id) &&
+        o.map("binaryData", packet.binary_data)))
     return false;
   packet.core_id = static_cast<lldb::core_id_t>(core_id);
   return true;
 }
 
 json::Value toJSON(const TraceCoreState &packet) {
   return json::Value(
       Object{{"coreId", packet.core_id}, {"binaryData", packet.binary_data}});
 }
 /// \}
 
 /// jLLDBTraceGetBinaryData
 /// \{
 json::Value toJSON(const TraceGetBinaryDataRequest &packet) {
   return json::Value(Object{{"type", packet.type},
                             {"kind", packet.kind},
-                            {"offset", packet.offset},
                             {"tid", packet.tid},
-                            {"coreId", packet.core_id},
-                            {"size", packet.size}});
+                            {"coreId", packet.core_id}});
 }
 
 bool fromJSON(const json::Value &value, TraceGetBinaryDataRequest &packet,
               Path path) {
   ObjectMapper o(value, path);
   Optional<uint64_t> core_id;
-  if (!o || !o.map("type", packet.type) || !o.map("kind", packet.kind) ||
-      !o.map("tid", packet.tid) || !o.map("offset", packet.offset) ||
-      !o.map("size", packet.size) || !o.map("coreId", core_id))
+  if (!(o && o.map("type", packet.type) && o.map("kind", packet.kind) &&
+        o.map("tid", packet.tid) && o.map("coreId", core_id)))
     return false;
 
   if (core_id)
     packet.core_id = static_cast<lldb::core_id_t>(*core_id);
   return true;
 }
 /// \}
 
 } // namespace lldb_private