This is an archive of the discontinued LLVM Phabricator instance.

Modify "platform connect" to connect to processes as well
ClosedPublic

Authored by tberghammer on Nov 24 2015, 7:00 AM.

Details

Summary

Modify "platform connect" to connect to processes as well

The standard remote debugging workflow with gdb is to start the
application on the remote host under gdbserver (e.g.: gdbserver :5039
a.out) and then connect to it with gdb.

The same workflow is supported by debugserver/lldb-gdbserver with a very
similar syntax but to access all features of lldb we need to be
connected also to an lldb-platform instance running on the target.

Before this change this had to be done manually with starting a separate
lldb-platform on the target machine and then connecting to it with lldb
before connecting to the process.

This change modifies the behavior of "platform connect" with
automatically connecting to the process instance if it was started by
the remote platform. With this command replacing gdbserver in a gdb
based worflow is usually as simple as replacing the command to execute
gdbserver with executing lldb-platform.

Example for the new workflow:

  • On the target: lldb-server platform --listen localhost:5432 -- a.out arg1 arg2
  • On the host: (lldb) platform select remote-android (lldb) platform connect connect://localhost:5432
  • As a result we will be connected to the remote platform and also connected to a process of a.out on the remote target what is stopped at the entry point (the executable file and all of its dependency will be downloaded by the platform to the module cache)

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Create new "platform process connect" command.
tberghammer updated this object.
tberghammer added reviewers: labath, clayborg.
tberghammer added a subscriber: lldb-commits.
labath edited edge metadata.Nov 24 2015, 7:18 AM

Seems like a reasonable thing to do. However, we should first add a test for the new functionality.

