[Lldb-commits] [PATCH] D107669: [trace] [intel pt] Create a "process trace save" command

Mon Aug 9 19:38:49 PDT 2021

wallace added inline comments.

================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:267-281
+llvm::Expected<std::vector<uint8_t>> PostMortemThreadDecoder::GetRawTrace() {
+  FileSpec trace_file = m_trace_thread->GetTraceFile();
+  ErrorOr<std::unique_ptr<MemoryBuffer>> trace_or_error =
+      MemoryBuffer::getFile(trace_file.GetPath());
+  if (std::error_code err = trace_or_error.getError())
+    return errorCodeToError(err);
+  MemoryBuffer &trace = **trace_or_error;
----------------
clayborg wrote:
> llvm::ArrayRef
Given that now we will just make it work for live processes, you can revert these changes and just invoke TraceIntelPT::GetLiveThreadBuffer directly

================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:277
+      trace.getBufferSize());
+  std::vector<uint8_t> result;
+  for (size_t i = 0; i < trace_data.size(); i++)
----------------
result.reserve(trace_data.size())

================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h:40
+  ///     raw trace from the thread buffer as bytes.
+  virtual llvm::Expected<std::vector<uint8_t>> GetRawTrace() = 0;
+
----------------
wallace wrote:
> clayborg wrote:
> > This data is quite large, can we just get a reference to the data without making a copy by using llvm::ArrayRef?
> Sadly this can't work. LiveThreadDecoder::GetRawTrace() gets the raw trace from lldb-server and puts it in a vector without any object owning it. So the actual vector will need to be returned because no one wants to own it. It's not that bad though, because Expected just moves the inner data without creating copies.
Another option is to store the raw trace in the ThreadDecoder itself, though might not be that useful because decoding happens only once and saving to disk as well.

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:1
+//===-- TraceIntelPTJSONStructs.cpp ---------------------------------===//
+//
----------------
this shuold have the same length as line 7

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:33-48
+  if (std::error_code ec =
+          sys::fs::create_directories(directory.GetPath().c_str())) {
+    return llvm::errorCodeToError(ec);
+  }
+
+  llvm::Expected<JSONTraceIntelPTTrace> json_trace_intel_pt_trace =
+      BuildTraceSection(trace_ipt);
----------------
remove braces for one-line if statements

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:50
+
+  struct JSONTraceIntelPTSchema json_trace_intel_pt_schema(
+      json_trace_intel_pt_trace.get(), json_trace_session_base.get());
----------------
remove the word struct

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:65-66
+  std::ofstream os(trace_path.GetPath());
+  os << out;
+  return;
+}
----------------
for checking errors while writing, do this:

  os << out;
  os.close();
  if (!os)
    return createStringError(unconvertibleErrorCode(), formatv("couldn't write to the file {0}", trace_path.getPath());
  return Error::success(); 

then you have to make this method return llvm::Error

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:72-74
+  if (!cpu_info) {
+    return cpu_info.takeError();
+  }
----------------
remove braces

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:76
+
+  struct JSONTraceIntelPTCPUInfo json_trace_intel_pt_cpu_info(
+      static_cast<int64_t>(cpu_info->family),
----------------
wallace wrote:
> remove the keyword struct. It's very old-style
it'd be better to create a constructor of JSONTraceIntelPTCPUInfo that receives a pt_cpu object and then does this conversion inside

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:76-82
+  struct JSONTraceIntelPTCPUInfo json_trace_intel_pt_cpu_info(
+      static_cast<int64_t>(cpu_info->family),
+      static_cast<int64_t>(cpu_info->model),
+      static_cast<int64_t>(cpu_info->stepping),
+      cpu_info->vendor == pcv_intel ? "intel" : "Unknown");
+
+  struct JSONTraceIntelPTTrace result("intel-pt", json_trace_intel_pt_cpu_info);
----------------
remove the keyword struct. It's very old-style

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:93-95
+  if (!json_threads) {
+    return json_threads.takeError();
+  }
----------------
remove braces for one-liners

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:99
+
+  struct JSONProcess json_process(
+      process_sp->GetID(),
----------------
ditto

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:102
+      process_sp->GetTarget().GetArchitecture().GetTriple().getTriple(),
+      json_threads.get(), json_modules);
+  result.processes.push_back(json_process);
----------------
invoke BuildModulesSection(process_sp) here directly to avoid an unnecessary copy

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:118-119
+    FileSystem::Instance().Resolve(directory);
+    FileSpec json_path = directory;
+    json_path.AppendPathComponent(std::to_string(thread_sp->GetID()) +
+                                  ".trace");
----------------
json_path is a bad name, call it raw_trace_path

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:126-128
+    if (!raw_trace) {
+      return raw_trace.takeError();
+    }
----------------
braces

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:132
+    raw_trace_file.write(reinterpret_cast<const char *>(&raw_trace->at(0)),
+                         raw_trace->size() * sizeof(uint8_t));
+  }
----------------
do the same thing here of closing the stream and then checking if it's correct

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:147-163
+    if (!module_sp->GetFileSpec().IsAbsolute()) {
+      continue;
+    }
+    std::string file = module_sp->GetFileSpec().GetPath();
+    ObjectFile *objfile = module_sp->GetObjectFile();
+    if (objfile == nullptr) {
+      continue;
----------------
remove braces for one-liners

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:164
+    }
+    JSONAddress load_address(load_addr);
+    JSONModule json_module(system_path, file, load_address,
----------------
you should copy the file to the directory as well. The reason is that if the original file changes, then decoding will fail, so we need to make this self-contained. 
You can use the function https://llvm.org/doxygen/namespacellvm_1_1sys_1_1fs.html#abe768b38d21bfc2bc91a1c1d09cd84de to copy files

You can follow this idea:
```
If a module exists in the path /usr/lib/foo/libbar.so, then you should copy it to <directory>/modules/usr/lib/foo/libbar.so.

Then the "path" entry is the json file should be "modules/usr/lib/foo/libbar.so", and the "systemPath" entry should be "/usr/lib/foo/libbar.so"
```

Then, update all the comments mentioning that the original modules will be copied over to the <directory/modules> folder with the goal of making it self-contained. A cool benefit if this is that you could zip this folder, send it over to another person, and they could see the same exact trace as you.

================
Comment at: lldb/test/API/commands/trace/TestTraceSave.py:10
+
+    def testErrorMessages(self):
+        # We first check the output when there are no targets
----------------
nice test

================
Comment at: lldb/test/API/commands/trace/TestTraceSave.py:34-36
+        self.expect("trace load -v " +
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+            substrs=["intel-pt"])
----------------
now that we are changing to live processes, this won't work. Instead, trace one of the existing binary files in the test folder, run it, get the first and last 20 instructions, save the contents in strings, then save the trace, then load it and dump the instructions again, finally assert that the outputs are the same

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

https://reviews.llvm.org/D107669