Add a "Trace" plug-in to LLDB to add process trace support in stages.

[wallace]

Summary

This is the first in a series of patches that will adds a new processor trace plug-in to LLDB.

The idea for this first patch to to add the plug-in interface with simple commands for the trace files that can "load" and "dump" the trace information. We can test the functionality and ensure people are happy with the way things are done and how things are organized before moving on to adding more functionality.

Processor trace information can be view in a few different ways:

• post mortem where a trace is saved off that can be viewed later in the debugger
• gathered while a process is running and allow the user to step back in time (with no variables, memory or registers) to see how each thread arrived at where it is currently stopped.

This patch attempts to start with the first solution of loading a trace file after the fact. The idea is that we will use a JSON file to load the trace information. JSON allows us to specify information about the trace like:

• plug-in name in LLDB
• path to trace file
• shared library load information so we can re-create a target and symbolicate the information in the trace
• any other info that the trace plug-in will need to be able to successfully parse the trace information
  • cpu type
  • version info
  • ???

A new "trace" command was added at the top level of the LLDB commmands:

• "trace load"
• "trace dump"

I did this because if we load trace information we don't need to have a process and we might end up creating a new target for the trace information that will become active. If anyone has any input on where this would be better suited, please let me know. Walter Erquinigo will end up filling in the Intel PT specific plug-in so that it works and is tested once we can agree that the direction of this patch is the correct one, so please feel free to chime in with ideas on comments!

[clayborg]

Unresolved issues in this patch:

• For "trace load", I get the plugin for the JSON file by matching it up with the "name" field in the JSON, but I don't store the "trace_sp" anywhere. We will need to store it with the target that we create, or for later commands add it to a target that is stopped when the trace data is loaded via the process interface (through lldb-server is the current thinking for this).
• "trace dump" does nothing for now, but this is what we can use to test that "trace load" worked and was able to create a target.

This patch keeps things really simple for now so we can checkin small testable patches.

[clayborg]

The idea for the JSON file is that it must have a "name" (or maybe "plug-in"?) to identify which trace plug-in and a "trace-file" at the very least:

{
  "name": "intel-pt", 
  "trace-file": "/tmp/trace-info" 
}

The "name" field would match the name of the trace plug-in in LLDB.

For intel PT they need info about the CPU. Some intel PT libraries have a structure like:

// A cpu vendor.
enum pt_cpu_vendor {
    pcv_unknown,
    pcv_intel
};

struct pt_cpu {
  enum pt_cpu_vendor vendor;
  uint16_t family;
  uint8_t model;
  uint8_t stepping;
};

So the trace JSON file for intel-pt might look like:

{ 
  "name": "intel-pt", 
  "trace-file": "/tmp/a",
  "pt_cpu": {
    "vendor": "intel",
    "family": 10,
    "model": 2,
    "stepping": 1
  }
}

[vsk]

This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable.

For "trace load", I get the plugin for the JSON file by matching it up with the "name" field in the JSON, but I don't store the "trace_sp" anywhere. We will need to store it with the target that we create, or for later commands add it to a target that is stopped when the trace data is loaded via the process interface (through lldb-server is the current thinking for this).

Have you considered what might happen if there are multiple targets covered by a single trace? Strawman proposal: would it make sense to register the trace with a Debugger instance? This can be a list of traces if it makes sense to support debugging more than one trace at a time.

"trace dump" does nothing for now, but this is what we can use to test that "trace load" worked and was able to create a target.

It'd be great to have some test for this, even if all 'trace load' does at this point is print an error about bad JSON input.

lldb/include/lldb/Target/Trace.h

28

nit: 'may have', or omit the 'may'?

lldb/source/Commands/CommandObjectTrace.cpp

2

nit: formatting seems a bit off here?

lldb/source/Commands/Options.td

207

nit: seems like an accidental change here?

754

ditto

[clayborg]

In D85705#2211073, @vsk wrote:

This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable.

For "trace load", I get the plugin for the JSON file by matching it up with the "name" field in the JSON, but I don't store the "trace_sp" anywhere. We will need to store it with the target that we create, or for later commands add it to a target that is stopped when the trace data is loaded via the process interface (through lldb-server is the current thinking for this).

Have you considered what might happen if there are multiple targets covered by a single trace? Strawman proposal: would it make sense to register the trace with a Debugger instance? This can be a list of traces if it makes sense to support debugging more than one trace at a time.

I hadn't thought of that but that does make sense!

We can work this all into the JSON format. We should actually make a schema for the common parts of information that should be represented in the JSON and also allow each plug-in to supply a schema for the parts that is requires in the JSON.

Some ideas that this information should contain:

• array of process infos dictionaries
• process info dictionary
  • pid
  • array of shared library dictionaries
• shared library dictionary:
  • original path
  • UUID if available
  • MD5 of file if no UUID
  • load location
  • optional URL to download

And LLDB will easily be able to load up N targets with everything setup correctly.

"trace dump" does nothing for now, but this is what we can use to test that "trace load" worked and was able to create a target.

It'd be great to have some test for this, even if all 'trace load' does at this point is print an error about bad JSON input.

I agree! If we like this format, Walter Erquinigo can commandeer this revision and fill in actual Intel PT guts and have this work. The idea is I am going to get the infrastructure in place and once we work this out, I will let Walter take over the patch and actually fill it in with real Intel PT stuff and test it fully.

lldb/source/Commands/Options.td

207

yeah, my editor removes trailing spaces.

[vsk]

In D85705#2211607, @clayborg wrote:

In D85705#2211073, @vsk wrote:

This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable.

For "trace load", I get the plugin for the JSON file by matching it up with the "name" field in the JSON, but I don't store the "trace_sp" anywhere. We will need to store it with the target that we create, or for later commands add it to a target that is stopped when the trace data is loaded via the process interface (through lldb-server is the current thinking for this).

Have you considered what might happen if there are multiple targets covered by a single trace? Strawman proposal: would it make sense to register the trace with a Debugger instance? This can be a list of traces if it makes sense to support debugging more than one trace at a time.

I hadn't thought of that but that does make sense!

We can work this all into the JSON format. We should actually make a schema for the common parts of information that should be represented in the JSON and also allow each plug-in to supply a schema for the parts that is requires in the JSON.

Some ideas that this information should contain:

• array of process infos dictionaries
• process info dictionary
  • pid
  • array of shared library dictionaries
• shared library dictionary:
  • original path
  • UUID if available
  • MD5 of file if no UUID
  • load location
  • optional URL to download

And LLDB will easily be able to load up N targets with everything setup correctly.

I'm generally on board with this. On Darwin we tend not to have the original path to a DSO available, so I'd argue that that ought to be optional as well. Really the only mandatory stuff for debugging purposes is the load address and some kind of UUID.

"trace dump" does nothing for now, but this is what we can use to test that "trace load" worked and was able to create a target.

It'd be great to have some test for this, even if all 'trace load' does at this point is print an error about bad JSON input.

I agree! If we like this format, Walter Erquinigo can commandeer this revision and fill in actual Intel PT guts and have this work. The idea is I am going to get the infrastructure in place and once we work this out, I will let Walter take over the patch and actually fill it in with real Intel PT stuff and test it fully.

[clayborg]

Fix inline comment issues from vsk.

[aprantl]

I only added a few non-substantial inline comments.

