Page MenuHomePhabricator

[lldb] [llgs] Support owning and detaching extra processes
ClosedPublic

Authored by mgorny on Apr 9 2021, 6:27 AM.

Details

Summary

Add a NativeDelegate API to pass new processes (forks) to LLGS,
and support detaching them via the 'D' packet. A 'D' packet without
a specific PID detaches all processes, otherwise it detaches either
the specified subprocess or the main process, depending on the passed
PID.

Diff Detail

Event Timeline

mgorny requested review of this revision.Apr 9 2021, 6:27 AM
mgorny created this revision.
mgorny updated this revision to Diff 336443.Apr 9 2021, 7:03 AM

Permit more-than-one-char 'D' packets.

I like this a lot more than the previous version. The thing I'd like to know is, whether we can replace m_additional_processes with something like m_debugged_processes (i.e., just have a single list of all processes we are debugging. We could replace all occurrences of m_debugged_process_up with m_current_process, which just points inside that list. Is there any reason for the "privileged" position of the original process? The way I see it, most of the operations would just work even if we treated them as equals. The only thing which might not work is resuming (or TBE, reporting the stops) of the subprocesses, but if we wanted to be safe, we could prevent that in some other way (preventing the changing of process through Hc; returning "crippled" process instances for subprocess, which return errors on resume operations; ...).

WDYT?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1065

You no longer have to worry about that...

I like this a lot more than the previous version. The thing I'd like to know is, whether we can replace m_additional_processes with something like m_debugged_processes (i.e., just have a single list of all processes we are debugging. We could replace all occurrences of m_debugged_process_up with m_current_process, which just points inside that list. Is there any reason for the "privileged" position of the original process? The way I see it, most of the operations would just work even if we treated them as equals. The only thing which might not work is resuming (or TBE, reporting the stops) of the subprocesses, but if we wanted to be safe, we could prevent that in some other way (preventing the changing of process through Hc; returning "crippled" process instances for subprocess, which return errors on resume operations; ...).

WDYT?

I've tried to limit changes to the minimum but sure, I can try doing that. I suppose it's going to make following children cleaner.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1065

Don't I? The process plugin puts the new instance in an std::unique_ptr, and passes that to all delegates. Only one delegate can take the pointer over. While I don't think we really have a case for multiple delegates doing NewSubprocess(), I suppose we should check rather than crash. Or maybe just put an assert for it.

labath added inline comments.Apr 13 2021, 4:50 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1065

I deleted the multi-delegate thingy in c9cf394f796e1 ;)

mgorny added inline comments.Apr 13 2021, 5:02 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1065

Ah, cool, I'll get to rebasing shortly.

@labath, if we're going to remove m_debugged_process_up, then I suppose it makes sense to merge D100256 first. Could you review that one, please?

mgorny updated this revision to Diff 337164.Apr 13 2021, 8:34 AM
mgorny edited the summary of this revision. (Show Details)

Updated to keep a single list of all attached processes.

mgorny updated this revision to Diff 337228.Apr 13 2021, 12:22 PM

Unset m_current_process and m_continue_process if we're about to detach it.

mgorny updated this revision to Diff 337236.Apr 13 2021, 12:43 PM

Now in a version that actually compiles.

labath added inline comments.Apr 14 2021, 11:36 AM
lldb/include/lldb/Host/common/NativeProcessProtocol.h
228

That way, the delegate _must_ do something with the child process.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3240–3241

I don't think this makes sense anymore....

3267

It might be better to carry on detaching even if one process fails, and then return the errors in batch (?)

Something like:

Error err = Error::success();
for (p : processes) {
  if (Error e = p.second.Detach().ToError())
    err = joinErrors(std::move(err), std::move(e));
}
if (err)
  SendErrorResonse(std::move(err));
3277

Open question: Should we return an error for a plain D packet, if we don't have _any_ processes around?

mgorny marked 2 inline comments as done.Apr 14 2021, 11:45 AM
mgorny added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3240–3241

Good catch. I must've accidentally reintroduced it in rebase.

3277

Practically, it probably doesn't matter. However, if client sends D without actually having process attached, then something has probably gone wrong, so might make sense to return some error.

We should also start thinking about tests. I suppose the smallest piece of functionality that could be usefully tested (with a lldb-server test) is debugging a process that forks, stopping after the fork, and detaching from the child. Shall we try making that work first?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3277

True. I suppose we could just go with whatever falls out naturally from the code, and not add any special code on account of that.

We should also start thinking about tests. I suppose the smallest piece of functionality that could be usefully tested (with a lldb-server test) is debugging a process that forks, stopping after the fork, and detaching from the child. Shall we try making that work first?

I'm really too exhausted on this to write more tests. A few commits later I'm adding a full set of basic tests for various fork scenarios. I think the patches are simple enough to justify limiting ourselves to integration testing for the whole thing, at least for the time being.

We should also start thinking about tests. I suppose the smallest piece of functionality that could be usefully tested (with a lldb-server test) is debugging a process that forks, stopping after the fork, and detaching from the child. Shall we try making that work first?

