[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/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

81

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

106

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

43–48

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

43–48

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.

158–159

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

106

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

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

48

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

48

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

48

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

158–159

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

2111 ↗

(On Diff #300134)

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/Target/Trace.h

public:
   ///     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.
   void DumpTraceInstructions(Thread &thread, Stream &s, size_t count,
-                             size_t start_position) const;
+                             size_t start_position);
+
+  /// Run the provided callback on the instructions of the trace of the given
+  /// thread.
+  ///
+  /// The instructions will be traversed starting at the \a from 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] callback
+  ///     The callback to execute on each instruction. If it returns \b true for
+  ///     the \a i-th instruction, then the instruction with index \a i + 1 will
+  ///     be inspected. If it returns \b false, no more instructions will be
+  ///     inspected.
+  ///
+  /// \param[in] from
+  ///     The first index to execute the callback on.
+  virtual void ForEachInstruction(
+      const Thread &thread,
+      std::function<bool(size_t index, llvm::Expected<lldb::addr_t> &load_addr)>
+          callback,
+      size_t from = 0) = 0;
+
+  /// Get the number of instructions of the trace of the given thread.
+  ///
+  /// \param[in] thread
+  ///     The thread whose trace will be inspected.
+  ///
+  /// \return
+  ///     The total number of instructions of the trace.
+  virtual size_t GetInstructionCount(const Thread &thread) = 0;
+
+  /// Get any errors in case the trace of the given thread can't be
+  /// reconstructed.
+  ///
+  /// If this returns an actual error, then no instructions are available in the
+  /// trace.
+  ///
+  /// \param[in] thread
+  ///     The thread whose trace will be inspected.
+  ///
+  /// \return
+  ///     An \a llvm::Error::success instance if the trace can be reconstructed,
+  ///     or an actual Error in case of failures.
+  virtual llvm::Error GetTraceErrorStatus(const Thread &thread) = 0;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TRACE_H

lldb/source/Plugins/Process/Trace/ProcessTrace.h

 //===-- ProcessTrace.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_PROCESS_TRACE_PROCESSTRACE_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_TRACE_PROCESSTRACE_H
 
+#include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
 
 namespace lldb_private {
 namespace process_trace {
 
 class ProcessTrace : public Process {
public:
 
   bool GetProcessInfo(ProcessInstanceInfo &info) override;
 
 protected:
   void Clear();
 
   bool UpdateThreadList(ThreadList &old_thread_list,
                         ThreadList &new_thread_list) override;
+
+private:
+  /// Create a map from virtual memory ranges to the sections that contain them.
+  const std::map<MemoryRegionInfo::RangeType, lldb::SectionSP> &GetSectionMap();
+
+  llvm::Optional<std::map<MemoryRegionInfo::RangeType, lldb::SectionSP>>
+      m_section_map;
 };
 
 } // namespace process_trace
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_TRACE_PROCESSTRACE_H

lldb/source/Plugins/Process/Trace/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 "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;
 using namespace lldb_private::process_trace;
 
 LLDB_PLUGIN_DEFINE(ProcessTrace)
 
PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                   GetPluginDescriptionStatic(), CreateInstance);
   });
 }
 
 ArchSpec ProcessTrace::GetArchitecture() {
   return GetTarget().GetArchitecture();
 }
 
