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