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

[Joel Brobecker <brobecker@adacore.com>]

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).

>    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.


-- 
Joel