wallace added inline comments.
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:136-140
+ /// Updates block_id: block mapping and append `block_id` to the block id
+ /// trace
+ void AddNewBlock(size_t block_id, HTRBlock &block);
+ /// Append `block_id` to the block id trace
+ void AppendBlockId(size_t block_id);
----------------
jj10306 wrote:
> wallace wrote:
> > why do you need both?
> `AppendBlockId` is for when we just need to append a block id to the block id
> trace. This is needed when we encounter a block that we have previously
> encountered, so we only need to append its block id and not worry about
> creating a new block.
> `AddNewBlock` is needed when a new block has been encountered. In this case,
> the new block needs to be added to `m_block_defs` and the its block id needs
> to be appended to the block id trace.
ahh, then better rename them. AppendRepeatingBlockToEnd and AppendNewBlockToEnd
might be better. You could omit the ToEnd suffix
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2145-2151
+ m_trace_export_format =
+ (TraceExportFormat)OptionArgParser::ToOptionEnum(
+ option_arg, GetDefinitions()[option_idx].enum_values,
+ eTraceExportUnknownFormat, error);
+ if (m_trace_export_format == eTraceExportUnknownFormat)
+ error.SetErrorStringWithFormat("unknown format '%s' specified.",
+ option_arg.str().c_str());
----------------
jj10306 wrote:
> wallace wrote:
> > as we are going to have exporter plugins, to simplify this code, just
> > expect the "ctf" string here explicitly. Later I'll do the smart lookup
> Are you suggesting that, for the time being, we just check that `option_arg`
> is "ctf" and set `m_trace_export_format` to `eTraceExportChromeTraceFormat`
> if so and report an error otherwise?
yes, just to simplify the code. This check will go away as soon as I implement
the barebones for the TraceExporter plugins
================
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();
----------------
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
================
Comment at: lldb/source/Target/TraceHTR.cpp:395
+ {"ph", "B"},
+ {"ts", (ssize_t)i}, // TODO: is there a way to serialize size_t to
JSON?
+ // if not, how should we handle overflow here?
----------------
jj10306 wrote:
> Is there a way to store uint64s in JSON? If not, what's the suggested way to
> handle overflow here?
no, only int64_t, that's the type you have to use. And you won't have an
overflow, AFAIK OSs don't use the last bits of addresses, as they use them
internally for other things.
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
