clayborg added inline comments.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:31
+ if (m_custom_error_message.empty()) {
+ std::ostringstream os;
+ os << pt_errstr(pt_errcode(GetErrorCode())) << ": 0x" << std::hex
----------------
labath wrote:
> `raw_string_ostream` would be more llvm-y (the std::hex part in particular is
> very non-idiomatic)
That or "lldb_private::StreamString". Both have similar functionality. I prefer
StreamString because it is simpler. With raw_string_ostream, you have to make a
std::string, put it into the raw_string_ostream and then flush it prior to
getting the string result.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41
+
+std::vector<IntelPTInstruction> &DecodedThread::GetInstructions() {
+ return m_instructions;
----------------
labath wrote:
> Do you want anyone to modify the vector? Return ArrayRef<IntelPTInstruction>
yeah llvm::ArrayRef to avoid making copies is good.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:32
+/// returned.
+int FindNextSynchronizationPoint(pt_insn_decoder &decoder) {
+ // Try to sync the decoder. If it fails, then get
----------------
labath wrote:
> static
Make static or add an anonymous namespace around all of these functions so you
don't have to mark them all as static.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:80
+/// error code in case of errors.
+int ProcessPTEvents(pt_insn_decoder &decoder, int errcode) {
+ while (errcode & pts_event_pending) {
----------------
labath wrote:
> static
Make static or add an anonymous namespace around all of these functions so you
don't have to mark them all as static.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:94
+
+ std::vector<IntelPTInstruction> instructions =
+ decoded_thread->GetInstructions();
----------------
labath wrote:
> this makes a copy, which you probably did not want.
returning a llvm::ArrayRef to avoid the copy
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:98
+ int delta = direction == TraceDirection::Forwards ? -1 : 1;
+ for (uint64_t i = position; i < instructions.size() && i >= 0; i += delta)
+ if (!callback(i, instructions[i].GetLoadAddress()))
----------------
labath wrote:
> `i>=0` is always true. You'll have to do this trick with signed numbers
> (ssize_t?)
Yes, switch to ssize_t, your delta is already signed. Also switch "delta" to
ssize_t as well.
================
Comment at: lldb/source/Target/Trace.cpp:106
+
+ size_t last_index = (int64_t)start_position + count - 1;
+ TraverseInstructions(
----------------
labath wrote:
> The cast to int64_t won't change the actual value of the result (though it
> may invoke UB due to signed wraparound). What exactly are you trying to
> achieve here?
Lots os signed/unsigned match issues possible. Best to make this rock solid.
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:48
+ [ 4] 0x40052d
+ [ 5] 0x400529
+ [ 6] 0x400525
----------------
If we reverse the direction, then hitting "enter" after doing one command won't
flow as nicely as it does now. That being said I agree with Pavel that we
should figure out what is expected. I generally think that earlier text is
older.
I would not switch the indexes so that they change with any options that are
specified. We currently have --start-position, but maybe this should be just
--position? Or we specify:
```
--from-end <offset>
```
<offset> would be the index offset from the end (newest) of the data?
```
--from-start <offset>
```
<offset> would be the index offset from the start (oldest) of the data?
I would be fine with:
```
[--forwards | -f] [--backwards | -b]
```
but I think it would make sense to show the indexes in a consistent way
regardless of what options are displayed. Maybe it makes sense to always show
the true index where zero is the oldest and N is the newest?
We do need to make sure the auto repeat command looks good though which will be
hard with oldest to newest ordering.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89283/new/
https://reviews.llvm.org/D89283
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
