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

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 4 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:46
 /// state and granularity.
 class TraceInstructionDumper {
 public:
----------------
jj10306 wrote:
> 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).
I'm renaming TraceInstructionWriter to OutputWriter, just because that one is an internal class


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2210
     size_t m_continue;
+    llvm::Optional<FileSpec> m_output_file;
     TraceInstructionDumperOptions m_dumper_options;
----------------
jj10306 wrote:
> 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?
basically just because this field is not used in the dumper at all. The dumper requires a Stream object, which is configured by the caller, either a file stream or a standard output stream (or maybe a python string stream). So in this case, because this field is only used by this command handler to create the actual Stream to pass to the dumper, I'm restricting this variable to 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:
----------------
jj10306 wrote:
> Move declarations to header?
in this case I'd rather not do that because I don't want anyone to see these classes. They are very restricted to the use case of the dumper and no one should try to use them for any reason. So I'm making them very private intentionally


================
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));
+
----------------
jj10306 wrote:
> 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?
good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128316