[intel-pt] Add a basic implementation of the dump command

[wallace]

Summary

Depends on D85705.
The previous diff left the Dump command unimplemented, so I'm implementing it.
As we add more stuff into the intel-pt plugin, we'll dump more information. For now, showing the current settings seems okay.

The dump command will print the information of the currently selected target.

This diff is also important because it introduces Target as the owner of the Trace object. If a trace corresponds
to multiple targets, then all of then will share the same Trace.

[JDevlieghere]

lldb/include/lldb/Target/Target.h

1109

Who owns the trace? If there's a 1:1 relationship between a trace and a target, can we make the target its owner? I'm trying to avoid adding shared pointers if possible.

1421

Doxygen comment?

[wallace]

lldb/include/lldb/Target/Target.h

1109

Well, there's a 1 to many relationship. Many targets can own the same trace object, as a single trace can have data of several processes. On the other hand, there should no other object that could own a trace. I haven't found a better solution :(

1421

good catch

[wallace]

lldb/include/lldb/Target/Target.h

1109

What do you think about changing GetTrace() to returns a Trace & instead of TraceSP &, so that no other object can share the ownership?

[clayborg]

lldb/include/lldb/Target/Target.h

1109

So this is an instance of a Trace plug-in for a given set of data described by the JSON. Since trace files can contain multiple processes, this will need to be shared between multiple Target instances if the "trace load" decides to load all processes, not just one. So I think this should remain a TraceSP so that an instance of the trace plug-in can be shared.

1421

rename to "m_trace_sp" to indicate shared pointer here. See my previous comment for why we need a share pointer.

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

55–146

Since one Trace plug-in instance can contain trace data for one or more processes, would it make sense to have more arguments here? "bool verbose" which would enable dumping of settings? "lldb::pid_t pid" to dump the data for just one process? "lldb::tid_t tid" to dump the data for just one thread?

Or we might want to add functions to lldb_private::Target that can dump the trace data? Like maybe:

void Target::DumpTrace(Stream &strm, lldb::tid_t tid = LLDB_INVALID_THREAD_ID) {
  if (!m_trace_sp)
    strm.PutCString("error: no trace data in target");
  ThreadSP thread_sp;
  if (tid != LLDB_INVALID_THREAD_ID)
    thread_sp = m_process_sp->FindThreadByID(tid);
  m_trace_sp->Dump(strm, m_process_sp.get(), thread_sp.get());
}

Then the Trace::Dump would look like:

void Trace::Dump(Stream &strm, Process *process, Thread *thread, bool verbose);

If there is no process, dump it all trace data for all processes and all threads in each process.
If there is a process and no thread, dump all threads from that process.
If there is a process and thread, dump just the data for that thread

The "trace dump" command can then compose calls using this API for any arguments might receive?

Dump all processes all threads:

(lldb) trace dump

Dump all threads for the specified processes:

(lldb) trace dump --pid 123  --pid 234

Dump all one thread from one process:

(lldb) trace dump --pid 123 --tid 345

If the --tid is specified, then we can have only one "--pid" argument.

56–64

Settings should probably be only dumped if the "bool verbose" argument is true?

lldb/source/Target/Target.cpp

2970

Should we return a "Trace *" from this? If nullptr, then there is no trace data, else we have valid trace data? No one other than Target instances should be adding to the ref count. But I guess this is safer as if someone wants to use the returned Trace pointer, some other thread could free it and cause a crash. So maybe leaving as a TraceSP is a good idea...

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

70 ↗

(On Diff #288155)

We should probably test --pid and --thread here if we decide the APIs.

[labath]

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

[clayborg]

In D86670#2244092, @labath wrote:

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

The idea was to iterate on the design of the trace feature using these patches and have the discussion here. The idea is one trace file can create N individual targets. Each target can be independent and the threads contained in each belong to each target. "trace dump" when no args are supplied it could maybe only dump the currently selected target to start with? I am not sure what the right semantics are, but that shouldn't stop us with proceeding with these patches IMHO. It will be easy to modify as we evolve if we do want to have target groups, but I don't think it is required for these patches. The target group is a much higher level design that can affect many things and could be used in this case, but isn't required. If a trace plug-in needs to iterate over all of the targets this can be achieved by using the debugger to grab all targets and iterate over them, so no real group is required.

So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

[wallace]

I agree with that Greg said.

So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

I'll go ahead with this in order to simplify the code. We can revisit that later

[wallace]

Addressed comments

• Now the "dump" command specifically refers to the currently selected thread. That will be aligned with the future stepping commands we will be adding.
• The format of the command is "trace dump [-t tid]"
• The verbose option is used to dump the original JSON settings
• For now, I'm printing the trace file of each thread. Eventually we'll have more information about each thread.

[wallace]

Rebase. Had to add several const qualifiers to match the const of Dump, which is a good thing.

[clayborg]

I think we should probably make a TraceDumpOptions class to contain all of the dumping options and use that in the Target.h, Trace.h and all plug-ins. We will no doubt expand the number of options in the future and add more. We current have "tid" and "verbose". See inlined comments for details!

lldb/include/lldb/Target/Target.h

1129–1130

So we are going to want probably more options for this in the future. We currently have "tid" and "verbose". We probably also need to specify the process. If we add more options in the future, we would need to add more arguments here. One way around this is to create a "TraceDumpOptions" structure and pass it in:

struct TraceDumpOptions {
  Stream *s;
  Process *process; // If NULL, dump all processes
  std::vector<lldb::tid_t> tids; // Thread IDs, if empty dump all threads
  bool verbose;
};

void DumpTrace(const TraceDumpOptions &trace_options);

1381–1386

Remove whitespace only changes.

lldb/include/lldb/Target/Trace.h

69–70

Maybe use "struct TraceDumpOptions" as mentioned before?

lldb/source/Commands/CommandObjectTrace.cpp

162–167

Should we be able to specify this more than once?

(lldb) trace dump --tid 123 --tid 234

176–177

replace with TraceDumpOptions struct:

TraceDumpOptions m_dump_options;

Then fill in this structure instead of m_verbose and m_tid.

194–195

target.DumpTrace(m_options.m_dump_options);

Again this will allow us to add new dumping options easily in the future.

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

21–25

Use TraceDumpOptions

lldb/source/Target/Target.cpp

2972

Use TraceDumpOptions. You will want to populate the "Process *process;" setting from the target's process and anything else in the dump settings that isn't filled in already that is target specific.

[wallace]

• Now using the TraceDumpOptions struct
• Added support for multiple thread-ids in the dump command. Included a test for this

[clayborg]

I think we should remove the "Stream *s;" from the TraceDumpOptions struct since we have a "Stream &s" as an argument already. We can't use a reference in the TraceDumpOptions without jumping through a lot of hoops.

lldb/include/lldb/Target/Target.h

1130

We don't need the stream option as an argument if the TraceDumpOptions has a stream.

lldb/include/lldb/Target/Trace.h

23

We don't need the stream if the dump method takes this as an argment.

23

inline init if we leave this in here:

Stream *s = nullptr;

24

inline init:

Process *process = nullptr;

26

inline init:

bool verbose = false;

60

Might be nice to leave Stream as an option here since we can use a reference and remove the "Stream *" from the TraceDumpOptions struct.

[labath]

In D86670#2244876, @clayborg wrote:

In D86670#2244092, @labath wrote:

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

The idea was to iterate on the design of the trace feature using these patches and have the discussion here. The idea is one trace file can create N individual targets. Each target can be independent and the threads contained in each belong to each target. "trace dump" when no args are supplied it could maybe only dump the currently selected target to start with? I am not sure what the right semantics are, but that shouldn't stop us with proceeding with these patches IMHO. It will be easy to modify as we evolve if we do want to have target groups, but I don't think it is required for these patches. The target group is a much higher level design that can affect many things and could be used in this case, but isn't required. If a trace plug-in needs to iterate over all of the targets this can be achieved by using the debugger to grab all targets and iterate over them, so no real group is required.

So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

In D86670#2244880, @wallace wrote:

I agree with that Greg said.

I don't think a review like this is good for a broader discussion because it obscures the woods by lots of individual trees. Like, I could write a page about what I don't like about the Target::SetTrace method, but I don't think that's the most important thing to sort out right now. Before going into the details of how trace dump options should be passed around, I think we should have some understanding and agreement about the feature. Like a short introduction to the problem, mentioning what is in scope for the project, what isn't; what are the new user-facing commands/apis being added, new concepts; and finally a very brief overview of new important new lldb internal classes. It doesn't have to be as polished as the memory tagging RFC (that was probably the best rfc lldb has ever had, and I feel guilty for not responding to it), something simpler will also do. I'm sure the two of you have talked about this a lot, and already have something to say about all of the points above, but I don't know how many other people do. And without that its hard to evaluate whether any particular patch is good or not, and reduces the comments to "i think this should be a unique pointer".

Even if noone responds to the rfc, it's not a waste because anyone can go back to it for details when reviewing any particular patch.

[wallace]

Fair enough. I'll work on a document. Hopefully the feedback is going to be good :)

[wallace]

Addresses Greg's comments.

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.

[wallace]

• format

[wallace]

Rebased and did some code cleanup. The output of the dump command looks like this

Settings directory:

/home/wallace/llvm-sand/external/llvm-project/lldb/test/API/commands/trace/intelpt-trace

Trace files:

pid: '1234', tid: '3842849', trace file: /home/wallace/llvm-sand/external/llvm-project/lldb/test/API/commands/trace/intelpt-trace/3842849.trace

Modules:

uuid: 6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A, load address: 0x0000000000400000, file: /home/wallace/llvm-sand/external/llvm-project/lldb/test/API/commands/trace/intelpt-trace/a.out

[clayborg]

See inlined comments.

lldb/include/lldb/Target/Target.h

32

You don't need this as the only "Trace" in this file is the "TraceSP" which is a forward declaration. You will need to add it to the Target.cpp file though.

1261

remove whitespace only changes

1381

remove whitespace only changes

1384

remove whitespace only changes

lldb/source/Commands/CommandObjectTrace.cpp

182

s/from/in/ in the text?

183

add the "eCommandRequiresTarget" at the end here to indicate this command requires a valid target. It will then never even get to the DoExecute(...) function if there isn't a valid target.

193–195

If we add eCommandRequiresTarget to the constructor, then we can just call GetSelectedTarget(). The dummy target won't do us any good. This also means we don't have to add a test for the target being valid for "trace dump" since the eCommandRequiresTarget is already tested!

lldb/source/Commands/Options.td

1191–1192

Can or should this be able to be specified more than once?

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

141–143

remove braces for single line if statement.

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

19

This might change to "invalid target" if we use eCommandRequiresTarget in the command object. If so, we will need to create a target with no trace data and still test that we can get this error.

43

I would either use all "-t" or all "--thread-id" here.

[wallace]

lldb/include/lldb/Target/Target.h

32

It's needed for accessing TraceDumpOptions

[wallace]

comments addressed

[wallace]

format

[clayborg]

Just one nit about forward declaring the TraceDumpOptions.

lldb/include/lldb/Target/Target.h

32

Add TraceDumpOptions to lldb-forward.h and then this won't be needed here. Anytime you only have pointers or references to objects in headers, we should use forward declarations.

[clayborg]

Some active discussion on the Trace RFC right now that might change the outcome of this patch. Holding off accepting until we resolve.

[wallace]

I'm redoing this in https://reviews.llvm.org/D88769

lldb/include/lldb/Target/Target.h

 #define LLDB_TARGET_TARGET_H
 
 #include <list>
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "lldb/Breakpoint/BreakpointList.h"
 #include "lldb/Breakpoint/BreakpointName.h"
 #include "lldb/Breakpoint/WatchpointList.h"
 #include "lldb/Core/Architecture.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/UserSettingsController.h"
 #include "lldb/Expression/Expression.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/ExecutionContextScope.h"
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Target/SectionLoadHistory.h"
 #include "lldb/Target/ThreadSpec.h"
+#include "lldb/Target/Trace.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/Broadcaster.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-public.h"
 
 namespace lldb_private {
 
public:
 
   const ProcessLaunchInfo &GetProcessLaunchInfo();
 
   void SetProcessLaunchInfo(const ProcessLaunchInfo &launch_info);
 
   bool GetInjectLocalVariables(ExecutionContext *exe_ctx) const;
 
   void SetInjectLocalVariables(ExecutionContext *exe_ctx, bool b);
-  
   void SetRequireHardwareBreakpoints(bool b);
 
   bool GetRequireHardwareBreakpoints() const;
 
   bool GetAutoInstallMainExecutable() const;
 
   void UpdateLaunchInfoFromProperties();
 
public:
 
   bool SetSectionUnloaded(const lldb::SectionSP &section_sp);
 
   bool SetSectionUnloaded(const lldb::SectionSP &section_sp,
                           lldb::addr_t load_addr);
 
   void ClearAllLoadedSections();
 
+  /// Set the \a Trace object containing processor trace information of this
+  /// target.
+  ///
+  /// \param[in] trace_sp
+  ///   The trace object.
+  void SetTrace(const lldb::TraceSP &trace_sp);
+
+  /// Get the \a Trace object containing processor trace information of this
+  /// target.
+  ///
+  /// \return
+  ///   The trace object. It might be undefined.
+  lldb::TraceSP &GetTrace();
+
+  /// Dump the \a Trace information.
+  ///
+  /// \param[in] s
+  ///   The stream object where the information is printed.
+  ///
+  /// \param[in] options
+  ///   The trace dump options. See \a TraceDumpOptions for more information.
+  void DumpTrace(Stream &s, lldb_private::TraceDumpOptions &options);
+
   // Since expressions results can persist beyond the lifetime of a process,
   // and the const expression results are available after a process is gone, we
   // provide a way for expressions to be evaluated from the Target itself. If
   // an expression is going to be run, then it should have a frame filled in in
   // the execution context.
   lldb::ExpressionResults EvaluateExpression(
       llvm::StringRef expression, ExecutionContextScope *exe_scope,
       lldb::ValueObjectSP &result_valobj_sp,
public:
 
   private:
     std::string m_class_name;
     /// This holds the dictionary of keys & values that can be used to
     /// parametrize any given callback's behavior.
     StructuredDataImpl *m_extra_args; // We own this structured data,
                                       // but the SD itself manages the UP.
     /// This holds the python callback object.
     StructuredData::GenericSP m_implementation_sp; 
 
     /// Use CreateStopHook to make a new empty stop hook. The GetCommandPointer
     /// and fill it with commands, and SetSpecifier to set the specifier shared
     /// pointer (can be null, that will match anything.)
     StopHookScripted(lldb::TargetSP target_sp, lldb::user_id_t uid)
         : StopHook(target_sp, uid) {}
     friend class Target;
   };
private:
     ArchSpec m_spec;
     std::unique_ptr<Architecture> m_plugin_up;
   };
   // Member variables.
   Debugger &m_debugger;
   lldb::PlatformSP m_platform_sp; ///< The platform for this target.
   std::recursive_mutex m_mutex; ///< An API mutex that is used by the lldb::SB*
                                 /// classes make the SB interface thread safe
   /// When the private state thread calls SB API's - usually because it is 
   /// running OS plugin or Python ThreadPlan code - it should not block on the
   /// API mutex that is held by the code that kicked off the sequence of events
   /// that led us to run the code.  We hand out this mutex instead when we 
   /// detect that code is running on the private state thread.
   std::recursive_mutex m_private_mutex; 
   Arch m_arch;
   ModuleList m_images; ///< The list of images for this process (shared
                        /// libraries and anything dynamically loaded).
   SectionLoadHistory m_section_load_history;
   BreakpointList m_breakpoint_list;
   BreakpointList m_internal_breakpoint_list;
   using BreakpointNameList = std::map<ConstString, BreakpointName *>;
   BreakpointNameList m_breakpoint_names;
protected:
 
   typedef std::map<lldb::user_id_t, StopHookSP> StopHookCollection;
   StopHookCollection m_stop_hooks;
   lldb::user_id_t m_stop_hook_next_id;
   bool m_valid;
   bool m_suppress_stop_hooks;
   bool m_is_dummy_target;
   unsigned m_next_persistent_variable_index = 0;
+  /// An optional \a lldb_private::Trace object containing processor trace
+  /// information of this target.
+  lldb::TraceSP m_trace_sp;
   /// Stores the frame recognizers of this target.
   lldb::StackFrameRecognizerManagerUP m_frame_recognizer_manager_up;
 
   static void ImageSearchPathsChanged(const PathMappingList &path_list,
                                       void *baton);
 
   // Utilities for `statistics` command.
 private:

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 "llvm/Support/JSON.h"
 
 #include "lldb/Core/PluginInterface.h"
 #include "lldb/Target/TraceSettingsParser.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
+/// Helper class that holds the configuration of a Dump action.
+///
+/// The \a process field must be set before invoking \a Trace::Dump.
+///
+/// If \a tids is empty, then all threads of the given process are considered.
+struct TraceDumpOptions {
+  Process *process = nullptr;
+  std::set<lldb::tid_t> tids;
+  bool verbose = false;
+};
+
 /// \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 {
+class Trace : public PluginInterface,
+              public std::enable_shared_from_this<Trace> {
 public:
   /// 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.
+  /// readable format.
   ///
   /// \param[in] s
   ///     A stream object to dump the information to.
-  virtual void Dump(Stream *s) const = 0;
+  ///
+  /// \param[in] options
+  ///   The trace dump options. See \a TraceDumpOptions for more information.
+  virtual void Dump(lldb_private::Stream &s,
+                    const TraceDumpOptions &options) const = 0;
 
   /// Find a trace plug-in using JSON 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 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
protected:
   friend class TraceSettingsParser;
   /// JSON object that holds all settings for this trace session.
   llvm::json::Object m_settings;
   /// The directory that contains the settings file.
   std::string m_settings_dir;
 
   std::map<lldb::pid_t, std::map<lldb::tid_t, lldb_private::FileSpec>>
       m_thread_to_trace_file_map;
-  std::vector<lldb::TargetSP> m_targets;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TRACE_H

lldb/source/Commands/CommandObjectTrace.cpp

public:
 
     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': {
-        m_verbose = true;
+        m_dump_options.verbose = true;
+        break;
+      }
+      case 't': {
+        lldb::tid_t tid;
+        if (option_arg.empty() || option_arg.getAsInteger(0, tid))
+          error.SetErrorStringWithFormat("invalid thread id string '%s'",
+                                         option_arg.str().c_str());
+        else
+          m_dump_options.tids.insert(tid);
         break;
       }
       default:
         llvm_unreachable("Unimplemented option");
       }
       return error;
     }
 
     void OptionParsingStarting(ExecutionContext *execution_context) override {
-      m_verbose = false;
+      m_dump_options = TraceDumpOptions();
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_trace_dump_options);
     }
 
