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

Rebase + clang-format.

In D100191#2685039, @labath wrote:

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?

In D98822#2685246, @labath wrote:

I've reverted this (121cff78a8032a73aa4fb820625dc1ecae8e3997) because it made the linux bot super-flaky. I think it's likely that this affects _only_ linux (and the BSD code is fine -- though we don't have a bot to verify that), but I didn't want to experiment with partial reverts while the bots are wonky. However, it may be interesting the re-land the linux support in a separate commit, once we figure out the problem -- my money is on the nondeterminism of thread creation.

In D100146#2684942, @labath wrote:

looks great, just fix the build errors :)

clang-format

Fix missing QListThreadsInStopReply+.

Report vfork-done stop to the client as well.

Base class Did*Fork() definitions have been moved to the earlier patch.

Add 'vforkdone' stop reason. Move overrideable actions to this patch.

Let's close this as all the features are now part of other diffs.

(TODO: vfork support)

Copy software breakpoints to the forked process, to future-proof this patch for breakpoint support (I suppose there's no point in splitting it more).

Ok, found the culprit. Now it's ready ;-).

Fix uninitialized flags variable in GDBRemoteCommunicationServerLLGS::SetEnabledExtensions().

This is the final piece for moving minimal fork/vfork support to gdb-remote protocol. However, for some reason (unlike the original code in D99864), it crashes on random assertions about m_enabled_extensions value — probably memory corruption somewhere. I'm debugging it.

I've decided to take another approach for tracking forked processes. Rather than keeping them inside NativeProcess* and exposing via API to LLGS, I've decided to push them via NativeDelegate straight into LLGS and maintain there. This is now D100191.

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

Add to _KNOWN_QSUPPORTED_STUB_FEATURES and clang-format.

Note that I've included full fork-events+ and vfork-events+ as demo of the API. I can split them further and/or move to more relevant commits as I progress with the split.

And now moved Extension API to D100153. I've added an API to query features supported by platform plugin, so now server doesn't report them unless they're actually going to be used.

In D98822#2677961, @JDevlieghere wrote:

test/Shell/Subprocess/vfork-follow-parent-wp.test is failing on GreenDragon:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31016/testReport/junit/lldb-shell/Subprocess/vfork_follow_parent_wp_test/

I'm going to temporarily disable the test on Darwin. Happy to provide help to investigate :-)

Client-side qSupported processing is now refactored in D100146.

The server-side qSupported handling is now split into D100140, and all relevant remarks were addressed there.

This is split from D99864 as a minimal independent change.

In D99864#2676389, @labath wrote:

I'm going to have more questions, but let me start with this: What should the (v)fork-events extension actually mean? In the client, I guess it demonstrates its willingness to receive the appropriate type of event. But what should it mean on the server? Is it like "I promise to stop on every (v)fork", or just something like "hey, I know this word"? The second meaning only makes sense if these (v)fork-events come with some new protocol extensions/packets that the client can use when communicating with the server. Such is the case with the multiprocess extension (new thread id syntax), but I am not aware of anything similar in for these features. Are there any? The gdb documentation (This feature indicates whether GDB supports fork event extensions to the remote protocol. GDB does not use such extensions unless the stub also reports that it supports them by including ‘fork-events+’ in its ‘qSupported’ reply. ) seems to indicate they might exist, but I am not sure what would they be...

If there aren't any, then it would probably be better to use the first "definition" of the feature (or something similar), as the client can legitimately be interested in whether it will be able to intercept forks or not. And if that's the case, then we should first consult the relevant NativeProcess class before we reply (v)fork-events+.

Implemented @labath's requests.

Ok, I think I've implemented all the requested changes. I'm going to test it on all three platforms now and attach the patch if it miraculously still works ;-).

Answered where answer was due, will update the rest once I finish retesting the multiprocess patch.

@labath, any further comments or is this good to go then?

Removed the hanging test.

Added breakpoint cleanup via gdb-remote protocol. Only minimal code coverage so far. If this approach is okay, I guess I'll add subprocess guards to all server commands.

@labath, I think I've made all the requested changes.

Simplified FreeBSD/NetBSD to use per-PID waitpid(). Removed arch restrictions from wp tests — made them consistent with Python tests.

Update the Linux plugin and tests, mostly.

Added a skip to the hanging new test.

Add some tests.

Changed GetCurrentProcessAndThreadIDs() to return the vector, and reformatted.

Added better PID mismatch handling in SetThreadStopInfo(). Not that most of the call sites actually check the return value...

Removed software breakpoint cleanup. Modified tests not to expect child's output at any specific point.

In D98482#2658991, @stella.stamenova wrote:

Looks like this broke the Windows lldb buildbot:

https://lab.llvm.org/buildbot/#/builders/83/builds/5173

@labath, split GetCurrentThreadIDs() as suggested. I've also modified GetCurrentProcessID() to prefer explicit PID over TID when available.

In D98822#2658443, @labath wrote:

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.

In D98822#2658252, @labath wrote:

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

Limited to server changes. Added definitions for constexpr vars.

Ok, I'm going to use this review to push the server part, then commit the uint64 client change separately, then create a new diff for client.

Fix wrongly using the debugged process' TID instead of PID.

Complete FreeBSD and NetBSD support. Includes a fix to Detach() on NetBSD.

Ahh, I've seen them fail randomly recently too. But then it suddenly worked again and I couldn't reproduce...

This change breaks cross-compilation now, as it tries running an executable built for the target system:

Now includes initial FreeBSD support. The watchpoint test still fails, we probably need to clean dbregs on fork too.

Move saved breakpoint list directly into NativeProcessProtocol.