[Lldb-commits] [PATCH] D128316: [trace] Add an option to dump instructions in json and to a file

[Jakob Johnson via Phabricator via lldb-commits]

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