[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm still digesting this, but here's my first set of comments.

>From a high-level perspective. it's not clear to me how to test the exceptional states that can occur when decoding a trace. The decoding function is pretty complex, but the current tests only seem to cover the straight-line path which does not encounter any errors or missing data or whatnot. Have you given that any thought?



================
Comment at: lldb/include/lldb/Target/Trace.h:169
+      const Thread &thread,
+      std::function<bool(size_t index, const Instruction &insn)> callback,
+      size_t from = 0) = 0;
----------------
It  sounds like this could just be `llvm::function_ref<bool(size_t index, Expected<addr_t> load_addr>` and the Instruction class does not even need to exist. At least not here -- it may still be useful for the PT plugin to store the instructions in some sort of an error-or-load-addr union, but there's no need to impose that organization on anyone else.


================
Comment at: lldb/include/lldb/Target/Trace.h:193
+  ///     or an actual Error in case of failures.
+  virtual llvm::Error IsTraceFailed(const Thread &thread) = 0;
 };
----------------
I'd expect a method called `IsXXX` to return bool.


================
Comment at: lldb/include/lldb/Target/TraceThread.h:23
+/// files.
+class TraceThread : public Thread {
+public:
----------------
I'm very confused by TraceThread residing in the `Target` library and ProcessTrace in `Plugin/Process/Trace`. I think both of them should be in the same place (I could make a case for either location).


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:77
+  DecodedThread(std::vector<IntelPTInstruction> &&instructions)
+      : m_instructions(instructions) {}
+
----------------
std::move(instructions)


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:84
+  ///   The instructions of the trace.
+  const std::vector<IntelPTInstruction> &GetInstructions() const;
+
----------------
`ArrayRef<IntelPTInstruction>` maybe?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:24-31
+static std::vector<uint8_t> ReadTraceFile(const FileSpec &trace_file) {
+  char file_path[PATH_MAX];
+  trace_file.GetPath(file_path, sizeof(file_path));
+  std::ifstream src(file_path, std::ios::in | std::ios_base::binary);
+  std::vector<uint8_t> trace((std::istreambuf_iterator<char>(src)),
+                             std::istreambuf_iterator<char>());
+  return trace;
----------------
`llvm::MemoryBuffer::getFile(filename)`


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:33
+
+/// Get a list of the sections of the given process that are Read or Exec,
+/// which are the only sections that could correspond to instructions traced.
----------------
and? That's what the code seems to be doing.




================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:35
+/// which are the only sections that could correspond to instructions traced.
+std::vector<lldb::SectionSP> static GetReadExecSections(Process &process) {
+  Target &target = process.GetTarget();
----------------
Please put static first (I'm surprised this even compiles).


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:68
+///   The instructions decoded.
+void DecodeInstructions(pt_insn_decoder &decoder,
+                        std::vector<IntelPTInstruction> &instructions) {
----------------
`static std::vector<IntelPTInstruction> DecodeInstructions`


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:177
+///     A libipt error code in case of failure or 0 otherwise.
+int CreateDecoderAndDecode(Process &process, const pt_cpu &pt_cpu,
+                           std::vector<uint8_t> &trace,
----------------
Expected<vector<Insns>>


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:197
+    char path[PATH_MAX];
+    section_sp->GetModule()->GetPlatformFileSpec().GetPath(path, sizeof(path));
+    if (int errcode = pt_image_add_file(
----------------
Use the GetPath overload returing a std::string


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:199
+    if (int errcode = pt_image_add_file(
+            image, path, section_sp->GetFileOffset(), section_sp->GetByteSize(),
+            nullptr, section_sp->GetLoadBaseAddress(&process.GetTarget()))) {
----------------
So will this make the library load the file into memory once again? Is there no way to make it use the copy already loaded by lldb?


================
Comment at: lldb/source/Target/TraceSessionFileParser.cpp:125
+  ProcessSP process_sp(target_sp->CreateProcess(
+      /*listener*/ nullptr, "trace",
+      /*crash_file*/ nullptr));
----------------
This is not a dependency in the strictest sense but it still means that this code would explode if the ProcessTrace "plugin" is plugged "out". It sounds like that, in this design, the ProcessTrace class should just be a part of lldb core.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283