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

[Cristiano <cristiano_dealti at hotmail dot com>]

Thanks for reviewing this. Please see below.

On 03/18/2016 02:13 PM, Luis Machado wrote:
> 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?
Correct. You can compare the contents of prologue and img if the size of 
img is <= (len - vpc).

Actually, I've tested this only for AVR_PROLOGUE_NORMAL and corrected 
the comparison for the other prologue types.

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

Me neither. I don't have such long prologues.

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

No variation. AFAIK it's the usual prologue. Note that the original code 
worked most of the times. I've clearly indicated in the commit one case 
where it failed (stepi over sbiw r28, 0x0f instruction).

> >         && 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?

Updating info->size only when img has been matched prevents the above 
frame corruption.

I have no evidence that info->size needs to be set under other 
conditions. The original code always incrementing info->size is wrong 
however.

Again, only tested for AVR_PROLOGUE_NORMAL and assumed it works the same 
for the other prologue types.

>