[3/3] GDB: New target s12z

  gdb/
    * configure.tgt: Add configuration for s12z.
    * s12z-tdep.c:  New file.
    * NEWS: Mention new target.
---
 gdb/NEWS          |   4 +
 gdb/configure.tgt |   6 +
 gdb/s12z-tdep.c   | 402 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 412 insertions(+)
 create mode 100644 gdb/s12z-tdep.c
  

> From: John Darrington <john@darrington.wattle.id.au>
> Cc: John Darrington <john@darrington.wattle.id.au>
> Date: Thu, 23 Aug 2018 19:35:26 +0200
> 
> gdb/
>     * configure.tgt: Add configuration for s12z.
>     * s12z-tdep.c:  New file.
>     * NEWS: Mention new target.

OK for the NEWS part.

Thanks.
  

Hi John,

I did a first pass review and noted a few minor nits.  I didn't get too deep in the
frame_id/unwind code because I'm not too familiar with that.

You should include the new file in the ALL_TARGET_OBS makefile target, so that
it's included in a --enable-targets=all build.

On 2018-08-23 1:35 p.m., John Darrington wrote:
> gdb/
>     * configure.tgt: Add configuration for s12z.
>     * s12z-tdep.c:  New file.
>     * NEWS: Mention new target.
> ---
>  gdb/NEWS          |   4 +
>  gdb/configure.tgt |   6 +
>  gdb/s12z-tdep.c   | 402 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 412 insertions(+)
>  create mode 100644 gdb/s12z-tdep.c
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a7a3674375..c46056525a 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 8.2
>  
> +* New targets
> +
> + NXP S12Z		s12z-*-elf
> +
>  * GDB and GDBserver now support IPv6 connections.  IPv6 addresses
>    can be passed using the '[ADDRESS]:PORT' notation, or the regular
>    'ADDRESS:PORT' method.
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index 5e3bd5de71..72de1a1e4a 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -643,6 +643,12 @@ spu*-*-*)
>  	build_gdbserver=yes
>  	;;
>  
> +s12z-*-*)
> +	# Target: Freescale S12z
> +	gdb_target_obs="s12z-tdep.o"
> +	build_gdbserver=yes

I don't think you should include build_gdbserver.  It is only valid
if you have a gdbserver port, for when building natively on that
architecture.

> +	;;
> +
>  tic6x-*-*linux)
>  	# Target: GNU/Linux TI C6x
>  	gdb_target_obs="tic6x-tdep.o tic6x-linux-tdep.o solib-dsbt.o \
> diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
> new file mode 100644
> index 0000000000..5169025e6b
> --- /dev/null
> +++ b/gdb/s12z-tdep.c
> @@ -0,0 +1,402 @@
> +/* Target-dependent code for the S12Z, for the GDB.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* Much of this file is shamelessly copied from or1k-tdep.c and others */

You don't have to include this, it's perfectly encouraged to share code between ports :).

> +
> +#include "defs.h"
> +
> +#include "arch-utils.h"
> +#include "block.h"
> +#include "cli/cli-decode.h"
> +#include "common-defs.h"
> +#include "dis-asm.h"
> +#include "dwarf2-frame.h"
> +#include "elf-bfd.h"
> +#include "floatformat.h"
> +#include "frame-base.h"
> +#include "frame.h"
> +#include "frame-unwind.h"
> +#include "gdbcmd.h"
> +#include "gdbcore.h"
> +#include "gdbtypes.h"
> +#include "infcall.h"
> +#include "inferior.h"
> +#include "language.h"
> +#include "objfiles.h"
> +#include "observable.h"
> +#include "opcode/s12z.h"
> +#include "osabi.h"
> +#include "regcache.h"
> +#include "reggroups.h"
> +#include "remote.h"
> +#include "symcat.h"
> +#include "symfile.h"
> +#include "symtab.h"
> +#include "target-descriptions.h"
> +#include "target.h"
> +#include "trad-frame.h"
> +#include "user-regs.h"
> +#include "valprint.h"
> +#include "value.h"

