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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
