[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

Wed Apr 21 04:28:12 PDT 2021

labath added a comment.

In D100191#2704492 <https://reviews.llvm.org/D100191#2704492>, @mgorny wrote:

> In D100191#2704403 <https://reviews.llvm.org/D100191#2704403>, @labath wrote:
>
>> Let's identify the set of patches needed to make this testable via the lldb-server suite (this one, D100153 <https://reviews.llvm.org/D100153>, D100208 <https://reviews.llvm.org/D100208> (or equivalent for some other os), and what else?) and test that?
>
> In its current form, D100208 <https://reviews.llvm.org/D100208> relies at least on D100196 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D100196> if I split D100208 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191