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

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 8 inline comments as done.
wallace added a comment.

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

That's the instruction load address. In a later diff I'll implement pretty-printing similar to the "disassembly" command, but for now this is a good start.



================
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 =
----------------
clayborg wrote:
> 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.
> 
This is exactly what I needed! The name is just not very precise =P


================
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
----------------
clayborg wrote:
> What is the default count here? 19?  Seems like an odd number to choose as a default? 
The default is 20, but I'm adding the 20th-element here for clarity


================
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'''])
+
----------------
clayborg wrote:
> We should be showing addresses here. It doesn't matter if they are mapped or not. This will happen for JIT'ed code.
I'll do this in a later diff. Currently libipt doesn't report the addresses that it fails to decode, but I'm planning on making a patch on libipt to support that.


================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:163
+  [ 3] 0x400654
+  [ 4] no memory mapped at this address
+  [ 5] 0x400516
----------------
clayborg wrote:
> 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
> 
Repeating my message from above:

> I'll do this in a later diff. Currently libipt doesn't report the addresses that it fails to decode, but I'm planning on making a patch on libipt to support that.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283