From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 86318 invoked by alias); 18 Jan 2017 16:53:45 -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 86302 invoked by uid 89); 18 Jan 2017 16:53:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.6 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=*uiout, Wrapper, sk:branch_, uiout X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Jan 2017 16:53:42 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cTtUT-0005wu-Cg from Luis_Gustavo@mentor.com ; Wed, 18 Jan 2017 08:53:41 -0800 Received: from [172.30.8.199] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Wed, 18 Jan 2017 08:53:37 -0800 Reply-To: Luis Machado Subject: Re: [PATCH 2/6] Refactor disassembly code References: <1484051178-16013-1-git-send-email-yao.qi@linaro.org> <1484560977-8693-1-git-send-email-yao.qi@linaro.org> <1484560977-8693-3-git-send-email-yao.qi@linaro.org> <752191a9-f54b-a7a2-2b72-a9250ef3a5b2@codesourcery.com> <20170118163358.GO28060@E107787-LIN> To: Yao Qi CC: From: Luis Machado Message-ID: Date: Wed, 18 Jan 2017 16:53:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170118163358.GO28060@E107787-LIN> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00364.txt.bz2 On 01/18/2017 10:33 AM, Yao Qi wrote: > On 17-01-17 08:14:27, Luis Machado wrote: >>> >>> /* Like target_read_memory, but slightly different parameters. */ >> >> Should we update these comments to a more useful version? "slightly >> different parameters" doesn't explain much, as it is obvious they >> are slightly different. Other uses below have the same problem. > > How about this? > > /* Wrapper of target_read_code. */ > I think that's better than the previous one. >>> +int >>> +gdb_disassembler::print_insn (CORE_ADDR memaddr, >>> + int *branch_delay_insns) >>> +{ >>> + int length = gdbarch_print_insn (arch (), memaddr, &m_di); >>> + >>> + if (branch_delay_insns != NULL) >>> + { >>> + if (m_di.insn_info_valid) >>> + *branch_delay_insns = m_di.branch_delay_insns; >>> + else >>> + *branch_delay_insns = 0; >>> + } >>> + return length; >> >> length doesn't seem to have a purpose other than being returned. So >> just return gdbarch_print_insn (arch (), memaddr, &m_di)? >> > > branch_delay_insns needs update after gdbarch_print_insn call. > Indeed. Missed the m_di. >>> + >>> + /* Prints the instruction INSN into UIOUT and returns the length of >>> + the printed instruction in bytes. */ >>> + int pretty_print_insn (struct ui_out *uiout, >>> + const struct disasm_insn *insn, int flags); >> >> Can this function return negative? If not, use unsigned? >> > > I don't think pretty_print_insn can return negative, and we can change > it to size_t. However, this patch is a code refactor. I want to avoid > change like this in this patch. I can change 'int' to 'size_t' later. > Sounds good. >>> + >>> + /* Return the gdbarch of gdb_disassembler. */ >>> + struct gdbarch *arch () >>> + { return m_gdbarch; } >>> + >>> +protected: >>> + gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file, >>> + di_read_memory_ftype func); >>> + >>> + struct ui_file *stream () >>> + { return (struct ui_file *) m_di.stream; } >>> + >>> +private: >>> + struct gdbarch *m_gdbarch; >>> + struct disassemble_info m_di; >> >> Add comments explaining what m_gdbarch and m_di are used for and/or how? > > I don't know what kind of comments I can add for m_gdbarch. Something > like "gdbarch of gdb_disassembler" looks useless. We've already had I can still see value in describing it as a "Pointer to the target's gdbarch". But feel free not to document it if you don't think it's worth it. The documentation is likely not for us (experienced gdb developers), but for people getting started on gdb's code. > method arch(with comments) which returns m_gdbarch. > > I can think of the comments to m_di > > /* Pass it to opcodes disassembler and collect some outputs > from opcodes disassembler. */ > > struct disassemble_info m_di; > > What do you think? > I think something along the lines of: /* Stores disassembler data. */ or /* Stores data required for disassembling instructions. */ ... is good enough, as trivial as it may seem.