[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inlined comments.



================
Comment at: lldb/include/lldb/Target/Target.h:32
 #include "lldb/Target/ThreadSpec.h"
+#include "lldb/Target/Trace.h"
 #include "lldb/Utility/ArchSpec.h"
----------------
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.


================
Comment at: lldb/include/lldb/Target/Target.h:1261
     /// This holds the python callback object.
-    StructuredData::GenericSP m_implementation_sp; 
+    StructuredData::GenericSP m_implementation_sp;
 
----------------
remove whitespace only changes


================
Comment at: lldb/include/lldb/Target/Target.h:1381
                                 /// classes make the SB interface thread safe
-  /// When the private state thread calls SB API's - usually because it is 
+  /// 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
----------------
remove whitespace only changes


================
Comment at: lldb/include/lldb/Target/Target.h:1384
   /// 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 
+  /// 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.
----------------
remove whitespace only changes


================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:182
+            interpreter, "trace dump",
+            "Dump the loaded processor trace data from the current target.",
+            "trace dump"),
----------------
s/from/in/ in the text?


================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:183
+            "Dump the loaded processor trace data from the current target.",
+            "trace dump"),
         m_options() {}
----------------
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.


================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:193
     Status error;
-    // TODO: fill in the dumping code here!
+    Target &target = GetSelectedOrDummyTarget();
+    target.DumpTrace(result.GetOutputStream(), m_options.m_dump_options);
----------------
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!


================
Comment at: lldb/source/Commands/Options.td:1191-1192
     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 or should this be able to be specified more than once?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:141-143
+  if (options.verbose) {
+    DumpJSONSettings(s);
+  }
----------------
remove braces for single line if statement.


================
Comment at: lldb/test/API/commands/trace/TestTraceDump.py:18
+    def testDumpTrace(self):
+        self.expect("trace dump", substrs=["error: no trace data in this target"])
+
----------------
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.


================
Comment at: lldb/test/API/commands/trace/TestTraceDump.py:42
+
+        self.expect("trace dump -t 21 --thread-id 22", substrs=["Trace files"],
+          patterns=["pid: '2', tid: '21', trace file: .*/test/API/commands/trace/intelpt-trace/3842849.trace",
----------------
I would either use all "-t" or all "--thread-id" here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670