[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:272
+  ///     The map of block IDs to \a HTRBlock.
+  std::unordered_map<size_t, HTRBlock> const &GetBlockDefs() const;
+
----------------
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>"


================
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);
+
----------------
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


================
Comment at: lldb/source/Target/TraceHTR.cpp:152
+      valid_cursor = cursor.Next();
+      s.Printf("%s\n", toString(std::move(err)).c_str());
+    } else {
----------------
jj10306 wrote:
> wallace wrote:
> > this Printf is too noisy. You don't really need that. Just remove it and instead of invoking cursor.GetError(), invoke cursor.IsError() after rebasing the code
> Initially I wasn't planning on displaying the error, but then I was getting runtime errors because I wasn't doing anything with the error. As a result, I decided to display the error and looked at how `TraceCursor::DumpInstructions` does it: `s << toString(std::move(err));`.
> I believe if an `isError()` method existed then I would not have this issue about not checking/using the error and I could just use the `isError()` method to check for the presence of an error without actually retrieving the error object.
yes my bad =P After rebasing you should be able to use IsError() :) Sorry for the hiccup


================
Comment at: lldb/source/Target/TraceHTR.cpp:197
+HTRBlock IHTRLayer::MergeUnits(size_t start_unit_index, size_t num_units) {
+  assert(num_units > 0);
+  llvm::Optional<HTRBlockMetadata> merged_metadata = llvm::None;
----------------
jj10306 wrote:
> wallace wrote:
> > remove this assert, the code wouldn't break if num_units is 0
> If `num_units` is 0 then the loop body would never execute and thus `merged_metadata` would be `None` at the time of return. This would be problematic since we return `*merged_metadata`.
Ahh, true. Then also mention in the documentation that num_units must be >= 1, otherwise someone might think that could somehow get an empty HTRBlock ><


================
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 =
----------------
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.


================
Comment at: lldb/test/API/commands/trace/TestTraceExport.py:60
+                error=True)
+
----------------
jj10306 wrote:
> wallace wrote:
> > Add a new test in which you run this algorithm on a program with a few levels of function calls, and check that the output trace makes sense. Don't check for individual load addresses, but instead check for the appearance of the methods and blocks that you expect.
> Yes, this makes sense. Is the way to do this by loading the JSON file's contents into memory in the test and doing assertions on that structure?
yes, that should be fine. You can also precompile a non-trivial a.out file and include it here, if you want the code to be fully deterministic. If you make it non-trivial enough, we should be able to reuse for other intel pt tests as well


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

https://reviews.llvm.org/D105741