Are all these includes necessary?  Can you try to reduce to what you actually use?

> +#include <stdio.h>

This should not be necessary.

> +
> +#define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2)
> +
> +static const int reg_perm[N_PHYSICAL_REGISTERS] =
> +  {
> +   REG_D0,
> +   REG_D1,
> +   REG_D2,
> +   REG_D3,
> +   REG_D4,
> +   REG_D5,
> +   REG_D6,
> +   REG_D7,
> +   REG_X,
> +   REG_Y,
> +   REG_S,
> +   REG_P,
> +   REG_CCW
> +  };
> +
> +static const char *
> +s12z_register_name (struct gdbarch *gdbarch, int regnum)
> +{
> +  return registers[reg_perm[regnum]].name;

Which "registers" variable does this refer to?

> +}
> +
> +static CORE_ADDR
> +s12z_skip_prologue (struct gdbarch *gdbarch,
> +		    CORE_ADDR       pc)

Remove extra spaces, and this can fit on a single line.

> +{
> +  CORE_ADDR start_pc = 0;
> +
> +  if (find_pc_partial_function (pc, NULL, &start_pc, NULL))
> +    {
> +      CORE_ADDR prologue_end = skip_prologue_using_sal (gdbarch, pc);
> +
> +      if (0 != prologue_end)
> +	{
> +	  struct symtab_and_line prologue_sal = find_pc_line (start_pc, 0);
> +	  struct compunit_symtab *compunit
> +	    = SYMTAB_COMPUNIT (prologue_sal.symtab);
> +	  const char *debug_format = COMPUNIT_DEBUGFORMAT (compunit);
> +
> +	  if ((NULL != debug_format)
> +	      && (strlen ("dwarf") <= strlen (debug_format))
> +	      && (0 == strncasecmp ("dwarf", debug_format, strlen ("dwarf"))))
> +	    return (prologue_end > pc) ? prologue_end : pc;
> +	}
> +    }
> +
> +  fprintf_unfiltered (gdb_stdlog, "%s:%d %s FAILED TO FIND END OF PROLOGUE PC = %08x\n", __FILE__, __LINE__,
> +                      __FUNCTION__, (unsigned int) pc);

Maybe use the warning() function?  Also it's probably not necessary to use all
caps.  To print a CORE_ADDR, use:

  "%s", paddress (gdbarch, pc);

Also, user-visible messages should use _() for i18n.

> +
> +  return 0;
> +}
> +
> +static CORE_ADDR
> +s12z_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  return frame_unwind_register_unsigned (next_frame, REG_P);
> +}
> +
> +/* Implement the unwind_sp gdbarch method.  */

This comment is good, can you put a similar for other gdbarch callbacks?

> +
> +static CORE_ADDR
> +s12z_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  return frame_unwind_register_unsigned (next_frame, REG_S);
> +}
> +
> +
> +

Remove the extra lines.

> +static struct type *
> +s12z_register_type (struct gdbarch *gdbarch, int reg_nr)
> +{
> +  switch (registers[reg_perm[reg_nr]].bytes)
> +    {
> +    case 1:
> +      return builtin_type (gdbarch)->builtin_uint8;
> +    case 2:
> +      return builtin_type (gdbarch)->builtin_uint16;
> +    case 3:
> +      return builtin_type (gdbarch)->builtin_uint24;
> +    case 4:
> +      return builtin_type (gdbarch)->builtin_uint32;
> +    default:
> +      return builtin_type (gdbarch)->builtin_uint32;
> +    }
> +  return builtin_type (gdbarch)->builtin_int0;
> +}
> +
> +
> +static int
> +s12z_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int num)
> +{
> +  switch (num)
> +    {
> +    case 15:      return REG_S;
> +    case 7:       return REG_X;
> +    case 8:       return REG_Y;
> +    case 42:      return REG_D0;
> +    case 43:      return REG_D1;
> +    case 44:      return REG_D2;
> +    case 45:      return REG_D3;
> +    case 46:      return REG_D4;
> +    case 47:      return REG_D5;
> +    case 48:      return REG_D6;
> +    case 49:      return REG_D7;
> +    }
> +  return -1;
> +}
> +
> +
> +/* Support functions for frame handling.  */
> +
> +/* Initialize a prologue cache */
> +
> +volatile int frame_size = 0;

