[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:136-140
+  /// Updates block_id: block mapping and append `block_id` to the block id
+  /// trace
+  void AddNewBlock(size_t block_id, HTRBlock &block);
+  /// Append `block_id` to the block id trace
+  void AppendBlockId(size_t block_id);
----------------
wallace wrote:
> why do you need both?
`AppendBlockId` is for when we just need to append a block id to the block id trace. This is needed when we encounter a block that we have previously encountered, so we only need to append its block id and not worry about creating a new block.
`AddNewBlock` is needed when a new block has been encountered. In this case, the new block needs to be added to `m_block_defs` and the its block id needs to be appended to the block id trace.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2145-2151
+        m_trace_export_format =
+            (TraceExportFormat)OptionArgParser::ToOptionEnum(
+                option_arg, GetDefinitions()[option_idx].enum_values,
+                eTraceExportUnknownFormat, error);
+        if (m_trace_export_format == eTraceExportUnknownFormat)
+          error.SetErrorStringWithFormat("unknown format '%s' specified.",
+                                         option_arg.str().c_str());
----------------
wallace wrote:
> as we are going to have exporter plugins, to simplify this code, just expect the "ctf" string here explicitly. Later I'll do the smart lookup
Are you suggesting that, for the time being, we just check that `option_arg` is "ctf" and set `m_trace_export_format` to `eTraceExportChromeTraceFormat` if so and report an error otherwise?


================
Comment at: lldb/source/Target/TraceHTR.cpp:151
+  while (valid_cursor) {
+    // TODO: how should we handle cursor errors in this loop?
+    lldb::addr_t current_instruction_load_address = cursor->GetLoadAddress();
----------------
wallace wrote:
> you need to enhance the Instruction object, to support errors. You can store the error string in it using a char * pointer from a ConstString. Errors are not frequent, so that should be fine. You need to display the errors in the export data as well, as hiding them could be misleading to the user
What Instruction object are you referring to?
Given the way the InstructionLayer currently works, vector of load addresses and a map for metadata, how would you suggest incorporating the error information?
My first thought was to store a special value (such as 0) in the vector of load addresses for each error, but this seems a bit hacky and doesn't allow a way to map a specific error back to its error message. 
What are your thoughts?


================
Comment at: lldb/source/Target/TraceHTR.cpp:395
+        {"ph", "B"},
+        {"ts", (ssize_t)i}, // TODO: is there a way to serialize size_t to JSON?
+                            // if not, how should we handle overflow here?
----------------
Is there a way to store uint64s in JSON? If not, what's the suggested way to handle overflow here?


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

https://reviews.llvm.org/D105741