a review and questions on avr_scan_prologue()

a review and questions on avr_scan_prologue()

[Petr Hluzín]

Hello

I took a look at avr-tdep.c [1] and I found some places which are
either bug or are not clear to me. Here it goes:


      else if (len >= sizeof (img) - 2
	       && memcmp (img + 2, prologue, sizeof (img) - 2) == 0)
	{
          info->prologue_type = AVR_PROLOGUE_SIG;
          vpc += sizeof (img) - 2;
          info->saved_regs[AVR_SREG_REGNUM].addr = 3;
          info->saved_regs[0].addr = 2;
          info->saved_regs[1].addr = 1;
-          info->size += 3;
+          info->size += 2;
	}

Since the "img + 2" skips "push r1" I believe the scan should record
smaller size.

  if (vpc >= AVR_MAX_PROLOGUE_SIZE)
     fprintf_unfiltered (gdb_stderr,
                         _("Hit end of prologue while scanning pushes\n"));

This condition is never true due to a way `len' is calculated and
`vpc' always being less than `len'. (This is not a bug but per se but
the author might expected something what is not true.)

      else if (insn == 0x920f)  /* push r0 */
        {
          info->size += 1;
          vpc += 2;
        }

The condition is never true because of the preceding "Scan pushes
(saved registers)" loop's exit condition.

Also:
The avr_scan_prologue()'s recognizes several well-known prologues. Is
there a reason why it does not use the general prologue analysis
algorithm as described in the documentation [2]?

I think universal prologue analysis is quite easy with AVR arch. The
code might be shorter (though less clear).
I might try to write the code if you are interested.
(The current prologue scan code chokes on hand-crafted assembly.)

[1] http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/avr-tdep.c?cvsroot=src
[2] http://sources.redhat.com/gdb/current/onlinedocs/gdbint/Algorithms.html

-- 
Petr Hluzin

RE: a review and questions on avr_scan_prologue()

[Weddington, Eric]

 

> -----Original Message-----
> From: Petr Hluzín [mailto:petr.hluzin@gmail.com] 
> Sent: Sunday, February 14, 2010 5:27 AM
> To: gdb@sourceware.org
> Cc: Tristan Gingold
> Subject: a review and questions on avr_scan_prologue()
> 
> Hello
> 
> I took a look at avr-tdep.c [1] and I found some places which are
> either bug or are not clear to me.

Could you please at least fill out bug reports for these 2 issues? Please put me on the CC list. That way they're recorded.

> Also:
> The avr_scan_prologue()'s recognizes several well-known prologues. Is
> there a reason why it does not use the general prologue analysis
> algorithm as described in the documentation [2]?

IIRC, the AVR GCC prologue and epilogue used to be some fixed code. Recently the AVR GCC port generates RTL-based prologues and epiloges. I'm sure AVR GDB has not kept up.

 
> I think universal prologue analysis is quite easy with AVR arch. The
> code might be shorter (though less clear).
> I might try to write the code if you are interested.
> (The current prologue scan code chokes on hand-crafted assembly.)

Yes, we would be very interested.

Eric Weddington

Re: a review and questions on avr_scan_prologue()

[Tristan Gingold]

On Feb 14, 2010, at 12:56 AM, Petr Hluzín wrote:

> Hello

Hi,

> I took a look at avr-tdep.c [1] and I found some places which are
> either bug or are not clear to me. Here it goes:

Thanks for doing reviews!

>      else if (len >= sizeof (img) - 2
> 	       && memcmp (img + 2, prologue, sizeof (img) - 2) == 0)
> 	{
>          info->prologue_type = AVR_PROLOGUE_SIG;
>          vpc += sizeof (img) - 2;
>          info->saved_regs[AVR_SREG_REGNUM].addr = 3;
>          info->saved_regs[0].addr = 2;
>          info->saved_regs[1].addr = 1;
> -          info->size += 3;
> +          info->size += 2;
> 	}
> 
> Since the "img + 2" skips "push r1" I believe the scan should record
> smaller size.

Yes, you're right.  I will fix that.

>  if (vpc >= AVR_MAX_PROLOGUE_SIZE)
>     fprintf_unfiltered (gdb_stderr,
>                         _("Hit end of prologue while scanning pushes\n"));
> 
> This condition is never true due to a way `len' is calculated and
> `vpc' always being less than `len'. (This is not a bug but per se but
> the author might expected something what is not true.)

I will change that to an assert.

>      else if (insn == 0x920f)  /* push r0 */
>        {
>          info->size += 1;
>          vpc += 2;
>        }
> 
> The condition is never true because of the preceding "Scan pushes
> (saved registers)" loop's exit condition.

I don't think so.  You can have:
  rcall .+0
  push r0


> Also:
> The avr_scan_prologue()'s recognizes several well-known prologues. Is
> there a reason why it does not use the general prologue analysis
> algorithm as described in the documentation [2]?

Historical reason: it was written before the general prologue analysis.
Then, when AdaCore did its AVR port, I fixed all issues I found, but didn't rewrite it from scratch.

> I think universal prologue analysis is quite easy with AVR arch. The
> code might be shorter (though less clear).
> I might try to write the code if you are interested.
> (The current prologue scan code chokes on hand-crafted assembly.)

Feel free to work on that.  Improvements are always welcome!

Tristan.

Re: a review and questions on avr_scan_prologue()

[Petr Hluzín]

Hi

On 17 February 2010 10:02, Tristan Gingold <gingold@adacore.com> wrote:
>>      else if (insn == 0x920f)  /* push r0 */
>>        {
>>          info->size += 1;
>>          vpc += 2;
>>        }
>>
>> The condition is never true because of the preceding "Scan pushes
>> (saved registers)" loop's exit condition.
>
> I don't think so.  You can have:
>  rcall .+0
>  push r0

You are right. (I have not realized that such case would be useful for
something.)

(Since you already committed the changes I am not filling the bug reports
Eric Weddington requested.)

-- 
Petr Hluzin