lldb/include/lldb/Target/Trace.h

39

I'm just curious: What's the purpose of this? Is the destructor = 0 in the base class and we want to not have to write this line in Trace implementations?

lldb/source/Commands/CommandObjectTrace.cpp

88

Would it be possible to early-exit-ify this function? Perhaps by using a lambda for the result.AppendErrorWithFormat error return part?

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

25

FYI: The Doxygen way of writing this is

/// Static Functions.
/// \{
  static void Initialize();
  static void Terminate();
/// \}

[wallace]

Thanks for doing this. There are some basic bits to make this that I was unaware of, so this saves a lot of time for me :)
I'll take over this patch next week and fill it with intel-pt stuff, I already have a good amount of code that is quite mergeable with this patch.

[wallace]

• Added libipt as a dependency to build the new intel pt plugin. It's merely a copy paste of what the old intel pt plugin does.
• Added a test with a sample trace.json definition file. It includes a simple a.out object file, the source and an intel pt trace that goes from main to the final return 0. The test checks that the plugin does the parsing of the structured data correctly and creates the target, process and modules correctly.
• Added the necessary parsing functionalities in Trace.cpp to make sense of the structured data.
• Added a simple parsing of the intel pt-specific information in the structured data.

I didn't implement Dump nor Decoding, as in this case the test is quite comprehensive regarding Load and I want to keep the diff small. I'll implement the other features later.

[clayborg]

Lots of little things regarding the encoding and format of the JSON.

lldb/source/Target/Trace.cpp

38

Dump schema here as part of the error?

54

dump schema when we have an error?

66

dump schema?

106–107

rename "filePath" to just "path"?

110

Should load address be encoded as an integer to begin with? I know it is less readable as an integer since we can't use hex numbers It would simplify the logic here. If we want to use strings, we should make a helper function that decodes an address from a key's value in a dictionary so we can re-use elsewhere.

110–111

does JSON typically use camel case? Should this be "load-address"?

117–120

We shouldn't assume that "file_path" is relative. This code should be something like:

FileSpec file_spec(file_path);
if (file_spec.IsRelative())
  file_spec.PrependPathComponent(m_info_dir);

128

We should store a target triple as a mandatory key/value pair in the top level trace JSON file and access via a getter. Then we should also fill out a ModuleSpec with a path, UUID (optional) and architecture to make sure we don't end up getting the wrong file:

ModuleSpec module_spec;
module_spec.GetFileSpec() = file_spec;
module_spec.GetArchitecture() = Trace::GetArchitecture(); // This would be a new accessor that will hand out a architecture for the "triple" setting in the JSON trace settings
StringRef uuid_str;
if (module->GetValueForKeyAsString("uuid", uuid_str))
  module_spec.GetUUID().SetFromStringRef(uuid_str);
lldb::ModuleSP module_sp = target_sp->GetOrCreateModule(module_spec, false /* notify */, &error);

We wouldn't want to accidentally load "/usr/lib/libc.so" on a different machine with the wrong architecture since "libc.so" can exist on many systems.

132

Remove this since we create the module above already in the target in my inline comment code changes.

182

This should be more generic like:

Trace::ParseSettings(...)

We will add more top level key/value pairs that follow a structured data schema which we should make available through a command eventually, so lets not limit this to just processes. We might add architecture or target triple, and much more in the future.

199

Maybe we should confine all plug-in specific settings to a key/value pair where the key is "arguments" or "plug-in-arguments" and the value is a dictionary. This will help ensure that no plug-in specific settings can ever conflict.

{ 
  "type": "intel-pt",
  "processes": [...],
  ....  (other settings that are for this "lldb_private::Trace::ParseSettings(...)")
  "arguments": {
      ... (plug-in specific arguments go here)
  }
}

Or maybe a structure like:

{
  "plug-in": {
    "name": "intel-pt",
    "arguments: {
      ... (TraceIntelPT plug-in specific arguments go here)
    }
  }
  "processes": [...],
  "triple": "armv7-apple-ios",
  ....  (other settings that are for this "lldb_private::Trace::ParseSettings(...)")
}

[wallace]

lldb/source/Target/Trace.cpp

110

The way this is implemented is that it's expecting a string number in any radix. You can pass "123123" or "0xFFAABBFF" for example. You cannot pass it directly as a simple number because JSON doesn't support 64-bit integers

110–111

yes, it's camel case typically

[wallace]

• Addressed comments.
• Added schemas for both the base class and the implementation plugins.
• Added two tests checking the error messages along with the schema when parsing failed.
• Made some general clean up of the code

[wallace]

• Added a command that shows the schema of a given trace plugin.
• Added a test for such command

[JDevlieghere]

A large part of this patch is concerned with parsing which worries me from a maintenance perspective. Did you consider using Yaml I/O? While I'm not a particularly big fan of the format, the benefits of being able to (de)serialize any class by implementing the appropriate traits are quite significant. We already have traits implemented for a bunch of utility classes, such as ArchSpec and FileSpec which we could reuse for this. I know changing the format would be invasive, but I think it might be worth it in the long term.

[wallace]

That's a very good idea! I'll revisit this patch then. I was not fond of doing the manual parsing but I hadn't found a more automated way.

[clayborg]

Looking better. The main thing we need to do it modify StructuredData classes a bit by moving a lot of the static functions we are using here into the appropriate classes. See inlined comments.

lldb/source/Target/Trace.cpp

60

Do we want an optional "trace-file" here in case trace files can contain all data for all processes?

63

Do we want an optional "trace-file" here in case trace files can contain all data for a single process?

67

Do all trace files have one file per thread? Should this be "trace-file" instead of "traceFile"?

Might make sense to make this optional and also include an optional traceFile at the process level and possibly one at the root level? See inline comments above

72

This should probably be optional as we won't always have the file copied into the trace directory?

Do we want a "system-path" for the original path here?

73

"load-address" instead of "loadAddress" and should be an integer instead of a string.

115–124

This entire function should go into the StructuredData::Array class for re-use

126–134

This entire function should go into StructuredData::Dictionary so other code can re-use this.

140

Seems like we should modify GetValueForKeyAsInteger to take a pointer to an error as the 3rd parameter here? Then that can return an appropriate error in case there is a "tid" entry, but it isn't an integer.

152

Seems like we should modify GetValueForKeyAsArray to take an pointer to an error as the 3rd parameter here? Then that can return an appropriate error in case there is a "threads" entry, but it isn't an array.

157

This functionality should be moved into StructuredData::Array::ExtractItemAsArray(...)

169

"load-address" might be a better identifier and it is also weird to store an address as a string. Should be an integer.

194–195

This will need to be able to fail gracefully here. We might not always have the module file on disk. We might need to create a Placeholder module like the ProcessMinidump does.

[clayborg]

After speaking with Walter a bit, key names in the JSON should be camel case. Many languages (Swift and JavaScript) can auto import JSON and create objects and use the key names as member variable names, and if they are camelCase, then no name conversion needs to happen.

[clayborg]

In D85705#2237397, @JDevlieghere wrote:

A large part of this patch is concerned with parsing which worries me from a maintenance perspective. Did you consider using Yaml I/O? While I'm not a particularly big fan of the format, the benefits of being able to (de)serialize any class by implementing the appropriate traits are quite significant. We already have traits implemented for a bunch of utility classes, such as ArchSpec and FileSpec which we could reuse for this. I know changing the format would be invasive, but I think it might be worth it in the long term.

