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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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

> 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`?

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.


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

https://reviews.llvm.org/D100191