Page MenuHomePhabricator

[trace][intel-pt] Scaffold the 'thread trace start | stop' commands
ClosedPublic

Authored by wallace on Nov 3 2020, 4:16 PM.

Details

Summary

Depends on D90490.

The stop command is simple and invokes the new method Trace::StopTracingThread(thread).

On the other hand, the start command works by delegating its implementation to a CommandObject provided by the Trace plugin. This is necessary because each trace plugin needs different options for this command. There's even the chance that a Trace plugin can't support live tracing, but instead supports offline decoding and analysis, which means that "thread trace dump instructions" works but "thread trace start" doest. Because of this and a few other reasons, it's better to have each plugin provide this implementation.

Besides, I'm using the GetSupportedTraceType method introduced in D90490 to quickly infer what's the trace plug-in that works for the current process.

As an implementation note, I moved CommandObjectIterateOverThreads to its header so that I can use it from the IntelPT plugin. Besides, the actual start and stop logic for intel-pt is not part of this diff.

Diff Detail

Event Timeline

wallace created this revision.Nov 3 2020, 4:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
wallace requested review of this revision.Nov 3 2020, 4:16 PM
wallace edited the summary of this revision. (Show Details)Nov 3 2020, 4:16 PM
wallace edited the summary of this revision. (Show Details)
clayborg requested changes to this revision.Nov 5 2020, 5:06 PM
clayborg added inline comments.
lldb/include/lldb/Target/Trace.h
187

We need to have StartTracingThread() in this interface as well. I know right now we can just do the work in the command plug-in, but eventually we will need to expose the trace stuff through the SB API, so we need the StartTracingThread in the API here.

lldb/source/Commands/CommandObjectThread.cpp
1983

We should probably create a CommandObjectDelegate in CommandObjectDelegate.cpp and CommandObjectDelegate.h. This class will have one pure virtual function named GetDelegateCommand() which we would override in this class and return GetTracePluginCommand(). Then this class will become much simpler and the next people that will create a similar kind of command that just forwards to another command can use it.

1996

Move to CommandObjectDelegate.cpp

1997–1998

Store as a shared pointer here please and use it:

CommandObjectSP delegate_cmd_sp = GetDelegateCommand();
if (delegate_cmd_sp)
  delegate_cmd_sp->GetOptions();
2003

Move to CommandObjectDelegate.cpp

2004–2005

Store as a shared pointer like above comment.

2010

Move to CommandObjectDelegate.cpp

2011–2012

Store as a shared pointer like above comment.

2017

Move to CommandObjectDelegate.cpp

2018–2019

Store as a shared pointer like above comment.

2027

Why are you taking over execute instead of DoExecute?

2033–2044

If we require that only one tracing mechanism is available in a process, then why do we need this check?

2059

make virtual override of CommandObjectDelegate::GetDelegateCommand()

