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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Great start to this. Many comments inlined.

When you are dumping instructions we are only showing one hex value. Is this the instruction address or the opcode itself?



================
Comment at: lldb/include/lldb/Target/Trace.h:122-123
+
+  /// Run the provided callback on the instructions of the trace of the given
+  /// thread.
+  ///
----------------
I am going to comment here on what this function should look like and how it will be used by all of the APIs. This function can probably be used to implement the forward and reverse stepping/continue commands eventually. So I would propose that this function should be able to start from a given index and be able to go forward or backward in the instructions (for forward/reverse step/continue).

So with this in mind how about:

```
void TraverseInstructions(const Thread &thread, size_t position, bool forward, std::function<...> callback) = 0;
```



================
Comment at: lldb/include/lldb/Target/Trace.h:146
+      const Thread &thread,
+      std::function<bool(size_t index, llvm::Expected<lldb::addr_t> &load_addr)>
+          callback,
----------------
This should probably be passed by value as it could contain an error. If there is an error the error must be consumed. If we pass by reference then it is unclear who must consume the error.


================
Comment at: lldb/include/lldb/Target/Trace.h:157
+  ///     The total number of instructions of the trace.
+  virtual size_t GetInstructionCount(const Thread &thread) = 0;
+
----------------
Will we always know instruction count? Could this to very expensive to calculate? Can we add this to the generic trace API and expect all trace formats to implement this?


================
Comment at: lldb/include/lldb/Target/Trace.h:171
+  ///     or an actual Error in case of failures.
+  virtual llvm::Error GetTraceErrorStatus(const Thread &thread) = 0;
 };
----------------
Can we just use the ForEachInstruction and get the error during that call? Is this call redundant? If there is an error, it might be better to get it via the ForEachInstruction function and know where the problem is. If there is no data, you will get the error on the first access to the first instruction. Knowing where the error happened might help the user have more context.


================
Comment at: lldb/source/Target/ProcessTrace.cpp:151-152
 
 size_t ProcessTrace::DoReadMemory(addr_t addr, void *buf, size_t size,
                                   Status &error) {
+  const std::map<MemoryRegionInfo::RangeType, SectionSP> &section_map =
----------------
You should be able to just call:

```
size_t Target::ReadMemoryFromFileCache(const Address &addr, void *dst, size_t dst_len, Status &error);
```
It already does what you are doing here if all that is happening here is reading from loaded object file section data.



================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:41-61
         self.expect("thread trace dump instructions",
-            substrs=['thread #1: tid = 3842849, total instructions = 1000',
-                     'would print 20 instructions from position 0'])
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  [ 0] 0x40052d
+  [ 1] 0x400529
+  [ 2] 0x400525
+  [ 3] 0x400521
+  [ 4] 0x40052d
----------------
What is the default count here? 19?  Seems like an odd number to choose as a default? 


================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:137
+  [16] 0x40052d''', result.GetOutput())
+
+    def testWrongImage(self):
----------------
Do we have a test for when the offset is invalid? Another test for the count being too large and the output would get truncated?


================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:142-143
+            substrs=['''thread #1: tid = 3842849, total instructions = 2
+  [0] no memory mapped at this address
+  [1] no memory mapped at this address'''])
+
----------------
We should be showing addresses here. It doesn't matter if they are mapped or not. This will happen for JIT'ed code.


================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:163
+  [ 3] 0x400654
+  [ 4] no memory mapped at this address
+  [ 5] 0x400516
----------------
Why aren't we showing the address here? We will run into cases, for possibly JIT'ed code where we won't have a section for an address, so we should still show the address



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89283/new/

https://reviews.llvm.org/D89283