From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender-0.a4lg.com [IPv6:2401:2500:203:30b:4000:6bfe:4757:0]) by sourceware.org (Postfix) with ESMTPS id 13923385AC27 for ; Wed, 10 Aug 2022 13:48:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 13923385AC27 Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail-sender-0.a4lg.com (Postfix) with ESMTPSA id D6C78300089; Wed, 10 Aug 2022 13:48:47 +0000 (UTC) Message-ID: Date: Wed, 10 Aug 2022 22:48:45 +0900 Mime-Version: 1.0 Subject: Re: [PATCH 1/4] RISC-V: Fix immediates to have `immediate' style Content-Language: en-US To: Andrew Burgess References: <4919138bb21768d0cdc3a6da81dbbbd62cc96855.1657719581.git.research_trasio@irq.a4lg.com> <811c2c02e7ad0afe48eb2b8a2db210c823cdd9a2.1659500861.git.research_trasio@irq.a4lg.com> <871qto1h7l.fsf@redhat.com> From: Tsukasa OI Cc: Binutils In-Reply-To: <871qto1h7l.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Aug 2022 13:48:52 -0000 On 2022/08/10 20:16, Andrew Burgess wrote: > Tsukasa OI via Binutils writes: > >> This commit fixes certain print calls on immediate operands to have >> `dis_style_immediate'. >> >> opcodes/ChangeLog: >> >> * riscv-dis.c (print_insn_args): Fix immediates to have >> `immediate' style. >> (riscv_disassemble_data): Likewise. > > I'm not a binutils maintainer, so can't approve this patch. But... > > ... this looks good to me, with just one minor comment, see below. > >> --- >> opcodes/riscv-dis.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c >> index 164fd209dbd..fe091f294e7 100644 >> --- a/opcodes/riscv-dis.c >> +++ b/opcodes/riscv-dis.c >> @@ -472,7 +472,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info >> break; >> >> case 'y': >> - print (info->stream, dis_style_text, "0x%x", >> + print (info->stream, dis_style_immediate, "0x%x", >> (int)EXTRACT_OPERAND (BS, l)); >> break; >> >> @@ -541,17 +541,17 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info >> if (riscv_csr_hash[csr] != NULL) >> print (info->stream, dis_style_text, "%s", riscv_csr_hash[csr]); > > You haven't updated this line, but, I wonder if this should be > dis_style_register? That might be an option but I wasn't confident enough. Maybe RISC-V related developers can talk about it. For now, I prefer to keep this line. > > Otherwise, LGTM. > > Thanks, > Andrew > > >> else >> - print (info->stream, dis_style_text, "0x%x", csr); >> + print (info->stream, dis_style_immediate, "0x%x", csr); >> break; >> } >> >> case 'Y': >> - print (info->stream, dis_style_text, "0x%x", >> + print (info->stream, dis_style_immediate, "0x%x", >> (int) EXTRACT_OPERAND (RNUM, l)); >> break; >> >> case 'Z': >> - print (info->stream, dis_style_text, "%d", rs1); >> + print (info->stream, dis_style_immediate, "%d", rs1); >> break; >> >> default: >> @@ -907,7 +907,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, >> (*info->fprintf_styled_func) >> (info->stream, dis_style_assembler_directive, ".byte\t"); >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, "0x%02llx", >> + (info->stream, dis_style_immediate, "0x%02llx", >> (unsigned long long) data); >> break; >> case 2: >> -- >> 2.34.1 >