So the nice thing about using StructuredData is it can be in any format: JSON, XML, Apple property list, YAML etc. It seems like the functions that were added to ArchSpec and FileSpec for the YAML I/O could be converted to use StructuredData and then any of the formats would work.

[JDevlieghere]

In D85705#2237506, @clayborg wrote:

In D85705#2237397, @JDevlieghere wrote:

A large part of this patch is concerned with parsing which worries me from a maintenance perspective. Did you consider using Yaml I/O? While I'm not a particularly big fan of the format, the benefits of being able to (de)serialize any class by implementing the appropriate traits are quite significant. We already have traits implemented for a bunch of utility classes, such as ArchSpec and FileSpec which we could reuse for this. I know changing the format would be invasive, but I think it might be worth it in the long term.

So the nice thing about using StructuredData is it can be in any format: JSON, XML, Apple property list, YAML etc. It seems like the functions that were added to ArchSpec and FileSpec for the YAML I/O could be converted to use StructuredData and then any of the formats would work.

If we invest in creating a library like Yaml IO for StructuredData, which allows you to declaratively specify how a class gets (de)serialized, then I'm all for using it here and for the reproducers. I think that would be really, really valuable. Given the way that structured data works and how similar it is to how these formats look, I'm also fairly confident it can be done. Still it would be a significant amount of work. But barring that I don't see a benefit from switching from YAML I/O to StructuredData, at least not for the reproducers and not really for this either. Unless we need to be able to support all these formats? Like I said, I don't really care about YAML. What I do care about is that I don't want to be in the business of writing a parser for every data structure I need to serialize. That's really where Yaml I/O shines, even though it has other drawbacks, like the traits themselves being verbose and the parser being far from fast.

[wallace]

Given that this feature is intended to be used by non debugger developers, I want to stick to using JSON as a default input format, as it's more ubiquitous than YAML and I prefer to make this decision based on the user experience than on the technical solution. I like the YAML parser though, but converting JSON to YAML for parsing might throw away relevant error messaging.

[wallace]

• As error messaging is important when parsing, I create a new set of functions in StructuredData for parsing that return an llvm::Expected. This allowed me to move the helper functions for parsing from the Trace file to StructuredData, so that in the future other contributors can

use those methods when needed.

• I modified the schema to allow for the "traceFile" entry at several levels.
• "systemPath" was also added for the module entry
• I didn't use the PlaceholderObject yet, as Greg suggested. That might involve a few other changes and I need to see if that class is sufficient for decoding intel-pt traces. However, as soon as I'm working on the decoding part, I'll move to PlaceholderObject, as it will be nice to avoid failing hard if a module is not available.
• I simplified the code here and there as well.
• I'm still using strings for addresses, like "0xFFFFFFFF" or "12312412". After checking some recommendations on stack overflow, using strings seems to be a good practice for having compatibility with JS, which has 54-bit sized integers. I imagine that these trace files might be send over the network and JS could potentially be used then.

[JDevlieghere]

In D85705#2240249, @wallace wrote:

Given that this feature is intended to be used by non debugger developers, I want to stick to using JSON as a default input format, as it's more ubiquitous than YAML and I prefer to make this decision based on the user experience than on the technical solution.

Alright, fair enough I guess. In that case I would prefer to split off the parser from the Trace data structure.

I like the YAML parser though, but converting JSON to YAML for parsing might throw away relevant error messaging.

I don't think anyone suggested this.

One more thing about the parser. I really like the usage of llvm::Error/Excepted/Optional in this path btw. However, these functions take a non-const ref to classes like Process, Thread, etc. It seems like they might be left in a potentially inconsistent state when we error out. Is that something to worry about or are we guaranteed to discard them in the caller if an error occurred? Could they for instance return an Expected<Debugger> instead?

lldb/include/lldb/Target/Trace.h

87

StringRef

106

StringRef

128

StringRef

151

StringRef

lldb/include/lldb/Utility/StructuredData.h

196 ↗