-    bool m_verbose; // Enable verbose logging for debugging purposes.
+    TraceDumpOptions m_dump_options;
   };
 
   CommandObjectTraceDump(CommandInterpreter &interpreter)
-      : CommandObjectParsed(interpreter, "trace dump",
-                            "Dump the loaded processor trace data.",
-                            "trace dump"),
+      : CommandObjectParsed(
+            interpreter, "trace dump",
+            "Dump the loaded processor trace data in the current target.",
+            "trace dump", eCommandRequiresTarget),
         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!
+    Target &target = GetSelectedTarget();
+    target.DumpTrace(result.GetOutputStream(), m_options.m_dump_options);
+
     if (error.Success()) {
+      result.AppendMessage("\n");
       result.SetStatus(eReturnStatusSuccessFinishResult);
     } else {
       result.AppendErrorWithFormat("%s\n", error.AsCString());
       result.SetStatus(eReturnStatusFailed);
     }
     return result.Succeeded();
   }
 

lldb/source/Commands/Options.td

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.">;
+  def trace_dump_thread_id : Option<"thread-id", "t">, Group<1>,
+    Arg<"ThreadID">, Desc<"The thread id to dump trace information of. Can be "
+    "repeated multiple times to dump the information of several threads.">;
 }
 
 let Command = "trace schema" in {
   def trace_schema_verbose : Option<"verbose", "v">, Group<1>,
     Desc<"Show verbose trace schema logging for debugging the plug-in.">;
 }

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_
 
 #include "intel-pt.h"
 #include "llvm/ADT/Optional.h"
 
 #include "TraceIntelPTSettingsParser.h"
 #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;
