[lldb] [Process] Watch for fork/vfork notifications

[mgorny]

Summary

Watch for fork(2)/vfork(2) (also fork/vfork-style clone(2) on Linux)
notifications and explicitly detach the forked child process, and add
initial tests for these cases. The code covers FreeBSD, Linux
and NetBSD process plugins. There is no new user-visible functionality
provided -- this change lays foundations over subsequent work on fork
support.

[mgorny]

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

2150

This is not intended to send errors like this, I've just added it to test logging.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

187 ↗

(On Diff #331393)

@labath, I'm not convinced this is the best way of doing it. Do you have any suggestions?

[mgorny]

Added option setting to the Linux plugin.

[mgorny]

Add a helper function to get PID (TGID) from TID. Recognize fork notifications from parent (SIGTRAP) and from child (SIGSTOP with a TGID != GetID()) and call a helper for them. The helper doesn't do anything except for logging right now.

[mgorny]

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

459

For some reason, this doesn't print the correct tgid value (plain printf works). Any clue what's wrong here?

[labath]

Seems mostly ok. The getPidForTid business does not seem completely ideal, though. I think it may be cleaner to keep the threads for which we have received the creation event in some kind of a purgatory list until we receive the corresponding parent event (which will clarify what kind of a child we are dealing with).

We actually did something like that a very long time ago, but I removed it for being too complicated. Now, I think it may be worth going back to it.

BTW, this made me realize of a funky scenario we may need to handle somehow. I think it's possible (on linux) that two threads will initiate a fork operation concurrently, and so by the time that everything stops, we end up with three (or more) processes...

lldb/include/lldb/Host/common/NativeProcessProtocol.h

401–404

The llvm way to do this is via an enum class in combination with LLVM_MARK_AS_BITMASK_ENUM.

lldb/source/Host/linux/Host.cpp

319

just say lldb_private::getPIDForTID

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

459

what does it print?

651

I guess we should also do something like the WaitForNewThread call in the clone case.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

187 ↗

(On Diff #331393)

I'd probably do this by creating a new virtual function (with a signature like vector<string>(ArrayRef<StringRef>)`), which would receive the client features and return the server features. The base qSupported handler would be implemented in terms of this function.

This would also allow us to avoid sending features that clearly have no place in the platform connections.

[mgorny]

In D98822#2635647, @labath wrote:

Seems mostly ok. The getPidForTid business does not seem completely ideal, though. I think it may be cleaner to keep the threads for which we have received the creation event in some kind of a purgatory list until we receive the corresponding parent event (which will clarify what kind of a child we are dealing with).

We actually did something like that a very long time ago, but I removed it for being too complicated. Now, I think it may be worth going back to it.

BTW, this made me realize of a funky scenario we may need to handle somehow. I think it's possible (on linux) that two threads will initiate a fork operation concurrently, and so by the time that everything stops, we end up with three (or more) processes...

I'm actually working on such a thing right now, though initially for forks. I can surely extend it to threads later. I will update the diff with it in a few minutes.

[mgorny]

lldb/include/lldb/Host/common/NativeProcessProtocol.h

401–404

Heh, and you've just told me to use constexprs instead of enums ;-).

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

459

tid 118029 belongs to a different tgid 4295085325, assuming child

When tgid was 0, it also printed this huge number. If I cast it to int, it prints fine. But then, first arg is also lldb::pid_t and it prints fine.

651

Yeah, I was wondering about it. On the other hand, won't the main loop handle it anyway?

Does doing it explicitly have any real gain? One thing I was worried about is that the case of parent signal first is much more common than child signal first, so having a different logic on both branches would make it more likely for bugs in the child-first branch not to be noticed.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

187 ↗

(On Diff #331393)

Yeah, that's better than my ideas ;-).

[mgorny]

Actually wait for two signals. Store the extra info on parent signal, and use it when handling the fork. For now, the handler just detaches the child and resumes the parent.

[mgorny]

Switched Extension to bitmask-enum, fixed prototype and decl for getPIDForTID and switched server features into a virtual as suggested by @labath.

[mgorny]

Fix typo: qSupported needs to be joined using ;, not ,.

[jingham]

The gdb model - since gdb only supports one debugee per gdb - is to either follow the fork or follow the parent. It would be more in keeping with lldb's model to make a new target for the child side of the fork, and use that to follow the child. That way you can continue to debug both the parent and the child processes. It doesn't look like you've gotten that far yet, but IMO that's the direction we should be going for lldb.

[mgorny]

Now using the new thingie for threads. getTIDForPID() temporarily commented out but I plan to use it to distinguish clone() for new thread from clone() for new process.

[mgorny]

In D98822#2637871, @jingham wrote:

The gdb model - since gdb only supports one debugee per gdb - is to either follow the fork or follow the parent. It would be more in keeping with lldb's model to make a new target for the child side of the fork, and use that to follow the child. That way you can continue to debug both the parent and the child processes. It doesn't look like you've gotten that far yet, but IMO that's the direction we should be going for lldb.

Long-term, sure. Between life and deadlines, we're only working on the simpler one process model right now.

[mgorny]

Add a super-trivial test that the debugger doesn't lose parent after the fork.

Next on the list: clear software breakpoints in the detached fork.

[mgorny]

Generalize the waiting function and use it for forks as well.

[labath]

In D98822#2637871, @jingham wrote:

The gdb model - since gdb only supports one debugee per gdb - is to either follow the fork or follow the parent. It would be more in keeping with lldb's model to make a new target for the child side of the fork, and use that to follow the child. That way you can continue to debug both the parent and the child processes. It doesn't look like you've gotten that far yet, but IMO that's the direction we should be going for lldb.

This is no longer true -- gdb supports multiple inferiors these days (I'm not sure when exactly did it grow that support). The automatic detaching of the fork child can be disabled by the "detach-on-fork" setting, at which point you get two inferiors for each fork.

Michal doesn't appear to be interested in this feature right now, just the ability to follow the child process. I think that's fair, as the two inferior setup would be more complicated. However, I am keeping this scenario in mind and making sure that it's possible to support it in the future. The idea is that at the gdb protocol level we will have the notion of two inferiors (using the same syntax that gdb does), but the only thing that the client (and server, mostly) support is to immediately detach from one of the processes after a fork. Later this could be changed to do something more interesting.

[jingham]

In D98822#2640881, @labath wrote:

In D98822#2637871, @jingham wrote:

The gdb model - since gdb only supports one debugee per gdb - is to either follow the fork or follow the parent. It would be more in keeping with lldb's model to make a new target for the child side of the fork, and use that to follow the child. That way you can continue to debug both the parent and the child processes. It doesn't look like you've gotten that far yet, but IMO that's the direction we should be going for lldb.

This is no longer true -- gdb supports multiple inferiors these days (I'm not sure when exactly did it grow that support). The automatic detaching of the fork child can be disabled by the "detach-on-fork" setting, at which point you get two inferiors for each fork.

Michal doesn't appear to be interested in this feature right now, just the ability to follow the child process. I think that's fair, as the two inferior setup would be more complicated. However, I am keeping this scenario in mind and making sure that it's possible to support it in the future. The idea is that at the gdb protocol level we will have the notion of two inferiors (using the same syntax that gdb does), but the only thing that the client (and server, mostly) support is to immediately detach from one of the processes after a fork. Later this could be changed to do something more interesting.

Oh, I'm glad to hear that, both about gdb and what sounds like a good plan for lldb going forward.. We just need to make sure supporting both sides of the fork stays the natural direction, even if we don't support it right off.

Using background apps to create sandbox barriers for lower-permissions tasks is becoming a more and more common architecture. Having a framework for doing "task level" debugging that can cross these process barriers as the tasks move back and forth would be a great addition to lldb, but for that we need to be able to keep both sides of the fork under lldb's control.

[mgorny]

I'm certainly not planning to put any obstacles to full multiprocess debugging in the future, and I'm going to try to pave the way for it whenever possible. One thing I'm considering right now is adding some support for tracking additional NativeProcess* instances to LLGS. While it's not technically necessary, I think it will make clearing software breakpoints before detaching cleaner. However, I'm not yet sure how to integrate vfork into this later.

[mgorny]

So I've decided to circle back a bit and start by adding server-side detaching-on-fork support. This means that this patch doesn't depend on multiprocess support anymore.

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 suppose this will make things easier when we want to make the class more persistent in the future.

TODO: vfork, clone

[mgorny]

Included vfork(). Still have to look into other clone() variants, then other platforms.

[mgorny]

Handle clone(2) for new process as well (and add a test for it).

[mgorny]

Also add clone+watchpoint test.

[mgorny]

lldb/test/Shell/Subprocess/fork-follow-parent.test

7

The tests sometimes fail if child starts after the breakpoint is hit (and therefore this check happens before stop reason below). Any suggestion how to make it work independently of where 'function run in child' happens?

[mgorny]

lldb/include/lldb/Host/common/NativeProcessProtocol.h

145

Long term, these functions are probably unnecessary. It just occurred to me to check how GDB handles it, and it handles clearing and restoring breakpoints on client side (i.e. by sending z and Z packets).

[mgorny]

lldb/include/lldb/Host/common/NativeProcessProtocol.h

145

Hmm, actually that depends on how much compatibility with old client versions we want to preserve. If we want forks/vforks to stop being broken with breakpoints when using an old client, we need to keep them as a fallback logic for when fork/vfork-events aren't supported by client.

[mgorny]

Move saved breakpoint list directly into NativeProcessProtocol.

[mgorny]

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

[mgorny]

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

[mgorny]

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

[labath]

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.

Btw, have you by any chance looked at how gdb determines which clone()s constitute a new thread?

lldb/include/lldb/Host/common/NativeProcessProtocol.h

145

I certainly don't think we need to try to "fix" older clients by doing stuff to the server. (And on top of that, I am less worried about people using older clients, than I am about older servers -- those can be harder to update.)

401–404

That's cause you were using an enum to define constants. If you were using it to define a type, then I would be fine (though I might ask to make it a scoped enum).

lldb/source/Host/linux/Host.cpp

323

Why is this checking for zero ? (When it's being set to INVALID_PID above?)

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

459

I dunno. I recommend debugging it. It smells like some kind of undefined behavior.

924

if (pgid_ret != child_pid) ?

lldb/test/Shell/Subprocess/fork-follow-parent.test

7

There are several ways around that, but I don't know which one is the best, as I'm not sure what exactly you're trying to test. You could try to capture this nondeterminism with CHECK-DAGs, but CHECK-DAGs don't combine well with CHECK-NOTs. Another option is to make this predictable by adding synchronization to the inferior. If you just wanted to check that the child process finishes correctly, you could have the parent process check its exit status and match that...

[mgorny]

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.

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.

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.

[mgorny]

lldb/test/Shell/Subprocess/fork-follow-parent.test

7

The goal is to check that parent is stopped before printing but the child is not. I suppose your last suggestion, i.e. checking for exit status, should be good enough.

[labath]

In D98822#2658426, @mgorny wrote:

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.

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

[mgorny]

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.

Ok, I guess I can do that.

What about debug registers on FreeBSD? This system is pretty unique because child processes inherit dbregs from parent, and therefore we need to clear them. GDB doesn't do that and watchpoints crash forked children. Should we keep the clearing part as a hack specific to FreeBSD, or have lldb generic clear all hardware breakpoints and watchpoints on fork? Independently of that, I'm going to file a bug for FreeBSD to see if this is really the intended behavior.

[mgorny]

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

[labath]

In D98822#2658780, @mgorny wrote:

What about debug registers on FreeBSD? This system is pretty unique because child processes inherit dbregs from parent, and therefore we need to clear them. GDB doesn't do that and watchpoints crash forked children. Should we keep the clearing part as a hack specific to FreeBSD, or have lldb generic clear all hardware breakpoints and watchpoints on fork?

I think we can keep the freebsd-specific clearing code (in the server). Clearing all hardware breakpoints in the client would be pretty logical, but if we wanted to make that work consistently, then we might need to add code to /other/ operating system implementations to /set/ the breakpoints (so that the client can clear them properly)...

lldb/include/lldb/Host/linux/Host.h

14

These spaces are pretty common in lldb, but that's not actually consistent with how the rest of llvm formats them...

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp

737

And the netbsd comment might apply to freebsd as well...

lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp

290–291 ↗

(On Diff #334296)

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

647–665

It looks like these two (and probably also PTRACE_EVENT_CLONE) could be bundled together.

659

If this fails (I guess that can only happen if the entire process was SIGKILLED), how does continuing with pid=0 help?
I'm not sure that resuming the parent (as the clone case does) is significantly better, but it seems like we should at least be consistent...

914

auto *

939

I think we have some macro or attribute for that these days...

944

Copying the breakpoints is not needed yet, right? We could just group these two (fork/vfork) paths for now...

960

llvm_unreachable("unknown clone_info.event");

963

erase(find_it) would be slightly more efficient. (And it might be better to put this before the switch, to make sure it executes in case of early exits.)

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

784 ↗

(On Diff #334296)

For NetBSD, it might be cleaner to keep waiting for the parent only, and then do a separate waitpid for the child once you get its process id. That would make the sequence of events (and the relevant code) predictable.

I think this is how this interface was meant to be used, even on linux, but linux makes it tricky because it's not possible to wait for all threads belonging to a given process in a single call -- one would have to iterate through the threads and wait for each one separately (it might be interesting to try to implement and benchmark that).

lldb/test/Shell/Subprocess/Inputs/clone.c

23 ↗

(On Diff #334296)

Better include an alignas directive, just in case.

lldb/test/Shell/Subprocess/Inputs/vfork.c

2

Maybe merge these three files into one, parameterized by a define or something?

lldb/test/Shell/Subprocess/clone-follow-parent-wp.test

1 ↗

(On Diff #334296)

Why only these three arches?

lldb/test/Shell/Subprocess/fork-follow-parent.test

2

This (and maybe others) should have UNSUPPORTED: system-windows, I guess.

[mgorny]

lldb/test/Shell/Subprocess/clone-follow-parent-wp.test

1 ↗

(On Diff #334296)

Right, I was supposed to replace this with something better later. It's supposed to limit the test to arches where we support watchpoints.

[mgorny]

Update the Linux plugin and tests, mostly.

TODO: FreeBSD, NetBSD and watchpoint feature for tests.

[mgorny]

lldb/include/lldb/Host/linux/Host.h

14

Will remove. However, note that this causes clang-format to reorder the includes.

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

784 ↗

(On Diff #334296)

I've been thinking about it and I suppose it makes sense. I was considering what would be better to future-proof it for the future full multiprocess support but I suppose we could just loop over all process classes and let every one wait for its own process rather than creating a dedicated dispatcher that calls waitpid(-1...).

[mgorny]

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

[mgorny]

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

[labath]

In D98822#2666257, @mgorny wrote:

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

This looks pretty good now. Just some small nits inline...

lldb/include/lldb/Host/linux/Host.h

14

And it should do that -- that's part of the point :)

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp

734–737

Is the loop still necessary?

990–996

assert(event&PL_FLAG_FORKED) would be shorter, but I am not sure if we need even that, as this is already checked in the caller. You could just drop the entire event argument, if it is unused..

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h

124

move this next to the other Monitor functions ?

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

651

Maybe obsolete by now, but I just realized what you actually meant here.

There will always be some divergence between the two sequence of events. Letting this be handled from the main loop may make it smaller though. I don't really mind doing it that way, as long as the (v)fork and clone cases do it the same way.

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

760 ↗

(On Diff #334957)

same here

784 ↗

(On Diff #334296)

I think that the looping would be the cleanest solution for the BSDs. For linux, we will need to call waitpid centrally, and then somehow choose which instance to notify.

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h

117 ↗

(On Diff #334957)

same here

lldb/test/API/functionalities/gdb_remote_client/TestMultiprocess.py

3 ↗

(On Diff #334957)

bad merge

lldb/test/Shell/Subprocess/Inputs/fork.cpp

30–42 ↗

(On Diff #334957)

How about:

#if def TEST_CLONE
  pid_t pid = clone(child_func, &stack[sizeof(stack)], 0, NULL);
#elif defined TEST_FORK
  pid_t pid = TEST_FORK(); // defined to fork of vfork
  if (pid == 0)
    _exit(child_func(NULL));
#endif

?

lldb/test/Shell/Subprocess/clone-follow-parent.test

2 ↗

(On Diff #334957)

The UNSUPPORTED:windows is _not_ required if you explicitly list supported operating systems. :)

[mgorny]

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

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp

734–737

I am not sure but I think there's no harm in keeping it, and it might save us from some hard-to-debug corner conditions in the future.

990–996

The distinction is necessary to know how to deal with software breakpoints, and it will be used in the followup patch (D99864, when I extend it to non-Linux targets). I think removing event at this point to reintroduce it would be a wasted effort.

That said, I guess this is yet another case for llvm_unreachable(). Also, I think it's worth keeping assert()s like this as they make sure we don't break stuff accidentally when changing code on the caller's end.

[labath]

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp

734–737

I don't mind that much, though I would be surprised if this can help with anything. IIUC, the BSD kernel always sends one notification when the process stops, and I believe this code is written in a way which expects that (only linux has special code to delay some actions until every thread registers as stopped). If, for whatever reason, we end up getting two notifications here, I expect things would blow up spectacularly...

990–996

True, but it's even better when the code is written such that the asserts/unreachables are unnecessary. One way to do that might be to do the decoding in the caller:

if (info.pl_flags & PL_FLAG_FORKED) {
  MonitorClone(info.pl_child_pid, /*bool is_vfork*/ fork_flags & PL_FLAG_VFORKED);
  return;
}

It's hard to say whether that would work without seeing the real implementation.

[mgorny]

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp

990–996

In the long run, we would probably also want a separate event for posix_spawn on NetBSD. I've figured out that reusing native constants avoids reinventing the wheel unnecessarily.

[mgorny]

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

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp

990–996

Hmm, though I guess it doesn't matter for FreeBSD right now and since this is entirely platform-specific code, I'll simplify it for now.

[mgorny]

Implemented @labath's requests.

[labath]

ship it

[JDevlieghere]

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

[mgorny]

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

I guess that vfork()-ed process doesn't start with cleared dbregs on Darwin. I suppose you don't handle forks at all, so I suppose skipping/xfailing the test is the thing to do — knowing that there's a potential issue but extremely unlikely in real life.

Forks are handled explicitly on FreeBSD, Linux and NetBSD right now but I've left these tests open to all platforms since they merely verify that LLDB doesn't explode when the debugged program forks, and that it (probably) doesn't try to debug the child accidentally.

[labath]

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.

[labath]

I don't really understand what's going on, but in the one case I was able to catch this "in the act" the inferior process had one thread which was stopped one byte after the breakpoint location (there's no valid instruction there). This leads me to believe that the FixupBreakpointPCAsNeeded logic did not fire for some reason.

[mgorny]

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.

How about recommitting without the clone() change, i.e. assuming that clone always means thread, and seeing if that causes trouble still?

[labath]

In D98822#2685256, @mgorny wrote:

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.

How about recommitting without the clone() change, i.e. assuming that clone always means thread, and seeing if that causes trouble still?

I think I have it figured out -- see the inlined comment. It seems this was one of those (rare) cases where enabling logging actually makes a race _more_ likely...

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

930–931

This is resuming the thread a second time. If the thread manages to stop between the resume inside AddThread, and here, we will miss the breakpoint (and subsequently crash).

Deleting these two lines should fix things.

[mgorny]

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

930–931

Hmm, I wonder why it ended up being added here. Probably my mistake, sorry, and thanks for debugging it. I'm going to test and reland it shortly!

lldb/include/lldb/Host/common/NativeProcessProtocol.h

public:
   virtual const ArchSpec &GetArchitecture() const = 0;
 
   // Breakpoint functions
   virtual Status SetBreakpoint(lldb::addr_t addr, uint32_t size,
                                bool hardware) = 0;
 
   virtual Status RemoveBreakpoint(lldb::addr_t addr, bool hardware = false);
 
   // Hardware Breakpoint functions
   virtual const HardwareBreakpointMap &GetHardwareBreakpointMap() const;
 
   virtual Status SetHardwareBreakpoint(lldb::addr_t addr, size_t size);
 
   virtual Status RemoveHardwareBreakpoint(lldb::addr_t addr);
 
   // Watchpoint functions
   virtual const NativeWatchpointList::WatchpointMap &GetWatchpointMap() const;
public:
   }
 
   /// \copydoc Process::GetSupportedTraceType()
   virtual llvm::Expected<TraceTypeInfo> GetSupportedTraceType() {
     return llvm::make_error<UnimplementedError>();
   }
 
 protected:
   struct SoftwareBreakpoint {
     uint32_t ref_count;
     llvm::SmallVector<uint8_t, 4> saved_opcodes;
     llvm::ArrayRef<uint8_t> breakpoint_opcodes;
   };
 
   std::unordered_map<lldb::addr_t, SoftwareBreakpoint> m_software_breakpoints;
   lldb::pid_t m_pid;
 
   std::vector<std::unique_ptr<NativeThreadProtocol>> m_threads;
   lldb::tid_t m_current_thread_id = LLDB_INVALID_THREAD_ID;
   mutable std::recursive_mutex m_threads_mutex;
protected:
   /// Notify the delegate that an exec occurred.
   ///
   /// Provide a mechanism for a delegate to clear out any exec-
   /// sensitive data.
   void NotifyDidExec();
 
   NativeThreadProtocol *GetThreadByIDUnlocked(lldb::tid_t tid);
 
+  // Software breakpoints saved for the duration of vfork().
+  llvm::SmallVector<std::pair<lldb::addr_t, size_t>, 8> m_saved_breakpoints;
+
+  // Disable all current software breakpoints and store them for reenabling
+  // later.
+  llvm::Error DisableSoftwareBreakpoints();
+
+  // Reenable all software breakpoints (undo DisableSoftwareBreakpoints()).
+  llvm::Error EnableSoftwareBreakpoints();
+
 private:
   void SynchronouslyNotifyProcessStateChanged(lldb::StateType state);
   llvm::Expected<SoftwareBreakpoint>
   EnableSoftwareBreakpoint(lldb::addr_t addr, uint32_t size_hint);
 };
 } // namespace lldb_private
 
 #endif // LLDB_HOST_COMMON_NATIVEPROCESSPROTOCOL_H

lldb/include/lldb/Host/linux/Host.h

+//===-- Host.h --------------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_HOST_LINUX_HOST_H
+#define LLDB_HOST_LINUX_HOST_H
+
+#include "llvm/ADT/Optional.h"
+
+#include "lldb/lldb-types.h"
+
+namespace lldb_private {
+
+// Get PID (i.e. the primary thread ID) corresponding to the specified TID.
+llvm::Optional<lldb::pid_t> getPIDForTID(lldb::pid_t tid);
+
+} // namespace lldb_private
+
+#endif // #ifndef LLDB_HOST_LINUX_HOST_H

lldb/source/Host/common/NativeProcessProtocol.cpp

 //===-- NativeProcessProtocol.cpp -----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/common/NativeBreakpointList.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/common/NativeThreadProtocol.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
 #include "lldb/lldb-enumerations.h"
uint32_t NativeProcessProtocol::GetStopID() const {
   std::lock_guard<std::recursive_mutex> guard(m_state_mutex);
   return m_stop_id;
 }
 
 void NativeProcessProtocol::DoStopIDBumped(uint32_t /* newBumpId */) {
   // Default implementation does nothing.
 }
 
+llvm::Error NativeProcessProtocol::DisableSoftwareBreakpoints() {
+  while (!m_software_breakpoints.empty()) {
+    auto &bp = *m_software_breakpoints.begin();
+    m_saved_breakpoints.emplace_back(bp.first, bp.second.saved_opcodes.size());
+    Status ret = RemoveSoftwareBreakpoint(bp.first);
+    if (ret.Fail())
+      return ret.ToError();
+  }
+
+  return llvm::Error::success();
+}
+
+llvm::Error NativeProcessProtocol::EnableSoftwareBreakpoints() {
+  for (auto it = m_saved_breakpoints.begin(); it != m_saved_breakpoints.end();
+       it = m_saved_breakpoints.erase(it)) {
+    Status ret = SetSoftwareBreakpoint(it->first, it->second);
+    if (ret.Fail())
+      return ret.ToError();
+  }
+  return llvm::Error::success();
+}
+
 NativeProcessProtocol::Factory::~Factory() = default;

lldb/source/Host/linux/Host.cpp

 #include <sys/types.h>
 #include <sys/utsname.h>
 #include <unistd.h>
 
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Object/ELF.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/ProcessInfo.h"
 #include "lldb/Utility/Status.h"
 
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Host/linux/Host.h"
 #include "lldb/Host/linux/Support.h"
 #include "lldb/Utility/DataExtractor.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace {
 enum class ProcessState {

 };
 }
 
 namespace lldb_private {
 class ProcessLaunchInfo;
 }
 
 static bool GetStatusInfo(::pid_t Pid, ProcessInstanceInfo &ProcessInfo,
-                          ProcessState &State, ::pid_t &TracerPid) {
+                          ProcessState &State, ::pid_t &TracerPid,
+                          ::pid_t &Tgid) {
   Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
 
   auto BufferOrError = getProcFile(Pid, "status");
   if (!BufferOrError)
     return false;
 
   llvm::StringRef Rest = BufferOrError.get()->getBuffer();
   while (!Rest.empty()) {
if (Line.consume_front("Gid:")) {
                   .Case("Z", ProcessState::Zombie)
                   .Default(ProcessState::Unknown);
       if (State == ProcessState::Unknown) {
         LLDB_LOG(log, "Unknown process state {0}", Line);
       }
     } else if (Line.consume_front("TracerPid:")) {
       Line = Line.ltrim();
       Line.consumeInteger(10, TracerPid);
+    } else if (Line.consume_front("Tgid:")) {
+      Line = Line.ltrim();
+      Line.consumeInteger(10, Tgid);
     }
   }
   return true;
 }
 
 static bool IsDirNumeric(const char *dname) {
   for (; *dname; dname++) {
     if (!isdigit(*dname))
while (!Rest.empty()) {
     std::tie(Var, Rest) = Rest.split('\0');
     process_info.GetEnvironment().insert(Var);
   }
 }
 
 static bool GetProcessAndStatInfo(::pid_t pid,
                                   ProcessInstanceInfo &process_info,
                                   ProcessState &State, ::pid_t &tracerpid) {
+  ::pid_t tgid;
   tracerpid = 0;
   process_info.Clear();
 
   process_info.SetProcessID(pid);
 
   GetExePathAndArch(pid, process_info);
   GetProcessArgs(pid, process_info);
   GetProcessEnviron(pid, process_info);
 
   // Get User and Group IDs and get tracer pid.
-  if (!GetStatusInfo(pid, process_info, State, tracerpid))
+  if (!GetStatusInfo(pid, process_info, State, tracerpid, tgid))
     return false;
 
   return true;
 }
 
 uint32_t Host::FindProcessesImpl(const ProcessInstanceInfoMatch &match_info,
                                  ProcessInstanceInfoList &process_infos) {
   static const char procdir[] = "/proc/";
bool Host::GetProcessInfo(lldb::pid_t pid, ProcessInstanceInfo &process_info) {
   return GetProcessAndStatInfo(pid, process_info, State, tracerpid);
 }
 
 Environment Host::GetEnvironment() { return Environment(environ); }
 
 Status Host::ShellExpandArguments(ProcessLaunchInfo &launch_info) {
   return Status("unimplemented");
 }
+
+llvm::Optional<lldb::pid_t> lldb_private::getPIDForTID(lldb::pid_t tid) {
+  ::pid_t tracerpid, tgid = LLDB_INVALID_PROCESS_ID;
+  ProcessInstanceInfo process_info;
+  ProcessState state;
+
+  if (!GetStatusInfo(tid, process_info, state, tracerpid, tgid) || tgid == 0)
+    return llvm::None;
+  return tgid;
+}

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h

 //===-- NativeProcessFreeBSD.h -------------------------------- -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_NativeProcessFreeBSD_H_
 #define liblldb_NativeProcessFreeBSD_H_
 
 #include "Plugins/Process/POSIX/NativeProcessELF.h"
 #include "Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h"
 
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/FileSpec.h"
 
 #include "NativeThreadFreeBSD.h"
 
private:
   void MonitorSignal(lldb::pid_t pid, int signal);
 
   Status PopulateMemoryRegionCache();
   void SigchldHandler();
 
   Status Attach();
   Status SetupTrace();
   Status ReinitializeThreads();
+
+  struct CloneInfo {
+    int32_t event;
+    lldb::tid_t parent_tid;
+  };
+
+  // Map of child processes that have been signaled once, and we are
+  // waiting for the second signal.
+  llvm::DenseMap<lldb::pid_t, llvm::Optional<CloneInfo>> m_pending_pid_map;
+
+  // Handle a clone()-like event.  If received by parent, clone_info contains
+  // additional info.  Returns true if the event is handled, or false if it
+  // is pending second notification.
+  bool MonitorClone(lldb::pid_t child_pid,
+                    llvm::Optional<CloneInfo> clone_info);
 };
 
 } // namespace process_freebsd
 } // namespace lldb_private
 
 #endif // #ifndef liblldb_NativeProcessFreeBSD_H_

lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp

void NativeProcessFreeBSD::MonitorExited(lldb::pid_t pid, WaitStatus status) {
 
   SetExitStatus(status, true);
 
   // Notify delegate that our process has exited.
   SetState(StateType::eStateExited, true);
 }
 
 void NativeProcessFreeBSD::MonitorSIGSTOP(lldb::pid_t pid) {
+  if (pid == GetID()) {
     /* Stop all Threads attached to Process */
     for (const auto &thread : m_threads) {
       static_cast<NativeThreadFreeBSD &>(*thread).SetStoppedBySignal(SIGSTOP,
                                                                      nullptr);
     }
     SetState(StateType::eStateStopped, true);
+  } else
+    MonitorClone(pid, llvm::None);
 }
 
 void NativeProcessFreeBSD::MonitorSIGTRAP(lldb::pid_t pid) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
   struct ptrace_lwpinfo info;
 
   const auto siginfo_err = PtraceWrapper(PT_LWPINFO, pid, &info, sizeof(info));
   if (siginfo_err.Fail()) {
if (info.pl_flags & PL_FLAG_EXEC) {
     NotifyDidExec();
 
     for (const auto &thread : m_threads)
       static_cast<NativeThreadFreeBSD &>(*thread).SetStoppedByExec();
     SetState(StateType::eStateStopped, true);
     return;
   }
 
+  if (info.pl_flags & (PL_FLAG_FORKED | PL_FLAG_VFORKED)) {
+    MonitorClone(info.pl_child_pid,
+                 {{info.pl_flags & (PL_FLAG_FORKED | PL_FLAG_VFORKED), pid}});
+    return;
+  }
+
+  if (info.pl_flags & PL_FLAG_VFORK_DONE) {
+    // restore breakpoints
+    llvm::Error bp_err = EnableSoftwareBreakpoints();
+    if (bp_err) {
+      LLDB_LOG_ERROR(log, std::move(bp_err),
+                     "failed to restore breakpoints after vfork: {0}");
+      SetState(StateType::eStateInvalid);
+      return;
+    }
+    Status error =
+        PtraceWrapper(PT_CONTINUE, pid, reinterpret_cast<void *>(1), 0);
+    if (error.Fail())
+      SetState(StateType::eStateInvalid);
+    return;
+  }
+
   if (info.pl_lwpid > 0) {
     for (const auto &t : m_threads) {
       if (t->GetID() == static_cast<lldb::tid_t>(info.pl_lwpid))
         thread = static_cast<NativeThreadFreeBSD *>(t.get());
       static_cast<NativeThreadFreeBSD *>(t.get())->SetStoppedWithNoReason();
     }
     if (!thread)
       LLDB_LOG(log, "thread not found in m_threads, pid = {0}, LWP = {1}", pid,
for (const auto &it : m_mem_region_cache) {
     }
   }
   return Status("No load address found for file %s.", file_name.str().c_str());
 }
 
 void NativeProcessFreeBSD::SigchldHandler() {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
   // Process all pending waitpid notifications.
+  while (true) {
     int status;
     ::pid_t wait_pid =
-      llvm::sys::RetryAfterSignal(-1, waitpid, GetID(), &status, WNOHANG);
+        llvm::sys::RetryAfterSignal(-1, waitpid, -1, &status, WNOHANG);
 
     if (wait_pid == 0)
       return; // We are done.
 
     if (wait_pid == -1) {
       Status error(errno, eErrorTypePOSIX);
-    LLDB_LOG(log, "waitpid ({0}, &status, _) failed: {1}", GetID(), error);
+      LLDB_LOG(log, "waitpid (-1, &status, _) failed: {0}", error);
     }
 
     WaitStatus wait_status = WaitStatus::Decode(status);
     bool exited = wait_status.type == WaitStatus::Exit ||
                   (wait_status.type == WaitStatus::Signal &&
                    wait_pid == static_cast<::pid_t>(GetID()));
 
     LLDB_LOG(log,
-           "waitpid ({0}, &status, _) => pid = {1}, status = {2}, exited = {3}",
-           GetID(), wait_pid, status, exited);
+             "waitpid (-1, &status, _) => pid = {0}, status = {1}, exited = {2}",
+             wait_pid, status, exited);
 
     if (exited)
       MonitorExited(wait_pid, wait_status);
     else {
       assert(wait_status.type == WaitStatus::Stop);
       MonitorCallback(wait_pid, wait_status.status);
     }
   }
+}
 
 bool NativeProcessFreeBSD::HasThreadNoLock(lldb::tid_t thread_id) {
   for (const auto &thread : m_threads) {
     assert(thread && "thread list should not contain NULL threads");
     if (thread->GetID() == thread_id) {
       // We have this thread.
       return true;
     }

 
 Status NativeProcessFreeBSD::SetupTrace() {
   // Enable event reporting
   int events;
   Status status =
       PtraceWrapper(PT_GET_EVENT_MASK, GetID(), &events, sizeof(events));
   if (status.Fail())
     return status;
-  events |= PTRACE_LWP;
+  events |= PTRACE_LWP | PTRACE_FORK | PTRACE_VFORK;
   status = PtraceWrapper(PT_SET_EVENT_MASK, GetID(), &events, sizeof(events));
   if (status.Fail())
     return status;
 
   return ReinitializeThreads();
 }
 
 Status NativeProcessFreeBSD::ReinitializeThreads() {
for (lwpid_t lwp : lwp_ids)
     AddThread(lwp);
 
   return error;
 }
 
 bool NativeProcessFreeBSD::SupportHardwareSingleStepping() const {
   return !m_arch.IsMIPS();
 }
+
+bool NativeProcessFreeBSD::MonitorClone(
+    lldb::pid_t child_pid,
+    llvm::Optional<NativeProcessFreeBSD::CloneInfo> clone_info) {
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
+  LLDB_LOG(log, "clone, child_pid={0}, clone info?={1}", child_pid,
+           clone_info.hasValue());
+
+  auto find_it = m_pending_pid_map.find(child_pid);
+  if (find_it == m_pending_pid_map.end()) {
+    // not in the map, so this is the first signal for the PID
+    m_pending_pid_map.insert({child_pid, clone_info});
+    return false;
+  }
+
+  // second signal for the pid
+  assert(clone_info.hasValue() != find_it->second.hasValue());
+  if (!clone_info) {
+    // child signal does not indicate the event, so grab the one stored
+    // earlier
+    clone_info = find_it->second;
+  }
+
+  LLDB_LOG(log, "second signal for child_pid={0}, parent_tid={1}, event={2}",
+           child_pid, clone_info->parent_tid, clone_info->event);
+
+  MainLoop unused_loop;
+  NativeProcessFreeBSD child_process{static_cast<::pid_t>(child_pid),
+                                     m_terminal_fd, *m_delegates[0], m_arch,
+                                     unused_loop};
+
+  switch (clone_info->event) {
+  case PL_FLAG_FORKED: {
+    child_process.m_software_breakpoints = m_software_breakpoints;
+    llvm::Error bp_err = child_process.DisableSoftwareBreakpoints();
+    if (bp_err) {
+      LLDB_LOG_ERROR(
+          log, std::move(bp_err),
+          "failed to remove breakpoints from forked process {1}: {0}",
+          child_pid);
+      SetState(StateType::eStateInvalid);
+      return true;
+    }
+    break;
+  }
+  case PL_FLAG_FORKED|PL_FLAG_VFORKED: {
+    // vfork shares memory with the parent, so we need to temporarily remove
+    // breakpoints
+    llvm::Error bp_err = DisableSoftwareBreakpoints();
+    if (bp_err) {
+      LLDB_LOG_ERROR(
+          log, std::move(bp_err),
+          "failed to remove breakpoints from vforked process {1}: {0}",
+          child_pid);
+      SetState(StateType::eStateInvalid);
+      return true;
+    }
+    break;
+  }
+  default:
+    assert(false && "unknown clone_info.event");
+  }
+
+  child_process.Detach();
+  Status error = PtraceWrapper(PT_CONTINUE, clone_info->parent_tid,
+                               reinterpret_cast<void *>(1), 0);
+  if (error.Fail())
+    SetState(StateType::eStateInvalid);
+
+  m_pending_pid_map.erase(child_pid);
+  return true;
+}

lldb/source/Plugins/Process/Linux/NativeProcessLinux.h

 //===-- NativeProcessLinux.h ---------------------------------- -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_NativeProcessLinux_H_
 #define liblldb_NativeProcessLinux_H_
 
 #include <csignal>
 #include <unordered_set>
 
 #include "lldb/Host/Debug.h"
 #include "lldb/Host/HostThread.h"
 #include "lldb/Host/linux/Support.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-types.h"
 
 #include "NativeThreadLinux.h"
private:
 
   // Returns a list of process threads that we have attached to.
   static llvm::Expected<std::vector<::pid_t>> Attach(::pid_t pid);
 
   static Status SetDefaultPtraceOpts(const lldb::pid_t);
 
   void MonitorCallback(lldb::pid_t pid, bool exited, WaitStatus status);
 
-  void WaitForNewThread(::pid_t tid);
+  void WaitForCloneNotification(::pid_t pid);
 
   void MonitorSIGTRAP(const siginfo_t &info, NativeThreadLinux &thread);
 
   void MonitorTrace(NativeThreadLinux &thread);
 
   void MonitorBreakpoint(NativeThreadLinux &thread);
 
   void MonitorWatchpoint(NativeThreadLinux &thread, uint32_t wp_index);
llvm::DenseMap<lldb::tid_t, ProcessorTraceMonitorUP>
       m_processor_trace_monitor;
 
   // Set for tracking threads being traced under
   // same process user id.
   llvm::DenseSet<lldb::tid_t> m_pt_traced_thread_group;
 
   lldb::user_id_t m_pt_proces_trace_id = LLDB_INVALID_UID;
   TraceOptions m_pt_process_trace_config;
+
+  struct CloneInfo {
+    uint32_t event;
+    lldb::tid_t parent_tid;
+  };
+
+  // Map of child processes that have been signaled once, and we are
+  // waiting for the second signal.
+  llvm::DenseMap<lldb::pid_t, llvm::Optional<CloneInfo>> m_pending_pid_map;
+
+  // Handle a clone()-like event.  If received by parent, clone_info contains
+  // additional info.  Returns true if the event is handled, or false if it
+  // is pending second notification.
+  bool MonitorClone(lldb::pid_t child_pid,
+                    llvm::Optional<CloneInfo> clone_info);
 };
 
 } // namespace process_linux
 } // namespace lldb_private
 
 #endif // #ifndef liblldb_NativeProcessLinux_H_

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

 #include "Procfs.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostProcess.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
+#include "lldb/Host/linux/Host.h"
 #include "lldb/Host/linux/Ptrace.h"
 #include "lldb/Host/linux/Uio.h"
 #include "lldb/Host/posix/ProcessLauncherPosixFork.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/State.h"

 Status NativeProcessLinux::SetDefaultPtraceOpts(lldb::pid_t pid) {
   long ptrace_opts = 0;
 
   // Have the child raise an event on exit.  This is used to keep the child in
   // limbo until it is destroyed.
   ptrace_opts |= PTRACE_O_TRACEEXIT;
 
   // Have the tracer trace threads which spawn in the inferior process.
-  // TODO: if we want to support tracing the inferiors' child, add the
-  // appropriate ptrace flags here (PTRACE_O_TRACEFORK, PTRACE_O_TRACEVFORK)
   ptrace_opts |= PTRACE_O_TRACECLONE;
 
   // Have the tracer notify us before execve returns (needed to disable legacy
   // SIGTRAP generation)
   ptrace_opts |= PTRACE_O_TRACEEXEC;
 
+  // Have the tracer trace forked children.
+  ptrace_opts |= PTRACE_O_TRACEFORK;
+
+  // Have the tracer trace vforks.
+  ptrace_opts |= PTRACE_O_TRACEVFORK;
+
+  // Have the tracer trace vfork-done in order to restore breakpoints after
+  // the child finishes sharing memory.
+  ptrace_opts |= PTRACE_O_TRACEVFORKDONE;
+
   return PtraceWrapper(PTRACE_SETOPTIONS, pid, nullptr, (void *)ptrace_opts);
 }
 
 // Handles all waitpid events from the inferior process.
 void NativeProcessLinux::MonitorCallback(lldb::pid_t pid, bool exited,
                                          WaitStatus status) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
if (info_err.Fail()) {
                "Ingoring this notification.",
                pid, info_err);
       return;
     }
 
     LLDB_LOG(log, "tid {0}, si_code: {1}, si_pid: {2}", pid, info.si_code,
              info.si_pid);
 
-    NativeThreadLinux &thread = AddThread(pid);
-
-    // Resume the newly created thread.
-    ResumeThread(thread, eStateRunning, LLDB_INVALID_SIGNAL_NUMBER);
-    ThreadWasCreated(thread);
+    MonitorClone(pid, llvm::None);
     return;
   }
 
   // Get details on the signal raised.
   if (info_err.Success()) {
     // We have retrieved the signal info.  Dispatch appropriately.
     if (info.si_signo == SIGTRAP)
       MonitorSIGTRAP(info, *thread_sp);
     else
       MonitorSignal(info, *thread_sp, exited);
   } else {
if (info_err.GetError() == EINVAL) {
                  "tell delegate anything since thread disappeared out "
                  "from underneath us",
                  GetID(), pid);
       }
     }
   }
 }
 
-void NativeProcessLinux::WaitForNewThread(::pid_t tid) {
+void NativeProcessLinux::WaitForCloneNotification(::pid_t pid) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
 
-  if (GetThreadByID(tid)) {
-    // We are already tracking the thread - we got the event on the new thread
-    // (see MonitorSignal) before this one. We are done.
-    return;
-  }
-
-  // The thread is not tracked yet, let's wait for it to appear.
+  // The PID is not tracked yet, let's wait for it to appear.
   int status = -1;
   LLDB_LOG(log,
-           "received thread creation event for tid {0}. tid not tracked "
-           "yet, waiting for thread to appear...",
-           tid);
-  ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, ::waitpid, tid, &status, __WALL);
-  // Since we are waiting on a specific tid, this must be the creation event.
+           "received clone event for pid {0}. pid not tracked yet, "
+           "waiting for it to appear...",
+           pid);
+  ::pid_t wait_pid =
+      llvm::sys::RetryAfterSignal(-1, ::waitpid, pid, &status, __WALL);
+  // Since we are waiting on a specific pid, this must be the creation event.
   // But let's do some checks just in case.
-  if (wait_pid != tid) {
+  if (wait_pid != pid) {
     LLDB_LOG(log,
-             "waiting for tid {0} failed. Assuming the thread has "
+             "waiting for pid {0} failed. Assuming the pid has "
              "disappeared in the meantime",
-             tid);
+             pid);
     // The only way I know of this could happen is if the whole process was
     // SIGKILLed in the mean time. In any case, we can't do anything about that
     // now.
     return;
   }
   if (WIFEXITED(status)) {
     LLDB_LOG(log,
-             "waiting for tid {0} returned an 'exited' event. Not "
-             "tracking the thread.",
-             tid);
+             "waiting for pid {0} returned an 'exited' event. Not "
+             "tracking it.",
+             pid);
     // Also a very improbable event.
+    m_pending_pid_map.erase(pid);
     return;
   }
 
-  LLDB_LOG(log, "pid = {0}: tracking new thread tid {1}", GetID(), tid);
-  NativeThreadLinux &new_thread = AddThread(tid);
-
-  ResumeThread(new_thread, eStateRunning, LLDB_INVALID_SIGNAL_NUMBER);
-  ThreadWasCreated(new_thread);
+  MonitorClone(pid, llvm::None);
 }
 
 void NativeProcessLinux::MonitorSIGTRAP(const siginfo_t &info,
                                         NativeThreadLinux &thread) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
   const bool is_main_thread = (thread.GetID() == GetID());
 
   assert(info.si_signo == SIGTRAP && "Unexpected child signal!");
 
   switch (info.si_code) {
-  // TODO: these two cases are required if we want to support tracing of the
-  // inferiors' children.  We'd need this to debug a monitor. case (SIGTRAP |
-  // (PTRACE_EVENT_FORK << 8)): case (SIGTRAP | (PTRACE_EVENT_VFORK << 8)):
-
   case (SIGTRAP | (PTRACE_EVENT_CLONE << 8)): {
-    // This is the notification on the parent thread which informs us of new
-    // thread creation. We don't want to do anything with the parent thread so
-    // we just resume it. In case we want to implement "break on thread
-    // creation" functionality, we would need to stop here.
+    // This can either mean a new thread or a new process spawned via
+    // clone(2) without SIGCHLD or CLONE_VFORK flag.  Note that clone(2)
+    // can also cause PTRACE_EVENT_FORK and PTRACE_EVENT_VFORK if one
+    // of these flags are passed.
 
     unsigned long event_message = 0;
     if (GetEventMessage(thread.GetID(), &event_message).Fail()) {
       LLDB_LOG(log,
-               "pid {0} received thread creation event but "
-               "GetEventMessage failed so we don't know the new tid",
+               "pid {0} received clone() event but GetEventMessage failed "
+               "so we don't know the new pid/tid",
                thread.GetID());
-    } else
-      WaitForNewThread(event_message);
-
       ResumeThread(thread, thread.GetState(), LLDB_INVALID_SIGNAL_NUMBER);
+    } else {
+      if (!MonitorClone(event_message, {{PTRACE_EVENT_CLONE, thread.GetID()}}))
+        WaitForCloneNotification(event_message);
+    }
+
     break;
   }
 
   case (SIGTRAP | (PTRACE_EVENT_EXEC << 8)): {
     LLDB_LOG(log, "received exec event, code = {0}", info.si_code ^ SIGTRAP);
 
     // Exec clears any pending notifications.
     m_pending_notification_tid = LLDB_INVALID_THREAD_ID;
if (!StateIsRunningState(state)) {
       // inferior is resumed and exits normally.
       state = eStateRunning;
     }
     ResumeThread(thread, state, LLDB_INVALID_SIGNAL_NUMBER);
 
     break;
   }
 
+  case (SIGTRAP | (PTRACE_EVENT_FORK << 8)): {
+    unsigned long data = 0;
+    if (GetEventMessage(thread.GetID(), &data).Fail())
+      data = 0;
+
+    if (!MonitorClone(data, {{PTRACE_EVENT_FORK, thread.GetID()}}))
+      WaitForCloneNotification(data);
+    break;
+  }
+
+  case (SIGTRAP | (PTRACE_EVENT_VFORK << 8)): {
+    unsigned long data = 0;
+    if (GetEventMessage(thread.GetID(), &data).Fail())
+      data = 0;
+
+    if (!MonitorClone(data, {{PTRACE_EVENT_VFORK, thread.GetID()}}))
+      WaitForCloneNotification(data);
+    break;
+  }
+
+  case (SIGTRAP | (PTRACE_EVENT_VFORK_DONE << 8)): {
+    // restore breakpoints
+    llvm::Error bp_err = EnableSoftwareBreakpoints();
+    if (bp_err) {
+      LLDB_LOG_ERROR(log, std::move(bp_err),
+                     "failed to restore breakpoints after vfork: {0}");
+      SetState(StateType::eStateInvalid);
+      return;
+    }
+    ResumeThread(thread, thread.GetState(), LLDB_INVALID_SIGNAL_NUMBER);
+    break;
+  }
+
   case 0:
   case TRAP_TRACE:  // We receive this on single stepping.
   case TRAP_HWBKPT: // We receive this on watchpoint hit
   {
     // If a watchpoint was hit, report it
     uint32_t wp_index;
     Status error = thread.GetRegisterContext().GetWatchpointHitIndex(
         wp_index, (uintptr_t)info.si_addr);
void NativeProcessLinux::MonitorSignal(const siginfo_t &info,
   // This thread is stopped.
   LLDB_LOG(log, "received signal {0}", Host::GetSignalAsCString(signo));
   thread.SetStoppedBySignal(signo, &info);
 
   // Send a stop to the debugger after we get all other threads to stop.
   StopRunningThreads(thread.GetID());
 }
 
+bool NativeProcessLinux::MonitorClone(
+    lldb::pid_t child_pid,
+    llvm::Optional<NativeProcessLinux::CloneInfo> clone_info) {
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
+  LLDB_LOG(log, "clone, child_pid={0}, clone info?={1}", child_pid,
+           clone_info.hasValue());
+
+  auto find_it = m_pending_pid_map.find(child_pid);
+  if (find_it == m_pending_pid_map.end()) {
+    // not in the map, so this is the first signal for the PID
+    m_pending_pid_map.insert({child_pid, clone_info});
+    return false;
+  }
+
+  // second signal for the pid
+  assert(clone_info.hasValue() != find_it->second.hasValue());
+  if (!clone_info) {
+    // child signal does not indicate the event, so grab the one stored
+    // earlier
+    clone_info = find_it->second;
+  }
+
+  LLDB_LOG(log, "second signal for child_pid={0}, parent_tid={1}, event={2}",
+           child_pid, clone_info->parent_tid, clone_info->event);
+
+  auto parent_thread = GetThreadByID(clone_info->parent_tid);
+  assert(parent_thread);
+
+  switch (clone_info->event) {
+  case PTRACE_EVENT_CLONE: {
+    // PTRACE_EVENT_CLONE can either mean a new thread or a new process.
+    // Try to grab the new process' PGID to figure out which one it is.
+    // If PGID is the same as the PID, then it's a new process.  Otherwise,
+    // it's a thread.
+    auto pgid_ret = getPIDForTID(child_pid);
+    if (!pgid_ret || pgid_ret.getValue() != child_pid) {
+      // A new thread should have PGID matching our process' PID.
+      assert(!pgid_ret || pgid_ret.getValue() == clone_info->parent_tid);
+
+      NativeThreadLinux &child_thread = AddThread(child_pid);
+      // Resume the newly created thread.
+      ResumeThread(child_thread, eStateRunning, LLDB_INVALID_SIGNAL_NUMBER);
+      ThreadWasCreated(child_thread);
+
+      // Resume the parent.
+      ResumeThread(*parent_thread, parent_thread->GetState(),
+                   LLDB_INVALID_SIGNAL_NUMBER);
+      break;
+    }
+  }
+  // fallthrough
+  case PTRACE_EVENT_FORK: {
+    MainLoop unused_loop;
+    NativeProcessLinux child_process{child_pid, m_terminal_fd, *m_delegates[0],
+                                     m_arch,    unused_loop,   {child_pid}};
+    child_process.m_software_breakpoints = m_software_breakpoints;
+    llvm::Error bp_err = child_process.DisableSoftwareBreakpoints();
+    if (bp_err) {
+      LLDB_LOG_ERROR(log, std::move(bp_err),
+                     "failed to remove breakpoints from forked process {1}: {0}",
+                     child_pid);
+      SetState(StateType::eStateInvalid);
+      return true;
+    }
+    child_process.Detach();
+    ResumeThread(*parent_thread, parent_thread->GetState(),
+                 LLDB_INVALID_SIGNAL_NUMBER);
+    break;
+  }
+  case PTRACE_EVENT_VFORK: {
+    MainLoop unused_loop;
+    NativeProcessLinux child_process{child_pid, m_terminal_fd, *m_delegates[0],
+                                     m_arch,    unused_loop,   {child_pid}};
+    // vfork shares memory with the parent, so we need to temporarily remove
+    // breakpoints
+    llvm::Error bp_err = DisableSoftwareBreakpoints();
+    if (bp_err) {
+      LLDB_LOG_ERROR(log, std::move(bp_err),
+                     "failed to remove breakpoints from vforked process {1}: {0}",
+                     child_pid);
+      SetState(StateType::eStateInvalid);
+      return true;
+    }
+    child_process.Detach();
+    ResumeThread(*parent_thread, parent_thread->GetState(),
+                 LLDB_INVALID_SIGNAL_NUMBER);
+    break;
+  }
+  default:
+    assert(false && "unknown clone_info.event");
+  }
+
+  m_pending_pid_map.erase(child_pid);
+  return true;
+}
+
 bool NativeProcessLinux::SupportHardwareSingleStepping() const {
   if (m_arch.GetMachine() == llvm::Triple::arm || m_arch.IsMIPS())
     return false;
   return true;
 }
 
 Status NativeProcessLinux::Resume(const ResumeActionList &resume_actions) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
if (traceid == m_pt_proces_trace_id) {
     // traceid maps to the whole process so we have to erase it from the thread
     // group.
     LLDB_LOG(log, "traceid maps to process");
     m_pt_traced_thread_group.erase(thread);
   }
   m_processor_trace_monitor.erase(iter);
 
   return error;
 }

lldb/test/Shell/Subprocess/Inputs/fork.c

+#include <sys/types.h>
+#include <sys/wait.h>
+#include <assert.h>
+#include <stdio.h>
+#include <unistd.h>
+
+int g_val = 0;
+
+void parent_func() {
+  g_val = 1;
+  printf("function run in parent\n");
+}
+
+void child_func() {
+  g_val = 2;
+  printf("function run in child\n");
+}
+
+int main() {
+  pid_t pid = fork();
+  assert(pid != -1);
+
+  if (pid == 0) {
+    child_func();
+    _exit(0);
+  }
+
+  parent_func();
+  int status;
+  pid_t waited = waitpid(pid, &status, 0);
+  assert(waited == pid);
+  assert(WIFEXITED(status));
+  printf("child exited: %d\n", WEXITSTATUS(status));
+
+  return 0;
+}

lldb/test/Shell/Subprocess/Inputs/vfork.c

+#include <sys/types.h>
+#include <sys/wait.h>
+#include <assert.h>
+#include <stdio.h>
+#include <unistd.h>
+
+int g_val = 0;
+
+void parent_func() {
+  g_val = 1;
+  printf("function run in parent\n");
+}
+
+void child_func() {
+  // do something relatively safe
+  volatile int val = 0;
+  ++val;
+}
+
+int main() {
+  pid_t pid = vfork();
+  assert(pid != -1);
+
+  if (pid == 0) {
+    child_func();
+    _exit(0);
+  }
+
+  parent_func();
+  int status;
+  pid_t waited = waitpid(pid, &status, 0);
+  assert(waited == pid);
+  assert(WIFEXITED(status));
+  printf("child exited: %d\n", WEXITSTATUS(status));
+
+  return 0;
+}

lldb/test/Shell/Subprocess/fork-follow-parent-softbp.test

+# REQUIRES: native
+# RUN: %clang_host %p/Inputs/fork.c -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+b parent_func
+b child_func
+process launch
+# CHECK: function run in child
+# CHECK-NOT: function run in parent
+# CHECK: stop reason = breakpoint
+continue
+# CHECK: function run in parent
+# CHECK: child exited: 0

lldb/test/Shell/Subprocess/fork-follow-parent-wp.test

+# REQUIRES: native && (target-x86 || target-x86_64 || target-aarch64) && dbregs-set
+# RUN: %clang_host -g %p/Inputs/fork.c -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+process launch -s
+watchpoint set variable -w write g_val
+# CHECK: Watchpoint created:
+continue
+# CHECK: function run in child
+# CHECK-NOT: function run in parent
+# CHECK: stop reason = watchpoint
+continue
+# CHECK: function run in parent
+# CHECK: child exited: 0

lldb/test/Shell/Subprocess/fork-follow-parent.test

+# REQUIRES: native
+# RUN: %clang_host %p/Inputs/fork.c -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+b parent_func
+process launch
+# CHECK: function run in child
+# CHECK-NOT: function run in parent
+# CHECK: stop reason = breakpoint
+continue
+# CHECK: function run in parent
+# CHECK: child exited: 0

lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test

+# REQUIRES: native
+# RUN: %clang_host %p/Inputs/vfork.c -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+b parent_func
+b child_func
+process launch
+# CHECK-NOT: function run in parent
+# CHECK: stop reason = breakpoint
+continue
+# CHECK: function run in parent
+# CHECK: child exited: 0

lldb/test/Shell/Subprocess/vfork-follow-parent.test

+# REQUIRES: native
+# RUN: %clang_host %p/Inputs/vfork.c -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+b parent_func
+process launch
+# CHECK-NOT: function run in parent
+# CHECK: stop reason = breakpoint
+continue
+# CHECK: function run in parent
+# CHECK: child exited: 0