[Lldb-commits] [PATCH] D98822: [lldb] [Process] Watch for fork/vfork notifications

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D98822#2658426 <https://reviews.llvm.org/D98822#2658426>, @mgorny wrote:

> In D98822#2658252 <https://reviews.llvm.org/D98822#2658252>, @labath wrote:
>
>> In D98822#2646052 <https://reviews.llvm.org/D98822#2646052>, @mgorny wrote:
>>
>>> It includes refactoring clone/fork/vfork event monitoring into a single function. Right now, only forks are supported — the handler creates a local `NativeProcessLinux` instance (@labath, any suggestions how to make this less hacky?), duplicates parent's breakpoints into it (just like the child gets parent's memory) and uses it to clear these breakpoints and detach the child process.
>>
>> I don't find this particularly hacky. However, I have doubts about the breakpoint clearing aspect. If we add that now, I think it will be tricky to move that process to the client in the future (how will the client know whether the breakpoints were auto-removed?) Given that  breakpoints in forked process have been borked since forever, I don't see harm in keeping that broken for a little while longer. Even a client with extremely limited of multiple processes (like the one we had in mind) should not have a problem with sending the appropriate `z` packets before it detaches. This patch could still come first (it's great that you've separated that out) -- it just needs to ensure that there are no breakpoints in the child path which would need clearing.
>
> Actually, that's not a problem at all. If client doesn't indicate `fork-events` support via `qSupported`, we handle breakpoints server-side. If it does, we deliver `fork` stop reason and then the client is responsible for dealing with breakpoints.

Yeah, but then we have to maintain two copies of breakpoint-unsetting code in perpetuity.  Given that we've managed to come this far with this being completely broken, I think we can wait a little longer until the client support is in place.

>> Btw, have you by any chance looked at how gdb determines which clone()s constitute a new thread?
>
> If I read the code correctly, GDB always assumes `PTRACE_EVENT_CLONE` is a new thread, and does not support spawning processes via it.

Ah, cute. I guess someone should let them know about this (and maybe also tell kernel folks that differentiating cloned processes is really hard). :)


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

https://reviews.llvm.org/D98822