+const std::map<MemoryRegionInfo::RangeType, SectionSP> &
+ProcessTrace::GetSectionMap() {
+  if (m_section_map)
+    return *m_section_map;
+
+  m_section_map.emplace();
+
+  ModuleList &modules = GetTarget().GetImages();
+  SectionLoadList &load_list = GetTarget().GetSectionLoadList();
+  modules.ForEach([&](const ModuleSP &module_sp) {
+    SectionList *sections = module_sp->GetSectionList();
+    for (size_t i = 0; i < sections->GetSize(); i++) {
+      SectionSP section_sp = sections->GetSectionAtIndex(i);
+
+      addr_t load_addr = load_list.GetSectionLoadAddress(section_sp);
+      if (load_addr == LLDB_INVALID_ADDRESS)
+        continue;
+      MemoryRegionInfo::RangeType section_range(load_addr,
+                                                section_sp->GetByteSize());
+      m_section_map->insert(std::make_pair(section_range, section_sp));
+    }
+    return true;
+  });
+
+  return *m_section_map;
+}
+
 bool ProcessTrace::GetProcessInfo(ProcessInstanceInfo &info) {
   info.Clear();
   info.SetProcessID(GetID());
   info.SetArchitecture(GetArchitecture());
   ModuleSP module_sp = GetTarget().GetExecutableModule();
   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) {
+  const std::map<MemoryRegionInfo::RangeType, SectionSP> &section_map =
+      GetSectionMap();
+
+  MemoryRegionInfo::RangeType address_range(addr, size);
+  // Doing (upper_bound - 1) gives us the maximum range whose base address is <=
+  // addr
+  auto it = section_map.upper_bound(address_range);
+  if (it != section_map.begin()) {
+    --it;
+    const SectionSP &section = it->second;
+    const MemoryRegionInfo::RangeType &section_range = it->first;
+
+    if (section_range.Contains(address_range) && section->GetObjectFile()) {
+      section->GetObjectFile()->CopyData(addr - section_range.GetRangeBase(),
+                                         size, buf);
+      return size;
+    }
+  }
+
+  error.SetErrorString("could not parse memory info");
   return 0;
 }

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 "lldb/Target/Trace.h"
+
+#include "intel-pt.h"
+
+namespace lldb_private {
+namespace trace_intel_pt {
+
+/// \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() = delete;
+
+  IntelPTInstruction(const pt_insn &pt_insn)
+      : m_pt_insn(pt_insn), m_libipt_error_code(0) {}
+
+  IntelPTInstruction(int libipt_error_code)
+      : m_libipt_error_code(libipt_error_code) {}
+
+  /// Check if this object represents an error (i.e. a gap).
+  ///
+  /// \return
+  ///     Whether this object represents an error.
+  bool IsError() const;
+
+  /// Get the instruction pointer if this is not an error.
+  ///
+  /// \return
+  ///     The instruction pointer address.
+  llvm::Expected<lldb::addr_t> GetLoadAddress() const;
+
+  /// Return the libipt error code.
+  ///
+  /// libipt error codes are negative numbers.
+  ///
+  /// \return
+  ///     0 if it is not an error, or the error value otherwise.
+  int GetErrorCode() const;
+
+  /// Return a descriptive error message if this is an error.
+  ///
+  /// \return
+  ///    The error message.
+  llvm::StringRef GetErrorMessage() const;
+
+private:
+  pt_insn m_pt_insn;
+  int m_libipt_error_code;
+};
+
+/// \class DecodedThread
+/// Class holding the instructions and function call hierarchy obtained from
+/// decoding a trace.
+class DecodedThread {
+public:
+  DecodedThread(std::vector<IntelPTInstruction> &&instructions)
+      : m_instructions(std::move(instructions)) {}
+
+  /// 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;
+
+private:
+  std::vector<IntelPTInstruction> m_instructions;
+};
+
+} // 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"
+
+using namespace lldb_private;
+using namespace lldb_private::trace_intel_pt;
+using namespace llvm;
+
+bool IntelPTInstruction::IsError() const { return m_libipt_error_code < 0; }
+
+Expected<lldb::addr_t> IntelPTInstruction::GetLoadAddress() const {
+  if (IsError())
+    return createStringError(std::errc::invalid_argument,
+                             GetErrorMessage().data());
+  return m_pt_insn.ip;
+}
+
+int IntelPTInstruction::GetErrorCode() const { return m_libipt_error_code; }
+
+llvm::StringRef IntelPTInstruction::GetErrorMessage() const {
+  return pt_errstr(pt_errcode(GetErrorCode()));
+}
+
+ArrayRef<IntelPTInstruction> DecodedThread::GetInstructions() const {
+  return makeArrayRef(m_instructions);
+}

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 {
+
+class IntelPTDecoder {
+public:
+  /// \param[in] process
+  ///     The process associated with the traces to decode.
+  ///
+  /// \param[in] pt_cpu
+  ///     The libipt \a pt_cpu information of the CPU where the traces were
+  ///     recorded.
+  IntelPTDecoder(Process &process, const pt_cpu &pt_cpu)
+      : m_process(process), m_pt_cpu(pt_cpu) {}
+
+  /// Decode a single-thread trace file.
+  ///
+  /// \param[in] trace_file
+  ///     The binary trace file obtained from tracing a thread.
+  ///
+  /// \return
+  ///     A \a DecodedTrace instance, or an error if decoding failed.
+  llvm::Expected<DecodedThread>
+  DecodeSingleThreadTraceFile(const FileSpec &trace_file);
+
+private:
+  Process &m_process;
+  pt_cpu m_pt_cpu;
+};
+
+/// \a lldb_private::TraceThread decoder that stores the output from decoding,
+/// avoiding recomputations, as decoding is expensive.
+class TraceThreadDecoder {
+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.
+  TraceThreadDecoder(const std::shared_ptr<TraceThread> &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 or an error in case of failures.
+  llvm::Expected<const DecodedThread &> Decode();
+
+private:
+  TraceThreadDecoder(const TraceThreadDecoder &other) = delete;
+
+  std::shared_ptr<TraceThread> m_trace_thread;
+  pt_cpu m_pt_cpu;
+  llvm::Optional<DecodedThread> m_decoded_thread;
+  llvm::Optional<std::string> m_error_message;
+};
+
+} // 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/TraceThread.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.
+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.
+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(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(errcode);
+        break;
+      }
+      pt_insn insn;
+      memset(&insn, 0, sizeof(insn));
+
+      errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
+      if (insn.iclass != ptic_error)
+        instructions.emplace_back(insn);
+
+      if (errcode == -pte_eos)
+        break;
+
+      if (errcode < 0) {
+        instructions.emplace_back(errcode);
+        break;
+      }
+    }
+  }
+
+  return instructions;
+}
+
+static Error CreateLibiptError(int errcode) {
+  return createStringError(std::errc::invalid_argument,
+                           "Intel PT decoding error %d: '%s'", errcode,
+                           pt_errstr(pt_errcode(errcode)));
+}
+
+/// 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 = reinterpret_cast<Process *>(context);
+
+  Status error;
+  int bytes_read = process->ReadMemory(pc, buffer, size, error);
+  if (error.Fail())
+    return -pte_nomap;
+  return bytes_read;
+}
+
+static Expected<std::vector<IntelPTInstruction>>
+CreateDecoderAndDecode(Process &process, const pt_cpu &pt_cpu,
+                       MemoryBuffer &trace) {
+  pt_config config;
+  pt_config_init(&config);
+  config.cpu = pt_cpu;
+
+  if (int errcode = pt_cpu_errata(&config.errata, &config.cpu))
+    return CreateLibiptError(errcode);
+
+  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 CreateLibiptError(pte_nomem);
+
+  pt_image *image = pt_insn_get_image(decoder);
+  if (int error = pt_image_set_callback(image, ReadProcessMemory, &process))
+    return CreateLibiptError(error);
+
+  std::vector<IntelPTInstruction> instructions = DecodeInstructions(*decoder);
+  // We'll need the instructions in reverse order chronologically, so we
+  // reverse them now
+  std::reverse(instructions.begin(), instructions.end());
+
+  pt_insn_free_decoder(decoder);
+  return instructions;
+}
+
+Expected<DecodedThread>
+IntelPTDecoder::DecodeSingleThreadTraceFile(const FileSpec &trace_file) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> buffer_or_error =
+      MemoryBuffer::getFile(trace_file.GetPath());
+  if (std::error_code err = buffer_or_error.getError())
+    return errorCodeToError(err);
+
+  if (Expected<std::vector<IntelPTInstruction>> instructions =
+          CreateDecoderAndDecode(m_process, m_pt_cpu, **buffer_or_error))
+    return DecodedThread(std::move(*instructions));
+  else
+    return instructions.takeError();
+}
+
+Expected<const DecodedThread &> TraceThreadDecoder::Decode() {
+  if (!m_decoded_thread.hasValue() && !m_error_message.hasValue()) {
+    IntelPTDecoder decoder(*m_trace_thread->GetProcess(), m_pt_cpu);
+
+    if (llvm::Expected<DecodedThread> decoded_thread =
+            decoder.DecodeSingleThreadTraceFile(m_trace_thread->GetTraceFile()))
+      m_decoded_thread = *decoded_thread;
+    else
+      // We create a copy of the error message, as we'll create a new instance
+      // of llvm::Error whenever this function is invoked. We have to do this
+      // because llvm::Error is only movable, not copyable.
+      m_error_message = llvm::toString(decoded_thread.takeError());
+  }
+
+  if (m_decoded_thread.hasValue())
+    return *m_decoded_thread;
+  else
+    return llvm::createStringError(std::errc::invalid_argument,
+                                   m_error_message->c_str());
+}

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 ForEachInstruction(
+      const Thread &thread,
+      std::function<bool(size_t index, llvm::Expected<lldb::addr_t> &load_addr)>
+          callback,
+      size_t from = 0) override;
+
+  size_t GetInstructionCount(const Thread &thread) override;
+
+  llvm::Error GetTraceErrorStatus(const Thread &thread) override;
+
 private:
   friend class TraceIntelPTSessionFileParser;
 
   /// \param[in] trace_threads
   ///     TraceThread instances, which are not live-processes and whose trace
   ///     files are fixed.
   TraceIntelPT(const pt_cpu &pt_cpu,
                const std::vector<std::shared_ptr<TraceThread>> &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 TraceThread, then its internal trace file will be
+  ///     decoded. Live threads are not currently supported.
+  ///
+  /// \return
+  ///   A \a DecodedThread instance if decoding was successful, or an error if
+  ///   the thread's trace is not managed by this class or if the trace couldn't
+  ///   be decoded.
+  llvm::Expected<const DecodedThread &> Decode(const Thread &thread);
+
   pt_cpu m_pt_cpu;
-  std::map<std::pair<lldb::pid_t, lldb::tid_t>, std::shared_ptr<TraceThread>>
+  std::map<std::pair<lldb::pid_t, lldb::tid_t>, TraceThreadDecoder>
       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<TraceThread>> &traced_threads)
     : m_pt_cpu(pt_cpu) {
   for (const std::shared_ptr<TraceThread> &thread : traced_threads)
     m_trace_threads.emplace(
-        std::make_pair(thread->GetProcess()->GetID(), thread->GetID()), thread);
+        std::piecewise_construct,
+        std::forward_as_tuple(
+            std::make_pair(thread->GetProcess()->GetID(), thread->GetID())),
+        std::forward_as_tuple(thread, pt_cpu));
+}
+
+Expected<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 createStringError(std::errc::invalid_argument,
+                             "The thread %" PRIu64 " is not traced.",
+                             thread.GetID());
+
+  return it->second.Decode();
+}
+
+void TraceIntelPT::ForEachInstruction(
+    const Thread &thread,
+    std::function<bool(size_t index, Expected<lldb::addr_t> &load_addr)>
+        callback,
+    size_t from) {
+  Expected<const DecodedThread &> decoded_thread = Decode(thread);
+  if (!decoded_thread)
+    return;
+
+  const std::vector<IntelPTInstruction> &instructions =
+      decoded_thread->GetInstructions();
+  for (size_t i = from; i < instructions.size(); i++) {
+    Expected<lldb::addr_t> load_addr = instructions[i].GetLoadAddress();
+    // We check the expected here to avoid forcing the callback check it.
+    (void)(bool) load_addr;
+
+    if (!callback(i, load_addr))
+      break;
+  }
+}
+
+size_t TraceIntelPT::GetInstructionCount(const Thread &thread) {
+  if (Expected<const DecodedThread &> decoded_thread = Decode(thread))
+    return decoded_thread->GetInstructions().size();
+  else
+    return 0;
+}
+
+Error TraceIntelPT::GetTraceErrorStatus(const Thread &thread) {
+  if (Expected<const DecodedThread &> decoded_thread = Decode(thread))
+    return Error::success();
+  else
+    return decoded_thread.takeError();
 }

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/PluginManager.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
 using namespace lldb_private;
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) {
+  int digits_count = 0;
+  do {
+    digits_count++;
+    num /= 10;
+  } while (num);
+  return digits_count;
+}
+
 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 start_position) {
+  size_t instruction_count = GetInstructionCount(thread);
+  s.Printf("thread #%u: tid = %" PRIu64 ", total instructions = %zu\n",
+           thread.GetIndexID(), thread.GetID(), instruction_count);
+
+  int digits_count =
+      GetNumberOfDigits(std::min(instruction_count, start_position + count));
+
+  if (Error err = GetTraceErrorStatus(thread))
+    s.Printf("  %s\n", llvm::toString(std::move(err)).c_str());
+
+  ForEachInstruction(
+      thread,
+      [&](size_t index, Expected<lldb::addr_t> &load_address) -> bool {
+        if (index >= start_position + count)
+          return false;
+
+        s.Printf("  [%*zu]", digits_count, index);
+        if (load_address)
+          s.Printf(" 0x%" PRIx64, *load_address);
+        else
+          s.Printf(" %s", toString(load_address.takeError()).c_str());
+        s.Printf("\n");
+
+        return true;
+      },
 }

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));
 
   for (const JSONThread &thread : process.threads)

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

 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 
 class TestTraceDumpInstructions(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
     NO_DEBUG_INFO_TESTCASE = True
 
     def setUp(self):
         TestBase.setUp(self)
         if 'intel-pt' not in configuration.enabled_plugins:
             self.skipTest("The intel-pt test plugin is not enabled")
   
     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"))
def testErrorMessages(self):
         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"),
             substrs=["intel-pt"])
 
         self.expect("thread trace dump instructions",
-            substrs=['thread #1: tid = 3842849, total instructions = 1000',
-                     'would print 20 instructions from position 0'])
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  [ 0] 0x40052d
+  [ 1] 0x400529
+  [ 2] 0x400525
+  [ 3] 0x400521
+  [ 4] 0x40052d
+  [ 5] 0x400529
+  [ 6] 0x400525
+  [ 7] 0x400521
+  [ 8] 0x40052d
+  [ 9] 0x400529
+  [10] 0x400525
+  [11] 0x400521
+  [12] 0x40052d
+  [13] 0x400529
+  [14] 0x400525
+  [15] 0x400521
+  [16] 0x40052d
+  [17] 0x400529
+  [18] 0x40051f'''])
 
         # 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'])
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  [10] 0x400525
+  [11] 0x400521
+  [12] 0x40052d
+  [13] 0x400529
+  [14] 0x400525'''])
 
         # 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'])
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  [ 0] 0x40052d'''])
 
         # 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):
         # We load a trace with two threads
         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'''])
