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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
