Re: [PATCH] Add support for Analog Devices Blackfin processor (part 1/6: gdb)

[Jim Blandy <jimb@red-bean.com>]

On 1/5/06, Jie Zhang <jzhang918@gmail.com> wrote:
> Here the a new patch with the following changes according to your
> suggestions.

Wonderful!

> > Am I reading bfin_frame_prev_register correctly when I conclude that
> > the only saved registers it can find are the PC and the FP?  How are
>
> It can also find other registers besides the PC and the FP. It looks like:
>
> if(regnum == BFIN_PC_REGNUM)
>    {
>      ...
>    }
> else if (regnum == BFIN_FP_REGNUM)
>    {
>      ...
>    }
> else
>    read_memory (...);

Right, but that's all under the control of:

  if (regnum < BFIN_NUM_REGS && cache->saved_regs[regnum] != -1)

and if I'm reading the rest of the code right, cache->saved_regs[R]
can only be != -1 when R is BFIN_PC_REGNUM or BFIN_FP_REGNUM.  But
those test suite results look pretty good.  Does the Blackfin GCC port
produce Dwarf CFI?  If so, then you're probably not exercising the
tdep unwinding code much.

Either way, this is a quality of implementation issue, not a coding
convention or interface usage issue, so if it's not a problem for you
(or someone willing to pay you), I don't think it needs to be
addressed for the port to go in.

So, now we can get to picky stuff:

/* ???  */
#define UPPER_LIMIT			(40)
#define RETS_OFFSET			4

That comment should probably be filled in.

  /* This would come after the LINK instruction in the ret_from_signal ()
     function, hence the frame id would be SP + 8.  */
  this_id = frame_id_build (sp + 8, frame_pc_unwind(next_frame));

GNU coding standards call for a space before the opening parenthesis
of a function call argument list.

	  /* Read the value in from memory.  */

	  if(regnum == BFIN_PC_REGNUM)

Similarly for 'if', 'while', and 'for' conditions.  I just searched in
Emacs for the regexp '[a-z]('.

static int
bfin_sim_regno (int regno)
{
  switch (regno)
    {
      case SIM_BFIN_ASTAT_REGNUM :
      case SIM_BFIN_CYCLES_REGNUM :
      case SIM_BFIN_CYCLES2_REGNUM :
      case SIM_BFIN_USP_REGNUM :

The GNU coding standards have 'case' labels un-indented two spaces
relative to the code they label, like this.

static int
bfin_sim_regno (int regno)
{
  switch (regno)
    {
    case SIM_BFIN_ASTAT_REGNUM :
    case SIM_BFIN_CYCLES_REGNUM :
    case SIM_BFIN_CYCLES2_REGNUM :
    case SIM_BFIN_USP_REGNUM :
    case SIM_BFIN_SEQSTAT_REGNUM :

Looking at:

static void
bfin_linux_sigtramp_frame_prev_register (struct frame_info *next_frame,
					 void **this_cache,
					 int regnum, int *optimizedp,
					 enum lval_type *lvalp,
					 CORE_ADDR *addrp,
					 int *realnump, gdb_byte *valuep)

When we have a long argument list like this, I think it's more legible
to have each argument on its own line, or at least have line groupings
reflect some sort of semantic grouping (an array and its length, for
example).  The bfin_frame_prev_register case is similar.  This is up
to you, though.

  /* Integral values greater than one word are stored in consecutive
     registers starting with R0.  This will always be a multiple of
     the regiser size.  */

Typo.

Overall, though, it looks pretty clean to me.