source/Commands/CommandObjectPlatform.cpp
2041 ↗(On Diff #41043)

Please update this as well.
With this many suboptions, we can go for something like
platform process <command> [<command-options>], as other commands do.

tberghammer edited edge metadata.

Add test

tberghammer marked an inline comment as done.Nov 26 2015, 10:41 AM
tberghammer added inline comments.
source/Commands/CommandObjectPlatform.cpp
2040–2041 ↗(On Diff #41266)

Done

Thank you for writing the test. I have just a couple of more comments and them I'm done... :)

Please wait for @clayborg to sign off on this as well.

packages/Python/lldbsuite/test/tools/lldb-server/platform-process-connect/TestPlatformProcessConnect.py
13 ↗(On Diff #41266)

I'm quite opposed to "inline skips", I'd like them to be more declarative. Let's add a "remote" parameter to the skipIf decorator and use that. Suggested semantics:

  • None: don't care
  • True: skip if the target is remote
  • False: skip if the target is *not* remote
26 ↗(On Diff #41266)

Please spell out the arguments in full. It makes it much more obvious what is going to happen.

29 ↗(On Diff #41266)

What will happen when the next test runs? Will we be able to reconnect to the old platform instance? Please check whether this works when run in --no-multiprocess mode with other tests...
If it does not work, we might be able to create a brand new SBDebugger instance and use that to connect...

tberghammer marked 4 inline comments as done.

Improve the test based on the comments

packages/Python/lldbsuite/test/tools/lldb-server/platform-process-connect/TestPlatformProcessConnect.py
13 ↗(On Diff #41266)

Done

26 ↗(On Diff #41266)

Done

29 ↗(On Diff #41266)

I changed it to create a new debugger because the previous implementation only worked if the test wasn't followed by a new one

clayborg requested changes to this revision.Nov 30 2015, 9:10 AM
clayborg edited edge metadata.

I am not sure that I like this new "platform process connect" command as it isn't really clear what this command does. "platform connect" connects to the remote platform, "process connect" connects directly to a remote GDB server that is already running. Can we just modify "platform connect" to do what "platform process connect" currently does? The code for "platform connect" would just need to detect that the platform has a process that it needs to attach do and do it. I believe this would keep the functionality that you want and keep our commands intact and a bit cleaner.

This revision now requires changes to proceed.Nov 30 2015, 9:10 AM

I decided to go with the "platform process connect" command as it connects to both to a platform and to a process instances at the same time but I agree it is not very clear.

We can make this the default functionality for "platform connect" but then we have to add a new packet to the gdb protocol where lldb can ask the platform if it have to connect to a process instance or not. I am not sure if using "platform connect" will be clearer enough to give us a reason to add a new packet because in that case it will have 2 slightly different functionality based on the way lldb-platform was started on the remote target.

If you prefer adding the functionality to "platform connect" then I am happy with that solution also but I am not convinced it is any better then this approach.

FWIW, I like Greg's idea. The operation seems quite intuitive to me: "platform connect" connects to the platform, and if that happens to have a process ready, then it sets that up as well..

I am fine with adding a new platform GDB remote packet that queries for a list of processes that we need to connect to after we attach. It might be nice for clients to be able to vend N number of processes that should be attached to when the platform is connected to. This could have people that implement the GDB remote platform for JTAG style devices that connected up to a chip with multiple cores. When they do "platform connect" they would be immediately connected to all cores.

So I suggest adding some sort of packet that gives back a list of process IDs that we should attach to, and then attach to each of them.

tberghammer updated this revision to Diff 41867.Dec 4 2015, 5:20 AM
tberghammer retitled this revision from Create new "platform process connect" command to Modify "platform connect" to connect to processes as well.
tberghammer updated this object.
tberghammer edited edge metadata.
labath added a comment.Dec 4 2015, 5:42 AM

Just a tiny remark from my side..

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
184 ↗(On Diff #41867)

The TMPDIR and "sleep" comments seem to be obsolete...

clayborg requested changes to this revision.Dec 4 2015, 9:54 AM
clayborg edited edge metadata.

So my main issue with this is the new virtual "Platform::GetPendingGdbServerList(...)" function. Can we remove this function and just put the functionality into Platform::ContectRemote()?

include/lldb/Target/Platform.h
1029–1030 ↗(On Diff #41867)

Why is something GDB server specific in Platform.h? There has to be a better way.

source/Commands/CommandObjectPlatform.cpp
413–429 ↗(On Diff #41867)

This should just be done inside ConnectRemote so we don't have to expose GetPendingGdbServerList() which is GDB remote specific. If we can't do this functionality inside ConnectRemote we need to add a more suitable abstract API that is protocol agnostic.

source/Plugins/Platform/POSIX/PlatformPOSIX.h
191–192 ↗(On Diff #41867)

Remove and move functionality into ConnectRemote()

This revision now requires changes to proceed.Dec 4 2015, 9:54 AM
tberghammer updated this revision to Diff 42063.Dec 7 2015, 5:42 AM
tberghammer edited edge metadata.

I can't move the functionality into Platform::ConnectRemote because to connect to a process we need a debugger instance while Platform::ConnectRemote is called in scenarios where we don't have access to it (from SBPlatform::ConnectRemote). To clean up the API I added a new method to the Platform class called ConnectToWaitingProcesses what is connecting to all process waiting for a new debugger connection. In case of remote debugging it can be based on the gdb protocol (what I implemented with this CL) but if somebody want to support an embedded system with 1 process for each core then ConnectToWaitingProcesses can create a process for each core instead of connecting over the gdb protocol and everything should just work.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
184 ↗(On Diff #41867)

Done

clayborg accepted this revision.Dec 7 2015, 10:18 AM
clayborg edited edge metadata.

Looks good, just check me inline comment and make changes if you think you need to.

source/Commands/CommandObjectPlatform.cpp
414–419 ↗(On Diff #42063)

So should the default Platform::ConnectToWaitingProcesses() return an error? Seems like this calls should be documented to say "only return an error if you actually tried to connect to a waiting process and that failed, and if there are no processes, return an error that has been cleared". We don't want "platform connect" to fail due to not being able to connect to a waiting process do we?

This revision is now accepted and ready to land.Dec 7 2015, 10:18 AM
tberghammer added inline comments.Dec 7 2015, 10:43 AM
source/Commands/CommandObjectPlatform.cpp
414–419 ↗(On Diff #42063)

I agree that we don't want "platform connect" to fail if no process is waiting (if a process is waiting but we failed to connect then I think it should but it isn't the case with the default platform).

Currently ConnectToWaitingProcesses tries to connect to all processes what are waiting what is 0 processes for the default platform and return an error if any of them failed, but with 0 processes waiting it can't happen.

This revision was automatically updated to reflect the committed changes.