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

Mon Oct 19 16:08:11 PDT 2020

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

@labath is right regarding the need of pre-baked binaries to test specific conditions. I'll remove the ld binary, as it really tests nothing useful, and i'll try to use yaml to represent the binaries in a more concise format.

================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:43-48
+  [ 0] 0x40052d
+  [ 1] 0x400529
+  [ 2] 0x400525
+  [ 3] 0x400521
+  [ 4] 0x40052d
+  [ 5] 0x400529
----------------
labath wrote:
> Are you sure that printing this backwards is the best way to display this? The resulting disassembly is going to look quite weird. I think that printing this in the "normal" direction would make it easier to figure out what the program was doing. For people who are only interested in the final PC value it should not be a problem to skip to the last line of the output (the last line is also more likely to remain visible if the dump produces lots of data).
First of all, I'm thinking about adding a flag to this command to choose the direction, as there are benefits of both.

Let's say, if you are interested in reading/understanding the last instructions up to a breakpoint, then reading the trace in reverse makes sense, as you don't know where to start reading from, but you know where to end. Imagine you have 100K instructions, where do you start? It seems sometimes better to read the last instructions and then ask for a few of the earlier instructions, and keep doing that until you find what you are interested in. On the other hand, if you want to analyze forwards what happens from a certain point, this API is quite annoying and I imagine you'd prefer to read it forwards.

So I propose

  thread trace dump instructions --count <> --start-position <> [--forwards | -f] [--backwards | -b]

I'd keep -b as default, as it's useful when analyzing crashes or stops on breakpoints. The default --start-position when reading forwards could be the oldest chronological instruction, and the default when reading backwards could be the earliest chronologically.

With this, I'd change the indices. I'd make index [0] to be the oldest chronologically and [|trace| -1] to be the most recent.

@labath, @clayborg, what do you think? This might be flexible enough for the different kind of usages.

================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:159-160
+            substrs=['''thread #1: tid = 3842849, total instructions = 2
+  [0] no memory mapped at this address: 0x400518
+  [1] no memory mapped at this address: 0x400511'''])
+
----------------
clayborg wrote:
> These lines should start with the address like all other lines. Then the question is what the output should look like. Do we really need to tell the user that there is no memory mapped here? Can we just print "<???>" or nothing if we have no information like:
> 
> ```
> [0] 0x400518: <???>
> [1] 0x400511: <???>
> ```
> 
I think it's highly important to tell the user that this is a very important error and not make it apparently inoffensive with the formatting. Let me elaborate why this is not an inoffensive error.

First of all, the encoded trace is composed of packets, composed of two main packets:

- PSB: synchronization packet that contains the current PC. These packets are sporadic (often one for each 4KB of data), as they are big in size.
- TNT: taken/not taken packet that contains one bit per branch executed by the processor. These packets are probably the most frequent and they don't include any PC.

When decoding, the decoder finds first a PSB packet, gaining the knowledge of the current PC, then it starts traversing the binary instruction by instruction until it finds a branch, in which case it finds the next TNT packet and learns if that branch was taken or not, then continuing the traversal in the correct direction. 

This means that when the decoder can't read a memory address, then it won't be able to decode any TNT packets until the next PSB synchronization point. In fact, in this diff, when there's an instruction decoding error, we skip to the next PSB and resume decoding from there. This problem implies that we are skipping potentially thousands of instructions. In other words, if you see

[0]: 0x400518
[1]: 0x400511
[2]: no memory mapped at this address: 0x400502
[3]: 0x400500

Then that means that between instructions [3] and [1] there were an unknown number of instructions that couldn't be decoded, the first one of them being at 0x400502. We won't be able to do anything useful with those instructions, and the user would need to provide the missing module and redecode to reconstruct the full trace.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283