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

[Jakob Johnson via Phabricator via lldb-commits]

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


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:272
+  ///     The map of block IDs to \a HTRBlock.
+  std::unordered_map<size_t, HTRBlock> const &GetBlockDefs() const;
+
----------------
jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > wallace wrote:
> > > > As this is const, the only way to use this unordered_map is to do look ups, then let's better hide the implementation detail that is an unordered map and create this method instead
> > > > 
> > > >   const HTRBlock &GetBlockById(size_t id);
> > > > 
> > > > that way we can later change unordered_map for anything else without affecting any callers
> > > Makes sense. How should the case when an `id` that the layer doesn't contain is passed in? I would like to use `llvm::Optional` here but it's not possible to have an Optional reference (i.e. `Optional<& const HTRBlock>`). I was thinking about creating different methods:
> > > 
> > > `bool ContainsBlock(size_t id) const` for checking if the layer contains an ID
> > > `HTRBlock const & GetBlockById(size_t id) const` for retrieving a reference to a block in the layer
> > > but this still doesn't solve the issue of if a caller calls `GetBlockId` with an invalid ID.
> > > 
> > > Another option would be to change `GetBlockId` to return `Optional<HTRBlock>` instead of a reference, but this would result in an unnecessary copy, when all we really need is an const reference.
> > Optional<const HTRBlock &> should work like a charm. You can't put the & before the const AFAIK. People normally read the & backwards as in "reference to <const HTRBlock>"
> I tried what you suggested and also just a reference (no const), but it none of them work - this is the error message:
> ```
> llvm/include/llvm/ADT/Optional.h:281:5: error: 'getPointer' declared as a pointer to a reference of type 'lldb_
> private::HTRBlock &'
>   T *getPointer() { return &Storage.getValue(); }
> ```
> A cursory search of the issue brought up many articles, here's one:
> https://www.fluentcpp.com/2018/10/05/pros-cons-optional-references/
Discussed offline - decided to return `HTRBlock * const` with a `nullptr` indicating that the layer does not contain a bock with the specified block id.


================
Comment at: lldb/source/Target/TraceHTR.cpp:160
+          current_instruction_load_address);
+      valid_cursor = cursor.Next();
+      if (current_instruction_type &
----------------
wallace wrote:
> valid_cursor is not a nice name, just call it no_more_data, end_of_trace, or something like that
I was trying to use "positive logic", otherwise using something like `end_of_trace` would require and negating every use of it and every time the `Next()` is called (i.e `end_of_trace =!cursor->Next()`).

This is obviously a very minor thing, but I think using a well named "positive logic" variable makes the code easier to read - thoughts on using a name like `more_data_in_trace`? It's definitely harder to come up with a well named positive logic variable so let me know if you have any better suggestions 



================
Comment at: lldb/source/Target/TraceHTR.cpp:245
+      os.flush();
+      if (os.has_error()) {
+        s.Printf("error exporting trace representation to %s\n", outfile_cstr);
----------------
wallace wrote:
> can you also show the actual error message from os?
I don't believe `llvm::raw_fd_ostream` stores the error message, only the error code.


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

https://reviews.llvm.org/D105741