wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
pretty good! I just left cosmetic requests
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:285-312
+ if (event.has_tsc) {
+ if (m_tsc_upper_bound && event.tsc > m_tsc_upper_bound.value()) {
+ // This event and all the remaining events of this PSB have a TSC
+ // outside the range of the "owning" ThreadContinuousExecution. For
+ // now we drop all of these events/instructions, future work can
+ // improve upon this by determining the "owning"
+ // ThreadContinuousExecution of the remaining PSB data.
----------------
I suggest moving all of this to another function because it's a bit long
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:286
+ if (event.has_tsc) {
+ if (m_tsc_upper_bound && event.tsc > m_tsc_upper_bound.value()) {
+ // This event and all the remaining events of this PSB have a TSC
----------------
use * instead of value(). value() is rarely used in LLVM code. Also use >=
instead of >
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:294-295
+ formatv(
+ "TSC {0} exceeds maximum TSC value {1}. Will skip decoding"
+ " the remining the remaining data of the PSB.",
+ event.tsc, m_tsc_upper_bound.value())
----------------
add a prefix message for easier regexp search
"decoding truncated: TSC packet {0} exceeds maximum TSC value {1} for this
PSB block . Will resume decoding with the next PSB."
Use a better prefix if you can, and btw, you wrote `the remining the remaining
` here.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:296
+ " the remining the remaining data of the PSB.",
+ event.tsc, m_tsc_upper_bound.value())
+ .str();
----------------
ditto
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:299-307
+ uint64_t offset;
+ int status = pt_insn_get_offset(m_decoder_up.get(), &offset);
+ if (!IsLibiptError(status)) {
+ err_msg =
+ formatv("At byte offset {0} of PSB with size {1} bytes, {2}",
+ offset, m_psb_block.size, err_msg)
+ .str();
----------------
I think let's better not include this to keep the error a bit smaller. In any
case, you can do `thread trace dump instructions <thread> -E` and then look for
the error prefix when debugging.
but if you insist, the byte offset message should come after the textual
description of the error
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:308
+ m_decoded_thread.AppendCustomError(err_msg);
+ return -1;
+ } else {
----------------
return -pte_internal;
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:345
DecodedThread &m_decoded_thread;
+ // Maximum allowed value of TSCs decoded from m_psb_block.
+ llvm::Optional<DecodedThread::TSC> m_tsc_upper_bound;
----------------
move this to the constructor so that it's highlighted by IDEs and appears in
the public documentation
Maximum allowed value of TSCs decoded from this psb block. If this value is
hit, then decoding for this block is stopped and an error is appended to the
trace.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136610/new/
https://reviews.llvm.org/D136610
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
