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

[walter erquinigo via Phabricator via lldb-commits]

wallace 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);
----------------
jj10306 wrote:
> 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.
ahh, then better rename them. AppendRepeatingBlockToEnd and AppendNewBlockToEnd might be better. You could omit the ToEnd suffix


================
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());
----------------
jj10306 wrote:
> 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?
yes, just to simplify the code. This check will go away as soon as I implement the barebones for the TraceExporter plugins


================
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();
----------------
jj10306 wrote:
> 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?
If in CTR you can embed error messages, it would be nice to have that in the metadata. It's also valid to have an instruction address of 0 in the case of errors. However, you have end up having abnormal blocks like
   insn1 insn2 error_kind1 insn1 insn2 error_kind2

so if you just look at the addresses, [insn1, insn2, 0] is a block that appears twice.

Now the question is: if your users will want to see the specific errors, then you'll have to handle it accordingly in the metadata. But if your users don't case what kind of errors are displayed in the chrome trace view, then replacing an error with address 0 makes sense. You could leave a TODO here in that case. Let's sync up offline anyway


================
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?
----------------
jj10306 wrote:
> Is there a way to store uint64s in JSON? If not, what's the suggested way to handle overflow here?
no, only int64_t, that's the type you have to use. And you won't have an overflow, AFAIK OSs don't use the last bits of addresses, as they use them internally for other things.


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

https://reviews.llvm.org/D105741