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

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

[Pierre Langlois]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

Firstly, this patch fixes issuing breakpoints using an address 
expression on AVR.

For example:

(gdb) break *0x10e
would result in a breakpoint at the address 0x80010e, out of range.

AVR is an harvard architecture and we use the top bits of the internal 
addresses to determine whether this is a code address or a data address. 
In this case, 0x800000 was applied to this address because it was 
considered to be a data address. A more detailed explanation of this 
behaviour can be found on bugzilla: 
https://sourceware.org/bugzilla/show_bug.cgi?id=16606#c1

When returning a struct value from the evaluation of *0x10e, nothing in 
this value indicates that it resides in code space. In this case the 
expression is a linespec, referring to source code, so we can safely 
assume the address is in code space. We can set the TYPE_CODE_SPACE 
instance flag on the type of the value. When the value is converted to 
an address, gdbarch_integer_to_address can apply the correct mask 
depending on TYPE_CODE_SPACE.

This fix unveiled another issue, the program counter was not decremented 
after hitting the breakpoint instruction.
This patch fixes this by adding gdbarch_decr_pc_after_break to AVR's 
gdbarch.

Best,

Pierre


[-- Attachment #2: pr-breakpoint-16606.patch --]
[-- Type: text/x-patch, Size: 1967 bytes --]

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);
 
   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);
 }
 
 \f

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

[Pierre Langlois]

Hello,

I'm terribly sorry, I didn't include a Changelog entry to my previous email.

Here it is:

2014-03-10  Pierre Langlois  <pierre.langlois@embecosm.com>

         PR breakpoints/16606:
         * linespec.c (linespec_expression_to_pc): Set
         TYPE_INSTANCE_FLAG_CODE_SPACE to address given by the expression
         parser.
         * avr-tdep.c (avr_integer_to_address): Add TYPE_CODE_SPACE.

Best,

Pierre

On 07/03/14 19:50, Pierre Langlois wrote:
> Firstly, this patch fixes issuing breakpoints using an address 
> expression on AVR.
>
> For example:
>
> (gdb) break *0x10e
> would result in a breakpoint at the address 0x80010e, out of range.
>
> AVR is an harvard architecture and we use the top bits of the internal 
> addresses to determine whether this is a code address or a data 
> address. In this case, 0x800000 was applied to this address because it 
> was considered to be a data address. A more detailed explanation of 
> this behaviour can be found on bugzilla: 
> https://sourceware.org/bugzilla/show_bug.cgi?id=16606#c1
>
> When returning a struct value from the evaluation of *0x10e, nothing 
> in this value indicates that it resides in code space. In this case 
> the expression is a linespec, referring to source code, so we can 
> safely assume the address is in code space. We can set the 
> TYPE_CODE_SPACE instance flag on the type of the value. When the value 
> is converted to an address, gdbarch_integer_to_address can apply the 
> correct mask depending on TYPE_CODE_SPACE.
>
> This fix unveiled another issue, the program counter was not 
> decremented after hitting the breakpoint instruction.
> This patch fixes this by adding gdbarch_decr_pc_after_break to AVR's 
> gdbarch.
>
> Best,
>
> Pierre
>

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

[Joel Brobecker]

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

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

[Pedro Alves]

On 03/10/2014 11:08 AM, Joel Brobecker wrote:

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

Yes please.  I'm quite mystified that only 'b *0xaddr' exposed the need
for this.  If PC adjustment is really necessary on this architecture,
then I can't imagine how the port managed to be functional thus
far without this.

-- 
Pedro Alves

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

[Pierre Langlois]

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?

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

[Joel Brobecker]

> 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 am not really sure about that. I know my example is not going to be
the most frequent situation ever, but what if the code is data memory?
I admit I didn't know about the @code, is there an equivalent for
data pointers as well? If that were a yes, I think we could argue
that indeed, @code would be a better default, and look into what
it would take to make that happen.

-- 
Joel

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

[Pierre Langlois]

>> 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 am not really sure about that. I know my example is not going to be
> the most frequent situation ever, but what if the code is data memory?
> I admit I didn't know about the @code, is there an equivalent for
> data pointers as well? If that were a yes, I think we could argue
> that indeed, @code would be a better default, and look into what
> it would take to make that happen.
>
I only just noticed the existence of the space qualifiers. So yes, there are both
@code and @data qualifiers for all targets. We could potentially have @code being
the default and then issuing break *(@data void*) 0xaddr if the architecture
supports executing data memory.

I realized none of these flags are set by default actually, the fact that break defaults
to a data address on AVR is because when evaluating "*0x10e", the resulting value is
given a type with a code TYPE_CODE_INT. The AVR target code interprets this as a data address.

I'm investigating where setting a default address spaces could be implemented.

Thank you for the comments