(On Diff #288145)

StringRef

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

4

Move this up to where this directory is included. e.g.

if (LLDB_BUILD_INTEL_PT)
  add_subdirectory(intel-pt)
endif()

That way a change to the CMake file won't trigger a reconfig if LLDB_BUILD_INTEL_PT is false.

23

Why not pass this in the first place? PATHS will be checked *in addition* to the default locations.

27

If you REQUIRED to find_library CMake will take care of the error reporting.

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

23

Redundant.

55

I think it would be nice to separate the plugin from the parser.

lldb/source/Target/Trace.cpp

28

StringRef

51

StringRef

147

StringRef should always be passed by value.

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

2 ↗

(On Diff #288145)

It seems like the key here should be trace rather than "plugin"?

[wallace]

Thanks for the review! Those are very useful comments. So, I'll split the parsing out from the Trace object. Regarding a possible inconsistent state, you are right. That could happen. The targets, modules and processes used in the parsing are all created there, and it should be enough to only "commit" those targets if there has been no error. I'll make the necessary updates.

[labath]

If the format is going to be json, why not use llvm::json from llvm/Support/JSON.h? That's what we been migrating most of the existing stuff to already, so going using that for new code makes perfect sense.

[clayborg]

In D85705#2241249, @labath wrote:

If the format is going to be json, why not use llvm::json from llvm/Support/JSON.h? That's what we been migrating most of the existing stuff to already, so going using that for new code makes perfect sense.

I am fine with this if we are ok always using JSON. StructuredData, when it parses JSON, is backed by llvm/Support/JSON.h, so we are already using it. If we don't need the flexibility to use other structured data formats we can do this.

lldb/include/lldb/Target/Trace.h

158

I know I suggested we might need a trace file for all process, one per process, or one per thread in the JSON schema. The question is do we need to add support for this right away? Might be easier to go with one trace file for now unless IntelPT already supports each of these?

lldb/source/Target/Trace.cpp

55

Can we inline the plug-ins schema right into this stream instead of putting the "<<<plugin_schema>>>" text in here?

[wallace]

Addressed comments

• Now using StringRef correctly whenever possible.
• Revert back to using "traceFile" only inside the thread section. That's how intel-pt works at the moment and, as Greg suggested, we can change the schema in the future if needed.
• Now the global schema construction inlines the plug-in schema.
• Simplified the CMake logic
• Separated the parser from the plugin. Now new plugins need to derive their own Trace implementation along with their own Parser.
• I'm still using StructuredData for the parsing. There's a chance that once we release this feature users will want support for other formats besides JSON, so for now I prefer to ask for JSON input but parse in a format-agnostic way. If eventually no one needs any other format, we can switch to JSON-only parsing.
• Also, now the Targets created during parsing are preserved only if the parsing succeeded. Otherwise, the Debugger object is left unchanged.

[labath]

In D85705#2243536, @wallace wrote:

• I'm still using StructuredData for the parsing. There's a chance that once we release this feature users will want support for other formats besides JSON, so for now I prefer to ask for JSON input but parse in a format-agnostic way. If eventually no one needs any other format, we can switch to JSON-only parsing.

That argument goes both ways. We could say that if anyone wants to add a non-json format, he can change the code to accept his preferred format. Overall, I am very sceptical of the StructuredData's ability to abstract diverse formats in a meaningful way. I doubt that a natural XML representation of this data would map to the same abstract format as this json and that it could be parsed by the same code -- simply because xml conventions are different, and xml has features that json doesn't (attributes, for one).

And since this file is just a descriptor, and actual plugin-specific data is contained in other files, I don't see why it's format couldn't be fixed.

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

11 ↗

(On Diff #288496)

What if one of the processes is 64-bit and the other 32? It sounds like this should be a property of a particular process.

[wallace]

You changed my mind. I'll move to Support/JSON.h

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

11 ↗

(On Diff #288496)

That's a very good point. Will do so

[wallace]

Addressed all comments.

• Wwitched to using llvm::json instead of StructuredData. Fortunately, the logic is pretty much the same.
• Moved the "triple" field to the process level

[wallace]

fix typo

[clayborg]

A few simple changes from inlined comments. Other than that looks good. Should get another OK from others. Also not sure if the JSON stuff in llvm needs to be done separately?

lldb/include/lldb/Target/Trace.h

116

would std::unique_ptr<> might be better? Are we actually handing out multiple instances of this and sharing it anywhere?

lldb/include/lldb/lldb-private-interfaces.h

14

We should avoid this import and use forward declaration here below the includes:

namespace llvm {
  namespace json {
    class Object;
  }
}

lldb/source/Commands/Options.td

206–207

revert whitespace only changes.

754

revert whitespace only changes.

[wallace]

• Removed whitespace only changes.
• Simplified the way a SettingsParser is created, now there's no need to passinga a pointer around.
• Fixed the forward declaration in lldb-private-interfaces.

This should address all comments so far except for the llvm::json changes. I'm okay with creating another patch with those changes if the reviewers prefer that. They are simple, though.

[clayborg]

LGTM now. Probably should get an ok from one more person.

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

1 ↗

(On Diff #288788)

Remove "//" after filename

[wallace]

fix nit. Waiting for an additional approval

[aprantl]

lldb/source/Commands/CommandObjectTrace.h

2

Can you add -*- C++ -*- markers the the .h files you are adding? Otherwise tools (like phabricator) will think it's a C header.

[wallace]

fix header c++ declaration

[labath]

I'm pretty sure the json changes should get a separate review. The json owner (let's call him @sammccall) might have a different idea on how to do this thing...

[wallace]

• Moved the JSON changes to another diff.
• Addressed all the comments so far.

As a side note, I'll work on a RFC for the LLDB mailing list to get feedback on the overall design of the feature, as @labath suggested. However, Greg and I want to get these patches in, as we need this functionality soon. We can easily make modifications whenever suggestions come, following an iterative approach to development.

[clayborg]

This looks good to me. We will need to wait until the JSON changes from https://reviews.llvm.org/D87335 to make it in though in case there are any modifications to that patch.

Walter will put up the RFC for trace plug-ins and if that differs from this patch much we can modify this to match. I don't see a need to hold up the patch since this won't affect anyone as it is new functionality that doesn't affect normal debugging or other workflows.

[wallace]

Based on the discussion from D87335, I'm moving all the JSON utilities as non-member functions to the settings parser file. Eventually this can be refactored if that patch moves forward.

There aren't any other pending issues.

[MaskRay]

lldb/source/Target/TraceSettingsParser.cpp

123 ↗

(On Diff #293299)

This requires sstream. Fixed.

128 ↗

(On Diff #293299)

The replacement here looks strange.

<regex> implementations tend to be large, slow and buggy. It'd be best if <regex> can be avoided. (llvm has llvm/Support/Regex.h)

[wallace]

lldb/source/Target/TraceSettingsParser.cpp

128 ↗

(On Diff #293299)

will do!

[labath]

I have two high-level remarks/questions about this:

• I am surprised that it was not necessary to create a special process plugin for this purpose. I have a feeling one will be necessary sooner or later because of the need to customize the step/continue/etc. flows. Currently, this will probably produce very bizarre if one tries to execute those commands. The reason I'm bringing this up is because of the Target::GetTrace method being added in the next patch. If there is a trace-specific process class, then it might make sense for this class to hold the trace object instead of adding the GetTrace method on every Target object out there even though most targets will have that null. I don't know if that will really be the case, but I think it's something worth keeping in mind as you work on the subsequent patches.
• I am puzzled by the TraceXXX vs. TraceXXXSettingsParser duality. The two classes are very tightly coupled, so it's not clear to me what is the advantage of separating it out this way (one could just move all the relevant methods directly into the Trace class. What's the reason for this design?

[wallace]

• I am surprised that it was not necessary to create a special process

plugin for this purpose. I have a feeling one will be necessary sooner or
later because of the need to customize the step/continue/etc. flows.
Currently, this will probably produce very bizarre if one tries to execute
those commands. The reason I'm bringing this up is because of the
Target::GetTrace method being added in the next patch. If there is a
trace-specific process class, then it might make sense for this class to
hold the trace object instead of adding the GetTrace method on every Target
object out there even though most targets will have that null. I don't know
if that will really be the case, but I think it's something worth keeping
in mind as you work on the subsequent patches.

Very good remark. Probably we'll end up doing that when we start
implementing reverse debugging. The tricky thing we'll need to solve in the
future is seamlessly transition between a trace and a live process. For
example, when reverse debugging a live process, you might be stopped at a
breakpoint in the trace, then you do "continue", it reaches the end of the
trace, and then it resumes the live process. I still haven't decided what
we'll propose for this, but probably we'll have to change a lot of the
current code to make that happen nicely.

• I am puzzled by the TraceXXX vs. TraceXXXSettingsParser duality. The

two classes are very tightly coupled, so it's not clear to me what is the
advantage of separating it out this way (one could just move all the
relevant methods directly into the Trace class. What's the reason for this
design?

Well, there's definitely coupling, but there are two reasons. One is that
the Trace of a live process won't need any parsing. Parsing is only used
when loading a trace from a json file. That makes parsing one of the two
main ways to create a Trace. And besides, I think that the single
responsibility principle is good to follow. The Parser class does the
parsing, and the Trace class holds an actual trace.

Il giorno gio 1 ott 2020 alle ore 07:12 Pavel Labath via Phabricator <
[e-mail]> ha scritto:

labath added a comment.

I have two high-level remarks/questions about this:

• I am surprised that it was not necessary to create a special process

plugin for this purpose. I have a feeling one will be necessary sooner or
later because of the need to customize the step/continue/etc. flows.
Currently, this will probably produce very bizarre if one tries to execute
those commands. The reason I'm bringing this up is because of the
Target::GetTrace method being added in the next patch. If there is a
trace-specific process class, then it might make sense for this class to
hold the trace object instead of adding the GetTrace method on every Target
object out there even though most targets will have that null. I don't know
if that will really be the case, but I think it's something worth keeping
in mind as you work on the subsequent patches.

• I am puzzled by the TraceXXX vs. TraceXXXSettingsParser duality. The two

classes are very tightly coupled, so it's not clear to me what is the
advantage of separating it out this way (one could just move all the
relevant methods directly into the Trace class. What's the reason for this
design?

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D85705/new/

https://reviews.llvm.org/D85705

[clayborg]

In D85705#2305994, @labath wrote:

I have two high-level remarks/questions about this:

• I am surprised that it was not necessary to create a special process plugin for this purpose. I have a feeling one will be necessary sooner or later because of the need to customize the step/continue/etc. flows. Currently, this will probably produce very bizarre if one tries to execute those commands. The reason I'm bringing this up is because of the Target::GetTrace method being added in the next patch. If there is a trace-specific process class, then it might make sense for this class to hold the trace object instead of adding the GetTrace method on every Target object out there even though most targets will have that null. I don't know if that will really be the case, but I think it's something worth keeping in mind as you work on the subsequent patches.

Eventually we should probably have a HistoryProcess class, much like the HistoryThread. This new class could then be used as a base class for the ProcessMachCore and ProcessELFCore and ProcessMinidump. This base class can then stub out the flow control (continue, step, stop etc) and all other virtual methods that are stubbed out in these classes. It can also add accessors for adding HistoryThread objects if needed. So we should probably move this way at some point.

When we have a live process, we will use the current process when we can, but the process and target will need to coordinate to be able to know if we have reversed stepped into the past so that the flow control (forward/reverse continue, forward/reverse step) can do the right things.

[wallace]

I totally agree with you, I didn't think about creating custom callbacks

P. I'll refactor the code accordingly. Thanks, man

Il giorno ven 2 ott 2020 alle ore 01:51 Pavel Labath <[e-mail]> ha
scritto:

lldb/include/lldb/Core/PluginManager.h

 //===-- PluginManager.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_CORE_PLUGINMANAGER_H
 #define LLDB_CORE_PLUGINMANAGER_H
 
 #include "lldb/Core/Architecture.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Utility/CompletionRequest.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "llvm/ADT/StringRef.h"
public:
   static bool RegisterPlugin(ConstString name, const char *description,
                              SymbolVendorCreateInstance create_callback);
 
   static bool UnregisterPlugin(SymbolVendorCreateInstance create_callback);
 
   static SymbolVendorCreateInstance
   GetSymbolVendorCreateCallbackAtIndex(uint32_t idx);
 
+  // Trace
+  static bool RegisterPlugin(ConstString name, const char *description,
+                             TraceCreateInstance create_callback);
+
+  static bool UnregisterPlugin(TraceCreateInstance create_callback);
+
+  static TraceCreateInstance GetTraceCreateCallback(ConstString plugin_name);
+
   // UnwindAssembly
   static bool RegisterPlugin(ConstString name, const char *description,
                              UnwindAssemblyCreateInstance create_callback);
 
   static bool UnregisterPlugin(UnwindAssemblyCreateInstance create_callback);
 
   static UnwindAssemblyCreateInstance
   GetUnwindAssemblyCreateCallbackAtIndex(uint32_t idx);

lldb/include/lldb/Target/Trace.h

+//===-- Trace.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_TARGET_TRACE_H
+#define LLDB_TARGET_TRACE_H
+
+#include "lldb/Core/PluginInterface.h"
+#include "lldb/Utility/Status.h"
+#include "lldb/Utility/StructuredData.h"
+#include "lldb/lldb-private.h"
+#include "llvm/Support/Error.h"
+
+namespace lldb_private {
+
+class StructuredData;
+
+/// \class Trace Trace.h "lldb/Target/Trace.h"
+/// A plug-in interface definition class for trace information.
+///
+/// Trace plug-ins allow processor trace information to be loaded into LLDB so
+/// that the data can be dumped, used for reverse and forward stepping to allow
+/// introspection into the reason your process crashed or found its way to its
+/// current state.
+///
+/// Trace information can be loaded into a target without a process to allow
+/// introspection of the trace information during post mortem analysis, such as
+/// 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:
+  ~Trace() override = default;
+
+  /// 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.
+  virtual void Dump(Stream *s) const = 0;
+
+  /// Find a trace plug-in using structured data.
+  ///
+  /// When loading trace data from disk, the information for the trace data
+  /// can be contained in multiple files and require plug-in specific
+  /// information about the CPU. Using structured data like JSON provides an
+  /// easy way to specify all of the settings and information that we will need
+  /// to load trace data into LLDB. This structured data can include:
+  ///   - The plug-in name (this allows a specific plug-in to be selected)
+  ///   - Architecture or target triple
+  ///   - one or more paths to the trace data file on disk
+  ///     - core trace data
+  ///     - thread events or related information
+  ///   - shared library load information to use for this trace data that
+  ///     allows a target to be created so the trace information can be
+  ///     symbolicated so that the trace information can be displayed to the
+  ///     user
+  ///     - shared library path
+  ///     - load address
+  ///     - UUID
+  ///     - information on how to fetch the shared library
+  ///       - path to locally cached file on disk
+  ///       - URL to download the file
+  ///   - Any information needed to load the trace file
+  ///     - CPU information
+  ///     - Custom plug-in information needed to decode the trace information
+  ///       correctly.
+  ///
+  ///
+  /// \param[in] info
+  ///     Structured data in the form of a dictionary.
+  static llvm::Expected<lldb::TraceSP>
+  FindPlugin(StructuredData::Dictionary info);
+
+protected:
+  Trace(StructuredData::Dictionary info) : m_info(std::move(info)) {}
+
+private:
+  Trace(const Trace &) = delete;
+  const Trace &operator=(const Trace &) = delete;
+
+  // Structured data that holds all settings for this trace session.
+  StructuredData::Dictionary m_info;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_TRACE_H

lldb/include/lldb/lldb-forward.h

 class ThreadPlanRunToAddress;
 class ThreadPlanStepInstruction;
 class ThreadPlanStepOut;
 class ThreadPlanStepOverBreakpoint;
 class ThreadPlanStepRange;
 class ThreadPlanStepThrough;
 class ThreadPlanTracer;
 class ThreadSpec;
+class Trace;
 class TraceOptions;
 class Type;
 class TypeAndOrName;
 class TypeCategoryImpl;
 class TypeCategoryMap;
 class TypeEnumMemberImpl;
 class TypeEnumMemberListImpl;
 class TypeFilterImpl;

 typedef std::weak_ptr<lldb_private::Target> TargetWP;
 typedef std::shared_ptr<lldb_private::TargetProperties> TargetPropertiesSP;
 typedef std::shared_ptr<lldb_private::Thread> ThreadSP;
 typedef std::weak_ptr<lldb_private::Thread> ThreadWP;
 typedef std::shared_ptr<lldb_private::ThreadCollection> ThreadCollectionSP;
 typedef std::shared_ptr<lldb_private::ThreadPlan> ThreadPlanSP;
 typedef std::weak_ptr<lldb_private::ThreadPlan> ThreadPlanWP;
 typedef std::shared_ptr<lldb_private::ThreadPlanTracer> ThreadPlanTracerSP;
+typedef std::shared_ptr<lldb_private::Trace> TraceSP;
 typedef std::shared_ptr<lldb_private::TraceOptions> TraceOptionsSP;
 typedef std::shared_ptr<lldb_private::Type> TypeSP;
 typedef std::weak_ptr<lldb_private::Type> TypeWP;
 typedef std::shared_ptr<lldb_private::TypeCategoryImpl> TypeCategoryImplSP;
 typedef std::shared_ptr<lldb_private::TypeImpl> TypeImplSP;
 typedef std::shared_ptr<lldb_private::TypeMemberFunctionImpl>
     TypeMemberFunctionImplSP;
 typedef std::shared_ptr<lldb_private::TypeEnumMemberImpl> TypeEnumMemberImplSP;

lldb/include/lldb/lldb-private-interfaces.h

 //===-- lldb-private-interfaces.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_LLDB_PRIVATE_INTERFACES_H
 #define LLDB_LLDB_PRIVATE_INTERFACES_H
 
 #if defined(__cplusplus)
 
+#include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-types.h"
 #include <memory>
 #include <set>
 
 namespace lldb_private {

 typedef lldb::TypeSystemSP (*TypeSystemCreateInstance)(
     lldb::LanguageType language, Module *module, Target *target);
 typedef lldb::REPLSP (*REPLCreateInstance)(Status &error,
                                            lldb::LanguageType language,
                                            Debugger *debugger, Target *target,
                                            const char *repl_options);
 typedef int (*ComparisonFunction)(const void *, const void *);
 typedef void (*DebuggerInitializeCallback)(Debugger &debugger);
+typedef lldb::TraceSP (*TraceCreateInstance)(StructuredData::Dictionary info);
 
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)
 
 #endif // LLDB_LLDB_PRIVATE_INTERFACES_H

lldb/source/Commands/CMakeLists.txt

add_lldb_library(lldbCommands
   CommandObjectRegister.cpp
   CommandObjectReproducer.cpp
   CommandObjectSession.cpp
   CommandObjectSettings.cpp
   CommandObjectSource.cpp
   CommandObjectStats.cpp
   CommandObjectTarget.cpp
   CommandObjectThread.cpp
+  CommandObjectTrace.cpp
   CommandObjectType.cpp
   CommandObjectVersion.cpp
   CommandObjectWatchpoint.cpp
   CommandObjectWatchpointCommand.cpp
 
   LINK_LIBS
     lldbBase
     lldbBreakpoint

lldb/source/Commands/CommandObjectTrace.h

+//===-- CommandObjectTrace.h ----------------------------------------------===//
+//
+// 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_COMMANDS_COMMANDOBJECTTRACE_H
+#define LLDB_SOURCE_COMMANDS_COMMANDOBJECTTRACE_H
+
+#include "lldb/Interpreter/CommandObjectMultiword.h"
+
+namespace lldb_private {
+
+class CommandObjectTrace : public CommandObjectMultiword {
+public:
+  CommandObjectTrace(CommandInterpreter &interpreter);
+
+  ~CommandObjectTrace() override;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_SOURCE_COMMANDS_COMMANDOBJECTTRACE_H

lldb/source/Commands/CommandObjectTrace.cpp

+//===-- CommandObjectTrace.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 "CommandObjectTrace.h"
+
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/OptionParser.h"
+#include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/CommandObject.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Interpreter/OptionGroupFormat.h"
+#include "lldb/Interpreter/OptionValueBoolean.h"
+#include "lldb/Interpreter/OptionValueLanguage.h"
+#include "lldb/Interpreter/OptionValueString.h"
+#include "lldb/Interpreter/Options.h"
+#include "lldb/Target/Trace.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace llvm;
+
+// CommandObjectTraceLoad
+#define LLDB_OPTIONS_trace_load
+#include "CommandOptions.inc"
+
+#pragma mark CommandObjectTraceLoad
+
+class CommandObjectTraceLoad : public CommandObjectParsed {
+public:
+  class CommandOptions : public Options {
+  public:
+    CommandOptions() : Options() { OptionParsingStarting(nullptr); }
+
+    ~CommandOptions() override = default;
+
+    Status SetOptionValue(uint32_t option_idx, StringRef option_arg,
+                          ExecutionContext *execution_context) override {
+      Status error;
+      const int short_option = m_getopt_table[option_idx].val;
+
+      switch (short_option) {
+      case 'v': {
+        bool success = false;
+        bool value = OptionArgParser::ToBoolean(option_arg, false, &success);
+        if (success)
+          m_verbose = value;
+        else
+          error.SetErrorStringWithFormat("invalid boolean option: \"%s\"",
+                                         option_arg.str().c_str());
+        break;
+      }
+      default:
+        llvm_unreachable("Unimplemented option");
+      }
+      return error;
+    }
+
+    void OptionParsingStarting(ExecutionContext *execution_context) override {
+      m_verbose = false;
+    }
+
+    ArrayRef<OptionDefinition> GetDefinitions() override {
+      return makeArrayRef(g_trace_load_options);
+    }
+
+    bool m_verbose; // Enable verbose logging for debugging purposes.
+  };
+
+  CommandObjectTraceLoad(CommandInterpreter &interpreter)
+      : CommandObjectParsed(interpreter, "trace load",
+                            "Load processor trace data from a JSON file.",
+                            "trace load"),
+        m_options() {}
+
+  ~CommandObjectTraceLoad() override = default;
+
+  Options *GetOptions() override { return &m_options; }
+
+protected:
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+    Status error;
+    if (command.size() != 1) {
+      error.SetErrorString("a single path to a JSON file containing trace "
+                           "information is required");
+    } else {
+      FileSpec json_file(command[0].ref());
+      auto data_sp = StructuredData::ParseJSONFromFile(json_file, error);
+      if (error.Success()) {
+        StructuredData::Dictionary *dict = data_sp->GetAsDictionary();
+        if (dict) {
+          if (Expected<lldb::TraceSP> traceOrErr = Trace::FindPlugin(*dict)) {
+            // TODO: fill in the loading code here that uses the trace plugin
+            // below that is in "trace_sp".
+            lldb::TraceSP trace_sp = traceOrErr.get();
+            result.AppendMessage("got trace plugin!");
+          } else {
+            result.AppendMessage("didn't get trace plugin...");
+            error.SetErrorString(llvm::toString(traceOrErr.takeError()));
+          }
+        } else {
+          error.SetErrorString("JSON data must be a dictionary");
+        }
+      }
+    }
+    if (error.Success()) {
+      result.SetStatus(eReturnStatusSuccessFinishResult);
+    } else {
+      result.AppendErrorWithFormat("%s\n", error.AsCString());
+      result.SetStatus(eReturnStatusFailed);
+    }
+    return result.Succeeded();
+  }
+
+  CommandOptions m_options;
+};
+
+// CommandObjectTraceDump
+#define LLDB_OPTIONS_trace_dump
+#include "CommandOptions.inc"
+
+#pragma mark CommandObjectTraceDump
+
+class CommandObjectTraceDump : public CommandObjectParsed {
+public:
+  class CommandOptions : public Options {
+  public:
+    CommandOptions() : Options() { OptionParsingStarting(nullptr); }
+
+    ~CommandOptions() override = default;
+
+    Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+                          ExecutionContext *execution_context) override {
+      Status error;
+      const int short_option = m_getopt_table[option_idx].val;
+
+      switch (short_option) {
+      case 'v': {
+        bool success = false;
+        bool value = OptionArgParser::ToBoolean(option_arg, false, &success);
+        if (success)
+          m_verbose = value;
+        else
+          error.SetErrorStringWithFormat("invalid boolean option: \"%s\"",
+                                         option_arg.str().c_str());
+        break;
+      }
+      default:
+        llvm_unreachable("Unimplemented option");
+      }
+      return error;
+    }
+
+    void OptionParsingStarting(ExecutionContext *execution_context) override {
+      m_verbose = false;
+    }
+
+    llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+      return llvm::makeArrayRef(g_trace_dump_options);
+    }
+
+    bool m_verbose; // Enable verbose logging for debugging purposes.
+  };
+
+  CommandObjectTraceDump(CommandInterpreter &interpreter)
+      : CommandObjectParsed(interpreter, "trace dump",
+                            "Dump the loaded processor trace data.",
+                            "trace dump"),
+        m_options() {}
+
+  ~CommandObjectTraceDump() override = default;
+
+  Options *GetOptions() override { return &m_options; }
+
+protected:
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+    Status error;
+    // TODO: fill in the dumping code here!
+    if (error.Success()) {
+      result.SetStatus(eReturnStatusSuccessFinishResult);
+    } else {
+      result.AppendErrorWithFormat("%s\n", error.AsCString());
+      result.SetStatus(eReturnStatusFailed);
+    }
+    return result.Succeeded();
+  }
+
+  CommandOptions m_options;
+};
+
+// CommandObjectTrace
+
+CommandObjectTrace::CommandObjectTrace(CommandInterpreter &interpreter)
+    : CommandObjectMultiword(interpreter, "trace",
+                             "Commands for loading and using processor "
+                             "trace information.",
+                             "trace [<sub-command-options>]") {
+  LoadSubCommand("load",
+                 CommandObjectSP(new CommandObjectTraceLoad(interpreter)));
+  LoadSubCommand("dump",
+                 CommandObjectSP(new CommandObjectTraceDump(interpreter)));
+}
+
+CommandObjectTrace::~CommandObjectTrace() = default;

lldb/source/Commands/Options.td

def breakpoint_set_address_slide : Option<"address-slide", "R">,
     "resolves to. At present this applies the offset directly as given, and "
     "doesn't try to align it to instruction boundaries.">;
   def breakpoint_set_move_to_nearest_code : Option<"move-to-nearest-code", "m">,
     Groups<[1,9,12]>, Arg<"Boolean">,
     Desc<"Move breakpoints to nearest code. If not set the "
     "target.move-to-nearest-codesetting is used.">;
   def breakpoint_set_file_colon_line : Option<"joint-specifier", "y">, Group<12>, Arg<"FileLineColumn">,
     Required, Completion<"SourceFile">, 
     Desc<"A specifier in the form filename:line[:column] for setting file & line breakpoints.">;  
   /* Don't add this option till it actually does something useful...
   def breakpoint_set_exception_typename : Option<"exception-typename", "O">,
     Arg<"TypeName">, Desc<"The breakpoint will only stop if an "
     "exception Object of this type is thrown.  Can be repeated multiple times "
     "to stop for multiple object types">;
    */
 }
 
 let Command = "breakpoint clear" in {
def source_list_name : Option<"name", "n">, Group<2>, Arg<"Symbol">,
     Completion<"Symbol">,
     Desc<"The name of a function whose source to display.">;
   def source_list_address : Option<"address", "a">, Group<3>,
     Arg<"AddressOrExpression">, Desc<"Lookup the address and display the source"
     " information for the corresponding file and line.">;
   def source_list_reverse : Option<"reverse", "r">, Group<4>, Desc<"Reverse the"
     " listing to look backwards from the last displayed block of source.">;
   def source_list_file_colon_line : Option<"joint-specifier", "y">, Group<5>,
     Arg<"FileLineColumn">, Completion<"SourceFile">, 
     Desc<"A specifier in the form filename:line[:column] from which to display"
          " source.">;
 }
 
 let Command = "target dependents" in {
   def dependents_no_dependents : Option<"no-dependents", "d">, Group<1>,
     OptionalEnumArg<"Value", "OptionEnumValues(g_dependents_enumaration)">,
     Desc<"Whether or not to load dependents when creating a target. If the "
def watchpoint_command_add_python_function : Option<"python-function", "F">,
     "to run as command for this watchpoint. Be sure to give a module name if "
     "appropriate.">;
 }
 
 let Command = "watchpoint delete" in {
   def watchpoint_delete_force : Option<"force", "f">, Group<1>,
     Desc<"Delete all watchpoints without querying for confirmation.">;
 }
+
+let Command = "trace load" in {
+  def trace_load_verbose : Option<"verbose", "v">, Group<1>,
+    Desc<"Show verbose trace load logging for debugging the plug-in "
+    "implementation.">;
+}
+
+let Command = "trace dump" in {
+  def trace_dump_verbose : Option<"verbose", "v">, Group<1>,
+    Desc<"Show verbose trace information.">;
+}

lldb/source/Core/PluginManager.cpp

 //===-- PluginManager.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/Core/PluginManager.h"
 
 #include "lldb/Core/Debugger.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"
bool PluginManager::UnregisterPlugin(
   return GetSymbolVendorInstances().UnregisterPlugin(create_callback);
 }
 
 SymbolVendorCreateInstance
 PluginManager::GetSymbolVendorCreateCallbackAtIndex(uint32_t idx) {
   return GetSymbolVendorInstances().GetCallbackAtIndex(idx);
 }
 
+#pragma mark Trace
+
+typedef PluginInstance<TraceCreateInstance> TraceInstance;
+typedef PluginInstances<TraceInstance> TraceInstances;
+
+static TraceInstances &GetTraceInstances() {
+  static TraceInstances g_instances;
+  return g_instances;
+}
+
+bool PluginManager::RegisterPlugin(ConstString name, const char *description,
+                                   TraceCreateInstance create_callback) {
+  return GetTraceInstances().RegisterPlugin(name, description, create_callback);
+}
+
+bool PluginManager::UnregisterPlugin(TraceCreateInstance create_callback) {
+  return GetTraceInstances().UnregisterPlugin(create_callback);
+}
+
+TraceCreateInstance
+PluginManager::GetTraceCreateCallback(ConstString plugin_name) {
+  return GetTraceInstances().GetCallbackForName(plugin_name);
+}
+
 #pragma mark UnwindAssembly
 
 typedef PluginInstance<UnwindAssemblyCreateInstance> UnwindAssemblyInstance;
 typedef PluginInstances<UnwindAssemblyInstance> UnwindAssemblyInstances;
 
 static UnwindAssemblyInstances &GetUnwindAssemblyInstances() {
   static UnwindAssemblyInstances g_instances;
   return g_instances;

lldb/source/Interpreter/CommandInterpreter.cpp

 #include "Commands/CommandObjectRegister.h"
 #include "Commands/CommandObjectReproducer.h"
 #include "Commands/CommandObjectSession.h"
 #include "Commands/CommandObjectSettings.h"
 #include "Commands/CommandObjectSource.h"
 #include "Commands/CommandObjectStats.h"
 #include "Commands/CommandObjectTarget.h"
 #include "Commands/CommandObjectThread.h"
+#include "Commands/CommandObjectTrace.h"
 #include "Commands/CommandObjectType.h"
 #include "Commands/CommandObjectVersion.h"
 #include "Commands/CommandObjectWatchpoint.h"
 
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Utility/Log.h"
void CommandInterpreter::LoadCommandDictionary() {
   REGISTER_COMMAND_OBJECT("reproducer", CommandObjectReproducer);
   REGISTER_COMMAND_OBJECT("script", CommandObjectScript);
   REGISTER_COMMAND_OBJECT("settings", CommandObjectMultiwordSettings);
   REGISTER_COMMAND_OBJECT("session", CommandObjectSession);
   REGISTER_COMMAND_OBJECT("source", CommandObjectMultiwordSource);
   REGISTER_COMMAND_OBJECT("statistics", CommandObjectStats);
   REGISTER_COMMAND_OBJECT("target", CommandObjectMultiwordTarget);
   REGISTER_COMMAND_OBJECT("thread", CommandObjectMultiwordThread);
+  REGISTER_COMMAND_OBJECT("trace", CommandObjectTrace);
   REGISTER_COMMAND_OBJECT("type", CommandObjectType);
   REGISTER_COMMAND_OBJECT("version", CommandObjectVersion);
   REGISTER_COMMAND_OBJECT("watchpoint", CommandObjectMultiwordWatchpoint);
   REGISTER_COMMAND_OBJECT("language", CommandObjectLanguage);
 
   // clang-format off
   const char *break_regexes[][2] = {
       {"^(.*[^[:space:]])[[:space:]]*:[[:space:]]*([[:digit:]]+)[[:space:]]*:[[:space:]]*([[:digit:]]+)[[:space:]]*$",

lldb/source/Plugins/CMakeLists.txt

 add_subdirectory(OperatingSystem)
 add_subdirectory(Platform)
 add_subdirectory(Process)
 add_subdirectory(ScriptInterpreter)
 add_subdirectory(StructuredData)
 add_subdirectory(SymbolFile)
 add_subdirectory(SystemRuntime)
 add_subdirectory(SymbolVendor)
+add_subdirectory(Trace)
 add_subdirectory(TypeSystem)
 add_subdirectory(UnwindAssembly)
 
 set(LLDB_STRIPPED_PLUGINS)
 get_property(LLDB_ALL_PLUGINS GLOBAL PROPERTY LLDB_PLUGINS)
 
 set(LLDB_ENUM_PLUGINS "")
 

lldb/source/Plugins/Trace/CMakeLists.txt

+add_subdirectory(intel-pt)

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

+add_lldb_library(lldbPluginTraceIntelPT PLUGIN
+  TraceIntelPT.cpp
+
+  LINK_LIBS
+    lldbCore
+    lldbSymbol
+    lldbTarget
+  LINK_COMPONENTS
+    Support
+  )

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 liblldb_TraceIntelPT_h_
+#define liblldb_TraceIntelPT_h_
+
+// Other libraries and framework includes
+#include <llvm/ADT/Optional.h>
+
+// Project includes
+#include "lldb/Target/Trace.h"
+#include "lldb/lldb-private.h"
+
+class TraceIntelPT : public lldb_private::Trace {
+public:
+  void Dump(lldb_private::Stream *s) const override;
+
+  //------------------------------------------------------------------
+  // Static Functions
+  //------------------------------------------------------------------
+
+  static void Initialize();
+
+  static void Terminate();
+
+  static lldb::TraceSP
+  CreateInstance(lldb_private::StructuredData::Dictionary info);
+
+  static lldb_private::ConstString GetPluginNameStatic();
+
+  //------------------------------------------------------------------
+  // PluginInterface protocol
+  //------------------------------------------------------------------
+
+  lldb_private::ConstString GetPluginName() override;
+
+  uint32_t GetPluginVersion() override;
+
+protected:
+  TraceIntelPT(lldb_private::StructuredData::Dictionary info) : Trace(info) {}
+};
+
+#endif // liblldb_TraceIntelPT_h_

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

+//===-- TraceIntelPT.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
+//
+//===----------------------------------------------------------------------===//
+
+// C Includes
+// C++ Includes
+// Other libraries and framework includes
+#include "TraceIntelPT.h"
+#include "lldb/Core/PluginManager.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+LLDB_PLUGIN_DEFINE_ADV(TraceIntelPT, TraceIntelPT)
+
+void TraceIntelPT::Initialize() {
+  PluginManager::RegisterPlugin(GetPluginNameStatic(), "Intel Processor Trace",
+                                CreateInstance);
+}
+
+void TraceIntelPT::Terminate() {
+  PluginManager::UnregisterPlugin(CreateInstance);
+}
+
+ConstString TraceIntelPT::GetPluginNameStatic() {
+  static ConstString g_name("intel-pt");
+  return g_name;
+}
+
+//------------------------------------------------------------------
+// PluginInterface protocol
+//------------------------------------------------------------------
+
+ConstString TraceIntelPT::GetPluginName() { return GetPluginNameStatic(); }
+
+uint32_t TraceIntelPT::GetPluginVersion() { return 1; }
+
+void TraceIntelPT::Dump(lldb_private::Stream *s) const {}
+
+lldb::TraceSP TraceIntelPT::CreateInstance(StructuredData::Dictionary info) {
+  return lldb::TraceSP(new TraceIntelPT(info));
+}

lldb/source/Target/CMakeLists.txt

add_lldb_library(lldbTarget
   ThreadPlanStepOverBreakpoint.cpp
   ThreadPlanStepOverRange.cpp
   ThreadPlanStepRange.cpp
   ThreadPlanStepThrough.cpp
   ThreadPlanStepUntil.cpp
   ThreadPlanTracer.cpp
   ThreadPlanStack.cpp
   ThreadSpec.cpp
+  Trace.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
   UnwindLLDB.cpp
 
   LINK_LIBS
     lldbBreakpoint
     lldbCore
     lldbExpression

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 "lldb/Core/PluginManager.h"
+#include "llvm/Support/Format.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace llvm;
+
+llvm::Expected<lldb::TraceSP>
+Trace::FindPlugin(StructuredData::Dictionary info) {
+  StringRef name;
+  if (!info.GetValueForKeyAsString("name", name))
+    return createStringError(std::errc::invalid_argument,
+                             "structured data is missing the \"name\" field "
+                             "that identifies the trace plug-in");
+  ConstString plugin_name(name);
+  auto create_callback = PluginManager::GetTraceCreateCallback(plugin_name);
+  if (create_callback)
+    return create_callback(std::move(info));
+  return createStringError(
+      std::errc::invalid_argument,
+      "no trace plug-in matches the specified name: \"%s\"",
+      name.str().c_str());
+}

lldb/source/Utility/StructuredData.cpp

 //===-- StructuredData.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/Utility/StructuredData.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <cerrno>
 #include <cstdlib>
 #include <inttypes.h>
 
 using namespace lldb_private;
StructuredData::ParseJSONFromFile(const FileSpec &input_spec, Status &error) {
 
   auto buffer_or_error = llvm::MemoryBuffer::getFile(input_spec.GetPath());
   if (!buffer_or_error) {
     error.SetErrorStringWithFormatv("could not open input file: {0} - {1}.",
                                     input_spec.GetPath(),
                                     buffer_or_error.getError().message());
     return return_sp;
   }
-  return ParseJSON(buffer_or_error.get()->getBuffer().str());
+  llvm::Expected<json::Value> value =
+      json::parse(buffer_or_error.get()->getBuffer().str());
+  if (value)
+    return ParseJSONValue(*value);
+  error.SetErrorString(toString(value.takeError()));
+  return StructuredData::ObjectSP();
 }
 
 static StructuredData::ObjectSP ParseJSONValue(json::Value &value) {
   if (json::Object *O = value.getAsObject())
     return ParseJSONObject(O);
 
   if (json::Array *A = value.getAsArray())
     return ParseJSONArray(A);