wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
only mostly cosmetic changes needed. Thanks for this. I'm glad that we are
bringing the usage down
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:46-47
+
+TraceInstructionControlFlowType DecodedThread::GetInstructionControlFlowType(
+ size_t insn_index, lldb::addr_t next_load_address) const {
+ if (IsInstructionAnError(insn_index))
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:54-55
- switch (m_pt_insn.iclass) {
+ lldb::addr_t ip = m_instruction_ips[insn_index];
+ uint8_t size = m_instruction_sizes[insn_index];
+ pt_insn_class iclass = m_instruction_classes[insn_index];
----------------
let's give better names to these variables
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:62
mask |= eTraceInstructionControlFlowTypeBranch;
- if (m_pt_insn.ip + m_pt_insn.size != next_load_address)
+ if (ip + size != next_load_address)
mask |= eTraceInstructionControlFlowTypeTakenBranch;
----------------
we can stop using next_load_address because we now have access to the entire
trace
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:176
+ sizeof(pt_insn::iclass) * m_instruction_classes.size() +
m_errors.getMemorySize();
}
----------------
you also need to include `m_instruction_timestamps`, probably
`m_instruction_timestamps.size() * (sizeof(size_t) + sizeof(uint64_t))`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:127
+ /// Append a decoding error with a corresponding TSC.
+ void AppendError(llvm::Error &&error, uint64_t TSC);
+
----------------
lowercase tsc because it's a variable
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:129-136
+ /// Get the instruction pointers from the decoded trace. Some of them might
+ /// indicate errors (i.e. gaps) in the trace. For an instruction error, you
+ /// can access its underlying error message with the \a
+ /// GetErrorByInstructionIndex() method.
///
/// \return
/// The instructions of the trace.
----------------
I regret having created this method because it forces us to have a certain
interface for the storage of the instructions. Let's just delete this method
and create one new method
size_t GetInstructionsCount() const;
which should be able to replace all the external calls to this method
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:139
+ /// \return
+ /// The instruction pointer address, or \a LLDB_INVALID_ADDRESS if it is
+ /// an error.
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:143-154
+ /// Get the \a lldb::TraceInstructionControlFlowType categories of the
+ /// instruction.
+ ///
+ /// \param[in] next_load_address
+ /// The address of the next instruction in the trace or \b
+ /// LLDB_INVALID_ADDRESS if not available.
+ ///
----------------
Now that DecodedThread knows everything, we can simplify this method a bit.
First, we can remove the parameter `next_load_address`, because it can easily
be gotten by checking the instruction at index `insn_index + 1`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:193-199
+ /// Record an error decoding a TSC timestamp.
+ ///
+ /// See \a GetTscErrors() for more documentation.
+ ///
+ /// \param[in] libipt_error_code
+ /// An error returned by the libipt library.
+ void RecordTscError(int libipt_error_code);
----------------
+1
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:218
+ std::vector<lldb::addr_t> m_instruction_ips;
+ /// The low level storage of all instruction sizes.
+ std::vector<uint8_t> m_instruction_sizes;
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:220
+ std::vector<uint8_t> m_instruction_sizes;
+ /// The low level storage of all instruction classes.
+ std::vector<pt_insn_class> m_instruction_classes;
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:23-30
+ assert(!m_decoded_thread_sp->GetInstructionPointers().empty() &&
"a trace should have at least one instruction or error");
- m_pos = m_decoded_thread_sp->GetInstructions().size() - 1;
+ m_pos = m_decoded_thread_sp->GetInstructionPointers().size() - 1;
m_tsc_range = m_decoded_thread_sp->CalculateTscRange(m_pos);
}
size_t TraceCursorIntelPT::GetInternalInstructionSize() {
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122991/new/
https://reviews.llvm.org/D122991
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
