From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16731 invoked by alias); 7 Mar 2013 15:12:31 -0000 Received: (qmail 16612 invoked by uid 22791); 7 Mar 2013 15:12:28 -0000 X-SWARE-Spam-Status: No, hits=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,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 15:12:17 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r27FCFfO012935 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 7 Mar 2013 10:12:15 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r27FCCR5022910; Thu, 7 Mar 2013 10:12:13 -0500 Message-ID: <5138AE4C.3060801@redhat.com> Date: Thu, 07 Mar 2013 15:12:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130219 Thunderbird/17.0.3 MIME-Version: 1.0 To: "Metzger, Markus T" CC: Jan Kratochvil , "gdb-patches@sourceware.org" , "markus.t.metzger@gmail.com" , "Himpel, Christian" Subject: Re: Crash of GDB with gdbserver btrace enabled [Re: [patch v9 00/23] branch tracing support for Atom] References: <20130306124334.GA29994@host2.jankratochvil.net> <20130306170622.GA25771@host2.jankratochvil.net> <20130307090632.GA11095@host2.jankratochvil.net> <20130307101350.GA14969@host2.jankratochvil.net> <20130307120644.GA21253@host2.jankratochvil.net> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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/msg00293.txt.bz2 On 03/07/2013 12:32 PM, Metzger, Markus T wrote: >> -----Original Message----- >> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com] >> Sent: Thursday, March 07, 2013 1:07 PM > > >>>> 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? > > Pedro already moved the target_close call after removing the target in > http://sourceware.org/ml/gdb-patches/2012-01/msg00701.html. > > He just did not consider the extra target_close call in pop_target, which > is not only closing the target twice but also breaking his attempt to > reorder target removing and closing. Indeed, that seems so. The close-twice isn't an issue for current targets, as their close methods are idempotent: static void remote_close (int quitting) { if (remote_desc == NULL) return; /* already closed */ But the reorder issue here does look like should be fixed. I think the double closes are actually a workaround for the "quitting" argument not being propagated to unpush_target (or maybe unpush_target didn't close the target itself at some point): void pop_all_targets_above (enum strata above_stratum, int quitting) { while ((int) (current_target.to_stratum) > (int) above_stratum) { target_close (target_stack, quitting); if (!unpush_target (target_stack)) ... void pop_all_targets (int quitting) { pop_all_targets_above (dummy_stratum, quitting); } ... /* Called by the event loop to process a SIGHUP. */ static void async_disconnect (gdb_client_data arg) { ... TRY_CATCH (exception, RETURN_MASK_ALL) { pop_all_targets (1); } ... } QUITTING is documented here: /* Does whatever cleanup is required for a target that we are no longer going to be calling. QUITTING indicates that GDB is exiting and should not get hung on an error (otherwise it is important to perform clean termination, even if it takes a while). This routine is automatically always called after popping the target off the target stack - the target's own methods are no longer available through the target vector. Closing file descriptors and freeing all memory allocated memory are typical things it should do. */ void target_close (struct target_ops *targ, int quitting); I'm not sure, but ISTR that no target nowadays actually does anything with the 'quitting' flag. -- Pedro Alves