I'm really too exhausted on this to write more tests. A few commits later I'm adding a full set of basic tests for various fork scenarios. I think the patches are simple enough to justify limiting ourselves to integration testing for the whole thing, at least for the time being.

Generally LLVM patches must have accompanying tests for any code they're adding - if it's not possible to test the patch because it's too isolated/lacks related infrastructure then it's possible the patch is too small. But we try pretty hard to isolate patches and find ways to test work incrementally as it's added.

mgorny updated this revision to Diff 337535.Apr 14 2021, 1:17 PM
mgorny marked 3 inline comments as done.

All pending changes, except for the change from std::unique_ptr<...> & to std::unique_ptr<...>.

mgorny added inline comments.Apr 14 2021, 2:27 PM
lldb/include/lldb/Host/common/NativeProcessProtocol.h
228

Indeed, it must. Unfortunately, this breaks MockDelegate:

/home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h: In member function ‘virtual testing::internal::Function<void()>::Result lldb_private::MockDelegate::NewSubprocess(testing::internal::Function<void(lldb_private::NativeProcessProtocol*)>::Argument1, testing::internal::Function<void(lldb_private::NativeProcessProtocol*, std::unique_ptr<lldb_private::NativeProcessProtocol>)>::Argument2)’:
/home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:405:73: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = lldb_private::NativeProcessProtocol; _Dp = std::default_delete<lldb_private::NativeProcessProtocol>]’
  405 |     return GMOCK_MOCKER_(2, constness, Method).Invoke(gmock_a1, gmock_a2); \
      |                                                                         ^
/home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:679:30: note: in expansion of macro ‘GMOCK_METHOD2_’
  679 | #define MOCK_METHOD2(m, ...) GMOCK_METHOD2_(, , , m, __VA_ARGS__)
      |                              ^~~~~~~~~~~~~~
/home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:28:3: note: in expansion of macro ‘MOCK_METHOD2’
   28 |   MOCK_METHOD2(NewSubprocess,
      |   ^~~~~~~~~~~~
[...]

and then there are a few pages of errors.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3239

Hmm, I suppose this should only happen if we're detaching all processes, or...?

We should also start thinking about tests. I suppose the smallest piece of functionality that could be usefully tested (with a lldb-server test) is debugging a process that forks, stopping after the fork, and detaching from the child. Shall we try making that work first?

How about using a MockProcess to unittest server's behavior wrt getting NewSubprocess() and Detach() calls?

mgorny updated this revision to Diff 338232.Apr 16 2021, 2:13 PM

Added a unittest covering the whole server life cycle, including attaching a (mocked) process, forking, detaching a child, detaching the parent, forking from the child and detaching all processes.

We should also start thinking about tests. I suppose the smallest piece of functionality that could be usefully tested (with a lldb-server test) is debugging a process that forks, stopping after the fork, and detaching from the child. Shall we try making that work first?

How about using a MockProcess to unittest server's behavior wrt getting NewSubprocess() and Detach() calls?

I have mixed feelings about that. On one hand, these kinds of tests really enable you to test scenarios (often to do with various borderline and error conditions) which would not be testable in other ways. And they can run on any platform. However, you're not actually testing the error conditions, and I also have to put a bunch of other things on the other tip of the scale:

  • this test is not a very good example of how tests should be written -- it doesn't test the public interface of the class, and it doesn't even set it up in a very realistic way. For example, the class does not have a connection object initialized. I'm guessing you cannot check the result of the Handle_D function (which is directly touched by this patch, and it is at least close to the public API), because it would always return an error due to a missing connection. All of this makes the test very fragile
  • writing this test wouldn't "absolve" us of the need to write a test at a higher level, which would test the integration with a real process class. All of the checks you do here could be done at the lldb-server test level, and within the existing test framework (you couldn't check that m_current_process is null, but you could for example check that a g packet returns an error, which should be close enough).

So overall, I think that David was right that this patch is too small to be tested independently. Though I didn't say that explicitly, this is what I was also getting at with the "minimal usefully testable functionality" remark. So, while I hate to waste the effort you put into this test (and I'm very sorry that my slow response made you waste it), I don't think it would be wise to waste more of it in trying to make the test look good.

Let's identify the set of patches needed to make this testable via the lldb-server suite (this one, D100153, D100208 (or equivalent for some other os), and what else?) and test that?

lldb/include/lldb/Host/common/NativeProcessProtocol.h
228

Right. I see. In that case, I think we should fix the problem inside MockDelegate. The simplest way to do that is to add a forwarding method which repackages the arguments in a gmock-friendly way. Something like this ought to work:

class MockDelegate {
  MOCK_METHOD2(NewSubprocessImpl,
               void(NativeProcessProtocol *parent_process,
                    std::unique_ptr<NativeProcessProtocol> &child_proces /*or NativeProcessProtocol &, or whatever works best*/));
  void NewSubprocess(NativeProcessProtocol *parent_process,
                    std::unique_ptr<NativeProcessProtocol> child_proces) { NewSubprocessImpl(parent_process, child_process); }
};

That way, gmock is happy, and the code remains unaffected by testing artifacts.