+  void Dump(lldb_private::Stream &s,
+            const lldb_private::TraceDumpOptions &options) const override;
 
   ~TraceIntelPT() override = default;
 
   /// PluginInterface protocol
   /// \{
   lldb_private::ConstString GetPluginName() override;
 
   static void Initialize();
 
   static void Terminate();
 
   static lldb::TraceSP CreateInstance();
 
   static lldb_private::ConstString GetPluginNameStatic();
 
   uint32_t GetPluginVersion() override;
   /// \}
 
 protected:
   TraceIntelPT() : Trace() {}
 
   std::unique_ptr<lldb_private::TraceSettingsParser> CreateParser() override;
 
 private:
+  void DumpJSONSettings(lldb_private::Stream &s) const;
+
+  void DumpTraceFiles(lldb_private::Stream &s,
+                      const lldb_private::TraceDumpOptions &options) const;
+
   friend class TraceIntelPTSettingsParser;
   pt_cpu m_pt_cpu;
 };
 
 #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
 //
 //===----------------------------------------------------------------------===//
 
 #include "TraceIntelPT.h"
 
+#include <cassert>
+
+#include "llvm/Support/Format.h"
+#include "llvm/Support/FormatVariadic.h"
+
 #include "TraceIntelPTSettingsParser.h"