Does this really need to be global?  And volatile?

> +
> +static struct trad_frame_cache *
> +s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache)
> +{
> +  struct trad_frame_cache *info;
> +
> +  CORE_ADDR this_sp;
> +  CORE_ADDR this_sp_for_id;
> +
> +  CORE_ADDR start_addr;
> +  CORE_ADDR end_addr;
> +
> +  /* Nothing to do if we already have this info.  */
> +  if (NULL != *prologue_cache)
> +    return (struct trad_frame_cache *) *prologue_cache;
> +
> +  /* Get a new prologue cache and populate it with default values.  */
> +  info = trad_frame_cache_zalloc (this_frame);
> +  *prologue_cache = info;
> +
> +  /* Find the start address of this function (which is a normal frame, even
> +     if the next frame is the sentinel frame) and the end of its prologue.  */
> +  CORE_ADDR this_pc = get_frame_pc (this_frame);
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  find_pc_partial_function (this_pc, NULL, &start_addr, NULL);

Check the return value?

> +
> +  /* Get the stack pointer if we have one (if there's no process executing
> +     yet we won't have a frame.  */
> +  this_sp = (NULL == this_frame) ? 0 :
> +    get_frame_register_unsigned (this_frame, REG_S);
> +
> +  /* Return early if GDB couldn't find the function.  */
> +  if (start_addr == 0)
> +    {
> +      fprintf_unfiltered (gdb_stdlog, "  couldn't find function\n");

Use warning (_()) ?

> +
> +      /* JPB: 28-Apr-11.  This is a temporary patch, to get round GDB
> +	 crashing right at the beginning.  Build the frame ID as best we
> +	 can.  */
> +      trad_frame_set_id (info, frame_id_build (this_sp, this_pc));
> +
> +      return info;
> +    }
> +
> +  /* The default frame base of this frame (for ID purposes only - frame
> +     base is an overloaded term) is its stack pointer.  For now we use the
> +     value of the SP register in this frame.  However if the PC is in the
> +     prologue of this frame, before the SP has been set up, then the value
> +     will actually be that of the prev frame, and we'll need to adjust it
> +     later.  */
> +  trad_frame_set_this_base (info, this_sp);
> +  this_sp_for_id = this_sp;
> +
> +  /* The default is to find the PC of the previous frame in the link
> +     register of this frame.  This may be changed if we find the link
> +     register was saved on the stack.  */
> +  //  trad_frame_set_reg_realreg (info, S12Z_NPC_REGNUM, S12Z_LR_REGNUM);

Why is this commented?  Either it should be used or it should not be there.

> +
> +  /* We should only examine code that is in the prologue.  This is all code
> +     up to (but not including) end_addr.  We should only populate the cache
> +     while the address is up to (but not including) the PC or end_addr,
> +     whichever is first.  */
> +  end_addr = s12z_skip_prologue (gdbarch, start_addr);
> +
> +  /* All the following analysis only occurs if we are in the prologue and
> +     have executed the code.  Check we have a sane prologue size, and if
> +     zero we are frameless and can give up here.  */
> +  if (end_addr < start_addr)
> +    error (_("end addr %s is less than start addr %s"),
> +	   paddress (gdbarch, end_addr), paddress (gdbarch, start_addr));
> +
> +  if (end_addr == start_addr)
> +    {
> +      frame_size = 0;
> +    }

Remove braces.

> +  else
> +    {
> +      CORE_ADDR addr = start_addr; /* Where we have got to */
> +
> +      gdb_byte byte;
> +
> +      if (0 != target_read_code (addr, &byte, 1))
> +        memory_error (TARGET_XFER_E_IO, addr);

I think if you call read_code, it throws memory_error for you on failure.

> +
> +      int simm;
> +      if (byte == 0x1a)   /* LEA S, (S, xxxx) */
> +        {
> +          if (0 != target_read_code (addr + 1, &byte, 1))
> +            memory_error (TARGET_XFER_E_IO, addr);

Same.

> +
> +          simm = (signed char) byte;
> +	  frame_size = -simm;
> +	  addr += 2;
> +
> +	  /* If the PC has not actually got to this point, then the frame
> +	     base will be wrong, and we adjust it.
> +
> +	     If we are past this point, then we need to populate the stack
> +	     accordingly.  */
> +	  if (this_pc <= addr)
> +	    {
> +	      /* Only do if executing.  */
> +	      if (0 != this_sp)
> +		{
> +		  this_sp_for_id = this_sp + frame_size;
> +		  trad_frame_set_this_base (info, this_sp_for_id);
> +		}
> +	    }
> +	  else
> +	    {
> +	      /* We are past this point, so the stack pointer of the prev
> +	         frame is frame_size greater than the stack pointer of this
> +	         frame.  */
> +	      trad_frame_set_reg_value (info, REG_S,
> +					this_sp + frame_size + 3);
> +	    }
> +        }
> +
> +      /* From now on we are only populating the cache, so we stop once we
> +	 get to either the end OR the current PC.  */
> +      //      end_addr = (this_pc < end_addr) ? this_pc : end_addr;
> +
> +
> +      trad_frame_set_reg_addr (info, REG_P, this_sp + frame_size);
> +    }
> +
> +  /* Build the frame ID */
> +  trad_frame_set_id (info, frame_id_build (this_sp_for_id, start_addr));
> +
> +  return info;
> +}
> +
> +/* Implement the this_id function for the stub unwinder.  */
> +
> +static void
> +s12z_frame_this_id (struct frame_info *this_frame,
> +		    void **prologue_cache, struct frame_id *this_id)
> +{
> +  struct trad_frame_cache *info = s12z_frame_cache (this_frame,
> +						    prologue_cache);
> +
> +  trad_frame_get_id (info, this_id);
> +}
> +
> +/* Implement the prev_register function for the stub unwinder.  */
> +
> +static struct value *
> +s12z_frame_prev_register (struct frame_info *this_frame,
> +			  void **prologue_cache, int regnum)
> +{
> +  struct trad_frame_cache *info = s12z_frame_cache (this_frame,
> +						    prologue_cache);
> +
> +  return trad_frame_get_register (info, this_frame, regnum);
> +}
> +
> +/* Data structures for the normal prologue-analysis-based unwinder.  */
> +
> +static const struct frame_unwind s12z_frame_unwind = {
> +  NORMAL_FRAME,
> +  default_frame_unwind_stop_reason,
> +  s12z_frame_this_id,
> +  s12z_frame_prev_register,
> +  NULL,
> +  default_frame_sniffer,
> +  NULL,
> +};
> +
> +
> +constexpr gdb_byte s12z_break_insn[] = {0x00};
> +
> +typedef BP_MANIPULATION (s12z_break_insn) s12z_breakpoint;
> +
> +struct gdbarch_tdep
> +{
> +};
> +
> +static struct gdbarch *
> +s12z_gdbarch_init (struct gdbarch_info info,
> +                    struct gdbarch_list *arches)

