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

On 01/26/2012 04:23 AM, Kostik Belousov wrote:
> On Wed, Jan 25, 2012 at 03:48:04PM -0800, Dmitry Mikulin wrote:
>> 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.
>>
> It was more a question, why PT_FLAG_EXEC is not enough.

I don't see it in the code (sp?)  If you mean PL_FLAG_FORKED, than it's used to return lwpinfo, and PT_FOLLOW_EXEC is a ptrace request.

>
>>
>>> 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().
> PT_FLAG_EXEC is only set if an exec-kind of syscall succeeded. The same
> is true for PT_FLAG_FORKED, the flag is set only if a new child was
> successfully created.

I found the test cases you sent me for your fork/exec tracing changes and now I see why I needed to make some of the changes. Before my patches tracing of fork/exec is done via PT_TO_SCE/PT_TO_SCX ptrace calls. Their semantics did not fit well into what gdb needs to do. They operate as a variant of PT_CONTINUE. What gdb needs is a process-wide setting that instructs the kernel to generate SIGTRAPs in fork/exec when the process is being controlled via the regular ptrace(PT_CONTINUE)/wait() sequence.

>
>> 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?
> Yes. Even more, it seems that fork changes should be separated into
> bug fixes and new functionality.

OK, that's doable.

>
>>> 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...
> No, I think the kernel changes are enough for now.
>
>>> 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.
>>
> I do not quite understand it. From the description it sounds as the problem
> that is not related to follow fork changes at all, and shall be presented
> regardless of the follow fork. If yes, I think this shall be separated
> into a standalone patch.

You're right, it's not related. It just happened to prevent me from finishing the work.
I'll break up the changes into multiple patches and re-send.