+#include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/SectionLoadList.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
 LLDB_PLUGIN_DEFINE_ADV(TraceIntelPT, TraceIntelPT)
 
 void TraceIntelPT::Initialize() {

 //------------------------------------------------------------------
 // PluginInterface protocol
 //------------------------------------------------------------------
 
 ConstString TraceIntelPT::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t TraceIntelPT::GetPluginVersion() { return 1; }
 
-void TraceIntelPT::Dump(lldb_private::Stream *s) const {}
+void DumpImages(Stream &s, const TraceDumpOptions &options) {
+  s << "\n\nModules:";
+  s.IndentMore();
+
+  Target &target = options.process->GetTarget();
+  const ModuleList &modules = target.GetImages();
+
+  for (size_t i = 0; i < modules.GetSize(); i++) {
+    const ModuleSP module = modules.GetModuleAtIndex(i);
+    s << "\n";
+    s.Indent();
+    if (module->GetUUID().IsValid()) {
+      s << "uuid: ";
+      module->GetUUID().Dump(&s);
+      s << ", ";
+    }
+
+    if (ObjectFile *objfile = module->GetObjectFile()) {
+      Address base_addr(objfile->GetBaseAddress());
+      if (base_addr.IsValid()) {
+        s << "load address: ";
+        if (!target.GetSectionLoadList().IsEmpty()) {
+          lldb::addr_t load_addr = base_addr.GetLoadAddress(&target);
+          if (load_addr == LLDB_INVALID_ADDRESS) {
+            base_addr.Dump(&s, &target, Address::DumpStyleModuleWithFileAddress,
+                           Address::DumpStyleFileAddress);
+          } else {
+            // Show the load address of the image
+            s.Printf("0x%" PRIx64, load_addr);
+          }
+        } else {
+          // The address was valid, but the image isn't loaded, output the
+          // address in an appropriate format
+          base_addr.Dump(&s, &target, Address::DumpStyleFileAddress);
+        }
+        s << ", ";
+      }
+    }
+    s << "file: ";
+    module->GetFileSpec().Dump(s.AsRawOstream());
+  }
+
+  s.IndentLess();
+}
+
+void TraceIntelPT::DumpJSONSettings(Stream &s) const {
+  s << "Settings:\n";
+  std::string str;
+  llvm::raw_string_ostream OS(str);
+  json::Object obj = m_settings;
+  OS << llvm::formatv("{0:2}", json::Value(std::move(obj)));
+  OS.flush();
+  s << OS.str();
+
+  s << "\n\nSettings directory:\n";
+  s.IndentMore();
+  s.Indent();
+  s << m_settings_dir << "\n\n";
+  s.IndentLess();
+}
+
+void TraceIntelPT::DumpTraceFiles(Stream &s,
+                                  const TraceDumpOptions &options) const {
+  s << "Trace files:";
+
+  lldb::pid_t pid = options.process->GetID();
+  const auto &process_it = m_thread_to_trace_file_map.find(pid);
+  if (process_it == m_thread_to_trace_file_map.end())
+    return;
+
+  s.IndentMore();
+  for (const auto &thread_trace_file : process_it->second) {
+    lldb::tid_t tid = thread_trace_file.first;
+    if (options.tids.empty() || options.tids.count(tid)) {
+      s << "\n";
+      s.Indent();
+      s.Printf("pid: '%" PRIu64 "', tid: '%" PRIu64 "', trace file: ", pid,
+               tid);
+      s << thread_trace_file.second;
+    }
+  }
+  s.IndentLess();
+}
+
+void TraceIntelPT::Dump(Stream &s, const TraceDumpOptions &options) const {
+  assert(options.process != nullptr && "The process field must be specified");
+  if (options.verbose)
+    DumpJSONSettings(s);
+
+  DumpTraceFiles(s, options);
+  DumpImages(s, options);
+}
 
 lldb::TraceSP TraceIntelPT::CreateInstance() {
   return lldb::TraceSP(new TraceIntelPT());
 }

lldb/source/Target/Target.cpp

 //===-- Target.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/Target.h"
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
 #include "lldb/Breakpoint/BreakpointIDList.h"
 #include "lldb/Breakpoint/BreakpointPrecondition.h"
 #include "lldb/Breakpoint/BreakpointResolver.h"
 #include "lldb/Breakpoint/BreakpointResolverAddress.h"
 #include "lldb/Breakpoint/BreakpointResolverFileLine.h"
 #include "lldb/Breakpoint/BreakpointResolverFileRegex.h"
 #include "lldb/Breakpoint/BreakpointResolverName.h"
Status Target::Launch(ProcessLaunchInfo &launch_info, Stream *stream) {
   default:
     error.SetErrorStringWithFormat("initial process state wasn't stopped: %s",
                                    StateAsCString(state));
     break;
   }
   return error;
 }
 
