This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] RISC-V GDB Port
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: Palmer Dabbelt <palmer at dabbelt dot com>
- Cc: Andrew Waterman <andrew at sifive dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, Alan Modra <amodra at gmail dot com>
- Date: Wed, 16 Nov 2016 21:39:59 +0000
- Subject: Re: [PATCH 1/2] RISC-V GDB Port
- Authentication-results: sourceware.org; auth=none
- References: <86y4107n6q.fsf@gmail.com> <mhng-2585e691-2d32-42d8-9527-6f71a85ba5a6@palmer-mbp2014>
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 (齐尧)