The indentation here looks wrongs (should use tabs for groups of 8 columns and
then complete with spaces, and there is one extra space).

> +{
> +  struct gdbarch_tdep *tdep = (struct gdbarch_tdep *) xmalloc (sizeof *tdep);

You could use XNEW (gdbarch_tdep), which has the advantage of causing a build failure
if it's unsafe to malloc a structure of this type.

> +  struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep);
> +
> +  /* Target data types.  */
> +  set_gdbarch_short_bit (gdbarch, 16);
> +  set_gdbarch_int_bit (gdbarch, 16);
> +  set_gdbarch_long_bit (gdbarch, 32);
> +  set_gdbarch_long_long_bit (gdbarch, 32);
> +  set_gdbarch_ptr_bit (gdbarch, 24);
> +  set_gdbarch_addr_bit (gdbarch, 24);
> +  set_gdbarch_char_signed (gdbarch, 0);
> +
> +  set_gdbarch_ps_regnum (gdbarch, REG_CCW);
> +  set_gdbarch_pc_regnum (gdbarch, REG_P);
> +  set_gdbarch_sp_regnum (gdbarch, REG_S);
> +
> +
> +  set_gdbarch_breakpoint_kind_from_pc (gdbarch,
> +				       s12z_breakpoint::kind_from_pc);
> +  set_gdbarch_sw_breakpoint_from_kind (gdbarch,
> +				       s12z_breakpoint::bp_from_kind);
> +
> +  set_gdbarch_num_regs (gdbarch, S12Z_N_REGISTERS - 2);
> +  set_gdbarch_register_name (gdbarch, s12z_register_name);
> +  set_gdbarch_skip_prologue (gdbarch, s12z_skip_prologue);
> +  set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
> +  set_gdbarch_dwarf2_reg_to_regnum (gdbarch, s12z_dwarf_reg_to_regnum);
> +
> +  set_gdbarch_register_type (gdbarch, s12z_register_type);
> +
> +  /* Functions to access frame data.  */
> +  set_gdbarch_unwind_pc (gdbarch, s12z_unwind_pc);
> +  set_gdbarch_unwind_sp (gdbarch, s12z_unwind_sp);
> +
> +  dwarf2_append_unwinders (gdbarch);
> +  frame_unwind_append_unwinder (gdbarch, &s12z_frame_unwind);
> +
> +  return gdbarch;
> +}
> +
> +void
> +_initialize_s12z_tdep (void)
> +{
> +  gdbarch_register (bfd_arch_s12z, s12z_gdbarch_init, NULL);
> +}
> 