+        self.expect("thread trace dump instructions 1 2 --count 2",
+            substrs=['''thread #1: tid = 3842849, total instructions = 21
+  [0] 0x40052d
+  [1] 0x400529
+thread #2: tid = 3842850, total instructions = 21
+  [0] 0x40052d
+  [1] 0x400529'''])
 
         # 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())
+        ci.HandleCommand("thread trace dump instructions 1 2 --count 4 --start-position 5", result, True)
+        self.assertIn('''thread #1: tid = 3842849, total instructions = 21
+  [5] 0x400529
+  [6] 0x400525
+  [7] 0x400521
+  [8] 0x40052d
+thread #2: tid = 3842850, total instructions = 21
+  [5] 0x400529
+  [6] 0x400525
+  [7] 0x400521
+  [8] 0x40052d''', 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())
+        self.assertIn('''thread #1: tid = 3842849, total instructions = 21
+  [ 9] 0x400529
+  [10] 0x400525
+  [11] 0x400521
+  [12] 0x40052d
+thread #2: tid = 3842850, total instructions = 21
+  [ 9] 0x400529
+  [10] 0x400525
+  [11] 0x400521
+  [12] 0x40052d''', result.GetOutput())
 
+        result = lldb.SBCommandReturnObject()
         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.assertIn('''thread #1: tid = 3842849, total instructions = 21
+  [13] 0x400529
+  [14] 0x400525
+  [15] 0x400521
+  [16] 0x40052d
+thread #2: tid = 3842850, total instructions = 21
+  [13] 0x400529
+  [14] 0x400525
+  [15] 0x400521
+  [16] 0x40052d''', result.GetOutput())
+
+    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] no memory mapped at this address
+  [1] 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 = 0
+  Intel PT decoding error -27: 'unknown cpu'''])
+
+    def testMultiFileTraceWithMissingModule(self):
+        self.expect("trace load " + os.path.join(self.getSourceDir(), "intelpt-trace-multi-file", "multi-file-no-ld.json"))
+        
+        # The trace will be composed of the first and last instructions of main.cpp, with an unmapped region in between
+        # corresponding to the dynamic linker, which is missing in the json file. Because of this error, the decoder moves
+        # to the next synchronization point, skipping the instructions of bar.cpp and foo.cpp.
+        self.expect("thread trace dump instructions",
+            substrs=['''thread #1: tid = 815455, total instructions = 11
+  [ 0] 0x40065f
+  [ 1] 0x40065a
+  [ 2] 0x400657
+  [ 3] 0x400654
+  [ 4] no memory mapped at this address
+  [ 5] 0x400516
+  [ 6] 0x400510
+  [ 7] 0x40054b
+  [ 8] 0x400546
+  [ 9] 0x400540
+  [10] 0x40064f'''])
+
+    def testMultiFileTrace(self):
+        self.expect("trace load " + os.path.join(self.getSourceDir(), "intelpt-trace-multi-file", "multi-file.json"))
+        
+        # last instructions of main.cpp
+        self.expect("thread trace dump instructions --count 3",
+            substrs=['''thread #1: tid = 815455, total instructions = 1153''', '''
+  [0] 0x40065f
+  [1] 0x40065a
+  [2] 0x400657'''])
+
+        # bar.cpp
+        self.expect("thread trace dump instructions --count 3 --start-position 4",
+            substrs=['''
+  [4] 0x7ffff7bd9703
+  [5] 0x7ffff7bd9702
+  [6] 0x7ffff7bd96fe'''])
+
+        # foo.cpp
+        self.expect("thread trace dump instructions --count 3 --start-position 13",
+            substrs=['''
+  [13] 0x7ffff79d76a9
+  [14] 0x7ffff79d76a6
+  [15] 0x7ffff79d76a3'''])
+
+        # ld 
+        self.expect("thread trace dump instructions --count 3 --start-position 21",
+            substrs=[ '''
+  [21] 0x7ffff7df1a16
+  [22] 0x7ffff7df1a12
+  [23] 0x7ffff7df1a0e'''])
+
+        # first instructions of main.cpp
+        self.expect("thread trace dump instructions --count 3 --start-position 1150",
+            substrs=[ '''
+  [1150] 0x400546
+  [1151] 0x400540
+  [1152] 0x40064f'''])

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 main() {
+  int res = foo();
+  res++;
+  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": "729BF711-43CB-0017-AADF-83304FEA5EC4-D46B45DD"
+        },
+        {
+          "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-multi-file/multi-file.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": "729BF711-43CB-0017-AADF-83304FEA5EC4-D46B45DD"
+        },
+        {
+          "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"
+        },
+        {
+          "file": "ld-2.17.so",
+          "systemPath": "ld-2.17.so",
+          "loadAddress": "0x00007ffff7ddb000",
+          "uuid": "27FFD1FB-C695-69C7-76E6-66474EED7233-95E6D727"
+        }
+      ]
+    }
+  ]
+}

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"
+        }
+      ]
+    }
+  ]
+}

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

