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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 marked an inline comment 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;
+
----------------
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/


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:339
+  ///     The file that the exported HTR data will be written to.
+  void Export(Stream &s, TraceExportFormat export_format, std::string outfile);
+
----------------
wallace wrote:
> jj10306 wrote:
> > wallace wrote:
> > > better return an llvm::Error that will contain the error message, if any. The CommandObject that invokes this method can then get the error and dump it if necessary. So, don't pass any streams here
> > I based this method off of `TraceCursor::DumpInstructions` which takes in a Stream & to output its success/error messages to. This method (`TraceHTR::Export`) prints a message to the console on success (not only failure) so I'm not sure how the success message could be propagated back up to the caller. I agree that using `llvm::Error` here would make sense if we were only printing if an Error occurred, but since we print on success I'm not sure if we should use llvm::Error over passing the Stream.
> It's different, because DumpInstructions is supposed to dump information in the stream, but in this case Export is printing to a file. So what if someone wants to invoke Export but not from a command? e.g. from a python script. You are making this case a little bit more complicated.
> You can just return llvm::success() and let the caller print anything to Stream if available. In any case, it's standard in lldb to not print anything if the operation is a success
Ah I see, that makes sense. I'll switch it to use `llvm::Error` and not print anything in the case of success, because the success indication isn't particularly useful.


================
Comment at: lldb/source/Target/TraceHTR.cpp:439-440
+
+      // Can't use ConstString here because no toJSON implementation exists for
+      // ConstString
+      llvm::Optional<std::string> most_freq_func =
----------------
wallace wrote:
> jj10306 wrote:
> > wallace wrote:
> > > this comment doens't make sense, as you are instead passing display_name to the json object
> > I was trying to indicate that `GetMostFrequentlyCalledFunction` returns a `std::string` instead of `ConstString` because eventually the `ConsString` function name will need to be converted to `std::string` in order to be serialized, but I will remove this comment and just change the function to return `ConstString` and then do the conversion in the ternary that defines `display_name`.
> you shouldn't change a generic API just to ease a single instance of a usage. Better return a ConstString, or even better, a llvm::StringRef, which can point to anything. ConstString has a method to get a StringRef out of it. StringRef::str() gets you a std::string if you need it, or you can use StringRef::data() to get the const char *, which helps you avoid one copy.
Will update that function to return a `StringRef` 🙂


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

https://reviews.llvm.org/D105741