From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 72127 invoked by alias); 16 Nov 2016 21:40:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 72109 invoked by uid 89); 16 Nov 2016 21:40:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=whoever, sorry!, Hx-languages-length:3920, xxx X-HELO: mail-qk0-f196.google.com Received: from mail-qk0-f196.google.com (HELO mail-qk0-f196.google.com) (209.85.220.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Nov 2016 21:40:01 +0000 Received: by mail-qk0-f196.google.com with SMTP id 124so23242777qkh.1 for ; Wed, 16 Nov 2016 13:40:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=+TPKsWt9CxvEDC4XOd9fyp3boiFZiw9VnAvvUb7WVp8=; b=EBuVsl5ADmXd2D2oSfPbrL4c6QAYK72jMByoaaLaY+0lh1BAo4Yu87i5kajVXC6G0k 1kp/jaYSb2n9zNJBOyYnI2QrRWZuSzqcZkw/TVBguLRX7voDieClkwSXVQ7ZAIFnMy7t PLCxB2V8MP8wkpFXMQUVOZs6NNI0AnkMkJ25wKe7dJ7OuCSwe7/nC/fhSOnUFuNh7203 V+T85EMOlGU+l4YjBch7Bx6Bv/TCymDi4s14YxhaSYcxGQPmGCbiwNZxWaPTa4IcmCdq OMFOdfklzuWE2uJq3hd3aHSQxug4kL+LypCdIePuiAislncASOXXYzfuDfpuhAwAmWzP r6qQ== X-Gm-Message-State: AKaTC019Jq5KNy+312o/KSRQdH/JDQgH5l3S6Aao6yyAe17raVjiDeEEXHmMGp7Ti/RsI/b7cM9U8CmR0hTMWg== X-Received: by 10.55.67.132 with SMTP id q126mr6640719qka.132.1479332400096; Wed, 16 Nov 2016 13:40:00 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.149.55 with HTTP; Wed, 16 Nov 2016 13:39:59 -0800 (PST) In-Reply-To: References: <86y4107n6q.fsf@gmail.com> From: Yao Qi Date: Wed, 16 Nov 2016 21:40:00 -0000 Message-ID: Subject: Re: [PATCH 1/2] RISC-V GDB Port To: Palmer Dabbelt Cc: Andrew Waterman , "gdb-patches@sourceware.org" , Alan Modra Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00461.txt.bz2 On Mon, Nov 14, 2016 at 10:39 PM, Palmer Dabbelt 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 =3D get_frame_arch (frame); >>> + int raw_size =3D register_size (gdbarch, regno); >>> + gdb_byte *raw_buffer =3D (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 c= an't > figure out why this was deprecated or why MIPS is still using the old ver= sion, > 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 *thi= s_cache, >>> + int regnum, CORE_ADDR offset) >>> +{ >>> + if (this_cache !=3D NULL && this_cache->saved_regs[regnum].addr =3D= =3D -1) >>> + this_cache->saved_regs[regnum].addr =3D offset; >>> +} >>> + >>> +static void >>> +reset_saved_regs (struct gdbarch *gdbarch, struct riscv_frame_cache *t= his_cache) >>> +{ >>> + const int num_regs =3D gdbarch_num_regs (gdbarch); >>> + int i; >>> + >>> + if (this_cache =3D=3D NULL || this_cache->saved_regs =3D=3D NULL) >>> + return; >>> + >>> + for (i =3D 0; i < num_regs; ++i) >>> + this_cache->saved_regs[i].addr =3D -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/73656f235a8b7cedaab1= 0ee49bbc55dbf02e86ce > > 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 comme= nts on this. >>> + opcode =3D inst & 0x7F; >>> + reg =3D (inst >> 7) & 0x1F; >>> + rs1 =3D (inst >> 15) & 0x1F; >>> + imm12 =3D (inst >> 20) & 0xFFF; >>> + rs2 =3D (inst >> 20) & 0x1F; >>> + offset12 =3D (((inst >> 25) & 0x7F) << 5) + ((inst >> 7) & 0x1F); >>> + funct3 =3D (inst >> 12) & 0x7; >>> + >>> + /* Look for common stack adjustment insns. */ >>> + if ((opcode =3D=3D 0x13 || opcode =3D=3D 0x1B) && reg =3D=3D RIS= CV_SP_REGNUM >>> + && rs1 =3D=3D 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/1b58570b0310850290ed= 5355776e78192e893d71 > Well, FIXME is not enough :-) Please replace these magic numbers with macr= o. --=20 Yao (=E9=BD=90=E5=B0=A7)