if (!error.Success()) {
       if (error_string.find("tracing not active") == std::string::npos)
         return;
     }
     // Delete the entry of 'tid' from this class (if any)
     mapThreadID_TraceInfo.erase(tid);
   }
 }
 
+#include <fstream>
+
 void Decoder::ReadTraceDataAndImageInfo(lldb::SBProcess &sbprocess,
                                         lldb::tid_t tid, lldb::SBError &sberror,
                                         ThreadTraceInfo &threadTraceInfo) {
   // Allocate trace data buffer and parse cpu info for 'tid' if it is registered
   // for the first time in class
   lldb::SBTrace &trace = threadTraceInfo.GetUniqueTraceInstance();
   Buffer &pt_buffer = threadTraceInfo.GetPTBuffer();
+
   lldb::SBError error;
   if (pt_buffer.size() == 0) {
     lldb::SBTraceOptions traceoptions;
     traceoptions.setThreadID(tid);
     trace.GetTraceConfig(traceoptions, error);
     if (!error.Success()) {
       sberror.SetErrorStringWithFormat("%s; ProcessID = %" PRIu64,
                                        error.GetCString(),
void Decoder::ReadTraceDataAndImageInfo(lldb::SBProcess &sbprocess,
   if (!error.Success()) {
     sberror.SetErrorStringWithFormat(
         "%s; thread_id = %" PRIu64 ",  ProcessID = %" PRIu64,
         error.GetCString(), tid, sbprocess.GetProcessID());
     return;
   }
   std::fill(pt_buffer.begin() + bytes_written, pt_buffer.end(), 0);
 
+  std::ofstream of("/tmp/multi-file.trace", std::ios::binary | std::ios::out);
+  for (auto bait : pt_buffer)
+    of << bait;
+  of.close();
+
   // Get information of all the modules of the inferior
   lldb::SBTarget sbtarget = sbprocess.GetTarget();
   ReadExecuteSectionInfos &readExecuteSectionInfos =
       threadTraceInfo.GetReadExecuteSectionInfos();
   GetTargetModulesInfo(sbtarget, readExecuteSectionInfos, sberror);
   if (!sberror.Success())
     return;
 }