From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45181 invoked by alias); 27 Aug 2015 18:13:24 -0000 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 Received: (qmail 45169 invoked by uid 89); 27 Aug 2015 18:13:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: e06smtp16.uk.ibm.com Received: from e06smtp16.uk.ibm.com (HELO e06smtp16.uk.ibm.com) (195.75.94.112) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Thu, 27 Aug 2015 18:13:23 +0000 Received: from /spool/local by e06smtp16.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 27 Aug 2015 19:13:19 +0100 Received: from d06dlp03.portsmouth.uk.ibm.com (9.149.20.15) by e06smtp16.uk.ibm.com (192.168.101.146) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 27 Aug 2015 19:13:17 +0100 X-MailFrom: uweigand@de.ibm.com X-RcptTo: gdb-patches@sourceware.org Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id D6BE01B08061 for ; Thu, 27 Aug 2015 19:14:49 +0100 (BST) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t7RIDEJI18415644 for ; Thu, 27 Aug 2015 18:13:17 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t7RID4id002372 for ; Thu, 27 Aug 2015 12:13:05 -0600 Received: from oc7340732750.ibm.com (dyn-9-152-213-24.boeblingen.de.ibm.com [9.152.213.24]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id t7RID4Ca002135; Thu, 27 Aug 2015 12:13:04 -0600 Received: by oc7340732750.ibm.com (Postfix, from userid 500) id B2A0C39FA; Thu, 27 Aug 2015 20:12:55 +0200 (CEST) Subject: Re: [PATCH 1/2] Re-check fastpoint after reloading symbols. To: cole945@gmail.com (Wei-cheng Wang) Date: Thu, 27 Aug 2015 18:13:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org, cole945@gmail.com In-Reply-To: <1440091938-42453-1-git-send-email-cole945@gmail.com> from "Wei-cheng Wang" at Aug 21, 2015 01:32:17 AM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20150827181255.B2A0C39FA@oc7340732750.ibm.com> X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15082718-0025-0000-0000-000006A001D6 X-SW-Source: 2015-08/txt/msg00805.txt.bz2 Wei-cheng Wang wrote: > Check fast tracepoints after symbols have been re-loaded. > For example, a pending tracepoint just becomes available after > a new shared object being loaded. We didn't check it before, > because we have no idea where it is. > > If the target rejects the tracepoint, an error is throw in > check_fast_tracepoint_sals, and we will disable the breakpoint. > > The checking is deliberately put after the loop for adding location > to breakpoint, so users can check the address for the tracepoint > with `info trace'. Otherwise, it will simply show instead > of the address. > > (gdb) info trace > Num Type Disp Enb Address What > 1 fast tracepoint keep n 0x00003fffb7f507dc > ^^^^^^^^^^^^^^^^^^ This makes sense to me. However, we now have allowed a disabled breakpoint with an invalid address in the list. If the user simply enables the breakpoint now and continues, we will run into the same internal error as before ... So we probably ought to add the same check to enable_breakpoint. > - /* If there's no new locations, and all existing locations are > - pending, don't do anything. This optimizes the common case where > - all locations are in the same shared library, that was unloaded. > - We'd like to retain the location, so that when the library is > - loaded again, we don't loose the enabled/disabled status of the > - individual locations. */ > - if (all_locations_are_pending (existing_locations) && sals.nelts == 0) > - return; I don't understand why you're moving this piece of code down. This just seems to disable the optimization mentioned in the comment ... > + if (b->type == bp_fast_tracepoint) > + { > + TRY > + { > + check_fast_tracepoint_sals (get_current_arch(), &sals); This should use b->gdbarch instead of get_current_arch(). > + } > + CATCH (e, RETURN_MASK_ERROR) > + { > + b->enable_state = bp_disabled; > + throw_exception (e); > + } > + END_CATCH > + } > + > /* If possible, carry over 'disable' status from existing > breakpoints. */ Hmm. I think the check still needs to be moved further down. Note that if there are other breakpoints, the have been re-set by the loop above. For those other breakpoints, we still should do the actions done in the rest of this routine: - carry over disable status - notify observers - update global location list But if you run into an error during check_fast_tracepoint_sals, you throw an exception, which skips all this code. That's not a good idea ... I think we should move the check to the very end of the routine. (In fact, we then might move it up into the caller, i.e. to the end of tracepoint_re_set.) Also, some test that all this works as expected would be good. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com