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

[Pierre Langlois <pierre.langlois@embecosm.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?