+void Target::SetTrace(const lldb::TraceSP &trace_sp) { m_trace_sp = trace_sp; }
+
+lldb::TraceSP &Target::GetTrace() { return m_trace_sp; }
+
+void Target::DumpTrace(Stream &s, TraceDumpOptions &options) {
+  if (!m_trace_sp) {
+    s << "error: no trace data in this target";
+    return;
+  }
+  if (!m_process_sp) {
+    s << "error: no process in this target";
+    return;
+  }
+  options.process = m_process_sp.get();
+  ThreadSP thread_sp;
+  for (lldb::tid_t tid : options.tids) {
+    if (!m_process_sp->GetThreadList().FindThreadByID(tid)) {
+      s.Printf("error: the thread id \"%" PRIu64
+               "\" does not exist in this target",
+               tid);
+    }
+  }
+  m_trace_sp->Dump(s, options);
+}
+
 Status Target::Attach(ProcessAttachInfo &attach_info, Stream *stream) {
   auto state = eStateInvalid;
   auto process_sp = GetProcessSP();
   if (process_sp) {
     state = process_sp->GetState();
     if (process_sp->IsAlive() && state != eStateConnected) {
       if (state == eStateAttaching)
         return Status("process attach is in progress");

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/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
 // Helper structs used to extract the type of a trace settings json without
 // having to parse the entire object.
 
return createStringError(
       std::errc::invalid_argument,
       "no trace plug-in matches the specified type: \"%s\"",
       plugin_name.AsCString());
 }
 
 llvm::Error Trace::ParseSettings(Debugger &debugger,
                                  const llvm::json::Value &settings,
                                  llvm::StringRef settings_dir) {
-  if (llvm::Error err =
-          CreateParser()->ParseSettings(debugger, settings, settings_dir))
-    return err;
-
-  return llvm::Error::success();
+  return CreateParser()->ParseSettings(debugger, settings, settings_dir);
 }
 
 llvm::StringRef Trace::GetSchema() { return CreateParser()->GetSchema(); }

lldb/source/Target/TraceSettingsParser.cpp

 //===-- TraceSettingParser.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/TraceSettingsParser.h"
 
 #include <sstream>
 
 #include "Plugins/Process/Utility/HistoryThread.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Target/Process.h"
 
 using namespace lldb;
if (llvm::Error err = ParseSettingsImpl(debugger, raw_settings)) {
     // We clean all the targets that were created internally, which should leave
     // the debugger unchanged
     for (auto target_sp : m_targets)
       debugger.GetTargetList().DeleteTarget(target_sp);
 
     return err;
   }
 
+  for (auto target_sp : m_targets)
+    target_sp->SetTrace(m_trace.shared_from_this());
+
   m_trace.m_settings = *raw_settings.getAsObject();
   m_trace.m_settings_dir = m_settings_dir;
   m_trace.m_thread_to_trace_file_map = m_thread_to_trace_file_map;
-  m_trace.m_targets = m_targets;
 
   return llvm::Error::success();
 }

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

+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDump(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")
+        self.src_dir = self.getSourceDir()
+
+    def testDumpMissingTrace(self):
+        self.expect("target create " + os.path.join(self.src_dir, 'intelpt-trace', 'a.out'))
+        self.expect("trace dump", substrs=["error: no trace data in this target"])
+
+    def testDumpTrace(self):
+        self.expect("trace load " + os.path.join(self.src_dir, "intelpt-trace", "trace2.json"))
+
+        # An invalid thread id should show an error message
+        self.expect("trace dump -t 5678", substrs=['error: the thread id "5678" does not exist in this target'])
+
+        # Only the specified thread should be printed
+        self.expect("trace dump -t 22", substrs=["Trace files"],
+          patterns=["pid: '2', tid: '22', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace",
+                    "Modules:\n  uuid: 6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A, load address: 0x0000000000400000, file: .*intelpt-trace/a.out"])
+
+        # Verbose should output the entire JSON settings besides the thread specific information
+        self.expect("trace dump -t 22 -v",
+          substrs=["Settings", "3842849.trace", "intel-pt", "Settings directory"],
+          patterns=["pid: '2', tid: '22', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace"])
+
+        # In this case all threads should be printed
+        self.expect("trace dump", substrs=["Trace files"],
+          patterns=["pid: '2', tid: '21', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace",
+            "pid: '2', tid: '22', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace"])
+
+        self.expect("trace dump -t 21 -t 22", substrs=["Trace files"],
+          patterns=["pid: '2', tid: '21', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace",
+            "pid: '2', tid: '22', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace"])
+
+        # We switch targets and check the threads of this target
+        self.dbg.SetSelectedTarget(self.dbg.FindTargetWithProcessID(1))
+        self.expect("trace dump", substrs=["Trace files"],
+          patterns=["pid: '1', tid: '11', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace"])

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

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