lldb/source/Commands/CommandObjectThread.h
16 ↗(On Diff #302710)

This should go in a file on its own near the CommandObject.cpp file. Maybe in CommandObjectThreadUtil.h/.cpp?

lldb/source/Interpreter/CommandObject.cpp
263–266

The command should specify it needs a process with eCommandRequiresProcess. Remove these lines.

This revision now requires changes to proceed.Nov 5 2020, 5:06 PM
wallace added inline comments.Nov 5 2020, 7:44 PM
lldb/include/lldb/Target/Trace.h
187

l'll add it later when I implement the actual StartTracingThread logic. I want to make sure I can cleanly allow each Trace plug-in to have its own set of tracing options for the StartTracingThread, and I'd rather do that in another diff.

lldb/source/Commands/CommandObjectThread.cpp
1983

Great idea!

2027

because Execute in the delegate configures the ExecutionContext correctly, so I need to invoke Execute in the Trace plug-in command. With the Delegate idea that you mention, this will look cleaner

2033–2044

that's right!

lldb/source/Interpreter/CommandObject.cpp
263–266

sure

wallace updated this revision to Diff 303614.Nov 6 2020, 8:52 PM
wallace marked 14 inline comments as done.

address all comments

clayborg requested changes to this revision.Nov 9 2020, 4:11 PM

So I am not sure I like the caching that we are doing in TracePluginCommandDelegate in the CommandObjectTraceStart class. See inline comments, but it might be better to cache the command object SP in lldb_private::Process and add accessors.

lldb/source/Commands/CommandObjectThread.cpp
2012

See my comment below about TracePluginCommandDelegate.process being a pointer. If we switch to using a weak pointer, then this should need to be:

if (m_delegate.process_wp.expired() || process != m_delegate.process_wp.lock().get())
2013

if we use weak pointer:

m_delegate = {process->shared_from_this(), /*command*/ nullptr, /*error*/ ""};
2033

ProcessWP would be safer here. Theoretically a Process instance could be freed and a new process could be allocated at the same address. So maybe:

ProcessWP process_wp;

The other option, which I think I prefer, would be to cache this in the lldb_private::Process class instead of in a cache in CommandObjectTraceStart

lldb/source/Commands/CommandObjectThreadUtil.cpp
2–3

Fix comment header to be on one line. This must be an auto lint thing?

lldb/source/Commands/CommandObjectThreadUtil.h
2–3

fix comment header to be on one line

lldb/test/API/commands/trace/TestTraceStartStop.py
25

Shouldn't we get a better error message here? Something like "error: can't start tracing on a loaded trace file"? Might be nice to be able to somehow detect that a process or thread comes from a core file. We would need to ask the process something like "process->IsLiveDebugSession()" and return a correct error if it isn't. If it is a live debug session, then the ""error: tracing is not supported for the current process. Not implemented" is better.

This revision now requires changes to proceed.Nov 9 2020, 4:11 PM
wallace updated this revision to Diff 304267.Nov 10 2020, 11:23 AM

Address comments

  • I didn't move the caching to Process.h, as creating the start command requires an Interpreter, which I don't know if I can assume as unique. Btw, I'm using weak pointers instead.
  • I added an IsLiveDebugSession, as Greg suggested.

Still wondering if we need to do caching in TracePluginCommandDelegate. How many times do we fetch the command object if we type "thread trace start"? I don't know how much performance is truly needed here at the expense of maintaining the caching code.

lldb/include/lldb/Target/Process.h
1403

This should return true as a default implementation. Then only the ProcessTrace, and the ProcessMachCore, ProcessMinidump, and ProcessELFCore would need to override. We could make a ProcessCore base class that overrides this by returning false and then having all of the mentioned core file classes inherit from ProcessCore. We also need header documentation for this.

Having a default implementation will help make sure the buildbots stay clean for Process plug-ins that are not always built, like ProcessWindows, which we missed in this diff.

lldb/include/lldb/Target/ProcessTrace.h
1

Should ProcessTrace and ThreadTrace be moved to the Process plug-in directory?

lldb/source/Commands/CommandObjectThread.cpp
2038–2039

Should we store the the "command" and "error" as:

llvm::Expected<CommandObjectSP> expected_command;

If so we need a destructor to consume the error, and a method to get the error as a string.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
128 ↗(On Diff #304317)

we can get rid this and make "true" the default implementation. This will help make sure the buildbots stay clean for Process plug-ins that are not always built, like ProcessWindows.

lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
78

It might be nice to create a Process subclass for core files and have ProcessMachCore, ProcessMinidump, ProcessTrace, and ProcessElfCore all inherit. They might have other functions we can refactor.

wallace planned changes to this revision.Nov 10 2020, 7:04 PM
  • After taking a second look I realized I don't need caching. For some reason I thought that autocompletion would be triggered with every keystroke, and that's why caching would be useful. But I was wrong all along! These queries will be rare and there's no need for caching.
  • Good idea with the default implementation for IsLiveDebugSession
wallace updated this revision to Diff 304651.Nov 11 2020, 2:09 PM

Address comments:

  • Don't cache the command delegate. It was overcomplicating the code for little gain.
  • Create a RecordedProcess base class for trace and coredump processes, so that we can put the IsLiveDebugSession method there, and in the future we can add more common methods there.
wallace marked 10 inline comments as done.Nov 11 2020, 2:13 PM
wallace added inline comments.
lldb/include/lldb/Target/ProcessTrace.h
1

can't do that. I think Pavel and other guys warned me of cross-referencing between plugin-folders. It is safer this way, as they are base classes for trace plug-ins.

clayborg added inline comments.Nov 11 2020, 3:14 PM
lldb/include/lldb/Interpreter/CommandObjectDelegate.h
30–32 ↗(On Diff #304651)

So currently you are storing the command object into m_delegate_command_sp in GetDelegateCommand(). This can cause an issue when you have two debuggers (Xcode creates a new debugger for each debug window). If thread A calls this function, and fills in m_delegate_command_sp, then thread B fills in m_delegate_command_sp from another command interpreter, we could end up using the one from thread B. Since the only reason we are storing the value in m_delegate_command_sp is to keep the shared pointer alive, we should just return the shared pointer. So I would suggest removing DoGetDelegateCommand() and just have:

virtual llvm::Expected<lldb::CommandObjectSP> GetDelegateCommand() = 0;

Don't store it. Each function that calls it will keep it around as long as needed. If we use expected then we don't need to store the m_delegate_error later...

lldb/source/Interpreter/CommandObjectDelegate.cpp
16–19 ↗(On Diff #304651)

This caching isn't really doing anything. If we have two debuggers, each one has its own command interpreter and this can actually cause things to fail. If thread A calls this, then thread B, then thread A continues to use the one populated by thread B. See inline comment in header file.

Walter and I discussed some issues offline and came to the conclusion we need to use CommandObjectProxy and get rid of CommandObjectDelegate. This will simplify the patch and use the pre-existing class that already does what we needed CommandObjectDelegate to do...

wallace updated this revision to Diff 304692.Nov 11 2020, 5:27 PM
  • Use CommandObjectProxy and delete the Delegate that I created
  • I had to add a few methods to the proxy to match with that I need. In any case, the implementation is not more complete.
wallace updated this revision to Diff 304867.Nov 12 2020, 9:16 AM

improve error messages

The diff is ready for review again

LGTM. A few inline nits, but nothing major. I only question if "RecordedProcess" is the right name for the base class for a core file/trace file, but don't have much of a better name. Maybe SerializedProcess? SavedProcess? Don't we usually have Pavel in on these reviews?

lldb/include/lldb/Target/RecordedProcess.h
18 ↗(On Diff #304867)

Might want to comment something like "common lldb_private::Process virtual functions overrides that are common between these kinds of processes can have default implementations in this class".

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
209

undo space only change

247–248

undo space only change

766

undo space only change

I am can't keep up with all the reviews, so don't wait on my account. I checking these out from time to time though, and I will speak up if I see something very out of place.

I like the idea of a common base class for different kinds of "core" processes. The name RecordedProcess also made me think twice, but I am not sure what would be a better name for it. Maybe SavedProcess is slightly better as the name does not carry the notion of "time" (which real core files do not have). Or some variation of PostMortemProcess?

wallace updated this revision to Diff 306177.Nov 18 2020, 11:49 AM
wallace marked 5 inline comments as done.

Renamed to SavedProcess

clayborg accepted this revision.Nov 18 2020, 1:53 PM

Thanks for chiming in Pavel. LGTM.

This revision is now accepted and ready to land.Nov 18 2020, 1:53 PM
This revision was landed with ongoing or failed builds.Nov 18 2020, 6:24 PM
This revision was automatically updated to reflect the committed changes.