Attached are 4 separate patches for each somewhat independent portion of the kernel work related to the follow-fork implementation. 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. > >> >>> 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. > >> 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. > >>> 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.
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:23 UTC