Re: [PATCH 1/2] RISC-V GDB Port

[Yao Qi <qiyaoltc@gmail.com>]

On Mon, Nov 14, 2016 at 10:39 PM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> /* Implement the return_value gdbarch method.  */
>
> I'm not sure what you mean by this.
>

I suggested that such comment is needed to any gdbarch methods implemented
for riscv.  In this case, it is riscv_return_value.

>>> +
>>> +static enum return_value_convention
>>> +riscv_return_value (struct gdbarch  *gdbarch,
>>> +                struct value *function,
>>> +                struct type     *type,
>>> +                struct regcache *regcache,
>>> +                gdb_byte        *readbuf,
>>> +                const gdb_byte  *writebuf)
>>

>> /* Implement the XXX gdbarch method.  */
>
> I'm afraid I'm not sure what you're trying to say here, sorry!
>

Likewise.  Such comments are needed.

>>> +{
>>> +  return regcache_raw_read (regcache, regnum, buf);
>>> +}
>>> +
>>
>>> +

>>> +static void
>>> +riscv_read_fp_register_single (struct frame_info *frame, int regno,
>>> +                           gdb_byte *rare_buffer)
>>> +{
>>> +  struct gdbarch *gdbarch = get_frame_arch (frame);
>>> +  int raw_size = register_size (gdbarch, regno);
>>> +  gdb_byte *raw_buffer = (gdb_byte *) alloca (raw_size);
>>> +
>>> +  if (!deprecated_frame_register_read (frame, regno, raw_buffer))
>>
>> Use get_frame_register_value, your code can be simpler.
>
> MIPS (where this code came from) still uses the deprecated function.  I can't
> figure out why this was deprecated or why MIPS is still using the old version,
> and since this code does dangerous looking things (silently copying half of a
> double-precision float) I'm not really sure how to change it.
>

riscv_read_fp_register_single is called for printing registers, so you can
use value and print it via val_print.  Then, riscv_read_fp_register_single can
be removed.  Just grep get_frame_register_value, and see how it is used
in other ports.

>>> +
>>> +static void
>>> +set_reg_offset (struct gdbarch *gdbarch, struct riscv_frame_cache *this_cache,
>>> +            int regnum, CORE_ADDR offset)
>>> +{
>>> +  if (this_cache != NULL && this_cache->saved_regs[regnum].addr == -1)
>>> +    this_cache->saved_regs[regnum].addr = offset;
>>> +}
>>> +
>>> +static void
>>> +reset_saved_regs (struct gdbarch *gdbarch, struct riscv_frame_cache *this_cache)
>>> +{
>>> +  const int num_regs = gdbarch_num_regs (gdbarch);
>>> +  int i;
>>> +
>>> +  if (this_cache == NULL || this_cache->saved_regs == NULL)
>>> +    return;
>>> +
>>> +  for (i = 0; i < num_regs; ++i)
>>> +    this_cache->saved_regs[i].addr = -1;
>>
>> IIRC, .addr's type is CORE_ADDR, which is unsigned.  Don't assign -1 to
>> a unsigned type.
>
> It looks like this is another one we got from MIPS.  I'm OK changing it, but
> I'm not sure what the implications are.  I think 0 is the sanest value to put
> in there?
>
>   https://github.com/riscv/riscv-binutils-gdb/commit/73656f235a8b7cedaab10ee49bbc55dbf02e86ce
>
> If that looks OK to you then I can go try to figure out if it's the right thing
> to do.
>

save_regs is a an array of struct trad_frame_saved_reg, and the type of
.addr is LONGEST, so it would be fine to set it to -1.  I withdraw my comments
on this.

>>> +      opcode = inst & 0x7F;
>>> +      reg = (inst >> 7) & 0x1F;
>>> +      rs1 = (inst >> 15) & 0x1F;
>>> +      imm12 = (inst >> 20) & 0xFFF;
>>> +      rs2 = (inst >> 20) & 0x1F;
>>> +      offset12 = (((inst >> 25) & 0x7F) << 5) + ((inst >> 7) & 0x1F);
>>> +      funct3 = (inst >> 12) & 0x7;
>>> +
>>> +      /* Look for common stack adjustment insns.  */
>>> +      if ((opcode == 0x13 || opcode == 0x1B) && reg == RISCV_SP_REGNUM
>>> +      && rs1 == RISCV_SP_REGNUM)
>>
>> Please use macros defined in include/opcode/riscv-opc.h, then the code
>> is much more readable.
>
> I agree.  Whoever wrote this code must have either not known about riscv-opc.h,
> or not liked those macros.  I added a FIXME for now
>
>   https://github.com/riscv/riscv-binutils-gdb/commit/1b58570b0310850290ed5355776e78192e893d71
>

Well, FIXME is not enough :-)  Please replace these magic numbers with macro.

-- 
Yao (齐尧)