From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26689 invoked by alias); 18 Jan 2017 16:34:19 -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 26671 invoked by uid 89); 18 Jan 2017 16:34:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=INSN, UD:stream, H*r:AES128-SHA X-HELO: mail-lf0-f68.google.com Received: from mail-lf0-f68.google.com (HELO mail-lf0-f68.google.com) (209.85.215.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Jan 2017 16:34:17 +0000 Received: by mail-lf0-f68.google.com with SMTP id h65so2467637lfi.3 for ; Wed, 18 Jan 2017 08:34:16 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=bW8gkYabojDMUb8yo+ih9QCbYhxcR3B8LUfNR6zU8ek=; b=s5A1RHGNxSpISBLO46Kzj9K0rfkBxjIONUlRfIIod4eMVa3muyoFGvxPLTKxOEEtxF iM5z8X53+9N7oxiSRg80HiQqc/m1NLU09dVDYiQKxcT0Fu36+MzWLPPSe8SRzLoJqjKO 8QjQcH2HtNE7906U9rgklQX73LBvNM3EvhDcmcgTbk3MWIbf+tHfp014dK9Kb3C0qJ7e SIYWt3aSprI+bDORx5k9apoxm7nkmogJ4XYo4Ki8YFjIPPK398N1cgJ3h/c8/hU3yTxY Wus9kmnnEjJXt527CTd59tCX+cQ6ZG88CPBdwuX8hANtKze/vbsxiZbTS+pouR05HpNH turw== X-Gm-Message-State: AIkVDXIM3tgeUbYDJxIZB+aaiUHDZZrBKIi7hF4tQuc6DkhRGVkR5gkU5wZHonoPNEA20g== X-Received: by 10.46.80.66 with SMTP id v2mr2028555ljd.6.1484757254904; Wed, 18 Jan 2017 08:34:14 -0800 (PST) Received: from E107787-LIN (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id 9sm482515ljg.33.2017.01.18.08.34.12 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 18 Jan 2017 08:34:14 -0800 (PST) Date: Wed, 18 Jan 2017 16:34:00 -0000 From: Yao Qi To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/6] Refactor disassembly code Message-ID: <20170118163358.GO28060@E107787-LIN> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <752191a9-f54b-a7a2-2b72-a9250ef3a5b2@codesourcery.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00360.txt.bz2 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. */ > > >-static int > >-dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len, > >- struct disassemble_info *info) > >+ > >+int > >+gdb_disassembler::dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, > >+ unsigned int len, > >+ struct disassemble_info *info) > > { > > return target_read_code (memaddr, myaddr, len); > > } > > > > /* Like memory_error with slightly different parameters. */ and how about /* Wrapper of memory_error. */ > >-static void > >-dis_asm_memory_error (int err, bfd_vma memaddr, > >- struct disassemble_info *info) > >+ > >+ > >+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. > >+ > >+ /* 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. > >+ > >+ /* 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 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? -- Yao (齐尧)