Re: [ptrace] please review follow fork/exec changes

Thanks for taking a look at it. I'll try to explain the changes the best I can: the work was done nearly 6 months ago...

> I would certainly appreciate some more words describing the changes.
>
> What is the goal of introducing the PT_FOLLOW_EXEC ? To not force
> the debugger to filter all syscall exits to see exec events ?

PT_FOLLOW_EXEC was added to trace the exec() family of calls. When it's enabled, the kernel will generate a trap to give debugger a chance to clean up old process info and initialize its state with the new binary.



>
> Why did you moved the stopevent/ptracestop for exec events from
> syscallret() to kern_execve() ? If moving, why the handling of TDB_EXEC
> is not removed from syscallret() ? I do not think that TDB_EXEC can be
> seen there after the patch. The same question about TDB_FORK.

The reason I moved the event notifications from syscallret() is because the debugger is interested successful completion of either fork or exec, not just the fact that they have been called. If, say, a call to fork() failed, as far as debugger is concerned, fork() might as well had never been called. Having a ptracestop in syscallret triggered a trap on every return from fork without telling the debugger whether a new process had been created or not. Same goes for exec().

Looking at the code now I think I should have removed TDB_EXEC/TDB_FORK processing from syscallret(). I'll do that and verify that it works as expected.

>
> If possible, I would greatly prefer to have fork changes separated.

You mean separate fork changes into a separate patch from exec changes?

>
> I doubt that disallowing RFMEM while tracing is the right change. It may
> be to fix some (undescribed) bug, but RFMEM is documented behaviour used
> not only for vfork(2), and the change just breaks rfork(2) for debugged
> processes.
>
> Even vfork() should not be broken, since I believe there are programs
> that rely on the vfork() model, in particular, C shell. It will be
> broken if vfork() is substituted by fork(). The fact that it breaks only
> under debugger will make it esp. confusing.

I need to dig up the details since I can't recall the exact reason for forcing fork() in cases of user calls to vfork() under gdb. I believe it had to do with when child process memory was available for writing in case of vfork() and it wasn't early enough to complete the switch over from parent to child in gdb. After consulting with our internal kernel experts we decided that doing fork() instead of vfork() under gdb is a low impact change.

>
> Why do we need to have TDB_FORK set for td2 ?

The debugger needs to intercept fork() in both parent and child so it can detach from the old process and attach to the new one. Maybe it'll make more sense in the context of gdb changes. Should I send them too? Don't think Marcel included that patch...

>
> Does the orphan list change intended to not lost the child after fork ?
> But the child shall be traced, so debugger would get the SIGTRAP on
> the attach on fork returning to usermode. I remember that I explicitely
> tested this when adding followfork changes.

Yes, the debugger gets SIGTRAPs. The problem arises when the real parent of the forked process has the code to collect termination status. Since attaching to a process changes the parent/child relationships, we need to keep track of the children lost due to re-parenting so we can properly attribute their exit status to the "real" parent.