Re: [RFC] Add support for the Renesas rl78 architecture

[Yao Qi <yao at codesourcery dot com>]

On 01/26/2012 07:58 AM, Kevin Buettner wrote:

Kevin,
I am not familiar with this arch, so some minor comments on code style only.

> +/* Strip bits to form an instruction address.  (When fetching a
> +   32-bit address from the stack, the high eight bits are garbage.
> +   This function strips off those unused bits.)  */
> +static CORE_ADDR
> +rl78_make_instruction_address (CORE_ADDR addr)
> +{
> +  return addr & 0xffffff;
> +}
> +
> +/* Set / clear bits necessary to make a data address.  */
> +static CORE_ADDR
> +rl78_make_data_address (CORE_ADDR addr)
> +{
> +  return (addr & 0xffff) | 0xf0000;
> +}
> +

Why can't we use macro instead of function here?

> +
> +/* Implement the "pseudo_register_write" gdbarch method.  */
> +static void
> +rl78_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
> +                            int reg, const gdb_byte *buffer)
> +{
> +  if (RL78_BANK0_RP0_REGNUM <= reg && reg <= RL78_BANK3_RP3_REGNUM)
> +    {
> +      int raw_regnum = 2 * (reg - RL78_BANK0_RP0_REGNUM) + RL78_BANK0_R0_REGNUM;
> +      regcache_raw_write (regcache, raw_regnum, buffer);
> +      regcache_raw_write (regcache, raw_regnum + 1, buffer + 1);
> +    }
> +  else if (reg == RL78_SP_REGNUM)
> +    {
> +      regcache_raw_write (regcache, RL78_SPL_REGNUM, buffer);
> +      regcache_raw_write (regcache, RL78_SPH_REGNUM, buffer + 1);
> +    }
> +  else if (RL78_X_REGNUM <= reg && reg <= RL78_H_REGNUM)
> +    {
> +      ULONGEST psw;
> +      int bank;
> +      int raw_regnum;

Any empty line is needed here.

> +      regcache_raw_read_unsigned (regcache, RL78_PSW_REGNUM, &psw);
> +      bank = ((psw >> 3) & 1) | ((psw >> 4) & 1);
> +      /* RSB0 is at bit 3; RSBS1 is at bit 5.  */
> +      raw_regnum = RL78_BANK0_R0_REGNUM + bank * RL78_REGS_PER_BANK
> +	           + (reg - RL78_X_REGNUM);
> +      regcache_raw_write (regcache, raw_regnum, buffer);
> +    }
> +  else if (RL78_AX_REGNUM <= reg && reg <= RL78_HL_REGNUM)
> +    {
> +      ULONGEST psw;
> +      int bank, raw_regnum;

Same here.

> +      regcache_raw_read_unsigned (regcache, RL78_PSW_REGNUM, &psw);
> +      bank = ((psw >> 3) & 1) | ((psw >> 4) & 1);
> +      /* RSB0 is at bit 3; RSBS1 is at bit 5.  */
> +      raw_regnum = RL78_BANK0_R0_REGNUM + bank * RL78_REGS_PER_BANK
> +		   + 2 * (reg - RL78_AX_REGNUM);
> +      regcache_raw_write (regcache, raw_regnum, buffer);
> +      regcache_raw_write (regcache, raw_regnum + 1, buffer + 1);
> +    }
> +  else
> +    gdb_assert_not_reached ("invalid pseudo register number");
> +}
> +
> +/* Implement the "breakpoint_from_pc" gdbarch method.  */
> +const gdb_byte *
> +rl78_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)
> +{
> +  static gdb_byte breakpoint[] = { 0xff };
> +
> +  /* The above are the bytes required for the BRK instruction.  However,
> +     instructions can be as short as one byte.  The simulator looks for
> +     memory writes to program areas using this pattern, however, and
> +     implements breakpoints via a different mechanism.  */

The code is clear to me, but the comment here is confusing.

> +  *lenptr = sizeof breakpoint;
> +  return breakpoint;
> +}

