[trace][intel-pt] Implement the basic decoding functionality

[wallace]

Summary

Depends on D89408.

This diff finally implements trace decoding!

The current interface is

$ trace load /path/to/trace/session/file.json
$ thread trace dump instructions

thread #1: tid = 3842849, total instructions = 22
  [ 0] 0x40052d
  [ 1] 0x40052f  
  ...
  [19] 0x400521

$ # simply enter, which is a repeat command
  [20] 0x40052d
  [21] 0x400529
  ...

This doesn't do any disassembly, which will be done in the next diff.

Changes:

• Added an IntelPTDecoder class, that is a wrapper for libipt, which is the actual library that performs the decoding.
• Added TraceThreadDecoder class that decodes traces and memoizes the result to avoid repeating the decoding step.
• Added a DecodedThread class, which represents the output from decoding and that for the time being only stores the list of reconstructed instructions. Later it'll contain the function call hierarchy, which will enable reconstructing backtraces.
• Added basic APIs for accessing the trace in Trace.h:
  • GetInstructionCount, which counts the number of instructions traced for a given thread
  • IsTraceFailed, which returns an Error if decoding a thread failed
  • ForEachInstruction, which iterates on the instructions traced for a given thread, concealing the internal storage of threads, as plug-ins can decide to generate the instructions on the fly or to store them all in a vector, like I do.
• DumpTraceInstructions was updated to print the instructions or show an error message if decoding was impossible.
• Tests included

[labath]

lldb/include/lldb/Target/Trace.h

167

