[Lldb-commits] [PATCH] D88769: [trace] Scaffold "thread trace dump instructions"
Greg Clayton via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Sat Oct 3 00:16:01 PDT 2020
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
See inline comments about "thread trace dump instructions" as I am not quite sure what --offset means.
After we load a trace file, what is the default "offset" value? Is there a kind of "trace data position" that can be moved around? Does the offset default to the start of all trace instructions for a thread? The end of the trace data?
================
Comment at: lldb/include/lldb/Target/Target.h:1122-1137
+ /// Dump the instructions of the given thread's \a Trace.
+ ///
+ /// \param[in] s
+ /// The stream object where the instructions are printed.
+ ///
+ /// \param[in] tid
+ /// The thread to print the instruction of.
----------------
This should probably go on the lldb_private::Thread now if we are only going to ever dump information for a thread at a time.
================
Comment at: lldb/source/Commands/Options.td:1011
+ Arg<"Count">,
+ Desc<"The number of total instructions to display.">;
+ def thread_trace_dump_instructions_offset: Option<"offset", "o">, Group<1>,
----------------
This option text might need to be a bit more descriptive. What if the user types:
```
(lldb) thread trace dump --count 22
```
What would that display? 22 instructions forward from the current point we are at? If we load a trace file, will the position be at the end by default or at the beginning? I would think we would selec the end. Would this go 22 instructions back? That doesn't make much sense if we use in conjunction with the --offset.
================
Comment at: lldb/source/Commands/Options.td:1014-1015
+ Arg<"Offset">,
+ Desc<"Offset from the last instruction of the trace to start displaying "
+ "instructions from. Defaults to 0.">;
+}
----------------
So what happens if the user just types:
```
(lldb) thread trace dump
```
If the trace position offset defaults to zero, and the current position is the end of the data by default, would this display nothing? Everything? Or would it display the stats for this thread? like:
```
(lldb) thread trace dump
thread #1: tid = 1234, 4096 trace branches available, position = end
thread #2: tid = 2345, 2345 trace branches available, position = end
```
================
Comment at: lldb/source/Plugins/Process/Trace/ProcessTrace.cpp:40-43
+bool ProcessTrace::CanDebug(lldb::TargetSP target_sp,
+ bool plugin_specified_by_name) {
+ return true;
+}
----------------
You must return "plugin_specified_by_name" here so that this plug-in only gets selected if it was specified by name. We don't want to have this ProcessTrace plug-in get selected by any real debug sessions. So this should be:
```
bool ProcessTrace::CanDebug(lldb::TargetSP target_sp,
bool plugin_specified_by_name) {
return plugin_specified_by_name;
}
```
That way we can request this process plug-in by name.
If this is set to true, then when a target is shopping for a process plug-in by iterating through all installed Process plug-ins, if this function gets called before the ProcessGDBRemote or ProcessWindows it will take precedence over these other plug-ins. Typically a process plug-in will look at the target triple and look for an architecture, os, or vendor that matches and also look at the target's platform ("windows" for ProcessWindows) to determine if it can be used for debugging. In our case we should only be able to get a ProcessTrace instance back if we ask for it by name, it won't care about the target's triple, platform or anything else as it could be anything.
================
Comment at: lldb/source/Plugins/Process/Trace/ProcessTrace.cpp:62-65
+Status ProcessTrace::DoLoadCore() {
+ SetCanJIT(false);
+ return Status();
+}
----------------
Probably shouldn't use the LoadCore functionality, see inline comments.
================
Comment at: lldb/source/Plugins/Process/Trace/ProcessTrace.h:39
+
+ lldb_private::Status DoLoadCore() override;
+
----------------
Seems weird to use LoadCore, see other inline comments.
================
Comment at: lldb/source/Target/Target.cpp:2972-2981
+void Target::DumpTraceInstructions(Stream &s, lldb::tid_t tid, size_t count,
+ size_t offset) const {
+ if (!m_trace_sp) {
+ s << "error: no trace in this target";
+ return;
+ }
+ s.Printf("placeholder trace of tid %" PRIu64 ", count = %zu, offset = %zu\n",
----------------
move to lldb_private::Thread?
================
Comment at: lldb/source/Target/TraceSettingsParser.cpp:68-73
+ // For now we are setting an invalid PC. Eventually we should be able to
+ // lazyly reconstruct this callstack after decoding the trace.
+ ThreadSP thread_sp(new HistoryThread(*process_sp, tid,
+ /*callstack*/ {LLDB_INVALID_ADDRESS},
+ /*pcs_are_call_addresses*/ false,
+ /*use_invalid_index_id*/ false));
----------------
We might need a ThreadTrace instead of a HistoryThread, and it might be a good idea to do that now. Soon we will end up wanting to step around and continue and reverse continue then we will need to muck with the thread and set its stop reason to things like "hit breakpoint" and change the stack frames.
================
Comment at: lldb/source/Target/TraceSettingsParser.cpp:114-115
ProcessSP process_sp(target_sp->CreateProcess(
- /*listener*/ nullptr, /*plugin_name*/ llvm::StringRef(),
- /*crash_file*/ nullptr));
+ /*listener*/ nullptr, /*plugin_name*/ "trace", /*crash_file*/ nullptr));
process_sp->SetID(static_cast<lldb::pid_t>(process.pid));
----------------
We will need to make sure that this only happens when doing "trace load", and not when we do "process trace start" as we will have a real process at that point. We should talk about architecture as I have some ideas to make things work smoothly, but that can wait until we start to try and do more forward/reverse run/step stuff.
================
Comment at: lldb/source/Target/TraceSettingsParser.cpp:128-132
+ // We invoke LoadCore to create a correct stopped state for the process and
+ // its threads
+ error = process_sp->LoadCore();
+ if (error.Fail())
+ return error.ToError();
----------------
Seems weird to use LoadCore here as this isn't a core file. Since we loaded the Process plug-in by name, you can cast the a Process * to be a ProcessTrace and call a custom method on it if we need to do custom setup. DidAttach might be better to override instead of LoadCore?
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:40
+
+ self.expect("thread trace dump instructions",
+ substrs=['placeholder trace of tid 3842849, count = 10, offset = 0'])
----------------
What will this do by default? I would assume it would dump all instructions starting from the beginning and dump all the way to the end. Where is the default "offset" for these instructions right after a trace is loaded? Is it set to the end of the trace instructions? If so, I am not sure what I would expect "thread trace dump instructions" to do. Again I would assume it would dump them all? Can you elaborate on what you are thinking here?
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:44
+ # We check if we can pass count and offset
+ self.expect("thread trace dump instructions -c 5 -o 10",
+ substrs=['placeholder trace of tid 3842849, count = 5, offset = 10'])
----------------
use the long options as this makes it much clearer what is happening.
Also what does --offset mean? Is it an offset in bytes within the trace data? Is it the number of branches from the start of the all of the trace data? Number of branches from the end of the trace data? Will this need to be specialized for each trace plug-in?
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:48-49
+ # We check if we can access the thread by index id
+ self.expect("thread trace dump instructions 1",
+ substrs=['placeholder trace of tid 3842849, count = 10, offset = 0'])
----------------
Add a test for an invalid thread index id?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88769/new/
https://reviews.llvm.org/D88769
More information about the lldb-commits
mailing list