Let's identify the set of patches needed to make this testable via the lldb-server suite (this one, D100153, D100208 (or equivalent for some other os), and what else?) and test that?

In its current form, D100208 relies at least on D100196 as well. I suppose we might get away without other patches for now. My logic is that as long as client doesn't indicate fork support, the regular LLDB behavior won't change. We can mock-enable fork-events in the server tests to get things rolling, unless I'm missing something.

I think we could avoid merging D100196 if I split D100208 into two parts, the earlier part just calling NewSubprocess() without actually reporting stop. This won't be really functional (or used in real sessions) but should suffice for testing.

To be honest, I really like to keep these patches small, even if it means it takes 2 or 3 patches to make a test. I would prefer just adding the test to the last patch in series.

Let's identify the set of patches needed to make this testable via the lldb-server suite (this one, D100153, D100208 (or equivalent for some other os), and what else?) and test that?

In its current form, D100208 relies at least on D100196 as well. I suppose we might get away without other patches for now. My logic is that as long as client doesn't indicate fork support, the regular LLDB behavior won't change. We can mock-enable fork-events in the server tests to get things rolling, unless I'm missing something.

Sounds great.

I think we could avoid merging D100196 if I split D100208 into two parts, the earlier part just calling NewSubprocess() without actually reporting stop. This won't be really functional (or used in real sessions) but should suffice for testing.

I'm not sure how would that work -- you'd still have to provide a way to notify the client (test) about the new process and its pid, so that the test can assert something meaningful. Let's just keep D100196, but leave out the client specific parts -- just keep it about adding the new enums and relevant server code (trivial case switches are fine).

To be honest, I really like to keep these patches small, even if it means it takes 2 or 3 patches to make a test. I would prefer just adding the test to the last patch in series.

I don't mind the multiple patches. These are a bit on the smaller side, but they are definitely easier to review than a single giant patch). It does create confusion though, since that is not the usual mode of operation around here.

Let's identify the set of patches needed to make this testable via the lldb-server suite (this one, D100153, D100208 (or equivalent for some other os), and what else?) and test that?

In its current form, D100208 relies at least on D100196 as well. I suppose we might get away without other patches for now. My logic is that as long as client doesn't indicate fork support, the regular LLDB behavior won't change. We can mock-enable fork-events in the server tests to get things rolling, unless I'm missing something.

Sounds great.

I think we could avoid merging D100196 if I split D100208 into two parts, the earlier part just calling NewSubprocess() without actually reporting stop. This won't be really functional (or used in real sessions) but should suffice for testing.

I'm not sure how would that work -- you'd still have to provide a way to notify the client (test) about the new process and its pid, so that the test can assert something meaningful. Let's just keep D100196, but leave out the client specific parts -- just keep it about adding the new enums and relevant server code (trivial case switches are fine).

Do you mean the DidFoo() methods, or the whole StopInfo stuff along with the logic in ProcessGDBRemote? If the latter, should I add some asserts for the unhandled stop reasons or just ignore them for now?

Let's identify the set of patches needed to make this testable via the lldb-server suite (this one, D100153, D100208 (or equivalent for some other os), and what else?) and test that?

In its current form, D100208 relies at least on D100196 as well. I suppose we might get away without other patches for now. My logic is that as long as client doesn't indicate fork support, the regular LLDB behavior won't change. We can mock-enable fork-events in the server tests to get things rolling, unless I'm missing something.

Sounds great.

I think we could avoid merging D100196 if I split D100208 into two parts, the earlier part just calling NewSubprocess() without actually reporting stop. This won't be really functional (or used in real sessions) but should suffice for testing.

I'm not sure how would that work -- you'd still have to provide a way to notify the client (test) about the new process and its pid, so that the test can assert something meaningful. Let's just keep D100196, but leave out the client specific parts -- just keep it about adding the new enums and relevant server code (trivial case switches are fine).

Do you mean the DidFoo() methods, or the whole StopInfo stuff along with the logic in ProcessGDBRemote?

All of it.

If the latter, should I add some asserts for the unhandled stop reasons or just ignore them for now?

Nah, just ignore it completely lldb-server will not be sending those as long as the client does not advertise support. It's possible lldb will misbehave if a broken/evil server sends reasons like these, but that's not a problem for this patch.

mgorny updated this revision to Diff 339217.Apr 21 2021, 6:42 AM
mgorny marked an inline comment as done.

Remove the test, update the API to pass std::unique_ptr<> instance instead of ref.

labath accepted this revision.Apr 23 2021, 12:30 PM
labath added inline comments.
lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
31–34

Maybe add a quick comment to explain why this is here.

This revision is now accepted and ready to land.Apr 23 2021, 12:30 PM
mgorny updated this revision to Diff 340184.Apr 23 2021, 3:40 PM
mgorny marked an inline comment as done.

Add the comment for mock stuff.

Move tests from the later patch here and skip them based on category.

mgorny updated this revision to Diff 340186.Apr 23 2021, 3:48 PM

Fix tests to include multiprocess+ in qSupported.

This revision was landed with ongoing or failed builds.Apr 24 2021, 2:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2021, 2:26 AM