From owner-freebsd-current@FreeBSD.ORG Thu Jan 26 20:43:30 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7747B1065674 for ; Thu, 26 Jan 2012 20:43:30 +0000 (UTC) (envelope-from dmitrym@juniper.net) Received: from exprod7og102.obsmtp.com (exprod7og102.obsmtp.com [64.18.2.157]) by mx1.freebsd.org (Postfix) with ESMTP id F30E78FC0C for ; Thu, 26 Jan 2012 20:43:29 +0000 (UTC) Received: from P-EMHUB01-HQ.jnpr.net ([66.129.224.36]) (using TLSv1) by exprod7ob102.postini.com ([64.18.6.12]) with SMTP ID DSNKTyG68IwdjtfQsK+cgUoBGogEjxH8hodF@postini.com; Thu, 26 Jan 2012 12:43:30 PST Received: from magenta.juniper.net (172.17.27.123) by P-EMHUB01-HQ.jnpr.net (172.24.192.33) with Microsoft SMTP Server (TLS) id 8.3.213.0; Thu, 26 Jan 2012 12:41:25 -0800 Received: from [172.24.26.191] (dmitrym-lnx.jnpr.net [172.24.26.191]) by magenta.juniper.net (8.11.3/8.11.3) with ESMTP id q0QKfO177551; Thu, 26 Jan 2012 12:41:25 -0800 (PST) (envelope-from dmitrym@juniper.net) Message-ID: <4F21BA6F.9020401@juniper.net> Date: Thu, 26 Jan 2012 12:41:19 -0800 From: Dmitry Mikulin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20111124 Thunderbird/8.0 MIME-Version: 1.0 To: Kostik Belousov References: <749E238A-A85F-4264-9DEB-BCE1BBD21C9D@juniper.net> <20120125074824.GD2726@deviant.kiev.zoral.com.ua> <4F2094B4.70707@juniper.net> <20120126122326.GT2726@deviant.kiev.zoral.com.ua> In-Reply-To: <20120126122326.GT2726@deviant.kiev.zoral.com.ua> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Mailman-Approved-At: Thu, 26 Jan 2012 20:57:24 +0000 Cc: freebsd-current Current , Marcel Moolenaar Subject: Re: [ptrace] please review follow fork/exec changes X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Jan 2012 20:43:30 -0000 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.