[Lldb-commits] [PATCH] D105531: [trace][intel pt] Implement the Intel PT cursor

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2111
   bool m_create_repeat_command_just_invoked;
-  size_t m_consecutive_repetitions = 0;
+  std::map<const Thread *, std::unique_ptr<TraceInstructionDumper>> m_dumpers;
 };
----------------
"Thread *" isn't a stable thing to use, I would suggest using the "tid" of the thread. Any reason you are making a map of thread to unique pointers? Can't this just be:
```
std::map<lldb::tid_t, TraceInstructionDumper> m_dumpers;
```


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41
+lldb::addr_t IntelPTInstruction::GetLoadAddress() const {
+  return IsError() ? LLDB_INVALID_ADDRESS : m_pt_insn.ip;
 }
----------------
Shouldn't the m_pt_insn.ip be set to LLDB_INVALID_ADDRESS already?


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:1
+//===-- TraceCursor.cpp ---------------------------------------------------===//
+//
----------------
Filename should be TraceInstructionDumper.cpp


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:22
+TraceInstructionDumper::TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up,
+                                               bool forwards, size_t skip,
+                                               bool raw)
----------------
Seems like the "forwards" and "skip" parameter shouldn't be in this class at all? Just setup the cursor before giving it to this class?


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:25-30
+  if (forwards)
+    m_cursor_up->SeekToBegin();
+  else
+    m_cursor_up->SeekToEnd();
+
+  Skip(skip);
----------------
Remove and have user set this up prior to creating one of these classes?


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:35
+  if (m_forwards) {
+    if (!m_cursor_up->Next()) {
+      m_no_more_data = true;
----------------
Should we modify the TraceCursor API for Next(...) to be able to take a count? Seems like that might be much more efficient than recursively calling this function?


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:41
+  } else {
+    if (!m_cursor_up->Prev()) {
+      m_no_more_data = true;
----------------
Ditto, should Prev(...) be able to take a count as well?


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:156-159
+                            /*show_fullpath*/ false,
+                            /*show_module*/ true, /*show_inlined_frames*/ false,
+                            /*show_function_arguments*/ true,
+                            /*show_function_name*/ true);
----------------
to match llvm coding guidelines, add '=' after variable name


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:167-171
+  insn.instruction->Dump(&s, /*show_address*/ false, /*show_bytes*/ false,
+                         /*max_opcode_byte_size*/ 0, &insn.exe_ctx, &insn.sc,
+                         /*prev_sym_ctx*/ nullptr,
+                         /*disassembly_addr_format*/ nullptr,
+                         /*max_address_text_size*/ 0);
----------------
Ditto about adding '=', same as above comment.


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:291
+    s.Printf("\n");
+    TryMoveOneInstruction();
+  }
----------------
You should be watching the return value of this right? What if this returns false?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531