jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.
looks great overall, just a couple minor comments!
================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:46
/// state and granularity.
class TraceInstructionDumper {
public:
----------------
nit: wdyt ab renaming this to `TraceInstructionsDumper` since now we have the
`TraceInstructionWriter` which is responsible for displaying a single
instruction? With how things are currently named it's slightly confusing at
first glance because there names sound like they are both doing the same thing
(something related to a single instruction).
================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:75
+ /// Indicate a user-level info message. It's not part of the actual trace.
+ virtual void InfoMessage(llvm::StringRef text) = 0;
+
----------------
since the JSON subclass doesn't need to implement this, consider providing a
default stubbed out implementation at the super class level so that subclasses
aren't necessarily required to implement it?
================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:120-122
+ /// Create an instruction entry for the current position without symbol
+ /// information.
+ InstructionEntry CreateBasicInstructionEntry();
----------------
nit: to be consistent with the `--raw` flag of the `thread trace dump
instructions` command which says:
```
-r ( --raw )
Dump only instruction address without disassembly nor symbol
information.
```
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2210
size_t m_continue;
+ llvm::Optional<FileSpec> m_output_file;
TraceInstructionDumperOptions m_dumper_options;
----------------
Why is this stored on the command object and not the
`TraceinstructionDumperOptions`?
Can you explain the separation of responsibility of the dumper options versus
the options here?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:121-122
bool TraceCursorIntelPT::GoToId(user_id_t id) {
if (m_decoded_thread_sp->GetInstructionsCount() <= id)
return false;
m_pos = id;
----------------
nit: maybe use the new `HasId` method here?
================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:90-91
-void TraceInstructionDumper::DumpInstructionSymbolContext(
- const Optional<InstructionSymbolInfo> &prev_insn,
- const InstructionSymbolInfo &insn) {
- if (prev_insn && IsSameInstructionSymbolContext(*prev_insn, insn))
- return;
+class TraceInstructionWriterCLI
+ : public TraceInstructionDumper::TraceInstructionWriter {
+public:
----------------
Move declarations to header?
================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:193
+ : m_s(s), m_options(options),
+ m_j(m_s.AsRawOstream(), options.pretty_print_json ? 2 : 0) {
+ m_j.arrayBegin();
----------------
================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:219-224
+ m_j.attribute("module", GetModuleName(insn));
+ m_j.attribute("symbol",
+ insn.symbol_info->sc.GetFunctionName().AsCString());
+ m_j.attribute("mnemonic", insn.symbol_info->instruction->GetMnemonic(
+ &insn.symbol_info->exe_ctx));
+
----------------
the values being passed to the `attribute` method are c strings. Unsure how
this method handles a nullptr, but consider checking if these are nullptr
before calling attribute if necessary?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128316/new/
https://reviews.llvm.org/D128316
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
