Re: [RFA] Implement support for PowerPC BookE ranged breakpoints

["Ulrich Weigand" <uweigand@de.ibm.com>]

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