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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 marked 39 inline comments as done.
jj10306 added inline comments.


================
Comment at: lldb/source/Target/TraceHTR.cpp:53-57
+std::vector<HTRBlockLayer> const &TraceHTR::GetBlockLayers() const {
+  return m_block_layers;
+}
+
+HTRInstructionLayer const &TraceHTR::GetInstructionLayer() const {
----------------
wallace wrote:
> wallace wrote:
> > as these layers could change because of merges and what not, better remove the consts out of these methods
> remove a llvm::ArrayRef<HTRBlockLayerUP>
Layers are not mutated after being constructed by a pass, so there is currently no reason to expose mutable references to these structures.


================
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:
> 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
Discussed offline - decided to replace errors with an address of 0 for the time being. If decoding errors are common/users want to know the specifics of the error, we can add logic to store the error's message in a future diff.


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

https://reviews.llvm.org/D105741