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

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100191#2704681 <https://reviews.llvm.org/D100191#2704681>, @labath wrote:

> 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).

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?


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

https://reviews.llvm.org/D100191