From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3141 invoked by alias); 14 Mar 2011 20:22:38 -0000 Received: (qmail 3133 invoked by uid 22791); 14 Mar 2011 20:22:35 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate2.uk.ibm.com (HELO mtagate2.uk.ibm.com) (194.196.100.162) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 Mar 2011 20:22:29 +0000 Received: from d06nrmr1507.portsmouth.uk.ibm.com (d06nrmr1507.portsmouth.uk.ibm.com [9.149.38.233]) by mtagate2.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p2EKMQnE023753 for ; Mon, 14 Mar 2011 20:22:26 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1507.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2EKMjsZ1728606 for ; Mon, 14 Mar 2011 20:22:45 GMT Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2EKMQWM006936 for ; Mon, 14 Mar 2011 14:22:26 -0600 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p2EKMO0u006929; Mon, 14 Mar 2011 14:22:25 -0600 Message-Id: <201103142022.p2EKMO0u006929@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Mon, 14 Mar 2011 21:22:24 +0100 Subject: Re: [RFA] Implement support for PowerPC BookE ranged breakpoints To: bauerman@br.ibm.com (Thiago Jung Bauermann) Date: Mon, 14 Mar 2011 21:02:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (gdb-patches ml) In-Reply-To: <1299890187.9288.185.camel@hactar> from "Thiago Jung Bauermann" at Mar 11, 2011 09:36:27 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2011-03/txt/msg00746.txt.bz2 Thiago Jung Bauermann wrote: > On Mon, 2011-02-28 at 17:52 +0100, Ulrich Weigand wrote: > > Thiago Jung Bauermann wrote: > > > > This adds the SAY_WHERE parameter so that the caller will go ahead to > > > > add location information. However, for a range breakpoint, this will > > > > just be the start of the range, which may or may not be particularly > > > > useful ... Shouldn't we either: > > > > - output the full range (start and end), or > > > > - output nothing ... we'll see the actual location where we stopped anyway > > > > > > > > (Either way, the API change to add SAY_WHERE is no longer needed.) > > > > > > I think it's useful. You have a point in that it makes more sense to > > > print the end of the range as well. I'd have to extend struct breakpoint > > > to save the source file and line number of the end of the range if I > > > were to print them. I'm not sure it's worth it, so I changed > > > print_mention_ranged_breakpoint to print only the start and end address > > > of the range. And dropped the say_where argument. What do you think? > > > > Don't you have to know the end of the range anyway, in order to be > > able to re-set the breakpoint? > > I was thinking of the struct breakpoint's source_file and line_number, > which are used mostly for printing (clear_command uses it for finding > breakpoints). For print_mention_ranged_breakpoint I'd need > source_file_range_end and line_number_range_end, but decided against it. > > But now that you mention breakpoint re-setting, I just realised that I > need to add addr_string_range_end and make breakpoint_re_set_one use it. > As the patch currently stands, ranged breakpoints will be dropped when > loading new binaries... I'm currently working on this but decided to > send this patch as it is since it addresses all the other points you > raised. Well, but you do have this: static void print_recreate_ranged_breakpoint (struct breakpoint *b, struct ui_file *fp) { fprintf_unfiltered (fp, "break-range %s", b->exp_string); } So I thought b->exp_string would already include the original start and end, but simply in an un-parsed fashion. If this could maybe be formatted consistently, you might be able to use it for print_details as well, I guess. > > It seems oddly asymmetrical to use parse_breakpoint_sals above but > > decode_line_1 here. Shouldn't start and end of the range use the > > same symtab defaulting rules and other extra treatment done by > > parse_breakpoint_sals? > > No, the end location should default to the same file and line number as > the start location. This makes it possible to have ranges like: > > (gdb) break-range foo.c:27, +14 > > Had I used parse_breakpoint_sals, GDB would interpret "+14" as "14 lines > from the current file and line number", and not "14 lines from the start > location". As for the extra treatment, I had a look at them before and > didn't think they applied in the context of resolving the end location. Ah, good point, this does make sense. Maybe a brief comment in the source to that effect would be useful here? > This was pointed out to me by Jan when reviewing another patch [1]: > > > > --- a/gdb/target.c > > > +++ b/gdb/target.c > > > @@ -601,11 +601,16 @@ update_current_target (void) > > > INHERIT (to_files_info, t); > > > INHERIT (to_insert_breakpoint, t); > > > INHERIT (to_remove_breakpoint, t); > > > + INHERIT (to_can_use_special_hw_point, t); > > > > There are now two target interface styles in use. This inheriting one and the > > runtime-inheriting one (see target_pid_to_str and others). I was told the > > target_pid_to_str style is now preferred and it makes sense to me. Please > > convert the new target vector methods to the new style. > > So I just followed his advice and used the new method instead of the > deprecated one which is used by the other breakpoint-related functions. I agree in general that new target methods should use the new style. The only issue that makes me a bit nervous is that the two styles do have slightly different effects (the struct target_ops that is passed in to the callback may be a different one); so I'm wondering if it is a good idea to mix the two methods within a group of closely related target methods ... But then again, in this particular case it probably doesn't matter much, as typical implementations of your newly added callback ought to be straightforward, and probably won't care about the target_ops they get. So I guess I withdraw my objection :-) The rest of the patch looks fine to me now. Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com