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

Yao Qi
2012-01-29T09:48:47Z

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 (é½å°§)