[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

[Greg Clayton via Phabricator via lldb-commits]

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