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