Thanks,

Simon
  

Hi Simon,

Thanks for the review.

On Sun, Aug 26, 2018 at 01:19:29PM -0400, Simon Marchi wrote:
     Hi John,
     
     I did a first pass review and noted a few minor nits.  I didn't get too deep in the
     frame_id/unwind code because I'm not too familiar with that.

Pity.  I was hoping someone could help me there :{P

Many of your comments and questions stem from the code which I used as a
pattern, so I'm unsure of the answers.  But I will find out as best I
can and prepare a new patch within the next few days.

J'
  

On 2018-08-26 1:41 p.m., John Darrington wrote:
> Hi Simon,
> 
> Thanks for the review.
> 
> On Sun, Aug 26, 2018 at 01:19:29PM -0400, Simon Marchi wrote:
>      Hi John,
>      
>      I did a first pass review and noted a few minor nits.  I didn't get too deep in the
>      frame_id/unwind code because I'm not too familiar with that.
> 
> Pity.  I was hoping someone could help me there :{P

I just haven't written enough of those to be able to spot bugs off-hand in them.

Maybe just one insight, it seems (I'm not sure) that s12z_frame_cache uses the SP value of the
current frame (the one for which we compute the id) in the frame id.

The usual thing to do (I looked at a few other arches) is to use the same value as the
canonical frame address as defined by DWARF (Section 6.4 in DWARF5.pdf), which is the
value of the stack pointer just before that frame was created.

This is of course not mandatory, but I suppose that adhering to this de facto rule could
make things clearer.

> Many of your comments and questions stem from the code which I used as a
> pattern, so I'm unsure of the answers.  But I will find out as best I
> can and prepare a new patch within the next few days.
> 
> J'

Thanks,

Simon
  

On 2018-08-26 2:15 p.m., Simon Marchi wrote:
> On 2018-08-26 1:41 p.m., John Darrington wrote:
>> Hi Simon,
>>
>> Thanks for the review.
>>
>> On Sun, Aug 26, 2018 at 01:19:29PM -0400, Simon Marchi wrote:
>>      Hi John,
>>      
>>      I did a first pass review and noted a few minor nits.  I didn't get too deep in the
>>      frame_id/unwind code because I'm not too familiar with that.
>>
>> Pity.  I was hoping someone could help me there :{P
> 
> I just haven't written enough of those to be able to spot bugs off-hand in them.
> 
> Maybe just one insight, it seems (I'm not sure) that s12z_frame_cache uses the SP value of the
> current frame (the one for which we compute the id) in the frame id.
> 
> The usual thing to do (I looked at a few other arches) is to use the same value as the
> canonical frame address as defined by DWARF (Section 6.4 in DWARF5.pdf), which is the
> value of the stack pointer just before that frame was created.
> 
> This is of course not mandatory, but I suppose that adhering to this de facto rule could
> make things clearer.
> 
>> Many of your comments and questions stem from the code which I used as a
>> pattern, so I'm unsure of the answers.  But I will find out as best I
>> can and prepare a new patch within the next few days.
>>
>> J'
> 
> Thanks,
> 
> Simon

I also noticed that the gdb.base/all-architectures test fails when testing s12z.  Try to run

  make check TESTS="gdb.base/all-architectures-*.exp"

in the gdb build directory, see if you get a bunch of:

  FAIL: gdb.base/all-architectures-6.exp: tests: osabi=AIX: arch=s12z: endian=auto: disassemble 0x0,+4 (GDB internal error)

Simon
  

On Sun, Aug 26, 2018 at 06:55:31PM -0400, Simon Marchi wrote:
     I also noticed that the gdb.base/all-architectures test fails when testing s12z.  Try to run
     
       make check TESTS="gdb.base/all-architectures-*.exp"
     
     in the gdb build directory, see if you get a bunch of:
     
       FAIL: gdb.base/all-architectures-6.exp: tests: osabi=AIX: arch=s12z: endian=auto: disassemble 0x0,+4 (GDB internal error)
     

No.  I get:


...

                === gdb Summary ===

 # of expected passes         24


but I'll run that check before I submit my next patch.

J'
  

On 2018-08-27 02:30, John Darrington wrote:
> On Sun, Aug 26, 2018 at 06:55:31PM -0400, Simon Marchi wrote:
>      I also noticed that the gdb.base/all-architectures test fails
> when testing s12z.  Try to run
> 
>        make check TESTS="gdb.base/all-architectures-*.exp"
> 
>      in the gdb build directory, see if you get a bunch of:
> 
>        FAIL: gdb.base/all-architectures-6.exp: tests: osabi=AIX:
> arch=s12z: endian=auto: disassemble 0x0,+4 (GDB internal error)
> 
> 
> No.  I get:
> 
> 
> ...
> 
>                 === gdb Summary ===
> 
>  # of expected passes         24
> 
> 
> but I'll run that check before I submit my next patch.
> 
> J'

I fails when trying unexpected combinations of arches and osabis (like 
s12z with AIX), so you'd probably have to try on an --enable-targets=all 
build, after you've added s12z-tdep.o to ALL_TARGET_OBS.

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> The usual thing to do (I looked at a few other arches) is to use the same value as the
Simon> canonical frame address as defined by DWARF (Section 6.4 in DWARF5.pdf), which is the
Simon> value of the stack pointer just before that frame was created.

Simon> This is of course not mandatory, but I suppose that adhering to this de facto rule could
Simon> make things clearer.

ISTR there was one obscure case where it was necessary -- I think some
architectures have a special epilogue unwinder, and it was important
that this unwinder agree with the DWARF unwinder about the CFA in some
case.  This was many years ago so as you can tell I don't really recall
the details.

Anyway, I think using the current frame's SP will fail if the function
uses alloca -- the frame id will change across a step.  The CFA is
usually used in order to make the frame id invariant.  There may be a
comment in one of the frame*.h files explaining this.

Tom