> +
> +/* Function for finding saved registers in a 'struct pv_area'; this
> +   function is passed to pv_area_scan.
> +
> +   If VALUE is a saved register, ADDR says it was saved at a constant
> +   offset from the frame base, and SIZE indicates that the whole
> +   register was saved, record its offset.  */
> +static void
> +check_for_saved (void *result_untyped, pv_t addr, CORE_ADDR size, pv_t value)
> +{
> +  struct rl78_prologue *result = (struct rl78_prologue *) result_untyped;
> +
> +  if (value.kind == pvk_register
> +      && value.k == 0
> +      && pv_is_register (addr, RL78_SP_REGNUM)
> +      && size == register_size (target_gdbarch, value.reg))
> +    result->reg_offset[value.reg] = addr.k;
> +}
> +
> +/* Analyze a prologue starting at START_PC, going no further than
> +   LIMIT_PC.  Fill in RESULT as appropriate.  */
> +static void
> +rl78_analyze_prologue (CORE_ADDR start_pc,
> +		       CORE_ADDR limit_pc, struct rl78_prologue *result)
> +{
> +  CORE_ADDR pc, next_pc;
> +  int rn;
> +  pv_t reg[RL78_NUM_TOTAL_REGS];
> +  struct pv_area *stack;
> +  struct cleanup *back_to;
> +  CORE_ADDR after_last_frame_setup_insn = start_pc;
> +  int bank = 0;
> +
> +  memset (result, 0, sizeof (*result));
> +
> +  for (rn = 0; rn < RL78_NUM_TOTAL_REGS; rn++)
> +    {
> +      reg[rn] = pv_register (rn, 0);
> +      result->reg_offset[rn] = 1;
> +    }
> +
> +  stack = make_pv_area (RL78_SP_REGNUM, gdbarch_addr_bit (target_gdbarch));
> +  back_to = make_cleanup_free_pv_area (stack);
> +
> +  /* The call instruction has saved the return address on the stack.  */
> +  reg[RL78_SP_REGNUM] = pv_add_constant (reg[RL78_SP_REGNUM], -4);
> +  pv_area_store (stack, reg[RL78_SP_REGNUM], 4, reg[RL78_PC_REGNUM]);
> +
> +  pc = start_pc;
> +  while (pc < limit_pc)
> +    {
> +      int bytes_read;
> +      struct rl78_get_opcode_byte_handle opcode_handle;
> +      RL78_Opcode_Decoded opc;
> +
> +      opcode_handle.pc = pc;
> +      bytes_read = rl78_decode_opcode (pc, &opc, rl78_get_opcode_byte,
> +				     &opcode_handle);
> +      next_pc = pc + bytes_read;
> +
> +      if (opc.id == RLO_sel)
> +	{
> +	  bank = opc.op[1].addend;
> +	}
> +      else if (opc.id == RLO_mov
> +               && opc.op[0].type == RL78_Operand_PreDec
> +               && opc.op[0].reg == RL78_Reg_SP
> +	       && opc.op[1].type == RL78_Operand_Register)
> +	{
> +	  int rsrc = (bank * RL78_REGS_PER_BANK) 
> +	           + 2 * (opc.op[1].reg - RL78_Reg_AX);

empty line is needed here.

> +	  reg[RL78_SP_REGNUM] = pv_add_constant (reg[RL78_SP_REGNUM], -1);
> +	  pv_area_store (stack, reg[RL78_SP_REGNUM], 1, reg[rsrc]);
> +	  reg[RL78_SP_REGNUM] = pv_add_constant (reg[RL78_SP_REGNUM], -1);
> +	  pv_area_store (stack, reg[RL78_SP_REGNUM], 1, reg[rsrc + 1]);
> +	  after_last_frame_setup_insn = next_pc;
> +	}
> +      else if (opc.id == RLO_sub
> +               && opc.op[0].type == RL78_Operand_Register
> +	       && opc.op[0].reg == RL78_Reg_SP
> +	       && opc.op[1].type == RL78_Operand_Immediate)
> +	{
> +	  int addend = opc.op[1].addend;

and here.

> +	  reg[RL78_SP_REGNUM] = pv_add_constant (reg[RL78_SP_REGNUM], -addend);
> +	  after_last_frame_setup_insn = next_pc;
> +	}
> +      else
> +	{
> +	  /* Terminate the prologue scan.  */
> +	  break;
> +	}
> +
> +      pc = next_pc;
> +    }
> +
> +  /* Is the frame size (offset, really) a known constant?  */
> +  if (pv_is_register (reg[RL78_SP_REGNUM], RL78_SP_REGNUM))
> +    result->frame_size = reg[RL78_SP_REGNUM].k;
> +
> +  /* Record where all the registers were saved.  */
> +  pv_area_scan (stack, check_for_saved, (void *) result);
> +
> +  result->prologue_end = after_last_frame_setup_insn;
> +
> +  do_cleanups (back_to);
> +}
> +

> +
> +/* Implement the "pointer_to_address" gdbarch method.  */
> +static CORE_ADDR
> +rl78_pointer_to_address (struct gdbarch *gdbarch,
> +                         struct type *type, const gdb_byte *buf)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  CORE_ADDR addr
> +    = extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);
> +
> +  /* Is it a code address?  */
> +  if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> +      || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
> +      || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type))
> +      || TYPE_LENGTH (type) == 4

If a pointer points to an integer (it is a data address, and size is
4-byte), we will get the instruction address, which is not correct, IMO.

> +      )

I think this parenthesis should be put in previous line, IMO.

> +    return rl78_make_instruction_address (addr);
> +  else
> +    return rl78_make_data_address (addr);
> +}
> +

> +
> +/* Implement the "unwind_pc" gdbarch method.  */
> +static CORE_ADDR
> +rl78_unwind_pc (struct gdbarch *arch, struct frame_info *next_frame)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (arch);

`tdep' is not used.

> +  return rl78_addr_bits_remove
> +           (arch, frame_unwind_register_unsigned (next_frame, RL78_PC_REGNUM));
> +}
> +
> +/* Implement the "unwind_sp" gdbarch method.  */
> +static CORE_ADDR
> +rl78_unwind_sp (struct gdbarch *arch, struct frame_info *next_frame)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (arch);

Likewise.

-- 
Yao (éå)