Re: [PATCH][PR breakpoints/16606] AVR8 breakpoint out of range, decrement pc after break

[Pierre Langlois <pierre dot langlois at embecosm dot com>]

Hello Joel,

Thank you for the input.

On 10/03/14 11:08, Joel Brobecker wrote:
> Hello Pierre,
>
> > For example:
> >
> > (gdb) break *0x10e
> > would result in a breakpoint at the address 0x80010e, out of range.
> > diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
> > index 6e58f04..a4a4a6d 100644
> > --- a/gdb/avr-tdep.c
> > +++ b/gdb/avr-tdep.c
> > @@ -333,7 +333,10 @@ avr_integer_to_address (struct gdbarch *gdbarch,
> >   {
> >     ULONGEST addr = unpack_long (type, buf);
> >
> > -  return avr_make_saddr (addr);
> > +  if (TYPE_CODE_SPACE (type))
> > +    return avr_make_iaddr (addr);
> > +  else
> > +    return avr_make_saddr (addr);
> >   }
> >
> >   static CORE_ADDR
> > @@ -1436,6 +1439,7 @@ avr_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> >     set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
> >
> >     set_gdbarch_breakpoint_from_pc (gdbarch, avr_breakpoint_from_pc);
> > +  set_gdbarch_decr_pc_after_break (gdbarch, 2);
> This part seems fine, but it would be good if you could submit it
> separately, with an explanation of the problem you are seeing
> (a copy of the gdb debugging session is often useful).

OK, I'll split this patch in two.

> >     frame_unwind_append_unwinder (gdbarch, &avr_frame_unwind);
> >     frame_base_set_default (gdbarch, &avr_frame_base);
> > diff --git a/gdb/linespec.c b/gdb/linespec.c
> > index 610809d..8355114 100644
> > --- a/gdb/linespec.c
> > +++ b/gdb/linespec.c
> > @@ -2588,6 +2588,7 @@ initialize_defaults (struct symtab **default_symtab, int *default_line)
> >   static CORE_ADDR
> >   linespec_expression_to_pc (const char **exp_ptr)
> >   {
> > +  struct value *val;
> >     if (current_program_space->executing_startup)
> >       /* The error message doesn't really matter, because this case
> >          should only hit during breakpoint reset.  */
> > @@ -2595,7 +2596,14 @@ linespec_expression_to_pc (const char **exp_ptr)
> >   				    "program space is in startup"));
> >
> >     (*exp_ptr)++;
> > -  return value_as_address (parse_to_comma_and_eval (exp_ptr));
> > +  val = parse_to_comma_and_eval (exp_ptr);
> > +  /* The value given by parse_to_comma_and_eval is an address but does not have
> > +     any information about the address space in which it resides.  Harvard
> > +     architectures need to know this when converting a value to an address with
> > +     gdbarch_integer_to_address. It is safe to assume linespecs give an address
> > +     in code space.  */
> > +  TYPE_INSTANCE_FLAGS (value_type (val)) |= TYPE_INSTANCE_FLAG_CODE_SPACE;
> > +  return value_as_address (val);
> I don't think it's correct to be doing that, as the type you are
> modifying is not specific to the value being returned. The change
> you are making is therefore affecting other values, and cause
> incorrect output for values of that type.
>
> The way we have been dealing with this sort of issue is basically
> to add a cast to a function pointer to the expression.  If you really
> want to support this feature, you'll probably want to store the
> information in the value rather than the type. But I am a little
> less familiar with struct value, so others might a better suggestion.

I just realized that support for multiple address spaces was added but 
never documented.
So the way to solve the issue is to add the @code qualifier as such:

(gdb) break * (@code void *) 0x10e

And it will set the TYPE_CODE_SPACE instance flag to the type when 
calling integer_to_address.

However, shouldn't @code be the default for breakpoints? I'm investigating
how to do this, any pointers?