It sounds like this could just be llvm::function_ref<bool(size_t index, Expected<addr_t> load_addr> and the Instruction class does not even need to exist. At least not here -- it may still be useful for the PT plugin to store the instructions in some sort of an error-or-load-addr union, but there's no need to impose that organization on anyone else.

191

I'd expect a method called IsXXX to return bool.

lldb/include/lldb/Target/TraceThread.h

23 ↗

(On Diff #297736)

I'm very confused by TraceThread residing in the Target library and ProcessTrace in Plugin/Process/Trace. I think both of them should be in the same place (I could make a case for either location).

lldb/source/Plugins/Trace/intel-pt/DecodedThread.h

78

std::move(instructions)

85

ArrayRef<IntelPTInstruction> maybe?

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

25–32

llvm::MemoryBuffer::getFile(filename)

34

and? That's what the code seems to be doing.

36

Please put static first (I'm surprised this even compiles).

69

static std::vector<IntelPTInstruction> DecodeInstructions

178

Expected<vector<Insns>>

198

Use the GetPath overload returing a std::string

200

So will this make the library load the file into memory once again? Is there no way to make it use the copy already loaded by lldb?

lldb/source/Target/TraceSessionFileParser.cpp

125

This is not a dependency in the strictest sense but it still means that this code would explode if the ProcessTrace "plugin" is plugged "out". It sounds like that, in this design, the ProcessTrace class should just be a part of lldb core.

[wallace]

Changes:

• Changed the callback signature of ForEachInstruction to receive an Expected<load_addr>
• Use Expected more ubiquitously in IntelPTDecoder
• Use MemoryBuffer to read the trace file
• Implemented ProcessTrace::DoReadMemory, which now is used by libipt instead of loading each object file inside libipt
• Made the decoding function a little bit more readable
• Small fixes here and there

Notes:

• I added a small test where a instruction can't be decoded due to missing memory mapping. I'll add later more robust tests.
• I'm also not fond of the ProcessTrace class being implemented in a plugin, however, it seems that all processes are implemented that way and the main method for creating processes Target::CreateProcess relies on plugins for creating the right instance. I could create an overload of that class that receives a callback that creates a specific process class, although it might create an way to unintendendly bypass the existing Target::CreateProcess. Another option is to move the class to lldb core and register the plugin from there, even though it'd be the only class doing that in the codebase. @labath , what do you think?

[wallace]

• Moved the non intel-pt changes to D89408
• Added some more complex tests:
  • A multifile case in which even the dynamic linker code is decoded
  • A variation of that case that doesn't declare the dynamic linker in the json file, which results in a trace with a missing block of instructions, but still is able to decode main.cpp
• Some general cleanup

[wallace]

cleanup

[wallace]

rebase

[wallace]

rebase

[clayborg]

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?

lldb/include/lldb/Target/Trace.h

143–144

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;

167

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.

178

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?

192

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.

lldb/source/Target/ProcessTrace.cpp

124–125

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.

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

42–65

What is the default count here? 19? Seems like an odd number to choose as a default?

100

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?

105–106

We should be showing addresses here. It doesn't matter if they are mapped or not. This will happen for JIT'ed code.

126

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

[wallace]

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.

lldb/source/Target/ProcessTrace.cpp

124–125

This is exactly what I needed! The name is just not very precise =P

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

42–65

The default is 20, but I'm adding the 20th-element here for clarity

105–106

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.

126

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.

[wallace]

• C hanged the instruction iterator to the proposed signature
• Added SupportsInstructionsCount to determine whether GetInstructionCount is supported by the plug-in. This count can be useful for debugging purposes, and at least for a while we won’t support lazy decoding of instructions.
• Removed GetTraceErrorStatus following Greg’s idea
• Added the requested test cases
• Some small fixes here and there

[wallace]

This comment was removed by wallace.

[wallace]

Figured out how to show the addressed that failed to decode. Now the output in those cases is something like

[ 4] no memory mapped at this address: 0x7ffff7df1950

This should address all of Greg's comments

[wallace]

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

105–106

Already fixed, disregard this comment

126

Already fixed, disregard this comment

[vsk]

I'm not sure if the current revision of the patch reflects the long-term testing strategy. But if so, I'm quite concerned about the proliferation of large binary files in the repo (like ld.so, or the raw trace binary itself). These are opaque blobs that are hard to understand. Also, each time we add one, we're imposing a sizeable tax on everyone working with the llvm-project monorepo.

One possible alternative:

• Design a textual description for the raw trace contents, and possibly a way to convert an existing trace file into this textual format
• Check in assembly, and use llvm-mc/clang to generate executables during testing

lldb/tools/intel-features/intel-pt/Decoder.cpp

251 ↗

(On Diff #298751)

Please group your includes.

260 ↗

(On Diff #298751)

unintentional whitespace diff?

301 ↗

(On Diff #298751)

Not sure what this is in aid of?

llvm/include/llvm/Support/Error.h

1016 ↗

(On Diff #298751)

Probably best to not add another escape-hatch to permit fast and loose error handling. This seems to be used in a lambda passed to TraverseInstructions. There might be a way to avoid invoking the callback in the case where the expected value is thrown away.

[wallace]

removing unwanted changes in lldb/tools/intel-features/intel-pt/Decoder.cpp

lldb/tools/intel-features/intel-pt/Decoder.cpp

301 ↗

(On Diff #298751)

I erroneously included this, I'm reverting the changes to this file.

[wallace]

@vsk, I agree with you regarding the files. At the moment our implementation of intel-pt tracing doesn't support collecting a trace, but soon we'll do so. Then, we'll be able to generate these trace files on the fly as the tests run, so I imagine I'll be deleting these binary files. For the time being I doubt I'll include any new binary, as what is included is more than enough to test the basic decoding functionalities.

[wallace]

llvm/include/llvm/Support/Error.h

1016 ↗

(On Diff #298751)

Sure, will remove this

[wallace]

• Remove the llvm::consumeExpected function, as @vsk suggested.

[clayborg]

Instead of just seeing the address, we should disassemble the instruction at the address in this patch for clarity if it is available. If we can't read the opcode from the object files, we need to display an appropriate message after the address in the output of "thread trace dump instructions".

[clayborg]

As I stated before we should be printing the disassembly for each instruction on the output lines. We should probably also normalize the address hex value to it doesn't change widths. Something like:

[ 0] 0x000000000040065f: pushq  %rbp
[ 1] 0x000000000040065a: movq   %rsp, %rbp
[ 2] 0x0000000000400657: movl   $0x0, -0x1c(%rbp)
[ 3] 0x0000000000400654: callq  0x100000f82               ; symbol stub for: printf
[ 4] 0x00007ffff7df1950: movl   %edi, -0x20(%rbp)

And possibly in this patch or in another patch, we should print out when the source file and line changes

a.out`main @ main.cpp:12
  [ 0] 0x000000000040065f: pushq  %rbp
  [ 1] 0x000000000040065a: movq   %rsp, %rbp
  [ 2] 0x0000000000400657: movl   $0x0, -0x1c(%rbp)
a.out`main @ main.cpp:13
  [ 3] 0x0000000000400654: callq  0x00007ffff7df1950               ; symbol stub for: printf
libc.so`printf
  [ 4] 0x00007ffff7df1950: movl   %edi, -0x20(%rbp)

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

121–122

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: <???>

142

[ 4] 0x7ffff7df1950 <???>

[vsk]

In D89283#2336120, @wallace wrote:

@vsk, I agree with you regarding the files. At the moment our implementation of intel-pt tracing doesn't support collecting a trace, but soon we'll do so. Then, we'll be able to generate these trace files on the fly as the tests run, so I imagine I'll be deleting these binary files. For the time being I doubt I'll include any new binary, as what is included is more than enough to test the basic decoding functionalities.

That seems promising. Deleting those binary files after the fact doesn't address the issue, though, as they'd be part of the history. I have a question about that ld-2.17.so file in particular: is there no way to decoder/traverse a trace of a process that loads a dylib, without copying all of ld.so into the source tree? That seems very surprising -- I'd expect the decoder API to allow you to skip right over PC ranges that have nothing to do with the binary you want to debug.

lldb/source/Target/Trace.cpp

85

Just 'assert(num); return ceill(log10(num));'?

[labath]

In D89283#2336280, @vsk wrote:

In D89283#2336120, @wallace wrote:

@vsk, I agree with you regarding the files. At the moment our implementation of intel-pt tracing doesn't support collecting a trace, but soon we'll do so. Then, we'll be able to generate these trace files on the fly as the tests run, so I imagine I'll be deleting these binary files. For the time being I doubt I'll include any new binary, as what is included is more than enough to test the basic decoding functionalities.

That seems promising. Deleting those binary files after the fact doesn't address the issue, though, as they'd be part of the history. I have a question about that ld-2.17.so file in particular: is there no way to decoder/traverse a trace of a process that loads a dylib, without copying all of ld.so into the source tree? That seems very surprising -- I'd expect the decoder API to allow you to skip right over PC ranges that have nothing to do with the binary you want to debug.

I would very much like to keep some tests which use pre-baked traces (binaries are a different matter). I have a couple of reasons for that:

• tests exercising the tracing code path require appropriate hardware. Checked in traces can run everywhere (assuming one can build the pt library there)
• due to their systemic nature, it will be hard to test various edge conditions (missing binaries, corrupt traces, ...) with a end-to-end test
• we had some issues with end-to-end tests being flaky due to the fact that cpu writes the traces asynchronously -- I am not sure we ever fully figured that out

Now, we're definitely going to need tests which check the tracing functionality, but I think that having these kinds of traces is definitely good. As for ld.so, while it's not the end of the world (we have much larger binaries), it would definitely be nice to avoid it. What functionality is that test exercising? If you want traces that cross multiple modules, maybe you could capture a trace from the middle of an application, after ld.so is done, and show how the application is ping-ponging between some functions in different shared libraries.

In D89283#2336090, @vsk wrote:

One possible alternative:

• Design a textual description for the raw trace contents, and possibly a way to convert an existing trace file into this textual format

I think that converting the textual trace description would essentially mean reimplementing the intel-pt library. That might be nice (llvm is definitely fond of reimplementing things), but I'm not sure if that's what these guys have signed up for.

• Check in assembly, and use llvm-mc/clang to generate executables during testing

This is definitely doable, though I'd probably go for yaml2obj, as llvm-mc&clang cannot guarantee the exact placement of instructions in memory. It looks like obj2yaml has grown program header support since last time I checked this (previously you had to write them by hand), so it may be that a plain obj2yaml | yaml2obj would just work now. (It probably won't produce a fully functional binary, but it might be close enough.)

lldb/include/lldb/Target/Trace.h

176–195

How about Optional<size_t> GetInstructionCount? That makes it less likely to develop an accidental dependancy on this interface (though I fear that might still happen without an subclass which actually returns None here).

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

32

raw_string_ostream would be more llvm-y (the std::hex part in particular is very non-idiomatic)

42

Do you want anyone to modify the vector? Return ArrayRef<IntelPTInstruction>

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

33

static

81

static

159

static_cast is enough here

179

I presume that the pt library does not actually modify this data. Maybe a short note saying that.

[labath]

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

189

It looks like this can only fail if the image argument is null (which can only happen if the decoder is null, which is checked). An assert would be enough for that. (For a proper error handling you should have also freed the decoder object on the error path, which is how i came to thing about this).

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

69

drop std::make_pair, it's cleaner.

94

this makes a copy, which you probably did not want.

98

i>=0 is always true. You'll have to do this trick with signed numbers (ssize_t?)

109

I'm having doubts about this "I have an thread but was not able to decode _anything_ about it" state is worth it. Having many different ways to report errors just increases the chance of something going wrong, and in TraverseInstructions you're already treating this state as a pseudo-instruction.

Maybe that representation should be used all the way down? Or (and this may be even better) we avoid creating such Threads in the first place (print an error/warning when the target is created).

lldb/source/Target/Trace.cpp

85

that would have to be log10(num+1), though I'm not sure what to thing of the floating point arithmetic...

222

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?

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

44–49

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).

[wallace]

@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.

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

44–49

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.

121–122

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.

[clayborg]

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

32

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.

42

yeah llvm::ArrayRef to avoid making copies is good.

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

33

Make static or add an anonymous namespace around all of these functions so you don't have to mark them all as static.

81

Make static or add an anonymous namespace around all of these functions so you don't have to mark them all as static.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

94

returning a llvm::ArrayRef to avoid the copy

98

Yes, switch to ssize_t, your delta is already signed. Also switch "delta" to ssize_t as well.

lldb/source/Target/Trace.cpp

222

Lots os signed/unsigned match issues possible. Best to make this rock solid.

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

49

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.

[wallace]

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

49

What about this:

We expose the indices in a chronologically increasing way, where [0] is the oldest instruction and [N] is the newest. Then we have the two options suggested by Greg

--from-end <offset>

Where offset is an index or the string "end", meaning the last instruction of the trace, in case the user doesn't know the index of it. Then the instructions are printed

[offset]
[offset - 1]
...
[offset - K]

And if there's a repeat command, this would be printed

[offset - K - 1]
[offset - K - 2]
...

Which would look nicely as a contiguous list of instructions if concatenated.

The other option would be

--from-start<offset>

Where offset is an index. Then the instructions are printed

[offset]
[offset + 1]
[offset + 2]
...
[offset + K]

And after a repeat command, you'd get

[offset + K + 1]
[offset + K + 2]
...

I think this would serve all purposes.

[wallace]

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

189

good catch!

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

98

TIL!

109

Maybe that representation should be used all the way down?

I'll follow that path. This will create consistency through the code

Or (and this may be even better) we avoid creating such Threads in the first place (print an error/warning when the target is created).

I wish I could do that, but decoding is very expensive and should be done lazily. According to Intel and my tests, if a thread was traced during T seconds, then decoding takes around 10T, which is a big amount of time if you were tracing 10 threads for 5 seconds, which would take 500 seconds to decode. At least for know we are not doing parallel decoding. I imagine at some point we'll have to work on that.

[wallace]

Addressed almost all changes.

What's left to discuss:

• I tried to use obj2yaml, but it creates files much larger than the binaries, so in terms of space, I'd rather keep the binaries. Besides, I removed ld.so, and the binaries that are left are tiny. I imagine that the binaries that will be committed in the future will also be tiny ones depicting specific edge cases.
• Showing the disassembly of the instructions, requested by Greg, is not yet done.
• I'm representing now all decoding errors as instructions, which in fact simplifies the API a good deal. However, I'm still not happy with the API, right now we have
  
  class Trace { void TraverseInstructions(Thread, callback, ...); int GetInstructionsCount(Thread...); }

Right now these methods are not doing anything (or returning 0 in the case of the instructions count) if the provided Thread is not traced by the Trace instance.

Besides, eventually we'll add methods like

vector<addr_t> GetBacktrace(Thread, position)

position ReverseNext(Thread, position)
position ReverseSingleStep(Thread, position)

We could leave the code as it is, which results in a simple API with some undefined behavior if an invalid Thread is passed.

Another option is to have

class Trace {
  TracedThreadSP GetTraceForThread(Thread ...); 
}

class TracedThread {
  void TraverseInstructions(callback, ...);
  int GetInstructionsCount()
 
  position ReverseNext(Thread, position)
  position ReverseSingleStep(Thread, position)
}

This creates a level of indirection that forces the caller to check if the returned TracedTrace is a valid pointer or not. If the pointer is invalid, the caller can show an error message or fail silently, otherwise, TraverseInstructions and GetInstructionsCount will perform valid operations.

This might become useful to avoid errors in complex scenarios as the API grows. But then, how often will someone invoke these methods with the wrong thread? And this TracedThread object would have to be passed around a lot if the developer doesn't want to if the thread is invalid in each callsite.

The first form of the API looks simpler without that level of indirection, which is cool. But the second one adds some boilerplate in each callsite, with some safety benefits.

What do you guys think?

[clayborg]

In D89283#2340586, @wallace wrote:

Addressed almost all changes.

What's left to discuss:

• I tried to use obj2yaml, but it creates files much larger than the binaries, so in terms of space, I'd rather keep the binaries. Besides, I removed ld.so, and the binaries that are left are tiny. I imagine that the binaries that will be committed in the future will also be tiny ones depicting specific edge cases.
• Showing the disassembly of the instructions, requested by Greg, is not yet done.
• I'm representing now all decoding errors as instructions, which in fact simplifies the API a good deal. However, I'm still not happy with the API, right now we have
  
  class Trace { void TraverseInstructions(Thread, callback, ...); int GetInstructionsCount(Thread...); }

Do we need to know the instruction count? Again, won't this be really expensive to calculate?

Right now these methods are not doing anything (or returning 0 in the case of the instructions count) if the provided Thread is not traced by the Trace instance.

If we get rid of

int Trace::GetInstructionsCount(Thread...);

Then the first call to TraverseInstructions can return an appropriate error to the callback in the Expected<addr_t> right?

Besides, eventually we'll add methods like

vector<addr_t> GetBacktrace(Thread, position)

We might want the thread to be able to produce real stack frames so we can re-use the current StackFrame classes. Each stack frame will probably only contain the PC. This call could be used to create the StackFrames in the trace thread class.

position ReverseNext(Thread, position)
position ReverseSingleStep(Thread, position)

We should be able to implement these in Trace.cpp and have it only use TraverseInstructions right? Do we also need ReverseContinue() to be able to backup until we hit a breakpoint or the start of the trace data?

We could leave the code as it is, which results in a simple API with some undefined behavior if an invalid Thread is passed.

I like the current API. Great if we can remove the Trace::GetInstructionsCount() API to keep things as simple as possible and allow us to lazily fetch instructions as needed as we eventually step and continue around.

If an invalid thread is passed, then TraverseInstructions can just return an error on the first callback. If we still need GetInstructionCount(), we can have it return 0 if the thread has no trace instructions.

Another option is to have

class Trace {
  TracedThreadSP GetTraceForThread(Thread ...); 
}

class TracedThread {
  void TraverseInstructions(callback, ...);
  int GetInstructionsCount()
 
  position ReverseNext(Thread, position)
  position ReverseSingleStep(Thread, position)
}

This creates a level of indirection that forces the caller to check if the returned TracedTrace is a valid pointer or not. If the pointer is invalid, the caller can show an error message or fail silently, otherwise, TraverseInstructions and GetInstructionsCount will perform valid operations.

I don't think we need this. I think we have enough API here to implement all of this stuff through the lldb_private::Thread class as it can call through to the Trace APIs with the thread class pointer.

This might become useful to avoid errors in complex scenarios as the API grows. But then, how often will someone invoke these methods with the wrong thread? And this TracedThread object would have to be passed around a lot if the developer doesn't want to if the thread is invalid in each callsite.

The first form of the API looks simpler without that level of indirection, which is cool. But the second one adds some boilerplate in each callsite, with some safety benefits.

What do you guys think?

Lets not do this extra indirection, I like the current API with or without GetInstructionsCount

[wallace]

Do we need to know the instruction count? Again, won't this be really expensive to calculate?

It is expensive, but today I realized that it might be necessary to decode the entire trace if you want things to work nicely.

Let's take, for example, a very common future use case: "you stop at a breakpoint, and then you do reverse-next and print the backtrace". Getting the backtrace requires decoding all the instructions, as potentially the first instruction in the trace corresponds to the oldest frame of the backtrace of your breakpoint position (which would be the case if you start tracing at main). You won't know all the frames of your backtrace unless you decode it all. In general, you can't have the backtrace of the i-th position unless you decode everything up to that position. And without the backtrace you really don't know where you are, it's hard for the user to make sense of the state of the program without it.

Also, if would be difficult to provide comprehensive position offsets to the user in the "dump instructions" command unless we also show the maximum index, which is equivalent to the instruction count.

Lets not do this extra indirection, I like the current API with or without GetInstructionsCount

That's reassuring. I'm also of that opinion.

I think we have enough API here to implement all of this stuff through the lldb_private::Thread class as it can call through to the Trace APIs with the thread class pointer.

Great!

If an invalid thread is passed, then TraverseInstructions can just return an error on the first callback. If we still need GetInstructionCount(), we can have it return 0 if the thread has no trace instructions.

Sure

We should be able to implement these in Trace.cpp and have it only use TraverseInstructions right? Do we also need ReverseContinue() to be able to backup until we hit a breakpoint or the start of the trace data?

Yes, definitely. We'll be adding more stuff to it like ReverseContinue.

[labath]

In D89283#2340586, @wallace wrote:

What's left to discuss:

• I tried to use obj2yaml, but it creates files much larger than the binaries, so in terms of space, I'd rather keep the binaries. Besides, I removed ld.so, and the binaries that are left are tiny. I imagine that the binaries that will be committed in the future will also be tiny ones depicting specific edge cases.

Well, yaml is text so it's not surprising that it'd be larger (though sometimes it can actually be smaller, because it e.g. omits padding, or because you can reduce by deleting irrelevant stuff; and git can store text diffs efficiently). But there's the other aspect that Vedant mentioned -- their opaqueness/reviewability. With a yaml file, one can see (directly in the review window or in his text editor) what kind input is the program being fed and correlate that with the expected output. This is not perfect because a lot of the details (e.g. the disassembly, and most importantly the trace file) is still obscured, but it's better than nothing. So if it works, I'd still go for the yaml option (and I have to send a big thank you to whoever implemented the program header support).

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

12

I guess this is not needed anymore (?)

lldb/source/Plugins/Trace/intel-pt/DecodedThread.h

67–69

Maybe better left for a separate patch, but if we're going to have loads of these objects, then the we'd better optimize its size.

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

33

Actually, the coding standards say anonymous namespaces should not be used for functions.

192

Can't have side-effects inside assertions.

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h

22–45

I don't see the value of this class -- it's only used once, and in a very ephemeral way. ThreadTraceDecoder::Decode could just call CreateDecoderAndDecode directly (and it'd be shorter for it).

67

please delete the copy assignment as well

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

109

If you can pull that off -- great. However, I have doubts (and judging by the other comments, you're starting to have some too) regarding how long will you be able to postpone the decoding. For example, lldb very much likes to know the PC value of each thread (can you really blame it?) -- so much that we've added a special field to the gdb-remote stop-reply packet which lists the pc values of all threads.

That leads me to believe that you'll need to decode at least the last block of each thread's trace very early on

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

49

Ah... this is tricky...

The concatenation aspect is nice, but I'm not sure it trumps the "earlier/higher text is older" intuition. Even if I'm analyzing backwards, I think I'd prefer seeing a discontinuous set of lists which go the "right way" instead of a single continuous list which goes "backwards". I.e. I think I'd find this:

6
7
8
9
10
(lldb)
1
2
3
4
5

easier to read than this:

10
9
8
7
6
(lldb)
5
4
3
2
1

However, I don't see myself using this anytime soon, so if you think the latter is the best way to represent this, then fine. The thing we choose here is not set in stone anyway, and we can re-examine this later...

121–122

This does beg the question of whether we shouldn't make the distinction even more obvious by breaking the sequence numbers in some way. I don't really have an answer to that question, though...

Some two level namespacing?

sequence 1:
[0]: 0x47
[1]: 0x48
error: no memory mapped at 0x42

sequence 2:
[0]: 0x147
...

[wallace]

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

109

Yes, I ended up understanding more of LLDB and it seems that it'll be as you describe. I'll try to limit the initial decoding to at most the PC of each thread, which would indeed be very beneficial, because we could catch early some critical errors. I'll do that early decoding in another diff.

[wallace]

Some updates, especially regarding the indexing and dump ordering, which I discussed at lenght with some coworkers:

• I've addressed most of the issues, except for the disassembly and the yaml object files, which I'll do tomorrow.
• I've changed the indexing to have [0] be the oldest instruction, and [N] the most recent one. This matches with most people's expectations on the order of lines in a text file.
• I've changed the instruction dump to be
  
  thread trace dump instructions --count <> --position <>
  
  And it prints in reverse order, for example

[10]
[11]
[12]
[13]
[14]

And after a repeat command, it prints

[5]
[6]
[7]
[8]
[9]

and so on

The common feedback I got is that this is the most intuitive way to understand the text.

• I've added a position member to DecodedThread, which in the future we'll move as we perform reverse debugging. For now, it's default value is the end of the trace. I'm using it for the instruction dump.
• Now when there is a sequence of errors, I'm not printing their indices and I'm adding a whitespace, like

[ 0] 0x40064f
[ 1] 0x400540
[ 2] 0x400546
[ 3] 0x40054b
[ 4] 0x400510
[ 5] 0x400516
error: no memory mapped at this address 0x7ffff7df1950
error: no memory mapped at this address 0x400516

[ 8] 0x400657
[ 9] 0x40065a
[10] 0x40065f

Also the feedback I got about this is that showing the index of the error can confuse some people, and that the whitespace makes it very clear that there's missing information and that the error is not inoffensive.

• I also made the requested small fixes here and there.

[wallace]

The diff is the now ready for review. There are a few updates, including some design decisions after some chats with Greg.

• Now the dump command includes disassembly information and symbol context information whenever relevant, e.g.

$ thread trace dump instructions --count 50

thread #1: tid = 815455, total instructions = 46
  a.out`main + 15 at main.cpp:10
    [ 0] 0x000000000040066f    callq  0x400540                  ; symbol stub for: foo()
  a.out`symbol stub for: foo()
    [ 1] 0x0000000000400540    jmpq   *0x200ae2(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 40
    [ 2] 0x0000000000400546    pushq  $0x2
    [ 3] 0x000000000040054b    jmp    0x400510
  a.out`(none)
    [ 4] 0x0000000000400510    pushq  0x200af2(%rip)            ; _GLOBAL_OFFSET_TABLE_ + 8
    [ 5] 0x0000000000400516    jmpq   *0x200af4(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 16
    [ 6] 0x00007ffff7df1950    error: no memory mapped at this address
    ...instructions missing
  a.out`main + 20 at main.cpp:10
    [ 7] 0x0000000000400674    movl   %eax, -0xc(%rbp)
  a.out`main + 23 at main.cpp:12
    [ 8] 0x0000000000400677    movl   -0xc(%rbp), %eax

This disassembly and symbol dumping also works for inline functions, as described in the tests.

• A flag for the command has been added (-r), which prints raw instruction addresses, similar to the one in the disassembly command, e.g.

$ thread trace dump instructions --raw

 thread #1: tid = 3842849, total instructions = 21
   [ 1] 0x0000000000400518    
   [ 2] 0x000000000040051f    
   [ 3] 0x0000000000400529    
   [ 4] 0x000000000040052d    
   [ 5] 0x0000000000400521    
   [ 6] 0x0000000000400525 

• I tried to use yaml to represent the binary fails, but it doesn't work. The yaml representation doesn't have all the required information and the decoder complaints about missing memory. At this point it's better to just include the binary and have strong tests.

• I'm leaving for a future diff restructuring the IntelPTInstruction class so that is more memory efficient. Something to keep in mind is that errors will be seldom, as they represent gaps in the trace, and most of the instructions will be correct addresses.

[labath]

I'm not sure if I'll have time to go through this again, but it seems ok. One question inline, though.

In D89283#2348815, @wallace wrote:

The diff is the now ready for review. There are a few updates, including some design decisions after some chats with Greg.

• Now the dump command includes disassembly information and symbol context information whenever relevant, e.g.

I should've said something earlier, but I think the decision to the actual disassembling in a separate patch was a good one. This patch was big enough as it was.

• I tried to use yaml to represent the binary fails, but it doesn't work. The yaml representation doesn't have all the required information and the decoder complaints about missing memory. At this point it's better to just include the binary and have strong tests.

That makes me sad, but I am not going to hold this patch over that. I would encourage you to find and implement the missing bits in yaml2obj though...

lldb/packages/Python/lldbsuite/test/lldbtest.py

2116

I think that we could just drop the len(cmd)==0 check. It's pretty unlikely that anyone will do that by accident (and have all of his checks will still pass)...

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

54–57

I don't see this used anywhere. And if it's not used, how is the dump command implemented?

(I'm guessing this is used to implement per-thread "last dumped instruction" positions. I'm not sure if that feature is worth it (the list command doesn't have that), but if that's the case maybe the name should also be more specific, as that position is unlikely to be useful for anything else.)

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

109

Sounds good.

I am slightly worried about the emphasis on sequential instruction numbers in this design. It seems like it'd be hard to avoid decoding the entire trace if one needs to assign a sequential id to each instruction. But let's see how it goes...

[wallace]

That makes me sad, but I am not going to hold this patch over that. I would encourage you to find and implement the missing bits in yaml2obj though...

I'll do that later as a way to learn yaml2obj.

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

54–57

I'm not using it in this patch, but I added it along with the GetCurrentPosition. I plan to use it for the reverse debugging case, in which a reverse-next would move this value to a different position, that would get picked by any subsequent dump or reverse command. I'll improve the documentation and the function name

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

109

I think that the actual problem is we can avoid decoding the entire trace in the first place. This will be unavoidable if we want to show backtraces, as the frames are scattered throughout the trace and there's no way to know where they are unless you decode it all. I don't know of any other efficient tracing mechanism that doesn't have this problem. When I implement the backtrace reconstruction we can have a much better picture of what's possible and what not.

[wallace]

Address issues.

[vsk]

@wallace thanks for winnowing the test objects. I left an inline suggestion about simplifying the error-handling in IntelPTInstruction. Other than that, mechanically this is looking good. Thanks!

lldb/source/Plugins/Trace/intel-pt/DecodedThread.h

37

Does this IntelPTInstruction constructor ever get called with a non-zero libipt error code?

41

It might be a bit cleaner to just have two IntelPTInstruction constructors: one that accepts a const pt_insn & and another that accepts an llvm::Error. To do that, you'd need to introduce a PT-specific ErrorInfo (class IntelPTError : public ErrorInfo<IntelPTError> { ... }). This can wrap some sort of generic error (as a std::string, perhaps), or a libipt-specific error.

It'd be a little more up front work, but the benefit is that it simplifies error handling (there's just one type of error, only one error value to check in IsError, etc).

[wallace]

Followed @vsk's suggestion. I didn't do exactly what he said, but I ended up creating my own IntelPTError to represent better the different kinds of errors and make sure we can create a correct llvm::Error when traversing the instructions.
The IntelPTInstruction code is much simpler now.

[wallace]

nits

[wallace]

friendly ping

[vsk]

lldb/source/Plugins/Trace/intel-pt/DecodedThread.h

27

Do we need a default constructor for this pure-virtual class?

37

Still wondering about this: why do we need to construct an IntelPTInstruction using a pt_insn as well as an error code?

117

Still doesn't feel like this is aligning with the design of llvm::Error - they're supposed to be unique and lightweight. Specifically:

• Why is the error instance shareable? Do multiple threads need to handle the same error?
• For that matter, why is IntelPTInstruction copyable?
• How much are we gaining by storing the libipt_error_code vs. storing a bool m_is_gap field?

Imo it seems a bit too complicated to have three different ways to construct an IntelPTInstruction with an error. Would be nice to just have one?

[wallace]

lldb/source/Plugins/Trace/intel-pt/DecodedThread.h

27

The compiler forces me to do it because I declare below a deleted copy constructor.

37

Indeed, there are cases in which the libipt decoder library fails to decode an instruction, but contains some information about it, like the address (among other info). Then you have a partially correct pt_insn object along with an error code

117

Will try to fix that now.

Definitely I shouldn't make it copyable, which will simplify the code.

Regarding the error, it's hard to receive an llvm::Error in the constructor, because every time the user iterates on each instruction, that Error will need to be returned, but Errors are not copyable, only movable. That's why I found it easier to create a custom error class that can create Error's on demand.

[wallace]

lldb/source/Plugins/Trace/intel-pt/DecodedThread.h

117

How much are we gaining by storing the libipt_error_code vs. storing a bool m_is_gap field?

Quite a lot. If we can report to the user why decoding failed, then the user can try to fix that accordingly. For example, if an address section is missing, we can report it and the user can provide that missing library and retry decoding.

[wallace]

Address changes.

• Made IntelPTError and IntelPTInstruction non-copyable, which simplified the code.
• I still keep IntelPTError as a lightweight way to create Error objects on demand when traversing the trace. Notice that Errors are only movable, so I can't use the Error itself as storage.
• I removed one error constructor and now there're only two, with improved documentation.

[labath]

A more light-weight approach to this would be to store the ErrorInfo object from the Error and copy that when needed. Something like:

class IntelPTInstruction {
  std::unique_ptr<ErrorInfoBase> m_error; // Technically, this should be a vector, but we don't really make use of the error list functionality anywhere.
  
  IntelPTInstruction(Error err) {
    handleAllErrors(std::move(err), [&](std::unique_ptr<ErrorInfoBase> info) { m_error = std::move(info); }); // Stash it for later use
  }

  Error ToError() {
    if (m_error->isA<IntelPTError>())
      return make_error<IntelPTError>(static_cast<IntelPTError&>(*m_error)); // IntelPTError is copyable
    return make_error<StringError>(m_error->message(), m_error->convertToErrorCode()); // Just copy the error message
  }
};

But maybe there are other solutions too. Adding @lhames, in case he knows about any...

[wallace]

Followed the suggestion by @labath. The code definitely looks better.

[labath]

I'd say this looks ok now. Another advantage of doing the decoding early is that we could then simplify the IntelPTInstruction class. If we handle the early decoding errors (file not found, etc.) early, then the only kind of errors this class could contain are errors from the pt library -- and in that case we could just store them as an int instead of this ErrorInfoBase business.

[wallace]

Thanks. I'm thinking about doing the early decoding, as doing "trace load" right now doesn't do anything very useful unless you do "dump instructions". However, I'll try to do it once I finish implementing decoding for live processes, because I want to have a unified process for both, and live process tracing should be lazy to avoid stopping the user flow for too long.

[clayborg]

I am good too.

lldb/include/lldb/Core/Disassembler.h

public:
   ~InstructionList();
 
   size_t GetSize() const;
 
   uint32_t GetMaxOpcocdeByteSize() const;
 
   lldb::InstructionSP GetInstructionAtIndex(size_t idx) const;
 
+  /// Get the instruction at the given address.
+  ///
+  /// \return
+  ///    A valid \a InstructionSP if the address could be found, or null
+  ///    otherwise.
+  lldb::InstructionSP GetInstructionAtAddress(const Address &addr);
+
   //------------------------------------------------------------------
   /// Get the index of the next branch instruction.
   ///
   /// Given a list of instructions, find the next branch instruction
   /// in the list by returning an index.
   ///
   /// @param[in] start
   ///     The instruction index of the first instruction to check.

lldb/include/lldb/Symbol/SymbolContext.h

public:
   ///     make the "function name" too long to include all the argument
   ///     types.
   ///
   /// \param[in] show_function_name
   ///     Normally this should be true - the function/symbol name should
   ///     be printed.  In disassembly formatting, where we want a format
   ///     like "<*+36>", this should be false and "*" will be printed
   ///     instead.
+  ///
+  /// \param[in] show_inline_callsite_line_info
+  ///     When processing an inline block, the line info of the callsite
+  ///     is dumped if this flag is \b true, otherwise the line info
+  ///     of the actual inlined function is dumped.
+  ///
+  /// \return
+  ///     \b true if some text was dumped, \b false otherwise.
   bool DumpStopContext(Stream *s, ExecutionContextScope *exe_scope,
                        const Address &so_addr, bool show_fullpaths,
                        bool show_module, bool show_inlined_frames,
-                       bool show_function_arguments,
-                       bool show_function_name) const;
+                       bool show_function_arguments, bool show_function_name,
+                       bool show_inline_callsite_line_info = true) const;
 
   /// Get the address range contained within a symbol context.
   ///
   /// Address range priority is as follows:
   ///     - line_entry address range if line_entry is valid and
   ///     eSymbolContextLineEntry is set in \a scope
   ///     - block address range if block is not nullptr and eSymbolContextBlock
   ///     is set in \a scope

lldb/include/lldb/Target/Trace.h

 /// when loading core files.
 ///
 /// Processor trace information can also be fetched through the process
 /// interfaces during a live debug session if your process supports gathering
 /// this information.
 class Trace : public PluginInterface,
               public std::enable_shared_from_this<Trace> {
 public:
+  enum class TraceDirection {
+    Forwards = 0,
+    Backwards,
+  };
+
   /// Dump the trace data that this plug-in has access to.
   ///
   /// This function will dump all of the trace data for all threads in a user
   /// readable format. Options for dumping can be added as this API is iterated
   /// on.
   ///
   /// \param[in] s
   ///     A stream object to dump the information to.
public:
   ///     Name of the trace plugin.
   static llvm::Expected<llvm::StringRef>
   FindPluginSchema(llvm::StringRef plugin_name);
 
   /// \return
   ///     The JSON schema of this Trace plug-in.
   virtual llvm::StringRef GetSchema() = 0;
 
-  /// Dump \a count instructions of the given thread's \a Trace starting at the
-  /// \a start_position position in reverse order.
+  /// Each decoded thread contains a cursor to the current position the user is
+  /// stopped at. When reverse debugging, each operation like reverse-next or
+  /// reverse-continue will move this cursor, which is then picked by any
+  /// subsequent dump or reverse operation.
+  ///
+  /// The initial position for this cursor is the last element of the thread,
+  /// which is the most recent chronologically.
   ///
-  /// The instructions are indexed in reverse order, which means that the \a
-  /// start_position 0 represents the last instruction of the trace
-  /// chronologically.
+  /// \return
+  ///     The current position of the thread's trace or \b 0 if empty.
+  virtual size_t GetCursorPosition(const Thread &thread) = 0;
+
+  /// Dump \a count instructions of the given thread's trace ending at the
+  /// given \a end_position position.
+  ///
+  /// The instructions are printed along with their indices or positions, which
+  /// are increasing chronologically. This means that the \a index 0 represents
+  /// the oldest instruction of the trace chronologically.
   ///
   /// \param[in] thread
   ///     The thread whose trace will be dumped.
   ///
   /// \param[in] s
   ///     The stream object where the instructions are printed.
   ///
   /// \param[in] count
   ///     The number of instructions to print.
   ///
-  /// \param[in] start_position
-  ///     The position of the first instruction to print.
+  /// \param[in] end_position
+  ///     The position of the last instruction to print.
+  ///
+  /// \param[in] raw
+  ///     Dump only instruction addresses without disassembly nor symbol
+  ///     information.
   void DumpTraceInstructions(Thread &thread, Stream &s, size_t count,
-                             size_t start_position) const;
+                             size_t end_position, bool raw);
+
+  /// Run the provided callback on the instructions of the trace of the given
+  /// thread.
+  ///
+  /// The instructions will be traversed starting at the given \a position
+  /// sequentially until the callback returns \b false, in which case no more
+  /// instructions are inspected.
+  ///
+  /// The purpose of this method is to allow inspecting traced instructions
+  /// without exposing the internal representation of how they are stored on
+  /// memory.
+  ///
+  /// \param[in] thread
+  ///     The thread whose trace will be traversed.
+  ///
+  /// \param[in] position
+  ///     The instruction position to start iterating on.
+  ///
+  /// \param[in] direction
+  ///     If \b TraceDirection::Forwards, then then instructions will be
+  ///     traversed forwards chronologically, i.e. with incrementing indices. If
+  ///     \b TraceDirection::Backwards, the traversal is done backwards
+  ///     chronologically, i.e. with decrementing indices.
+  ///
+  /// \param[in] callback
+  ///     The callback to execute on each instruction. If it returns \b false,
+  ///     the iteration stops.
+  virtual void TraverseInstructions(
+      const Thread &thread, size_t position, TraceDirection direction,
+      std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_addr)>
+          callback) = 0;
+
+  /// Get the number of available instructions in the trace of the given thread.
+  ///
+  /// \param[in] thread
+  ///     The thread whose trace will be inspected.
+  ///
+  /// \return
+  ///     The total number of instructions in the trace.
+  virtual size_t GetInstructionCount(const Thread &thread) = 0;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TRACE_H

lldb/packages/Python/lldbsuite/test/lldbtest.py

def switch_to_thread_with_stop_reason(self, stop_reason):
                 self.runCmd('thread select %s' % matched.group(1))
 
     def runCmd(self, cmd, msg=None, check=True, trace=False, inHistory=False):
         """
         Ask the command interpreter to handle the command and then check its
         return status.
         """
         # Fail fast if 'cmd' is not meaningful.
-        if not cmd or len(cmd) == 0:
+        if cmd is None:
             raise Exception("Bad 'cmd' parameter encountered")
 
         trace = (True if traceAlways else trace)
 
         if cmd.startswith("target create "):
             cmd = cmd.replace("target create ", "file ")
 
         running = (cmd.startswith("run") or cmd.startswith("process launch"))

lldb/source/Commands/CommandObjectThread.cpp

Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
             count < 0)
           error.SetErrorStringWithFormat(
               "invalid integer value for option '%s'",
               option_arg.str().c_str());
         else
           m_count = count;
         break;
       }
-      case 's': {
-        int32_t start_position;
-        if (option_arg.empty() || option_arg.getAsInteger(0, start_position) ||
-            start_position < 0)
+      case 'p': {
+        int32_t position;
+        if (option_arg.empty() || option_arg.getAsInteger(0, position) ||
+            position < 0)
           error.SetErrorStringWithFormat(
               "invalid integer value for option '%s'",
               option_arg.str().c_str());
         else
-          m_start_position = start_position;
+          m_position = position;
+        break;
+      }
+      case 'r': {
+        m_raw = true;
         break;
       }
       default:
         llvm_unreachable("Unimplemented option");
       }
       return error;
     }
 
     void OptionParsingStarting(ExecutionContext *execution_context) override {
       m_count = kDefaultCount;
-      m_start_position = kDefaultStartPosition;
+      m_position = llvm::None;
+      m_raw = false;
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_thread_trace_dump_instructions_options);
     }
 
-    static const uint32_t kDefaultCount = 20;
-    static const uint32_t kDefaultStartPosition = 0;
+    static const size_t kDefaultCount = 20;
 
     // Instance variables to hold the values for command options.
-    uint32_t m_count;
-    uint32_t m_start_position;
+    size_t m_count;
+    llvm::Optional<ssize_t> m_position;
+    bool m_raw;
   };
 
   CommandObjectTraceDumpInstructions(CommandInterpreter &interpreter)
       : CommandObjectIterateOverThreads(
             interpreter, "thread trace dump instructions",
             "Dump the traced instructions for one or more threads.  If no "
             "threads are specified, show the current thread.  Use the "
             "thread-index \"all\" to see all threads.",
             nullptr,
             eCommandRequiresProcess | eCommandTryTargetAPILock |
                 eCommandProcessMustBeLaunched | eCommandProcessMustBePaused),
         m_options(), m_create_repeat_command_just_invoked(false) {}
 
   ~CommandObjectTraceDumpInstructions() override = default;
 
   Options *GetOptions() override { return &m_options; }
 
   const char *GetRepeatCommand(Args &current_command_args,
                                uint32_t index) override {
     current_command_args.GetCommandString(m_repeat_command);
     m_create_repeat_command_just_invoked = true;
+    m_consecutive_repetitions = 0;
     return m_repeat_command.c_str();
   }
 
 protected:
   bool DoExecute(Args &args, CommandReturnObject &result) override {
+    if (IsRepeatCommand())
+      m_consecutive_repetitions++;
     bool status = CommandObjectIterateOverThreads::DoExecute(args, result);
-    PrepareRepeatArguments();
-    return status;
-  }
 
-  void PrepareRepeatArguments() {
-    m_repeat_start_position = m_options.m_count + GetStartPosition();
     m_create_repeat_command_just_invoked = false;
+    return status;
   }
 
   bool IsRepeatCommand() {
     return !m_repeat_command.empty() && !m_create_repeat_command_just_invoked;
   }
 
-  uint32_t GetStartPosition() {
-    return IsRepeatCommand() ? m_repeat_start_position
-                             : m_options.m_start_position;
-  }
-
   bool HandleOneThread(lldb::tid_t tid, CommandReturnObject &result) override {
     const TraceSP &trace_sp = m_exe_ctx.GetTargetSP()->GetTrace();
     if (!trace_sp) {
       result.SetError("error: this thread is not being traced");
       return false;
     }
 
     ThreadSP thread_sp =
         m_exe_ctx.GetProcessPtr()->GetThreadList().FindThreadByID(tid);
 
+    size_t count = m_options.m_count;
+    ssize_t position = m_options.m_position.getValueOr(
+                           trace_sp->GetCursorPosition(*thread_sp)) -
+                       m_consecutive_repetitions * count;
+    if (position < 0)
+      result.SetError("error: no more data");
+    else
       trace_sp->DumpTraceInstructions(*thread_sp, result.GetOutputStream(),
-                                    m_options.m_count, GetStartPosition());
+                                      count, position, m_options.m_raw);
     return true;
   }
 
   CommandOptions m_options;
 
   // Repeat command helpers
   std::string m_repeat_command;
   bool m_create_repeat_command_just_invoked;
-  uint32_t m_repeat_start_position;
+  size_t m_consecutive_repetitions = 0;
 };
 
 // CommandObjectMultiwordTraceDump
 class CommandObjectMultiwordTraceDump : public CommandObjectMultiword {
 public:
   CommandObjectMultiwordTraceDump(CommandInterpreter &interpreter)
       : CommandObjectMultiword(
             interpreter, "dump",

lldb/source/Commands/Options.td

def thread_plan_list_thread_id : Option<"thread-id", "t">, Group<1>,
     "specified more than once.">;
   def thread_plan_list_unreported : Option<"unreported", "u">, Group<1>,
     Desc<"Display thread plans for unreported threads">;
 }
 
 let Command = "thread trace dump instructions" in {
   def thread_trace_dump_instructions_count : Option<"count", "c">, Group<1>,
     Arg<"Count">,
-    Desc<"The number of instructions to display starting at the current "
-    "position in reverse order chronologically.">;
-  def thread_trace_dump_instructions_start_position:
-    Option<"start-position", "s">,
+    Desc<"The number of instructions to display ending at the current position.">;
+  def thread_trace_dump_instructions_position : Option<"position", "p">,
     Group<1>,
     Arg<"Index">,
-    Desc<"The position of the first instruction to print. Defaults to the "
-    "current position, i.e. where the thread is stopped. The instructions are "
-    "indexed in reverse order, which means that a start position of 0 refers "
-    "to the last instruction chronologically.">;
+    Desc<"The position to use instead of the current position of the trace.">;
+  def thread_trace_dump_instructions_raw : Option<"raw", "r">,
+    Group<1>,
+    Desc<"Dump only instruction address without disassembly nor symbol information.">;
 }
 
 let Command = "type summary add" in {
   def type_summary_add_category : Option<"category", "w">, Arg<"Name">,
     Desc<"Add this to the given category instead of the default one.">;
   def type_summary_add_cascade : Option<"cascade", "C">, Arg<"Boolean">,
     Desc<"If true, cascade through typedef chains.">;
   def type_summary_add_no_value : Option<"no-value", "v">,

lldb/source/Core/Disassembler.cpp

 
 InstructionSP InstructionList::GetInstructionAtIndex(size_t idx) const {
   InstructionSP inst_sp;
   if (idx < m_instructions.size())
     inst_sp = m_instructions[idx];
   return inst_sp;
 }
 
+InstructionSP InstructionList::GetInstructionAtAddress(const Address &address) {
+  uint32_t index = GetIndexOfInstructionAtAddress(address);
+  if (index != UINT32_MAX)
+    return GetInstructionAtIndex(index);
+  return nullptr;
+}
+
 void InstructionList::Dump(Stream *s, bool show_address, bool show_bytes,
                            const ExecutionContext *exe_ctx) {
   const uint32_t max_opcode_byte_size = GetMaxOpcocdeByteSize();
   collection::const_iterator pos, begin, end;
 
   const FormatEntity::Entry *disassembly_format = nullptr;
   FormatEntity::Entry format;
   if (exe_ctx && exe_ctx->HasTargetScope()) {

lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt

 if (NOT LIBIPT_INCLUDE_PATH)
   message (FATAL_ERROR "libipt include path not provided")
 endif()
 
 if (NOT EXISTS "${LIBIPT_INCLUDE_PATH}")
   message (FATAL_ERROR "invalid libipt include path provided")
 endif()
 include_directories(${LIBIPT_INCLUDE_PATH})
 
 find_library(LIBIPT_LIBRARY ipt PATHS ${LIBIPT_LIBRARY_PATH} REQUIRED)
 
 add_lldb_library(lldbPluginTraceIntelPT PLUGIN
+  DecodedThread.cpp
+  IntelPTDecoder.cpp
   TraceIntelPT.cpp
   TraceIntelPTSessionFileParser.cpp
 
   LINK_LIBS
     lldbCore
     lldbSymbol
     lldbTarget
     ${LIBIPT_LIBRARY}
   LINK_COMPONENTS
     Support
   )

lldb/source/Plugins/Trace/intel-pt/DecodedThread.h

+//===-- DecodedThread.h -----------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODEDTHREAD_H
+#define LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODEDTHREAD_H
+
+#include <vector>
+
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
+#include "lldb/Target/Trace.h"
+
+#include "intel-pt.h"
+
+namespace lldb_private {
+namespace trace_intel_pt {
+
+/// Class for representing a libipt decoding error.
+class IntelPTError : public llvm::ErrorInfo<IntelPTError> {
+public:
+  static char ID;
+
+  /// \param[in] libipt_error_code
+  ///     Negative number returned by libipt when decoding the trace and
+  ///     signaling errors.
+  ///
+  /// \param[in] address
+  ///     Optional instruction address. When decoding an individual instruction,
+  ///     its address might be available in the \a pt_insn object, and should be
+  ///     passed to this constructor. Other errors don't have an associated
+  ///     address.
+  IntelPTError(int libipt_error_code,
+               lldb::addr_t address = LLDB_INVALID_ADDRESS);
+
+  std::error_code convertToErrorCode() const override {
+    return llvm::errc::not_supported;
+  }
+
+  void log(llvm::raw_ostream &OS) const override;
+
+private:
+  int m_libipt_error_code;
+  lldb::addr_t m_address;
+};
+
+/// \class IntelPTInstruction
+/// An instruction obtained from decoding a trace. It is either an actual
+/// instruction or an error indicating a gap in the trace.
+///
+/// Gaps in the trace can come in a few flavors:
+///   - tracing gaps (e.g. tracing was paused and then resumed)
+///   - tracing errors (e.g. buffer overflow)
+///   - decoding errors (e.g. some memory region couldn't be decoded)
+/// As mentioned, any gap is represented as an error in this class.
+class IntelPTInstruction {
+public:
+  IntelPTInstruction(const pt_insn &pt_insn) : m_pt_insn(pt_insn) {}
+
+  /// Error constructor
+  ///
+  /// libipt errors should use the underlying \a IntelPTError class.
+  IntelPTInstruction(llvm::Error err) {
+    llvm::handleAllErrors(std::move(err),
+                          [&](std::unique_ptr<llvm::ErrorInfoBase> info) {
+                            m_error = std::move(info);
+                          });
+  }
+
+  /// Check if this object represents an error (i.e. a gap).
+  ///
+  /// \return
+  ///     Whether this object represents an error.
+  bool IsError() const;
+
+  /// \return
+  ///     The instruction pointer address, or an \a llvm::Error if it is an
+  ///     error.
+  llvm::Expected<lldb::addr_t> GetLoadAddress() const;
+
+  /// \return
+  ///     An \a llvm::Error object if this class corresponds to an Error, or an
+  ///     \a llvm::Error::success otherwise.
+  llvm::Error ToError() const;
+
+  IntelPTInstruction(IntelPTInstruction &&other) = default;
+
+private:
+  IntelPTInstruction(const IntelPTInstruction &other) = delete;
+  const IntelPTInstruction &operator=(const IntelPTInstruction &other) = delete;
+
+  pt_insn m_pt_insn;
+  std::unique_ptr<llvm::ErrorInfoBase> m_error;
+};
+
+/// \class DecodedThread
+/// Class holding the instructions and function call hierarchy obtained from
+/// decoding a trace, as well as a position cursor used when reverse debugging
+/// the trace.
+///
+/// Each decoded thread contains a cursor to the current position the user is
+/// stopped at. See \a Trace::GetCursorPosition for more information.
+class DecodedThread {
+public:
+  DecodedThread(std::vector<IntelPTInstruction> &&instructions)
+      : m_instructions(std::move(instructions)), m_position(GetLastPosition()) {
+  }
+
+  /// Get the instructions from the decoded trace. Some of them might indicate
+  /// errors (i.e. gaps) in the trace.
+  ///
+  /// \return
+  ///   The instructions of the trace.
+  llvm::ArrayRef<IntelPTInstruction> GetInstructions() const;
+
+  /// \return
+  ///   The current position of the cursor of this trace, or 0 if there are no
+  ///   instructions.
+  size_t GetCursorPosition() const;
+
+  /// Change the position of the cursor of this trace. If this value is to high,
+  /// the new position will be set as the last instruction of the trace.
+  ///
+  /// \return
+  ///     The effective new position.
+  size_t SetCursorPosition(size_t new_position);
+  /// \}
+
+private:
+  /// \return
+  ///     The index of the last element of the trace, or 0 if empty.
+  size_t GetLastPosition() const;
+
+  std::vector<IntelPTInstruction> m_instructions;
+  size_t m_position;
+};
+
+} // namespace trace_intel_pt
+} // namespace lldb_private
+
+#endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODEDTHREAD_H

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

+//===-- DecodedThread.cpp -------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DecodedThread.h"
+
+#include "lldb/Utility/StreamString.h"
+
+using namespace lldb_private;
+using namespace lldb_private::trace_intel_pt;
+using namespace llvm;
+
+char IntelPTError::ID;
+
+IntelPTError::IntelPTError(int libipt_error_code, lldb::addr_t address)
+    : m_libipt_error_code(libipt_error_code), m_address(address) {
+  assert(libipt_error_code < 0);
+}
+
+void IntelPTError::log(llvm::raw_ostream &OS) const {
+  const char *libipt_error_message = pt_errstr(pt_errcode(m_libipt_error_code));
+  if (m_address != LLDB_INVALID_ADDRESS && m_address > 0) {
+    write_hex(OS, m_address, HexPrintStyle::PrefixLower, 18);
+    OS << "    ";
+  }
+  OS << "error: " << libipt_error_message;
+}
+
+bool IntelPTInstruction::IsError() const { return (bool)m_error; }
+
+Expected<lldb::addr_t> IntelPTInstruction::GetLoadAddress() const {
+  if (IsError())
+    return ToError();
+  return m_pt_insn.ip;
+}
+
+Error IntelPTInstruction::ToError() const {
+  if (!IsError())
+    return Error::success();
+
+  if (m_error->isA<IntelPTError>())
+    return make_error<IntelPTError>(static_cast<IntelPTError &>(*m_error));
+  return make_error<StringError>(m_error->message(),
+                                 m_error->convertToErrorCode());
+}
+
+size_t DecodedThread::GetLastPosition() const {
+  return m_instructions.empty() ? 0 : m_instructions.size() - 1;
+}
+
+ArrayRef<IntelPTInstruction> DecodedThread::GetInstructions() const {
+  return makeArrayRef(m_instructions);
+}
+
+size_t DecodedThread::GetCursorPosition() const { return m_position; }
+
+size_t DecodedThread::SetCursorPosition(size_t new_position) {
+  m_position = std::min(new_position, GetLastPosition());
+  return m_position;
+}

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h

+//===-- IntelPTDecoder.h --======--------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODER_H
+#define LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODER_H
+
+#include "intel-pt.h"
+
+#include "DecodedThread.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Utility/FileSpec.h"
+
+namespace lldb_private {
+namespace trace_intel_pt {
+
+/// \a lldb_private::ThreadTrace decoder that stores the output from decoding,
+/// avoiding recomputations, as decoding is expensive.
+class ThreadTraceDecoder {
+public:
+  /// \param[in] trace_thread
+  ///     The thread whose trace file will be decoded.
+  ///
+  /// \param[in] pt_cpu
+  ///     The libipt cpu used when recording the trace.
+  ThreadTraceDecoder(const std::shared_ptr<ThreadTrace> &trace_thread,
+                     const pt_cpu &pt_cpu)
+      : m_trace_thread(trace_thread), m_pt_cpu(pt_cpu), m_decoded_thread() {}
+
+  /// Decode the thread and store the result internally.
+  ///
+  /// \return
+  ///     A \a DecodedThread instance.
+  const DecodedThread &Decode();
+
+private:
+  ThreadTraceDecoder(const ThreadTraceDecoder &other) = delete;
+  ThreadTraceDecoder &operator=(const ThreadTraceDecoder &other) = delete;
+
+  std::shared_ptr<ThreadTrace> m_trace_thread;
+  pt_cpu m_pt_cpu;
+  llvm::Optional<DecodedThread> m_decoded_thread;
+};
+
+} // namespace trace_intel_pt
+} // namespace lldb_private
+
+#endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODER_H

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

+//===-- IntelPTDecoder.cpp --------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "IntelPTDecoder.h"
+
+#include "llvm/Support/MemoryBuffer.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Core/Section.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Target/ThreadTrace.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::trace_intel_pt;
+using namespace llvm;
+
+/// Move the decoder forward to the next synchronization point (i.e. next PSB
+/// packet).
+///
+/// Once the decoder is at that sync. point, it can start decoding instructions.
+///
+/// \return
+///   A negative number with the libipt error if we couldn't synchronize.
+///   Otherwise, a positive number with the synchronization status will be
+///   returned.
+static int FindNextSynchronizationPoint(pt_insn_decoder &decoder) {
+  // Try to sync the decoder. If it fails, then get
+  // the decoder_offset and try to sync again from
+  // the next synchronization point. If the
+  // new_decoder_offset is same as decoder_offset
+  // then we can't move to the next synchronization
+  // point. Otherwise, keep resyncing until either
+  // end of trace stream (eos) is reached or
+  // pt_insn_sync_forward() passes.
+  int errcode = pt_insn_sync_forward(&decoder);
+
+  if (errcode != -pte_eos && errcode < 0) {
+    uint64_t decoder_offset = 0;
+    int errcode_off = pt_insn_get_offset(&decoder, &decoder_offset);
+    if (errcode_off >= 0) { // we could get the offset
+      while (true) {
+        errcode = pt_insn_sync_forward(&decoder);
+        if (errcode >= 0 || errcode == -pte_eos)
+          break;
+
+        uint64_t new_decoder_offset = 0;
+        errcode_off = pt_insn_get_offset(&decoder, &new_decoder_offset);
+        if (errcode_off < 0)
+          break; // We can't further synchronize.
+        else if (new_decoder_offset <= decoder_offset) {
+          // We tried resyncing the decoder and
+          // decoder didn't make any progress because
+          // the offset didn't change. We will not
+          // make any progress further. Hence,
+          // stopping in this situation.
+          break;
+        }
+        // We'll try again starting from a new offset.
+        decoder_offset = new_decoder_offset;
+      }
+    }
+  }
+
+  return errcode;
+}
+
+/// Before querying instructions, we need to query the events associated that
+/// instruction e.g. timing events like ptev_tick, or paging events like
+/// ptev_paging.
+///
+/// \return
+///   0 if there were no errors processing the events, or a negative libipt
+///   error code in case of errors.
+static int ProcessPTEvents(pt_insn_decoder &decoder, int errcode) {
+  while (errcode & pts_event_pending) {
+    pt_event event;
+    errcode = pt_insn_event(&decoder, &event, sizeof(event));
+    if (errcode < 0)
+      return errcode;
+  }
+  return 0;
+};
+
+/// Decode all the instructions from a configured decoder.
+/// The decoding flow is based on
+/// https://github.com/intel/libipt/blob/master/doc/howto_libipt.md#the-instruction-flow-decode-loop
+/// but with some relaxation to allow for gaps in the trace.
+///
+/// Error codes returned by libipt while decoding are:
+/// - negative: actual errors
+/// - positive or zero: not an error, but a list of bits signaling the status of
+/// the decoder
+///
+/// \param[in] decoder
+///   A configured libipt \a pt_insn_decoder.
+///
+/// \return
+///   The decoded instructions.
+static std::vector<IntelPTInstruction>
+DecodeInstructions(pt_insn_decoder &decoder) {
+  std::vector<IntelPTInstruction> instructions;
+
+  while (true) {
+    int errcode = FindNextSynchronizationPoint(decoder);
+    if (errcode == -pte_eos)
+      break;
+
+    if (errcode < 0) {
+      instructions.emplace_back(make_error<IntelPTError>(errcode));
+      break;
+    }
+
+    // We have synchronized, so we can start decoding
+    // instructions and events.
+    while (true) {
+      errcode = ProcessPTEvents(decoder, errcode);
+      if (errcode < 0) {
+        instructions.emplace_back(make_error<IntelPTError>(errcode));
+        break;
+      }
+      pt_insn insn;
+
+      errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
+      if (errcode == -pte_eos)
+        break;
+
+      if (errcode < 0) {
+        instructions.emplace_back(make_error<IntelPTError>(errcode, insn.ip));
+        break;
+      }
+
+      instructions.emplace_back(insn);
+    }
+  }
+
+  return instructions;
+}
+
+/// Callback used by libipt for reading the process memory.
+///
+/// More information can be found in
+/// https://github.com/intel/libipt/blob/master/doc/man/pt_image_set_callback.3.md
+static int ReadProcessMemory(uint8_t *buffer, size_t size,
+                             const pt_asid * /* unused */, uint64_t pc,
+                             void *context) {
+  Process *process = static_cast<Process *>(context);
+
+  Status error;
+  int bytes_read = process->ReadMemory(pc, buffer, size, error);
+  if (error.Fail())
+    return -pte_nomap;
+  return bytes_read;
+}
+
+static std::vector<IntelPTInstruction> makeInstructionListFromError(Error err) {
+  std::vector<IntelPTInstruction> instructions;
+  instructions.emplace_back(std::move(err));
+  return instructions;
+}
+
+static std::vector<IntelPTInstruction>
+CreateDecoderAndDecode(Process &process, const pt_cpu &pt_cpu,
+                       const FileSpec &trace_file) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> trace_or_error =
+      MemoryBuffer::getFile(trace_file.GetPath());
+  if (std::error_code err = trace_or_error.getError())
+    return makeInstructionListFromError(errorCodeToError(err));
+
+  MemoryBuffer &trace = **trace_or_error;
+
+  pt_config config;
+  pt_config_init(&config);
+  config.cpu = pt_cpu;
+
+  if (int errcode = pt_cpu_errata(&config.errata, &config.cpu))
+    return makeInstructionListFromError(make_error<IntelPTError>(errcode));
+
+  // The libipt library does not modify the trace buffer, hence the following
+  // cast is safe.
+  config.begin =
+      reinterpret_cast<uint8_t *>(const_cast<char *>(trace.getBufferStart()));
+  config.end =
+      reinterpret_cast<uint8_t *>(const_cast<char *>(trace.getBufferEnd()));
+
+  pt_insn_decoder *decoder = pt_insn_alloc_decoder(&config);
+  if (!decoder)
+    return makeInstructionListFromError(make_error<IntelPTError>(-pte_nomem));
+
+  pt_image *image = pt_insn_get_image(decoder);
+
+  int errcode = pt_image_set_callback(image, ReadProcessMemory, &process);
+  assert(errcode == 0);
+  (void)errcode;
+
+  std::vector<IntelPTInstruction> instructions = DecodeInstructions(*decoder);
+
+  pt_insn_free_decoder(decoder);
+  return instructions;
+}
+
+const DecodedThread &ThreadTraceDecoder::Decode() {
+  if (!m_decoded_thread.hasValue()) {
+    m_decoded_thread = DecodedThread(
+        CreateDecoderAndDecode(*m_trace_thread->GetProcess(), m_pt_cpu,
+                               m_trace_thread->GetTraceFile()));
+  }
+
+  return *m_decoded_thread;
+}

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h

 //===-- TraceIntelPT.h ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_TRACEINTELPT_H
 #define LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_TRACEINTELPT_H
 
-#include "intel-pt.h"
-#include "llvm/ADT/Optional.h"
-
+#include "IntelPTDecoder.h"
 #include "TraceIntelPTSessionFileParser.h"
-#include "lldb/Target/Trace.h"
-#include "lldb/lldb-private.h"
 
 namespace lldb_private {
 namespace trace_intel_pt {
 
 class TraceIntelPT : public Trace {
 public:
   void Dump(Stream *s) const override;
 
public:
 
   static ConstString GetPluginNameStatic();
 
   uint32_t GetPluginVersion() override;
   /// \}
 
   llvm::StringRef GetSchema() override;
 
+  void TraverseInstructions(
+      const Thread &thread, size_t position, TraceDirection direction,
+      std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_addr)>
+          callback) override;
+
+  size_t GetInstructionCount(const Thread &thread) override;
+
+  size_t GetCursorPosition(const Thread &thread) override;
+
 private:
   friend class TraceIntelPTSessionFileParser;
 
   /// \param[in] trace_threads
   ///     ThreadTrace instances, which are not live-processes and whose trace
   ///     files are fixed.
   TraceIntelPT(const pt_cpu &pt_cpu,
                const std::vector<std::shared_ptr<ThreadTrace>> &traced_threads);
 
+  /// Decode the trace of the given thread that, i.e. recontruct the traced
+  /// instructions. That trace must be managed by this class.
+  ///
+  /// \param[in] thread
+  ///     If \a thread is a \a ThreadTrace, then its internal trace file will be
+  ///     decoded. Live threads are not currently supported.
+  ///
+  /// \return
+  ///     A \a DecodedThread instance if decoding was successful, or a \b
+  ///     nullptr if the thread's trace is not managed by this class.
+  const DecodedThread *Decode(const Thread &thread);
+
   pt_cpu m_pt_cpu;
-  std::map<std::pair<lldb::pid_t, lldb::tid_t>, std::shared_ptr<ThreadTrace>>
+  std::map<std::pair<lldb::pid_t, lldb::tid_t>, ThreadTraceDecoder>
       m_trace_threads;
 };
 
 } // namespace trace_intel_pt
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_TRACEINTELPT_H

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

 }
 
 TraceIntelPT::TraceIntelPT(
     const pt_cpu &pt_cpu,
     const std::vector<std::shared_ptr<ThreadTrace>> &traced_threads)
     : m_pt_cpu(pt_cpu) {
   for (const std::shared_ptr<ThreadTrace> &thread : traced_threads)
     m_trace_threads.emplace(
-        std::make_pair(thread->GetProcess()->GetID(), thread->GetID()), thread);
+        std::piecewise_construct,
+        std::forward_as_tuple(thread->GetProcess()->GetID(), thread->GetID()),
+        std::forward_as_tuple(thread, pt_cpu));
+}
+
+const DecodedThread *TraceIntelPT::Decode(const Thread &thread) {
+  auto it = m_trace_threads.find(
+      std::make_pair(thread.GetProcess()->GetID(), thread.GetID()));
+  if (it == m_trace_threads.end())
+    return nullptr;
+  return &it->second.Decode();
+}
+
+size_t TraceIntelPT::GetCursorPosition(const Thread &thread) {
+  const DecodedThread *decoded_thread = Decode(thread);
+  if (!decoded_thread)
+    return 0;
+  return decoded_thread->GetCursorPosition();
+}
+
+void TraceIntelPT::TraverseInstructions(
+    const Thread &thread, size_t position, TraceDirection direction,
+    std::function<bool(size_t index, Expected<lldb::addr_t> load_addr)>
+        callback) {
+  const DecodedThread *decoded_thread = Decode(thread);
+  if (!decoded_thread)
+    return;
+
+  ArrayRef<IntelPTInstruction> instructions = decoded_thread->GetInstructions();
+
+  ssize_t delta = direction == TraceDirection::Forwards ? 1 : -1;
+  for (ssize_t i = position; i < (ssize_t)instructions.size() && i >= 0;
+       i += delta)
+    if (!callback(i, instructions[i].GetLoadAddress()))
+      break;
+}
+
+size_t TraceIntelPT::GetInstructionCount(const Thread &thread) {
+  if (const DecodedThread *decoded_thread = Decode(thread))
+    return decoded_thread->GetInstructions().size();
+  else
+    return 0;
 }

lldb/source/Symbol/SymbolContext.cpp

void SymbolContext::Clear(bool clear_target) {
   symbol = nullptr;
   variable = nullptr;
 }
 
 bool SymbolContext::DumpStopContext(Stream *s, ExecutionContextScope *exe_scope,
                                     const Address &addr, bool show_fullpaths,
                                     bool show_module, bool show_inlined_frames,
                                     bool show_function_arguments,
-                                    bool show_function_name) const {
+                                    bool show_function_name,
+                                    bool show_inline_callsite_line_info) const {
   bool dumped_something = false;
   if (show_module && module_sp) {
     if (show_fullpaths)
       *s << module_sp->GetFileSpec();
     else
       *s << module_sp->GetFileSpec().GetFilename();
     s->PutChar('`');
     dumped_something = true;
if (GetParentOfInlinedScope(addr, inline_parent_sc, inline_parent_addr)) {
       lldb_private::AddressRange block_range;
       if (inlined_block->GetRangeContainingAddress(addr, block_range)) {
         const addr_t inlined_function_offset =
             addr.GetOffset() - block_range.GetBaseAddress().GetOffset();
         if (inlined_function_offset) {
           s->Printf(" + %" PRIu64, inlined_function_offset);
         }
       }
+      if (show_inline_callsite_line_info) {
         const Declaration &call_site = inlined_block_info->GetCallSite();
         if (call_site.IsValid()) {
           s->PutCString(" at ");
           call_site.DumpStopContext(s, show_fullpaths);
         }
+      } else if (line_entry.IsValid()) {
+        s->PutCString(" at ");
+        line_entry.DumpStopContext(s, show_fullpaths);
+      }
+
       if (show_inlined_frames) {
         s->EOL();
         s->Indent();
         const bool show_function_name = true;
         return inline_parent_sc.DumpStopContext(
             s, exe_scope, inline_parent_addr, show_fullpaths, show_module,
             show_inlined_frames, show_function_arguments, show_function_name);
       }

lldb/source/Target/ProcessTrace.cpp

 //===-- ProcessTrace.cpp --------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/ProcessTrace.h"
 
 #include <memory>
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Section.h"
+#include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 ConstString ProcessTrace::GetPluginNameStatic() {
   static ConstString g_name("trace");
   return g_name;
bool ProcessTrace::GetProcessInfo(ProcessInstanceInfo &info) {
   if (module_sp) {
     const bool add_exe_file_as_first_arg = false;
     info.SetExecutableFile(GetTarget().GetExecutableModule()->GetFileSpec(),
                            add_exe_file_as_first_arg);
   }
   return true;
 }
 
 size_t ProcessTrace::DoReadMemory(addr_t addr, void *buf, size_t size,
                                   Status &error) {
-  return 0;
+  Address resolved_address;
+  GetTarget().GetSectionLoadList().ResolveLoadAddress(addr, resolved_address);
+
+  return GetTarget().ReadMemoryFromFileCache(resolved_address, buf, size,
+                                             error);
 }

lldb/source/Target/Trace.cpp

 //===-- Trace.cpp ---------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/Trace.h"
 
-#include <sstream>
-
 #include "llvm/Support/Format.h"
 
+#include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Symbol/Function.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
 // Helper structs used to extract the type of a trace session json without
Expected<StringRef> Trace::FindPluginSchema(StringRef name) {
   ConstString plugin_name(name);
   StringRef schema = PluginManager::GetTraceSchema(plugin_name);
   if (!schema.empty())
     return schema;
 
   return createInvalidPlugInError(name);
 }
 
+static int GetNumberOfDigits(size_t num) {
+  return num == 0 ? 1 : static_cast<int>(log10(num)) + 1;
+}
+
+/// Dump the symbol context of the given instruction address if it's different
+/// from the symbol context of the previous instruction in the trace.
+///
+/// \param[in] prev_sc
+///     The symbol context of the previous instruction in the trace.
+///
+/// \param[in] address
+///     The address whose symbol information will be dumped.
+///
+/// \return
+///     The symbol context of the current address, which might differ from the
+///     previous one.
+static SymbolContext DumpSymbolContext(Stream &s, const SymbolContext &prev_sc,
+                                       Target &target, const Address &address) {
+  AddressRange range;
+  if (prev_sc.GetAddressRange(eSymbolContextEverything, 0,
+                              /*inline_block_range*/ false, range) &&
+      range.ContainsFileAddress(address))
+    return prev_sc;
+
+  SymbolContext sc;
+  address.CalculateSymbolContext(&sc, eSymbolContextEverything);
+
+  if (!prev_sc.module_sp && !sc.module_sp)
+    return sc;
+  if (prev_sc.module_sp == sc.module_sp && !sc.function && !sc.symbol &&
+      !prev_sc.function && !prev_sc.symbol)
+    return sc;
+
+  s.Printf("  ");
+
+  if (!sc.module_sp)
+    s.Printf("(none)");
+  else if (!sc.function && !sc.symbol)
+    s.Printf("%s`(none)",
+             sc.module_sp->GetFileSpec().GetFilename().AsCString());
+  else
+    sc.DumpStopContext(&s, &target, address, /*show_fullpath*/ false,
+                       /*show_module*/ true, /*show_inlined_frames*/ false,
+                       /*show_function_arguments*/ true,
+                       /*show_function_name*/ true,
+                       /*show_inline_callsite_line_info*/ false);
+  s.Printf("\n");
+  return sc;
+}
+
+/// Dump an instruction given by its address using a given disassembler, unless
+/// the instruction is not present in the disassembler.
+///
+/// \param[in] disassembler
+///     A disassembler containing a certain instruction list.
+///
+/// \param[in] address
+///     The address of the instruction to dump.
+///
+/// \return
+///     \b true if the information could be dumped, \b false otherwise.
+static bool TryDumpInstructionInfo(Stream &s,
+                                   const DisassemblerSP &disassembler,
+                                   const ExecutionContext &exe_ctx,
+                                   const Address &address) {
+  if (!disassembler)
+    return false;
+
+  if (InstructionSP instruction =
+          disassembler->GetInstructionList().GetInstructionAtAddress(address)) {
+    instruction->Dump(&s, /*show_address*/ false, /*show_bytes*/ false,
+                      /*max_opcode_byte_size*/ 0, &exe_ctx,
+                      /*sym_ctx*/ nullptr, /*prev_sym_ctx*/ nullptr,
+                      /*disassembly_addr_format*/ nullptr,
+                      /*max_address_text_size*/ 0);
+    return true;
+  }
+
+  return false;
+}
+
+/// Dump an instruction instruction given by its address.
+///
+/// \param[in] prev_disassembler
+///     The disassembler that was used to dump the previous instruction in the
+///     trace. It is useful to avoid recomputations.
+///
+/// \param[in] address
+///     The address of the instruction to dump.
+///
+/// \return
+///     A disassembler that contains the given instruction, which might differ
+///     from the previous disassembler.
+static DisassemblerSP
+DumpInstructionInfo(Stream &s, const SymbolContext &sc,
+                    const DisassemblerSP &prev_disassembler,
+                    ExecutionContext &exe_ctx, const Address &address) {
+  // We first try to use the previous disassembler
+  if (TryDumpInstructionInfo(s, prev_disassembler, exe_ctx, address))
+    return prev_disassembler;
+
+  // Now we try using the current function's disassembler
+  if (sc.function) {
+    DisassemblerSP disassembler =
+        sc.function->GetInstructions(exe_ctx, nullptr, true);
+    if (TryDumpInstructionInfo(s, disassembler, exe_ctx, address))
+      return disassembler;
+  }
+
+  // We fallback to disassembly one instruction
+  Target &target = exe_ctx.GetTargetRef();
+  const ArchSpec &arch = target.GetArchitecture();
+  AddressRange range(address, arch.GetMaximumOpcodeByteSize() * 1);
+  DisassemblerSP disassembler = Disassembler::DisassembleRange(
+      arch, /*plugin_name*/ nullptr,
+      /*flavor*/ nullptr, target, range, /*prefer_file_cache*/ true);
+  if (TryDumpInstructionInfo(s, disassembler, exe_ctx, address))
+    return disassembler;
+  return nullptr;
+}
+
 void Trace::DumpTraceInstructions(Thread &thread, Stream &s, size_t count,
-                                  size_t start_position) const {
-  s.Printf("thread #%u: tid = %" PRIu64 ", total instructions = 1000\n",
-           thread.GetIndexID(), thread.GetID());
-  s.Printf("  would print %zu instructions from position %zu\n", count,
-           start_position);
+                                  size_t end_position, bool raw) {
+  size_t instructions_count = GetInstructionCount(thread);
+  s.Printf("thread #%u: tid = %" PRIu64 ", total instructions = %zu\n",
+           thread.GetIndexID(), thread.GetID(), instructions_count);
+
+  if (count == 0 || end_position >= instructions_count)
+    return;
+
+  size_t start_position =
+      end_position + 1 < count ? 0 : end_position + 1 - count;
+
+  int digits_count = GetNumberOfDigits(end_position);
+  auto printInstructionIndex = [&](size_t index) {
+    s.Printf("    [%*zu] ", digits_count, index);
+  };
+
+  bool was_prev_instruction_an_error = false;
+  Target &target = thread.GetProcess()->GetTarget();
+
+  SymbolContext sc;
+  DisassemblerSP disassembler;
+  ExecutionContext exe_ctx;
+  target.CalculateExecutionContext(exe_ctx);
+
+  TraverseInstructions(
+      thread, start_position, TraceDirection::Forwards,
+      [&](size_t index, Expected<lldb::addr_t> load_address) -> bool {
+        if (load_address) {
+          // We print an empty line after a sequence of errors to show more
+          // clearly that there's a gap in the trace
+          if (was_prev_instruction_an_error)
+            s.Printf("    ...missing instructions\n");
+
+          Address address;
+          if (!raw) {
+            target.GetSectionLoadList().ResolveLoadAddress(*load_address,
+                                                           address);
+
+            sc = DumpSymbolContext(s, sc, target, address);
+          }
+
+          printInstructionIndex(index);
+          s.Printf("0x%016" PRIx64 "    ", *load_address);
+
+          if (!raw) {
+            disassembler =
+                DumpInstructionInfo(s, sc, disassembler, exe_ctx, address);
+          }
+
+          was_prev_instruction_an_error = false;
+        } else {
+          printInstructionIndex(index);
+          s << toString(load_address.takeError());
+          was_prev_instruction_an_error = true;
+          if (!raw)
+            sc = SymbolContext();
+        }
+
+        s.Printf("\n");
+
+        return index < end_position;
+      });
 }

lldb/source/Target/TraceSessionFileParser.cpp

Error TraceSessionFileParser::ParseModule(lldb::TargetSP &target_sp,
 
   FileSpec local_file_spec(module.file.hasValue() ? *module.file
                                                   : module.system_path);
   NormalizePath(local_file_spec);
 
   ModuleSpec module_spec;
   module_spec.GetFileSpec() = local_file_spec;
   module_spec.GetPlatformFileSpec() = system_file_spec;
-  module_spec.SetObjectOffset(module.load_address.value);
 
   if (module.uuid.hasValue())
     module_spec.GetUUID().SetFromStringRef(*module.uuid);
 
   Status error;
   ModuleSP module_sp =
       target_sp->GetOrCreateModule(module_spec, /*notify*/ false, &error);
+
+  if (error.Fail())
     return error.ToError();
+
+  bool load_addr_changed = false;
+  module_sp->SetLoadAddress(*target_sp, module.load_address.value, false,
+                            load_addr_changed);
+  return llvm::Error::success();
 }
 
 Error TraceSessionFileParser::CreateJSONError(json::Path::Root &root,
                                               const json::Value &value) {
   std::string err;
   raw_string_ostream os(err);
   root.printErrorContext(value, os);
   return createStringError(
Status error = m_debugger.GetTargetList().CreateTarget(
       eLoadDependentsNo,
       /*platform_options*/ nullptr, target_sp);
 
   if (!target_sp)
     return error.ToError();
 
   ParsedProcess parsed_process;
   parsed_process.target_sp = target_sp;
 
   m_debugger.GetTargetList().SetSelectedTarget(target_sp.get());
 
   ProcessSP process_sp = target_sp->CreateProcess(
       /*listener*/ nullptr, "trace",
       /*crash_file*/ nullptr);
 
   process_sp->SetID(static_cast<lldb::pid_t>(process.pid));
 

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

class TestTraceDumpInstructions(TestBase):
 
     def testErrorMessages(self):
         # We first check the output when there are no targets
         self.expect("thread trace dump instructions",
             substrs=["error: invalid target, create a target using the 'target create' command"],
             error=True)
 
         # We now check the output when there's a non-running target
-        self.expect("target create " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+        self.expect("target create " + 
+            os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
 
         self.expect("thread trace dump instructions",
             substrs=["error: invalid process"],
             error=True)
 
         # Now we check the output when there's a running target without a trace
         self.expect("b main")
         self.expect("run")
 
         self.expect("thread trace dump instructions",
             substrs=["error: this thread is not being traced"],
             error=True)
 
-    def testDumpInstructions(self):
-        self.expect("trace load -v " + os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+    def testRawDumpInstructions(self):
+        self.expect("trace load -v " +
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
             substrs=["intel-pt"])
 
-        self.expect("thread trace dump instructions",
-            substrs=['thread #1: tid = 3842849, total instructions = 1000',
-                     'would print 20 instructions from position 0'])
-
-        # We check if we can pass count and offset
-        self.expect("thread trace dump instructions --count 5 --start-position 10",
-            substrs=['thread #1: tid = 3842849, total instructions = 1000',
-                     'would print 5 instructions from position 10'])
+        self.expect("thread trace dump instructions --raw",
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+    [ 1] 0x0000000000400518    
+    [ 2] 0x000000000040051f    
+    [ 3] 0x0000000000400529    
+    [ 4] 0x000000000040052d    
+    [ 5] 0x0000000000400521    
+    [ 6] 0x0000000000400525    
+    [ 7] 0x0000000000400529    
+    [ 8] 0x000000000040052d    
+    [ 9] 0x0000000000400521    
+    [10] 0x0000000000400525    
+    [11] 0x0000000000400529    
+    [12] 0x000000000040052d    
+    [13] 0x0000000000400521    
+    [14] 0x0000000000400525    
+    [15] 0x0000000000400529    
+    [16] 0x000000000040052d    
+    [17] 0x0000000000400521    
+    [18] 0x0000000000400525    
+    [19] 0x0000000000400529    
+    [20] 0x000000000040052d'''])
+
+        # We check if we can pass count and position
+        self.expect("thread trace dump instructions --count 5 --position 10 --raw",
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+    [ 6] 0x0000000000400525    
+    [ 7] 0x0000000000400529    
+    [ 8] 0x000000000040052d    
+    [ 9] 0x0000000000400521    
+    [10] 0x0000000000400525'''])
 
         # We check if we can access the thread by index id
-        self.expect("thread trace dump instructions 1",
-            substrs=['thread #1: tid = 3842849, total instructions = 1000',
-                     'would print 20 instructions from position 0'])
+        self.expect("thread trace dump instructions 1 --raw",
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+    [ 1] 0x0000000000400518'''])
 
         # We check that we get an error when using an invalid thread index id
         self.expect("thread trace dump instructions 10", error=True,
             substrs=['error: no thread with index: "10"'])
 
-    def testDumpInstructionsWithMultipleThreads(self):
+    def testDumpFullInstructionsWithMultipleThreads(self):
         # We load a trace with two threads
-        self.expect("trace load -v " + os.path.join(self.getSourceDir(), "intelpt-trace", "trace_2threads.json"))
+        self.expect("trace load -v " + 
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace_2threads.json"))
 
         # We print the instructions of two threads simultaneously
-        self.expect("thread trace dump instructions 1 2",
-            substrs=['''thread #1: tid = 3842849, total instructions = 1000
-  would print 20 instructions from position 0
-thread #2: tid = 3842850, total instructions = 1000
-  would print 20 instructions from position 0'''])
-
-        # We use custom --count and --start-position, saving the command to history for later
-        ci = self.dbg.GetCommandInterpreter()
-
-        result = lldb.SBCommandReturnObject()
-        ci.HandleCommand("thread trace dump instructions 1 2 --count 12 --start-position 5", result, True)
-        self.assertIn('''thread #1: tid = 3842849, total instructions = 1000
-  would print 12 instructions from position 5
-thread #2: tid = 3842850, total instructions = 1000
-  would print 12 instructions from position 5''', result.GetOutput())
-
-        # We use a repeat command and ensure the previous count is used and the start-position has moved to the next position
-        result = lldb.SBCommandReturnObject()
-        ci.HandleCommand("", result)
-        self.assertIn('''thread #1: tid = 3842849, total instructions = 1000
-  would print 12 instructions from position 17
-thread #2: tid = 3842850, total instructions = 1000
-  would print 12 instructions from position 17''', result.GetOutput())
-
-        ci.HandleCommand("", result)
-        self.assertIn('''thread #1: tid = 3842849, total instructions = 1000
-  would print 12 instructions from position 29
-thread #2: tid = 3842850, total instructions = 1000
-  would print 12 instructions from position 29''', result.GetOutput())
+        self.expect("thread trace dump instructions 1 2 --count 2",
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  a.out`main + 28 at main.cpp:4
+    [19] 0x0000000000400529    cmpl   $0x3, -0x8(%rbp)
+    [20] 0x000000000040052d    jle    0x400521                  ; <+20> at main.cpp:5
+thread #2: tid = 3842850, total instructions = 21
+  a.out`main + 28 at main.cpp:4
+    [19] 0x0000000000400529    cmpl   $0x3, -0x8(%rbp)
+    [20] 0x000000000040052d    jle    0x400521                  ; <+20> at main.cpp:5'''])
+
+        # We use custom --count and --position, saving the command to history for later
+        self.expect("thread trace dump instructions 1 2 --count 2 --position 20", inHistory=True,
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  a.out`main + 28 at main.cpp:4
+    [19] 0x0000000000400529    cmpl   $0x3, -0x8(%rbp)
+    [20] 0x000000000040052d    jle    0x400521                  ; <+20> at main.cpp:5
+thread #2: tid = 3842850, total instructions = 21
+  a.out`main + 28 at main.cpp:4
+    [19] 0x0000000000400529    cmpl   $0x3, -0x8(%rbp)
+    [20] 0x000000000040052d    jle    0x400521                  ; <+20> at main.cpp:5'''])
+
+        # We use a repeat command twice and ensure the previous count is used and the
+        # start position moves with each command.
+        self.expect("", inHistory=True,
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  a.out`main + 20 at main.cpp:5
+    [17] 0x0000000000400521    xorl   $0x1, -0x4(%rbp)
+  a.out`main + 24 at main.cpp:4
+    [18] 0x0000000000400525    addl   $0x1, -0x8(%rbp)
+thread #2: tid = 3842850, total instructions = 21
+  a.out`main + 20 at main.cpp:5
+    [17] 0x0000000000400521    xorl   $0x1, -0x4(%rbp)
+  a.out`main + 24 at main.cpp:4
+    [18] 0x0000000000400525    addl   $0x1, -0x8(%rbp)'''])
+
+        self.expect("", inHistory=True,
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  a.out`main + 28 at main.cpp:4
+    [15] 0x0000000000400529    cmpl   $0x3, -0x8(%rbp)
+    [16] 0x000000000040052d    jle    0x400521                  ; <+20> at main.cpp:5
+thread #2: tid = 3842850, total instructions = 21
+  a.out`main + 28 at main.cpp:4
+    [15] 0x0000000000400529    cmpl   $0x3, -0x8(%rbp)
+    [16] 0x000000000040052d    jle    0x400521                  ; <+20> at main.cpp:5'''])
+
+    def testInvalidBounds(self):
+        self.expect("trace load -v " +
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"))
+
+        # The output should be work when too many instructions are asked
+        self.expect("thread trace dump instructions --count 20 --position 2",
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  a.out`main + 4 at main.cpp:2
+    [0] 0x0000000000400511    movl   $0x0, -0x4(%rbp)
+  a.out`main + 11 at main.cpp:4
+    [1] 0x0000000000400518    movl   $0x0, -0x8(%rbp)
+    [2] 0x000000000040051f    jmp    0x400529                  ; <+28> at main.cpp:4'''])
+
+        # Should print no instructions if the position is out of bounds
+        self.expect("thread trace dump instructions --position 23",
+            endstr='thread #1: tid = 3842849, total instructions = 21\n')
+
+        # Should fail with negative bounds
+        self.expect("thread trace dump instructions --position -1", error=True)
+        self.expect("thread trace dump instructions --count -1", error=True)
+
+    def testWrongImage(self):
+        self.expect("trace load " +
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace_bad_image.json"))
+        self.expect("thread trace dump instructions",
+            substrs=['''thread #1: tid = 3842849, total instructions = 2
+    [0] 0x0000000000400511    error: no memory mapped at this address
+    [1] 0x0000000000400518    error: no memory mapped at this address'''])
+
+    def testWrongCPU(self):
+        self.expect("trace load " +
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace_wrong_cpu.json"))
+        self.expect("thread trace dump instructions",
+            substrs=['''thread #1: tid = 3842849, total instructions = 1
+    [0] error: unknown cpu'''])
+
+    def testMultiFileTraceWithMissingModule(self):
+        self.expect("trace load " +
+            os.path.join(self.getSourceDir(), "intelpt-trace-multi-file", "multi-file-no-ld.json"))
+
+        # This instructions in this test covers the following flow:
+        #
+        # - The trace starts with a call to libfoo, which triggers the dynamic
+        #   linker, but the dynamic linker is not included in the JSON file,
+        #   thus the trace reports a set of missing instructions after
+        #   instruction [6].
+        # - Then, the dump continues in the next synchronization point showing
+        #   a call to an inlined function, which is displayed as [inlined].
+        # - Finally, a call to libfoo is performed, which invokes libbar inside.
+        # 
+        # Whenever there's a line or symbol change, including the inline case, a
+        # line is printed showing the symbol context change.
+        #
+        # Finally, the instruction disassembly is included in the dump.
+        self.expect("thread trace dump instructions --count 50",
+            substrs=['''thread #1: tid = 815455, total instructions = 46
+  a.out`main + 15 at main.cpp:10
+    [ 0] 0x000000000040066f    callq  0x400540                  ; symbol stub for: foo()
+  a.out`symbol stub for: foo()
+    [ 1] 0x0000000000400540    jmpq   *0x200ae2(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 40
+    [ 2] 0x0000000000400546    pushq  $0x2
+    [ 3] 0x000000000040054b    jmp    0x400510
+  a.out`(none)
+    [ 4] 0x0000000000400510    pushq  0x200af2(%rip)            ; _GLOBAL_OFFSET_TABLE_ + 8
+    [ 5] 0x0000000000400516    jmpq   *0x200af4(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 16
+    [ 6] 0x00007ffff7df1950    error: no memory mapped at this address
+    ...missing instructions
+  a.out`main + 20 at main.cpp:10
+    [ 7] 0x0000000000400674    movl   %eax, -0xc(%rbp)
+  a.out`main + 23 at main.cpp:12
+    [ 8] 0x0000000000400677    movl   -0xc(%rbp), %eax
+    [ 9] 0x000000000040067a    addl   $0x1, %eax
+    [10] 0x000000000040067f    movl   %eax, -0xc(%rbp)
+  a.out`main + 34 [inlined] inline_function() at main.cpp:4
+    [11] 0x0000000000400682    movl   $0x0, -0x4(%rbp)
+  a.out`main + 41 [inlined] inline_function() + 7 at main.cpp:5
+    [12] 0x0000000000400689    movl   -0x4(%rbp), %eax
+    [13] 0x000000000040068c    addl   $0x1, %eax
+    [14] 0x0000000000400691    movl   %eax, -0x4(%rbp)
+  a.out`main + 52 [inlined] inline_function() + 18 at main.cpp:6
+    [15] 0x0000000000400694    movl   -0x4(%rbp), %eax
+  a.out`main + 55 at main.cpp:14
+    [16] 0x0000000000400697    movl   -0xc(%rbp), %ecx
+    [17] 0x000000000040069a    addl   %eax, %ecx
+    [18] 0x000000000040069c    movl   %ecx, -0xc(%rbp)
+  a.out`main + 63 at main.cpp:16
+    [19] 0x000000000040069f    callq  0x400540                  ; symbol stub for: foo()
+  a.out`symbol stub for: foo()
+    [20] 0x0000000000400540    jmpq   *0x200ae2(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 40
+  libfoo.so`foo() at foo.cpp:3
+    [21] 0x00007ffff7bd96e0    pushq  %rbp
+    [22] 0x00007ffff7bd96e1    movq   %rsp, %rbp
+  libfoo.so`foo() + 4 at foo.cpp:4
+    [23] 0x00007ffff7bd96e4    subq   $0x10, %rsp
+    [24] 0x00007ffff7bd96e8    callq  0x7ffff7bd95d0            ; symbol stub for: bar()
+  libfoo.so`symbol stub for: bar()
+    [25] 0x00007ffff7bd95d0    jmpq   *0x200a4a(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 32
+  libbar.so`bar() at bar.cpp:1
+    [26] 0x00007ffff79d7690    pushq  %rbp
+    [27] 0x00007ffff79d7691    movq   %rsp, %rbp
+  libbar.so`bar() + 4 at bar.cpp:2
+    [28] 0x00007ffff79d7694    movl   $0x1, -0x4(%rbp)
+  libbar.so`bar() + 11 at bar.cpp:3
+    [29] 0x00007ffff79d769b    movl   -0x4(%rbp), %eax
+    [30] 0x00007ffff79d769e    addl   $0x1, %eax
+    [31] 0x00007ffff79d76a3    movl   %eax, -0x4(%rbp)
+  libbar.so`bar() + 22 at bar.cpp:4
+    [32] 0x00007ffff79d76a6    movl   -0x4(%rbp), %eax
+    [33] 0x00007ffff79d76a9    popq   %rbp
+    [34] 0x00007ffff79d76aa    retq   
+  libfoo.so`foo() + 13 at foo.cpp:4
+    [35] 0x00007ffff7bd96ed    movl   %eax, -0x4(%rbp)
+  libfoo.so`foo() + 16 at foo.cpp:5
+    [36] 0x00007ffff7bd96f0    movl   -0x4(%rbp), %eax
+    [37] 0x00007ffff7bd96f3    addl   $0x1, %eax
+    [38] 0x00007ffff7bd96f8    movl   %eax, -0x4(%rbp)
+  libfoo.so`foo() + 27 at foo.cpp:6
+    [39] 0x00007ffff7bd96fb    movl   -0x4(%rbp), %eax
+    [40] 0x00007ffff7bd96fe    addq   $0x10, %rsp
+    [41] 0x00007ffff7bd9702    popq   %rbp
+    [42] 0x00007ffff7bd9703    retq   
+  a.out`main + 68 at main.cpp:16
+    [43] 0x00000000004006a4    movl   -0xc(%rbp), %ecx
+    [44] 0x00000000004006a7    addl   %eax, %ecx
+    [45] 0x00000000004006a9    movl   %ecx, -0xc(%rbp)'''])

lldb/test/API/commands/trace/intelpt-trace-multi-file/bar.h

+int bar();

lldb/test/API/commands/trace/intelpt-trace-multi-file/bar.cpp

+int bar() {
+  int x = 1;
+  x++;
+  return x;
+}

lldb/test/API/commands/trace/intelpt-trace-multi-file/foo.h

+int foo();

lldb/test/API/commands/trace/intelpt-trace-multi-file/foo.cpp

+#include "bar.h"
+
+int foo() {
+  int y = bar();
+  y++;
+  return y;
+}

lldb/test/API/commands/trace/intelpt-trace-multi-file/main.cpp

+#include "foo.h"
+
+int __attribute__((always_inline)) inline_function() {
+  int z = 0;
+  z++;
+  return z;
+}
+
+int main() {
+  int res = foo();
+
+  res++;
+
+  res += inline_function();
+
+  res += foo();
+
+  return res;
+}

lldb/test/API/commands/trace/intelpt-trace-multi-file/multi-file-no-ld.json

+{
+  "trace": {
+    "type": "intel-pt",
+    "pt_cpu": {
+      "vendor": "intel",
+      "family": 6,
+      "model": 79,
+      "stepping": 1
+    }
+  },
+  "processes": [
+    {
+      "pid": 815455,
+      "triple": "x86_64-*-linux",
+      "threads": [
+        {
+          "tid": 815455,
+          "traceFile": "multi-file.trace"
+        }
+      ],
+      "modules": [
+        {
+          "file": "a.out",
+          "systemPath": "a.out",
+          "loadAddress": "0x0000000000400000",
+          "uuid": "D2414468-7112-B7C5-408D-FF07E30D5B17-A5BFD2C4"
+        },
+        {
+          "file": "libfoo.so",
+          "systemPath": "libfoo.so",
+          "loadAddress": "0x00007ffff7bd9000",
+          "uuid": "B30FFEDA-8BB2-3D08-4580-C5937ED11E2B-21BE778C"
+        },
+        {
+          "file": "libbar.so",
+          "systemPath": "libbar.so",
+          "loadAddress": "0x00007ffff79d7000",
+          "uuid": "6633B038-EA73-D1A6-FF9A-7D0C0EDF733D-95FEA2CC"
+        }
+      ]
+    }
+  ]
+}

lldb/test/API/commands/trace/intelpt-trace/trace_bad_image.json

+{
+  "trace": {
+    "type": "intel-pt",
+    "pt_cpu": {
+      "vendor": "intel",
+      "family": 6,
+      "model": 79,
+      "stepping": 1
+    }
+  },
+  "processes": [
+    {
+      "pid": 1234,
+      "triple": "x86_64-*-linux",
+      "threads": [
+        {
+          "tid": 3842849,
+          "traceFile": "3842849.trace"
+        }
+      ],
+      "modules": [
+        {
+          "file": "a.out",
+          "systemPath": "a.out",
+          "loadAddress": "0x0000000000FFFFF0",
+          "uuid": "6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A"
+        }
+      ]
+    }
+  ]
+}

lldb/test/API/commands/trace/intelpt-trace/trace_wrong_cpu.json

+{
+  "trace": {
+    "type": "intel-pt",
+    "pt_cpu": {
+      "vendor": "intel",
+      "family": 2123123,
+      "model": 12123123,
+      "stepping": 1231231
+    }
+  },
+  "processes": [
+    {
+      "pid": 1234,
+      "triple": "x86_64-*-linux",
+      "threads": [
+        {
+          "tid": 3842849,
+          "traceFile": "3842849.trace"
+        }
+      ],
+      "modules": [
+        {
+          "file": "a.out",
+          "systemPath": "a.out",
+          "loadAddress": "0x0000000000400000",
+          "uuid": "6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A"
+        }
+      ]
+    }
+  ]
+}