Re: [PATCH] Minor fix in AVR prologue scan.

[Luis Machado <lgustavo at codesourcery dot com>]

On 03/17/2016 07:41 PM, Cristiano De Alti wrote:
> I use GDB from Eclipse with Avarice to debug AVR microcontrollers.
> While single stepping (stepi) through the instructions of a normal
> function prologue, stepping over the sbiw r28, 0x0f instruction, which
> is near the end of the prologue, causes the following change in the
> backtrace output:
>
> Before stepi:
>   #0 0x000042b0 in HID_Device_USBTask (HIDInterfaceInfo=0x80b8b8) at
>   ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157
>   #1 0x000002e0 in main () at HIDRadio.c:83
>
> After stepi:
>   #0 0x000042b2 in HID_Device_USBTask (HIDInterfaceInfo=0x8000ff) at
>   ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157
>   #1 0x00017170 in ?? ()
>
> Note that the frame #1 is inconsistent.
>
> If you now return from frame #0 with the finish command, the target does
> not stop at the return address at main. Instead, the target starts
> running and it's necessary to manually stop it.
>
> This is not a big issue and I'm not an expert of the gdb internals but
> the fix seems easy enough.
>
> gdb/ChangeLog:
> 2016-03-17  Cristiano De Alti  <cristiano_dealti@hotmail.com>
> 	* avr-tdep.c (avr_scan_prologue): fix comparison to detect
> 	the prologue limits.
> ---
>   gdb/avr-tdep.c |   11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
> index 993e92b..a8080db 100644
> --- a/gdb/avr-tdep.c
> +++ b/gdb/avr-tdep.c
> @@ -833,25 +833,26 @@ avr_scan_prologue (struct gdbarch *gdbarch, CORE_ADDR pc_beg, CORE_ADDR pc_end,
>            or signal handler functions, which is why we set the prologue type
>            when we saw the beginning of the prologue previously.  */
>
> -      if (vpc + sizeof (img_sig) < len
> +      if (vpc + sizeof (img_sig) <= len

Does changing < to <= mean the prologue size is being calculated 
incorrectly, off-by-one?

If you end up with a case of having to scan AVR_MAX_PROLOGUE_SIZE, you 
will eventually check 17 4-byte instructions rather than 16 4-byte ones. 
Not sure if that is correct by looking at the code.

Usually backtrace hiccups like these tend to come from small variations 
of prologues generated by the compiler, and GDB needs to be told what to 
expect from such a prologue.

If this is a new variation of prologue, adding handling for a new 
pattern would be the appropriate solution.

>   	  && memcmp (prologue + vpc, img_sig, sizeof (img_sig)) == 0)
>           {
> +          info->size += locals_size;
>             vpc += sizeof (img_sig);
>           }
> -      else if (vpc + sizeof (img_int) < len
> +      else if (vpc + sizeof (img_int) <= len
>   	       && memcmp (prologue + vpc, img_int, sizeof (img_int)) == 0)
>           {
> +          info->size += locals_size;
>             vpc += sizeof (img_int);
>           }
> -      if (vpc + sizeof (img) < len
> +      if (vpc + sizeof (img) <= len
>   	  && memcmp (prologue + vpc, img, sizeof (img)) == 0)
>           {
> +          info->size += locals_size;
>             info->prologue_type = AVR_PROLOGUE_NORMAL;
>             vpc += sizeof (img);
>           }
>
> -      info->size += locals_size;
> -

What is the reason for moving the above and replicating it across 
multiple if blocks? It may cause GDB to not set info->size under certain 
conditions. Is this what is expected?