From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14713 invoked by alias); 7 Mar 2013 14:45:05 -0000 Received: (qmail 14548 invoked by uid 22791); 7 Mar 2013 14:45:00 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Mar 2013 14:44:48 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r27EijM7031812 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 7 Mar 2013 09:44:45 -0500 Received: from host2.jankratochvil.net (ovpn-116-50.ams2.redhat.com [10.36.116.50]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r27EieOC028960 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 7 Mar 2013 09:44:43 -0500 Date: Thu, 07 Mar 2013 14:45:00 -0000 From: Jan Kratochvil To: "Metzger, Markus T" Cc: "gdb-patches@sourceware.org" , "markus.t.metzger@gmail.com" , "Himpel, Christian" , Pedro Alves Subject: Re: Crash of GDB with gdbserver btrace enabled [Re: [patch v9 00/23] branch tracing support for Atom] Message-ID: <20130307144440.GA477@host2.jankratochvil.net> References: <20130306170622.GA25771@host2.jankratochvil.net> <20130307090632.GA11095@host2.jankratochvil.net> <20130307101350.GA14969@host2.jankratochvil.net> <20130307120644.GA21253@host2.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-03/txt/msg00288.txt.bz2 On Thu, 07 Mar 2013 13:32:16 +0100, Metzger, Markus T wrote: > > > > OK, I agree target_close seems excessive here, it also does not correspond to > > > > the current description of target_close: > > > > This routine is automatically always called after popping the target > > > > off the target stack > > > > > > > > While it is nice cleanup I find it a separate patch, not required for btrace. > > > > > > With this patch, the crash is gone with only minimal changes to btrace. > > > > It is only a coincidence and workaround of it. > > Hmmm, if we must no longer call methods in a certain target, why is > removing that target a workaround? OK, wrong term, it is snot a "workaround" but it hides the incorrect implementation approach. > Pedro already moved the target_close call after removing the target in > http://sourceware.org/ml/gdb-patches/2012-01/msg00701.html. Yes, such fix of pop_target seems to be OK. The problem is that it may IMO have unexpected implications not good to check-in right before a release. And I do not see a real reason to push that fix for gdb-7.6 right now. > If we removed disabling btrace from to_close, we would need to add a > separate path for "record stop". Yes. > It would further break the symmetry with to_open. Tracing would be > enabled in to_open but it would not be disabled in to_close, any more. It is more questionable whether tracing of inferiors should really be done at to_open time. If you want symmetry then record_btrace_open could be split and btrace_enable calls could be done from a new method (after to_open finishes). But GDB already commonly does 'start' of the target from its to_open, this is also why to_open has args. This brings in the asymmetry. It can be safely done because to_open is called only under controlled conditions. Contrary to it to_close can be called in arbitrary crash/teardown state so it should not rely on much. > We also must not clear btrace in to_close since this would prevent btrace > from actually being disabled when the threads are discarded sometime > after the record target has been unpushed. We would thus leave threads > traced after the record target is gone and rely on thread cleanup to do > the actual disabling. This does not feel right. to_close can be called in controlled (such as "record stop") or uncontrolled (inferior dies / gets killed). In the first case "record stop" should explicitly disable the tracing before calling to_close. In the second case it does not matter when the tracing disable happens, it just needs to happen. For the second case there must be always someone who does the munmap + close. In the linux-nat case it can be easily done from btrace's to_close. In the gdbserver case it is gdbserver's responsibility (where it actually already works). By the plan above one avoids the situation of half-closed target, this is still (AFAIK) the Pedro's design from the original mail. pop_target fix is nice, thanks for catching it, but it is not a prerequisite. Thanks, Jan