%x requires `unsigned' type, not `int'. This commit fixes that. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Fix printf types from `int' to `unsigned'. --- opcodes/riscv-dis.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 164fd209dbd..c031ce9ae8f 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -283,15 +283,15 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info break; case 'u': print (info->stream, dis_style_immediate, "0x%x", - (int)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); + (unsigned)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); break; case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x3f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x3f); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x1f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x1f); break; case 'T': /* Floating-point RS2. */ print (info->stream, dis_style_register, "%s", @@ -473,7 +473,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'y': print (info->stream, dis_style_text, "0x%x", - (int)EXTRACT_OPERAND (BS, l)); + (unsigned)EXTRACT_OPERAND (BS, l)); break; case 'z': @@ -482,12 +482,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMT, l)); + (unsigned)EXTRACT_OPERAND (SHAMT, l)); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMTW, l)); + (unsigned)EXTRACT_OPERAND (SHAMTW, l)); break; case 'S': @@ -547,7 +547,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'Y': print (info->stream, dis_style_text, "0x%x", - (int) EXTRACT_OPERAND (RNUM, l)); + (unsigned) EXTRACT_OPERAND (RNUM, l)); break; case 'Z': base-commit: dd4c046506cd4da46b439a2b4f8b6d933ecbb961 -- 2.34.1
Hello, This patchset fixes various typing and styling errors on the RISC-V disassembler. Patches 1 and 2 are considered releatively important (compared to patches 3 and 4). Previous Patchset: <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13) Tracker on GitHub: <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> [Styling] - Print real immediates with the `immediate' style (PATCH 1/4) - Print comma and tabs with the `text' style (PATCH 4/4) [Typing on printf] - Fix wrong type for "%x" format specifier (PATCH 2/4) [Small Optimization] - Use smallest portable types possible to print (PATCH 3/4) Thanks, Tsukasa Tsukasa OI (4): RISC-V: Fix immediates to have `immediate' style RISC-V: Fix printf argument types corresponding %x RISC-V: Optimize riscv_disassemble_data printf RISC-V: Print comma and tabs as the `text' style opcodes/riscv-dis.c | 61 +++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 27 deletions(-) base-commit: 8aaafe957ceee581e691d2f7f984cb40c58f2b5e -- 2.34.1
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. --- 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]); 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
"%x" format specifier requires `unsigned' type, not `int'. This commit fixes this issue on the RISC-V disassembler. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Fix printf argument types where the format specifier is "%x". --- opcodes/riscv-dis.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index fe091f294e7..220a16e294f 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -283,15 +283,15 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info break; case 'u': print (info->stream, dis_style_immediate, "0x%x", - (int)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); + (unsigned)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); break; case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x3f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x3f); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x1f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x1f); break; case 'T': /* Floating-point RS2. */ print (info->stream, dis_style_register, "%s", @@ -473,7 +473,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'y': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (BS, l)); + (unsigned)EXTRACT_OPERAND (BS, l)); break; case 'z': @@ -482,12 +482,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMT, l)); + (unsigned)EXTRACT_OPERAND (SHAMT, l)); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMTW, l)); + (unsigned)EXTRACT_OPERAND (SHAMTW, l)); break; case 'S': @@ -547,7 +547,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'Y': print (info->stream, dis_style_immediate, "0x%x", - (int) EXTRACT_OPERAND (RNUM, l)); + (unsigned) EXTRACT_OPERAND (RNUM, l)); break; case 'Z': -- 2.34.1
This commit makes types of printf arguments on `riscv_disassemble_data' as small as possible (as long as we can preserve the portability) to reduce the cost of printf (especially on 32-bit host). opcodes/ChangeLog: * riscv-dis.c (riscv_disassemble_data): Use smallest possible type to printing data. --- opcodes/riscv-dis.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 220a16e294f..fe18c18642e 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -907,24 +907,22 @@ 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_immediate, "0x%02llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); break; case 2: info->bytes_per_line = 8; (*info->fprintf_styled_func) (info->stream, dis_style_assembler_directive, ".short\t"); (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%04llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); break; case 4: info->bytes_per_line = 8; (*info->fprintf_styled_func) (info->stream, dis_style_assembler_directive, ".word\t"); (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%08llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%08lx", + (unsigned long) data); break; case 8: info->bytes_per_line = 8; -- 2.34.1
On the RISC-V disassembler, some separators have non-`text' style when printed with another word with another style. This commit splits those, making sure that those comma and tabs are printed with the `text' style. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Split and print the comma as text. (riscv_disassemble_insn): Split and print tabs as text. (riscv_disassemble_data): Likewise. --- opcodes/riscv-dis.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index fe18c18642e..460f77676d2 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -373,9 +373,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info (int)EXTRACT_RVV_OFFSET (l)); break; case 'm': - if (! EXTRACT_OPERAND (VMASK, l)) - print (info->stream, dis_style_register, ",%s", - riscv_vecm_names_numeric[0]); + if (!EXTRACT_OPERAND (VMASK, l)) + { + print (info->stream, dis_style_text, ","); + print (info->stream, dis_style_register, "%s", + riscv_vecm_names_numeric[0]); + } break; } break; @@ -709,7 +712,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) case 4: case 8: (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".%dbyte\t", insnlen); + (info->stream, dis_style_assembler_directive, ".%dbyte", insnlen); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%llx", (unsigned long long) word); break; @@ -717,7 +721,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) { int i; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".byte\t"); + (info->stream, dis_style_assembler_directive, ".byte"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); for (i = 0; i < insnlen; ++i) { if (i > 0) @@ -905,21 +910,24 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, case 1: info->bytes_per_line = 6; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".byte\t"); - (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); + (info->stream, dis_style_assembler_directive, ".byte"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); + (*info->fprintf_styled_func) (info->stream, dis_style_immediate, + "0x%02x", (unsigned)data); break; case 2: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".short\t"); + (info->stream, dis_style_assembler_directive, ".short"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); break; case 4: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".word\t"); + (info->stream, dis_style_assembler_directive, ".word"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%08lx", (unsigned long) data); @@ -927,7 +935,8 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, case 8: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".dword\t"); + (info->stream, dis_style_assembler_directive, ".dword"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%016llx", (unsigned long long) data); -- 2.34.1
Tsukasa OI via Binutils <binutils@sourceware.org> 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? 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
Tsukasa OI via Binutils <binutils@sourceware.org> writes: > On the RISC-V disassembler, some separators have non-`text' style when > printed with another word with another style. > > This commit splits those, making sure that those comma and tabs are printed > with the `text' style. > > opcodes/ChangeLog: > > * riscv-dis.c (print_insn_args): Split and print the comma as text. > (riscv_disassemble_insn): Split and print tabs as text. > (riscv_disassemble_data): Likewise. I'm not a binutils maintainer, so can't approve this patch. However, this looks good to me with one minor nit, see below... > --- > opcodes/riscv-dis.c | 31 ++++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c > index fe18c18642e..460f77676d2 100644 > --- a/opcodes/riscv-dis.c > +++ b/opcodes/riscv-dis.c > @@ -373,9 +373,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info > (int)EXTRACT_RVV_OFFSET (l)); > break; > case 'm': > - if (! EXTRACT_OPERAND (VMASK, l)) > - print (info->stream, dis_style_register, ",%s", > - riscv_vecm_names_numeric[0]); > + if (!EXTRACT_OPERAND (VMASK, l)) > + { > + print (info->stream, dis_style_text, ","); > + print (info->stream, dis_style_register, "%s", > + riscv_vecm_names_numeric[0]); > + } > break; > } > break; > @@ -709,7 +712,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) > case 4: > case 8: > (*info->fprintf_styled_func) > - (info->stream, dis_style_assembler_directive, ".%dbyte\t", insnlen); > + (info->stream, dis_style_assembler_directive, ".%dbyte", insnlen); > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); > (*info->fprintf_styled_func) (info->stream, dis_style_immediate, > "0x%llx", (unsigned long long) word); > break; > @@ -717,7 +721,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) > { > int i; > (*info->fprintf_styled_func) > - (info->stream, dis_style_assembler_directive, ".byte\t"); > + (info->stream, dis_style_assembler_directive, ".byte"); > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); > for (i = 0; i < insnlen; ++i) > { > if (i > 0) > @@ -905,21 +910,24 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, > case 1: > info->bytes_per_line = 6; > (*info->fprintf_styled_func) > - (info->stream, dis_style_assembler_directive, ".byte\t"); > - (*info->fprintf_styled_func) > - (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); > + (info->stream, dis_style_assembler_directive, ".byte"); > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); > + (*info->fprintf_styled_func) (info->stream, dis_style_immediate, > + "0x%02x", (unsigned)data); You lost the space in '(unsigned) data'. Thanks, Andrew > break; > case 2: > info->bytes_per_line = 8; > (*info->fprintf_styled_func) > - (info->stream, dis_style_assembler_directive, ".short\t"); > + (info->stream, dis_style_assembler_directive, ".short"); > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); > (*info->fprintf_styled_func) > (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); > break; > case 4: > info->bytes_per_line = 8; > (*info->fprintf_styled_func) > - (info->stream, dis_style_assembler_directive, ".word\t"); > + (info->stream, dis_style_assembler_directive, ".word"); > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); > (*info->fprintf_styled_func) > (info->stream, dis_style_immediate, "0x%08lx", > (unsigned long) data); > @@ -927,7 +935,8 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, > case 8: > info->bytes_per_line = 8; > (*info->fprintf_styled_func) > - (info->stream, dis_style_assembler_directive, ".dword\t"); > + (info->stream, dis_style_assembler_directive, ".dword"); > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); > (*info->fprintf_styled_func) > (info->stream, dis_style_immediate, "0x%016llx", > (unsigned long long) data); > -- > 2.34.1
On 2022/08/10 20:16, Andrew Burgess wrote: > Tsukasa OI via Binutils <binutils@sourceware.org> 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 >
On 2022/08/10 20:20, Andrew Burgess wrote: > Tsukasa OI via Binutils <binutils@sourceware.org> writes: > >> On the RISC-V disassembler, some separators have non-`text' style when >> printed with another word with another style. >> >> This commit splits those, making sure that those comma and tabs are printed >> with the `text' style. >> >> opcodes/ChangeLog: >> >> * riscv-dis.c (print_insn_args): Split and print the comma as text. >> (riscv_disassemble_insn): Split and print tabs as text. >> (riscv_disassemble_data): Likewise. > > I'm not a binutils maintainer, so can't approve this patch. However, > this looks good to me with one minor nit, see below... > >> --- >> opcodes/riscv-dis.c | 31 ++++++++++++++++++++----------- >> 1 file changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c >> index fe18c18642e..460f77676d2 100644 >> --- a/opcodes/riscv-dis.c >> +++ b/opcodes/riscv-dis.c >> @@ -373,9 +373,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info >> (int)EXTRACT_RVV_OFFSET (l)); >> break; >> case 'm': >> - if (! EXTRACT_OPERAND (VMASK, l)) >> - print (info->stream, dis_style_register, ",%s", >> - riscv_vecm_names_numeric[0]); >> + if (!EXTRACT_OPERAND (VMASK, l)) >> + { >> + print (info->stream, dis_style_text, ","); >> + print (info->stream, dis_style_register, "%s", >> + riscv_vecm_names_numeric[0]); >> + } >> break; >> } >> break; >> @@ -709,7 +712,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) >> case 4: >> case 8: >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".%dbyte\t", insnlen); >> + (info->stream, dis_style_assembler_directive, ".%dbyte", insnlen); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) (info->stream, dis_style_immediate, >> "0x%llx", (unsigned long long) word); >> break; >> @@ -717,7 +721,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) >> { >> int i; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".byte\t"); >> + (info->stream, dis_style_assembler_directive, ".byte"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> for (i = 0; i < insnlen; ++i) >> { >> if (i > 0) >> @@ -905,21 +910,24 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, >> case 1: >> info->bytes_per_line = 6; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".byte\t"); >> - (*info->fprintf_styled_func) >> - (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); >> + (info->stream, dis_style_assembler_directive, ".byte"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_immediate, >> + "0x%02x", (unsigned)data); > > You lost the space in '(unsigned) data'. > > Thanks, > Andrew Interesting. I formatted this line using Visual Studio Code (which uses clang-format) and I think this is a minor difference between clang-format's "GNU" indent and real GNU Indent's defaults. Changing clang-format's settings just like "SpaceAfterCStyleCast: true" will mimic the default GNU Indent's behavior (better). It's too minor (this source file has other (type)(value) without any spaces) and will not republish the patchset unless there's some other reasons. But thank you for letting me know that there's something clang-format misses and I will follow "SpaceAfterCStyleCast: true" in the future. Thanks, Tsukasa > >> break; >> case 2: >> info->bytes_per_line = 8; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".short\t"); >> + (info->stream, dis_style_assembler_directive, ".short"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) >> (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); >> break; >> case 4: >> info->bytes_per_line = 8; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".word\t"); >> + (info->stream, dis_style_assembler_directive, ".word"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) >> (info->stream, dis_style_immediate, "0x%08lx", >> (unsigned long) data); >> @@ -927,7 +935,8 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, >> case 8: >> info->bytes_per_line = 8; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".dword\t"); >> + (info->stream, dis_style_assembler_directive, ".dword"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) >> (info->stream, dis_style_immediate, "0x%016llx", >> (unsigned long long) data); >> -- >> 2.34.1 >
Hello, This patchset fixes various typing and styling errors on the RISC-V disassembler. Previous Patchsets: <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13) <https://sourceware.org/pipermail/binutils/2022-August/122172.html> (2022-08-03) Tracker on GitHub: <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> [Changes: v1 -> v2] - Added PATCH 5-6/6 after T-Head extensions are merged. - Slightly improved the commit messages? [Styling] - Print real immediates with the `immediate' style (PATCH 1/6) - Print comma and tabs with the `text' style (PATCH 4/6) - Print T-Head literal operand with `immediate' style (PATCH 6/6) [Typing on printf] - Fix wrong type for "%x" format specifier (PATCH 2/6) - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) [Small Optimization] - Use smallest portable types possible to print (PATCH 3/6) Thanks, Tsukasa Tsukasa OI (6): RISC-V: Fix immediates to have "immediate" style RISC-V: Fix printf argument types corresponding %x RISC-V: Optimize riscv_disassemble_data printf RISC-V: Print comma and tabs as the "text" style RISC-V: Fix T-Head immediate types on printing RISC-V: Print XTheadMemPair literal as "immediate" opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 32 deletions(-) base-commit: c21736aed1d4877e090df60362413669dbdc391d -- 2.34.1
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. --- 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 f2d399260c1..a944e7865fe 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -480,7 +480,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; @@ -549,17 +549,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]); 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; case 'X': /* Integer immediate. */ @@ -963,7 +963,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
"%x" format specifier requires unsigned type, not int. This commit fixes this issue on the RISC-V disassembler. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Fix printf argument types where the format specifier is "%x". --- opcodes/riscv-dis.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index a944e7865fe..7adae3a1cd7 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -292,15 +292,15 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info break; case 'u': print (info->stream, dis_style_immediate, "0x%x", - (int)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); + (unsigned)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); break; case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x3f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x3f); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x1f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x1f); break; case 'T': /* Floating-point RS2. */ print (info->stream, dis_style_register, "%s", @@ -481,7 +481,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'y': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (BS, l)); + (unsigned)EXTRACT_OPERAND (BS, l)); break; case 'z': @@ -490,12 +490,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMT, l)); + (unsigned)EXTRACT_OPERAND (SHAMT, l)); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMTW, l)); + (unsigned)EXTRACT_OPERAND (SHAMTW, l)); break; case 'S': @@ -555,7 +555,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'Y': print (info->stream, dis_style_immediate, "0x%x", - (int) EXTRACT_OPERAND (RNUM, l)); + (unsigned) EXTRACT_OPERAND (RNUM, l)); break; case 'Z': -- 2.34.1
This commit makes types of printf arguments on riscv_disassemble_data as small as possible (as long as we can preserve the portability) to reduce the cost of printf (especially on 32-bit host). opcodes/ChangeLog: * riscv-dis.c (riscv_disassemble_data): Use smallest possible type to printing data. --- opcodes/riscv-dis.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 7adae3a1cd7..30cd90548a0 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -963,24 +963,22 @@ 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_immediate, "0x%02llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); break; case 2: info->bytes_per_line = 8; (*info->fprintf_styled_func) (info->stream, dis_style_assembler_directive, ".short\t"); (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%04llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); break; case 4: info->bytes_per_line = 8; (*info->fprintf_styled_func) (info->stream, dis_style_assembler_directive, ".word\t"); (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%08llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%08lx", + (unsigned long) data); break; case 8: info->bytes_per_line = 8; -- 2.34.1
On the RISC-V disassembler, some separators have non-text style when printed with another word with another style. This commit splits those, making sure that those comma and tabs are printed with the "text" style. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Split and print the comma as text. (riscv_disassemble_insn): Split and print tabs as text. (riscv_disassemble_data): Likewise. --- opcodes/riscv-dis.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 30cd90548a0..1c75317fad1 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -382,9 +382,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info (int)EXTRACT_RVV_OFFSET (l)); break; case 'm': - if (! EXTRACT_OPERAND (VMASK, l)) - print (info->stream, dis_style_register, ",%s", - riscv_vecm_names_numeric[0]); + if (!EXTRACT_OPERAND (VMASK, l)) + { + print (info->stream, dis_style_text, ","); + print (info->stream, dis_style_register, "%s", + riscv_vecm_names_numeric[0]); + } break; } break; @@ -765,7 +768,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) case 4: case 8: (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".%dbyte\t", insnlen); + (info->stream, dis_style_assembler_directive, ".%dbyte", insnlen); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%llx", (unsigned long long) word); break; @@ -773,7 +777,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) { int i; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".byte\t"); + (info->stream, dis_style_assembler_directive, ".byte"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); for (i = 0; i < insnlen; ++i) { if (i > 0) @@ -961,21 +966,24 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, case 1: info->bytes_per_line = 6; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".byte\t"); - (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); + (info->stream, dis_style_assembler_directive, ".byte"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); + (*info->fprintf_styled_func) (info->stream, dis_style_immediate, + "0x%02x", (unsigned)data); break; case 2: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".short\t"); + (info->stream, dis_style_assembler_directive, ".short"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); break; case 4: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".word\t"); + (info->stream, dis_style_assembler_directive, ".word"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%08lx", (unsigned long) data); @@ -983,7 +991,8 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, case 8: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".dword\t"); + (info->stream, dis_style_assembler_directive, ".dword"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%016llx", (unsigned long long) data); -- 2.34.1
This commit fixes three minor typing-related issues for T-Head immediate values. 1. The format string %i is renamed to %d (which is more common). 2. Signed type must be specified when printing with %d. 3. unsigned/signed int it not portable enough for max 32-bit immediates. Instead, we should use unsigned/signed long. The format string is changed accordingly. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Fix T-Head immediate types on printing. --- opcodes/riscv-dis.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 1c75317fad1..df2a6f4e131 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -596,11 +596,11 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info oparg--; if (!sign) - print (info->stream, dis_style_immediate, "%u", - (unsigned)EXTRACT_U_IMM (n, s, l)); + print (info->stream, dis_style_immediate, "%lu", + (unsigned long)EXTRACT_U_IMM (n, s, l)); else - print (info->stream, dis_style_immediate, "%i", - (unsigned)EXTRACT_S_IMM (n, s, l)); + print (info->stream, dis_style_immediate, "%ld", + (signed long)EXTRACT_S_IMM (n, s, l)); break; default: goto undefined_modifier; -- 2.34.1
The operand type "Xl(...)" denotes that (...) is a literal. Specifically, they are intended to be a constant immediate value. This commit prints "Xl(...)" operand with dis_style_immediate style, not dis_style_text. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Use dis_style_immediate on the constant literal of the "Xl..." operand. --- opcodes/riscv-dis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index df2a6f4e131..7aa45db75f2 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -577,7 +577,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info oparg++; while (*oparg && *oparg != ',') { - print (info->stream, dis_style_text, "%c", *oparg); + print (info->stream, dis_style_immediate, "%c", *oparg); oparg++; } oparg--; -- 2.34.1
Tsukasa OI via Binutils <binutils@sourceware.org> writes: > This commit fixes three minor typing-related issues for > T-Head immediate values. > > 1. The format string %i is renamed to %d (which is more common). > 2. Signed type must be specified when printing with %d. > 3. unsigned/signed int it not portable enough for max 32-bit immediates. typo: `it` should be `is`. Thanks, Andrew > Instead, we should use unsigned/signed long. > The format string is changed accordingly. > > opcodes/ChangeLog: > > * riscv-dis.c (print_insn_args): Fix T-Head immediate types on > printing. > --- > opcodes/riscv-dis.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c > index 1c75317fad1..df2a6f4e131 100644 > --- a/opcodes/riscv-dis.c > +++ b/opcodes/riscv-dis.c > @@ -596,11 +596,11 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info > oparg--; > > if (!sign) > - print (info->stream, dis_style_immediate, "%u", > - (unsigned)EXTRACT_U_IMM (n, s, l)); > + print (info->stream, dis_style_immediate, "%lu", > + (unsigned long)EXTRACT_U_IMM (n, s, l)); > else > - print (info->stream, dis_style_immediate, "%i", > - (unsigned)EXTRACT_S_IMM (n, s, l)); > + print (info->stream, dis_style_immediate, "%ld", > + (signed long)EXTRACT_S_IMM (n, s, l)); > break; > default: > goto undefined_modifier; > -- > 2.34.1
Tsukasa OI via Binutils <binutils@sourceware.org> writes: > Hello, > > This patchset fixes various typing and styling errors on the RISC-V > disassembler. I can't approve binutils patches. I've taken a look through this series, and other than the minor typo I spotted, this all looks good to me. Thanks for doing this. Andrew > > Previous Patchsets: > <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13) > <https://sourceware.org/pipermail/binutils/2022-August/122172.html> (2022-08-03) > Tracker on GitHub: > <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> > > > [Changes: v1 -> v2] > > - Added PATCH 5-6/6 after T-Head extensions are merged. > - Slightly improved the commit messages? > > > [Styling] > - Print real immediates with the `immediate' style (PATCH 1/6) > - Print comma and tabs with the `text' style (PATCH 4/6) > - Print T-Head literal operand with `immediate' style (PATCH 6/6) > [Typing on printf] > - Fix wrong type for "%x" format specifier (PATCH 2/6) > - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) > [Small Optimization] > - Use smallest portable types possible to print (PATCH 3/6) > > Thanks, > Tsukasa > > > > > Tsukasa OI (6): > RISC-V: Fix immediates to have "immediate" style > RISC-V: Fix printf argument types corresponding %x > RISC-V: Optimize riscv_disassemble_data printf > RISC-V: Print comma and tabs as the "text" style > RISC-V: Fix T-Head immediate types on printing > RISC-V: Print XTheadMemPair literal as "immediate" > > opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- > 1 file changed, 39 insertions(+), 32 deletions(-) > > > base-commit: c21736aed1d4877e090df60362413669dbdc391d > -- > 2.34.1
Hello, This patchset fixes various typing and styling errors on the RISC-V disassembler. Previous Patchsets: <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13) <https://sourceware.org/pipermail/binutils/2022-August/122172.html> (2022-08-03) <https://sourceware.org/pipermail/binutils/2022-September/123090.html> (2022-09-26) Tracker on GitHub: <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> [Changes: v2 -> v3] - PATCH 5/6: Changed to %li instead of %ld I proposed (before patch: %i) because changing to %d-based felt too intrusive. - PATCH 5/6: Fixed a typo Thanks to Andrew Burgess for feedback! [Changes: v1 -> v2] - Added PATCH 5-6/6 after T-Head extensions are merged. - Slightly improved the commit messages? [Styling] - Print real immediates with the "immediate" style (PATCH 1/6) - Print comma and tabs with the "text" style (PATCH 4/6) - Print T-Head literal operand with "immediate" style (PATCH 6/6) [Typing on printf] - Fix wrong type for "%x" format specifier (PATCH 2/6) - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) [Small Optimization] - Use smallest portable types possible to print (PATCH 3/6) Thanks, Tsukasa Tsukasa OI (6): RISC-V: Fix immediates to have "immediate" style RISC-V: Fix printf argument types corresponding %x RISC-V: Optimize riscv_disassemble_data printf RISC-V: Print comma and tabs as the "text" style RISC-V: Fix T-Head immediate types on printing RISC-V: Print XTheadMemPair literal as "immediate" opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 32 deletions(-) base-commit: 2820f08f2331820808ac2f54d8e3619f36ea258b -- 2.34.1
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. --- 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 6ac69490b78..a5bfa97058a 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -480,7 +480,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; @@ -549,17 +549,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]); 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; case 'X': /* Integer immediate. */ @@ -963,7 +963,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
"%x" format specifier requires unsigned type, not int. This commit fixes this issue on the RISC-V disassembler. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Fix printf argument types where the format specifier is "%x". --- opcodes/riscv-dis.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index a5bfa97058a..503b839f19e 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -292,15 +292,15 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info break; case 'u': print (info->stream, dis_style_immediate, "0x%x", - (int)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); + (unsigned)(EXTRACT_CITYPE_IMM (l) & (RISCV_BIGIMM_REACH-1))); break; case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x3f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x3f); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_CITYPE_IMM (l) & 0x1f); + (unsigned)EXTRACT_CITYPE_IMM (l) & 0x1f); break; case 'T': /* Floating-point RS2. */ print (info->stream, dis_style_register, "%s", @@ -481,7 +481,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'y': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (BS, l)); + (unsigned)EXTRACT_OPERAND (BS, l)); break; case 'z': @@ -490,12 +490,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case '>': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMT, l)); + (unsigned)EXTRACT_OPERAND (SHAMT, l)); break; case '<': print (info->stream, dis_style_immediate, "0x%x", - (int)EXTRACT_OPERAND (SHAMTW, l)); + (unsigned)EXTRACT_OPERAND (SHAMTW, l)); break; case 'S': @@ -555,7 +555,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info case 'Y': print (info->stream, dis_style_immediate, "0x%x", - (int) EXTRACT_OPERAND (RNUM, l)); + (unsigned) EXTRACT_OPERAND (RNUM, l)); break; case 'Z': -- 2.34.1
This commit makes types of printf arguments on riscv_disassemble_data as small as possible (as long as we can preserve the portability) to reduce the cost of printf (especially on 32-bit host). opcodes/ChangeLog: * riscv-dis.c (riscv_disassemble_data): Use smallest possible type to printing data. --- opcodes/riscv-dis.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 503b839f19e..70f484189f5 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -963,24 +963,22 @@ 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_immediate, "0x%02llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); break; case 2: info->bytes_per_line = 8; (*info->fprintf_styled_func) (info->stream, dis_style_assembler_directive, ".short\t"); (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%04llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); break; case 4: info->bytes_per_line = 8; (*info->fprintf_styled_func) (info->stream, dis_style_assembler_directive, ".word\t"); (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%08llx", - (unsigned long long) data); + (info->stream, dis_style_immediate, "0x%08lx", + (unsigned long) data); break; case 8: info->bytes_per_line = 8; -- 2.34.1
On the RISC-V disassembler, some separators have non-text style when printed with another word with another style. This commit splits those, making sure that those comma and tabs are printed with the "text" style. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Split and print the comma as text. (riscv_disassemble_insn): Split and print tabs as text. (riscv_disassemble_data): Likewise. --- opcodes/riscv-dis.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 70f484189f5..7f5ca4dcb85 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -382,9 +382,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info (int)EXTRACT_RVV_OFFSET (l)); break; case 'm': - if (! EXTRACT_OPERAND (VMASK, l)) - print (info->stream, dis_style_register, ",%s", - riscv_vecm_names_numeric[0]); + if (!EXTRACT_OPERAND (VMASK, l)) + { + print (info->stream, dis_style_text, ","); + print (info->stream, dis_style_register, "%s", + riscv_vecm_names_numeric[0]); + } break; } break; @@ -765,7 +768,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) case 4: case 8: (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".%dbyte\t", insnlen); + (info->stream, dis_style_assembler_directive, ".%dbyte", insnlen); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%llx", (unsigned long long) word); break; @@ -773,7 +777,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) { int i; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".byte\t"); + (info->stream, dis_style_assembler_directive, ".byte"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); for (i = 0; i < insnlen; ++i) { if (i > 0) @@ -961,21 +966,24 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, case 1: info->bytes_per_line = 6; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".byte\t"); - (*info->fprintf_styled_func) - (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); + (info->stream, dis_style_assembler_directive, ".byte"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); + (*info->fprintf_styled_func) (info->stream, dis_style_immediate, + "0x%02x", (unsigned)data); break; case 2: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".short\t"); + (info->stream, dis_style_assembler_directive, ".short"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); break; case 4: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".word\t"); + (info->stream, dis_style_assembler_directive, ".word"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%08lx", (unsigned long) data); @@ -983,7 +991,8 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, case 8: info->bytes_per_line = 8; (*info->fprintf_styled_func) - (info->stream, dis_style_assembler_directive, ".dword\t"); + (info->stream, dis_style_assembler_directive, ".dword"); + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%016llx", (unsigned long long) data); -- 2.34.1
This commit fixes two minor typing-related issues for T-Head immediate operands. 1. A signed type must be specified when printing with %i. 2. unsigned/signed int is not portable enough for max 32-bit immediates. Instead, we should use unsigned/signed long. The format string is changed accordingly. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Fix T-Head immediate types on printing. --- opcodes/riscv-dis.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index 7f5ca4dcb85..c0edc4c7d6b 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -596,11 +596,11 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info oparg--; if (!sign) - print (info->stream, dis_style_immediate, "%u", - (unsigned)EXTRACT_U_IMM (n, s, l)); + print (info->stream, dis_style_immediate, "%lu", + (unsigned long)EXTRACT_U_IMM (n, s, l)); else - print (info->stream, dis_style_immediate, "%i", - (unsigned)EXTRACT_S_IMM (n, s, l)); + print (info->stream, dis_style_immediate, "%li", + (signed long)EXTRACT_S_IMM (n, s, l)); break; default: goto undefined_modifier; -- 2.34.1
The operand type "Xl(...)" denotes that (...) is a literal. Specifically, they are intended to be a constant immediate value. This commit prints "Xl(...)" operand with dis_style_immediate style, not dis_style_text. opcodes/ChangeLog: * riscv-dis.c (print_insn_args): Use dis_style_immediate on the constant literal of the "Xl..." operand. --- opcodes/riscv-dis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index c0edc4c7d6b..7da96e15b65 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -577,7 +577,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info oparg++; while (*oparg && *oparg != ',') { - print (info->stream, dis_style_text, "%c", *oparg); + print (info->stream, dis_style_immediate, "%c", *oparg); oparg++; } oparg--; -- 2.34.1
[-- Attachment #1: Type: text/plain, Size: 1822 bytes --] On Mon, Sep 26, 2022 at 2:30 PM Tsukasa OI via Binutils < binutils@sourceware.org> wrote: > This commit fixes three minor typing-related issues for > T-Head immediate values. > > 1. The format string %i is renamed to %d (which is more common). > 2. Signed type must be specified when printing with %d. > 3. unsigned/signed int it not portable enough for max 32-bit immediates. > Instead, we should use unsigned/signed long. > The format string is changed accordingly. > Tested-by: Christoph Müllner <christoph.muellner@vrull.eu> > > opcodes/ChangeLog: > > * riscv-dis.c (print_insn_args): Fix T-Head immediate types on > printing. > --- > opcodes/riscv-dis.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c > index 1c75317fad1..df2a6f4e131 100644 > --- a/opcodes/riscv-dis.c > +++ b/opcodes/riscv-dis.c > @@ -596,11 +596,11 @@ print_insn_args (const char *oparg, insn_t l, > bfd_vma pc, disassemble_info *info > oparg--; > > if (!sign) > - print (info->stream, dis_style_immediate, "%u", > - (unsigned)EXTRACT_U_IMM (n, s, l)); > + print (info->stream, dis_style_immediate, "%lu", > + (unsigned long)EXTRACT_U_IMM (n, s, l)); > else > - print (info->stream, dis_style_immediate, "%i", > - (unsigned)EXTRACT_S_IMM (n, s, l)); > + print (info->stream, dis_style_immediate, "%ld", > + (signed long)EXTRACT_S_IMM (n, s, l)); > break; > default: > goto undefined_modifier; > -- > 2.34.1 > >
[-- Attachment #1: Type: text/plain, Size: 1286 bytes --] On Mon, Sep 26, 2022 at 2:28 PM Tsukasa OI via Binutils < binutils@sourceware.org> wrote: > The operand type "Xl(...)" denotes that (...) is a literal. Specifically, > they are intended to be a constant immediate value. > > This commit prints "Xl(...)" operand with dis_style_immediate style, > not dis_style_text. > Tested-by: Christoph Müllner <christoph.muellner@vrull.eu> > > opcodes/ChangeLog: > > * riscv-dis.c (print_insn_args): Use dis_style_immediate on > the constant literal of the "Xl..." operand. > --- > opcodes/riscv-dis.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c > index df2a6f4e131..7aa45db75f2 100644 > --- a/opcodes/riscv-dis.c > +++ b/opcodes/riscv-dis.c > @@ -577,7 +577,7 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma > pc, disassemble_info *info > oparg++; > while (*oparg && *oparg != ',') > { > - print (info->stream, dis_style_text, "%c", *oparg); > + print (info->stream, dis_style_immediate, "%c", > *oparg); > oparg++; > } > oparg--; > -- > 2.34.1 > >
On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote: > Tsukasa OI via Binutils <binutils@sourceware.org> writes: > >> Hello, >> >> This patchset fixes various typing and styling errors on the RISC-V >> disassembler. > > I can't approve binutils patches. > > I've taken a look through this series, and other than the minor typo I > spotted, this all looks good to me. My worry with these things is that it's sort of a grey area in terms of what our stable user interface is: there's definitely folks who try and parse the text output of tools like objdump and changing anything risks breaking that. IMO that's too strict of a stable interface to keep around as it pretty much prevents us from fixing any disassembly weirdness, but I'm not sure if that's what other ports do and I don't want to make a mess by breaking users' expectations here. That said, as far as I can tell the only user-visible change here is to print some bits with the correct syntax coloring in GDB. Seems pretty straight-forward to say anything users can't rely on parsing syntax highlighting colors being a stable interface, so maybe we can just punt on the grey area for now ;) > Thanks for doing this. Yep. These LGTM, but I'm still kind of buried thanks to the a few weeks of conference->COVID. They're probably fine, but hopefully Nelson has the time to chime in -- if not I'll leave them in the queue. Thanks! > > Andrew > > > >> >> Previous Patchsets: >> <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13) >> <https://sourceware.org/pipermail/binutils/2022-August/122172.html> (2022-08-03) >> Tracker on GitHub: >> <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> >> >> >> [Changes: v1 -> v2] >> >> - Added PATCH 5-6/6 after T-Head extensions are merged. >> - Slightly improved the commit messages? >> >> >> [Styling] >> - Print real immediates with the `immediate' style (PATCH 1/6) >> - Print comma and tabs with the `text' style (PATCH 4/6) >> - Print T-Head literal operand with `immediate' style (PATCH 6/6) >> [Typing on printf] >> - Fix wrong type for "%x" format specifier (PATCH 2/6) >> - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) >> [Small Optimization] >> - Use smallest portable types possible to print (PATCH 3/6) >> >> Thanks, >> Tsukasa >> >> >> >> >> Tsukasa OI (6): >> RISC-V: Fix immediates to have "immediate" style >> RISC-V: Fix printf argument types corresponding %x >> RISC-V: Optimize riscv_disassemble_data printf >> RISC-V: Print comma and tabs as the "text" style >> RISC-V: Fix T-Head immediate types on printing >> RISC-V: Print XTheadMemPair literal as "immediate" >> >> opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- >> 1 file changed, 39 insertions(+), 32 deletions(-) >> >> >> base-commit: c21736aed1d4877e090df60362413669dbdc391d >> -- >> 2.34.1
On 2022/10/04 2:40, Palmer Dabbelt wrote: > On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote: >> Tsukasa OI via Binutils <binutils@sourceware.org> writes: >> >>> Hello, >>> >>> This patchset fixes various typing and styling errors on the RISC-V >>> disassembler. >> >> I can't approve binutils patches. >> >> I've taken a look through this series, and other than the minor typo I >> spotted, this all looks good to me. > > My worry with these things is that it's sort of a grey area in terms of > what our stable user interface is: there's definitely folks who try and > parse the text output of tools like objdump and changing anything risks > breaking that. IMO that's too strict of a stable interface to keep > around as it pretty much prevents us from fixing any disassembly > weirdness, but I'm not sure if that's what other ports do and I don't > want to make a mess by breaking users' expectations here. > > That said, as far as I can tell the only user-visible change here is to > print some bits with the correct syntax coloring in GDB. Seems pretty > straight-forward to say anything users can't rely on parsing syntax > highlighting colors being a stable interface, so maybe we can just punt > on the grey area for now ;) > >> Thanks for doing this. > > Yep. These LGTM, but I'm still kind of buried thanks to the a few weeks > of conference->COVID. They're probably fine, but hopefully Nelson has > the time to chime in -- if not I'll leave them in the queue. > > Thanks! Understood. I hope you get better soon. It seems it looks good to you but no approval yet (I think Nelson will approve it because he approved related patch). For this particular patchset, that's better than just "approval of v2" because "PATCH v3" has minor changes (although no functional changes). PATCH v3: <https://sourceware.org/pipermail/binutils/2022-October/123274.html> CSR style change by Andrew Burgess (I also spotted it but didn't included in this patchset because it seemed non-obvious as the rest): <https://sourceware.org/pipermail/binutils/2022-October/123272.html> Nelson's Approval of the patch by Andrew: <https://sourceware.org/pipermail/binutils/2022-October/123306.html> Thanks, Tsukasa > >> >> Andrew >> >> >> >>> >>> Previous Patchsets: >>> <https://sourceware.org/pipermail/binutils/2022-July/121790.html> >>> (2022-07-13) >>> <https://sourceware.org/pipermail/binutils/2022-August/122172.html> >>> (2022-08-03) >>> Tracker on GitHub: >>> <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> >>> >>> >>> [Changes: v1 -> v2] >>> >>> - Added PATCH 5-6/6 after T-Head extensions are merged. >>> - Slightly improved the commit messages? >>> >>> >>> [Styling] >>> - Print real immediates with the `immediate' style (PATCH 1/6) >>> - Print comma and tabs with the `text' style (PATCH 4/6) >>> - Print T-Head literal operand with `immediate' style (PATCH 6/6) >>> [Typing on printf] >>> - Fix wrong type for "%x" format specifier (PATCH 2/6) >>> - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) >>> [Small Optimization] >>> - Use smallest portable types possible to print (PATCH 3/6) >>> >>> Thanks, >>> Tsukasa >>> >>> >>> >>> >>> Tsukasa OI (6): >>> RISC-V: Fix immediates to have "immediate" style >>> RISC-V: Fix printf argument types corresponding %x >>> RISC-V: Optimize riscv_disassemble_data printf >>> RISC-V: Print comma and tabs as the "text" style >>> RISC-V: Fix T-Head immediate types on printing >>> RISC-V: Print XTheadMemPair literal as "immediate" >>> >>> opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- >>> 1 file changed, 39 insertions(+), 32 deletions(-) >>> >>> >>> base-commit: c21736aed1d4877e090df60362413669dbdc391d >>> -- >>> 2.34.1 >
On Tue, Oct 4, 2022 at 1:40 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote: > > Tsukasa OI via Binutils <binutils@sourceware.org> writes: > > > >> Hello, > >> > >> This patchset fixes various typing and styling errors on the RISC-V > >> disassembler. > > > > I can't approve binutils patches. > > > > I've taken a look through this series, and other than the minor typo I > > spotted, this all looks good to me. > > My worry with these things is that it's sort of a grey area in terms of > what our stable user interface is: there's definitely folks who try and > parse the text output of tools like objdump and changing anything risks > breaking that. I have the same worry, but we probably won't hear someone really yell until the next release. However I am not the maintainer and user of these analysis tools either, so I cannot say such dis-assembler patches, including the patch to sort the opcode table, will definitely break something. But being too conservative does make development slower, even if stable things should still have a chance to be improved, so I struggle every time when I see these kinds of patches. What I can only say is that - if you think these kinds of things may be helpful to someone, and have not only one maintainer or experts say these are probably fine, then we can try to commit the patches after a while, if there are not any objections. Nelson > IMO that's too strict of a stable interface to keep > around as it pretty much prevents us from fixing any disassembly > weirdness, but I'm not sure if that's what other ports do and I don't > want to make a mess by breaking users' expectations here. > > That said, as far as I can tell the only user-visible change here is to > print some bits with the correct syntax coloring in GDB. Seems pretty > straight-forward to say anything users can't rely on parsing syntax > highlighting colors being a stable interface, so maybe we can just punt > on the grey area for now ;) > > Yep. These LGTM, but I'm still kind of buried thanks to the > a few weeks of conference->COVID. They're probably fine, but hopefully > Nelson has the time to chime in -- if not I'll leave them in the queue.
Palmer Dabbelt <palmer@dabbelt.com> writes: > On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote: >> Tsukasa OI via Binutils <binutils@sourceware.org> writes: >> >>> Hello, >>> >>> This patchset fixes various typing and styling errors on the RISC-V >>> disassembler. >> >> I can't approve binutils patches. >> >> I've taken a look through this series, and other than the minor typo I >> spotted, this all looks good to me. > > My worry with these things is that it's sort of a grey area in terms of > what our stable user interface is: there's definitely folks who try and > parse the text output of tools like objdump and changing anything risks > breaking that. IMO that's too strict of a stable interface to keep > around as it pretty much prevents us from fixing any disassembly > weirdness, but I'm not sure if that's what other ports do and I don't > want to make a mess by breaking users' expectations here. I guess we'd want to consider these things on a case-by-case basis. I'm struggling to think of a situation where we wouldn't want to fix something in the disassembler output if it was wrong over leaving it incorrect because someone, somewhere, might have a script that depends on the broken output. But unless I've missed something, I don't think this series makes any changes to the output other than styling, and I don't think a script that's trying to parse the disassembler output should be running with styling switched on.... ...or if they are, maybe because they want to figure out which bits of the text are which, then surely they'd welcome these fixes? > That said, as far as I can tell the only user-visible change here is to > print some bits with the correct syntax coloring in GDB. And in objdump, which also styles its output by default now when the output is going to a tty. Any scripts that capture objdump output should see no changes with this series. The styling is pretty new, and I knew when I added it there were going to be mistakes with things styled incorrectly. I was basically relying on getting something in that was not terrible, and then relying on folk like Tsukasa to come along and fix my mistakes. IMHO, any script that relies on a fixed styling is a bad script. I'd probably go further, and say that any script that can't handle minor changes in the disassembly output at all is a bad script, but without seeing every case that might be a little harsh. Anyway, FWIW, I think we should for sure approve this series. Thanks, Andrew > Seems pretty > straight-forward to say anything users can't rely on parsing syntax > highlighting colors being a stable interface, so maybe we can just punt > on the grey area for now ;) > >> Thanks for doing this. > > Yep. These LGTM, but I'm still kind of buried thanks to the > a few weeks of conference->COVID. They're probably fine, but hopefully > Nelson has the time to chime in -- if not I'll leave them in the queue. > > Thanks! > >> >> Andrew >> >> >> >>> >>> Previous Patchsets: >>> <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13) >>> <https://sourceware.org/pipermail/binutils/2022-August/122172.html> (2022-08-03) >>> Tracker on GitHub: >>> <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> >>> >>> >>> [Changes: v1 -> v2] >>> >>> - Added PATCH 5-6/6 after T-Head extensions are merged. >>> - Slightly improved the commit messages? >>> >>> >>> [Styling] >>> - Print real immediates with the `immediate' style (PATCH 1/6) >>> - Print comma and tabs with the `text' style (PATCH 4/6) >>> - Print T-Head literal operand with `immediate' style (PATCH 6/6) >>> [Typing on printf] >>> - Fix wrong type for "%x" format specifier (PATCH 2/6) >>> - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) >>> [Small Optimization] >>> - Use smallest portable types possible to print (PATCH 3/6) >>> >>> Thanks, >>> Tsukasa >>> >>> >>> >>> >>> Tsukasa OI (6): >>> RISC-V: Fix immediates to have "immediate" style >>> RISC-V: Fix printf argument types corresponding %x >>> RISC-V: Optimize riscv_disassemble_data printf >>> RISC-V: Print comma and tabs as the "text" style >>> RISC-V: Fix T-Head immediate types on printing >>> RISC-V: Print XTheadMemPair literal as "immediate" >>> >>> opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- >>> 1 file changed, 39 insertions(+), 32 deletions(-) >>> >>> >>> base-commit: c21736aed1d4877e090df60362413669dbdc391d >>> -- >>> 2.34.1
On 10/3/22 11:40, Palmer Dabbelt wrote:
> On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote:
>> Tsukasa OI via Binutils <binutils@sourceware.org> writes:
>>
>>> Hello,
>>>
>>> This patchset fixes various typing and styling errors on the RISC-V
>>> disassembler.
>>
>> I can't approve binutils patches.
>>
>> I've taken a look through this series, and other than the minor typo I
>> spotted, this all looks good to me.
>
> My worry with these things is that it's sort of a grey area in terms
> of what our stable user interface is: there's definitely folks who try
> and parse the text output of tools like objdump and changing anything
> risks breaking that. IMO that's too strict of a stable interface to
> keep around as it pretty much prevents us from fixing any disassembly
> weirdness, but I'm not sure if that's what other ports do and I don't
> want to make a mess by breaking users' expectations here.
>
> That said, as far as I can tell the only user-visible change here is
> to print some bits with the correct syntax coloring in GDB. Seems
> pretty straight-forward to say anything users can't rely on parsing
> syntax highlighting colors being a stable interface, so maybe we can
> just punt on the grey area for now ;)
I don't think the precise for of disassembly output needs to stay
stable; it's certainly not something we've ever promised or encouraged
folks to rely on. Obviously we shouldn't change it just for the sake of
changing it, but if we need to make a fix, we should just make it.
jeff
On Tue, 04 Oct 2022 01:46:01 PDT (-0700), aburgess@redhat.com wrote: > Palmer Dabbelt <palmer@dabbelt.com> writes: >> On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote: >>> Tsukasa OI via Binutils <binutils@sourceware.org> writes: >>> >>>> Hello, >>>> >>>> This patchset fixes various typing and styling errors on the RISC-V >>>> disassembler. >>> >>> I can't approve binutils patches. >>> >>> I've taken a look through this series, and other than the minor typo I >>> spotted, this all looks good to me. >> >> My worry with these things is that it's sort of a grey area in terms of >> what our stable user interface is: there's definitely folks who try and >> parse the text output of tools like objdump and changing anything risks >> breaking that. IMO that's too strict of a stable interface to keep >> around as it pretty much prevents us from fixing any disassembly >> weirdness, but I'm not sure if that's what other ports do and I don't >> want to make a mess by breaking users' expectations here. > > I guess we'd want to consider these things on a case-by-case basis. I'm > struggling to think of a situation where we wouldn't want to fix > something in the disassembler output if it was wrong over leaving it > incorrect because someone, somewhere, might have a script that depends > on the broken output. > > But unless I've missed something, I don't think this series makes any > changes to the output other than styling, and I don't think a script > that's trying to parse the disassembler output should be running with > styling switched on.... > > ...or if they are, maybe because they want to figure out which bits of > the text are which, then surely they'd welcome these fixes? > >> That said, as far as I can tell the only user-visible change here is to >> print some bits with the correct syntax coloring in GDB. > > And in objdump, which also styles its output by default now when the > output is going to a tty. Any scripts that capture objdump output > should see no changes with this series. Ah, cool, I hadn't seen that when poking around. I don't think it changes the reasoning, though. > The styling is pretty new, and I knew when I added it there were going > to be mistakes with things styled incorrectly. I was basically relying > on getting something in that was not terrible, and then relying on folk > like Tsukasa to come along and fix my mistakes. > > IMHO, any script that relies on a fixed styling is a bad script. Ya, that seems like an easy one -- doubly so if it's relying on styling that's wrong it a handful of cases. Like you said, if anything this would fix scripts rather than break them. > I'd probably go further, and say that any script that can't handle minor > changes in the disassembly output at all is a bad script, but without > seeing every case that might be a little harsh. I've definitely written scripts like that, and it felt pretty bad at the time. It'd still be a small headache if they broke: less actually fixing the script, but more that some test would break on a binutils upgrade and that tends to spook the HW folks into just never upgrading the toolchain again (even if it's just a test checking script that broken). That kind of stuff is someone else's problem these days, though, so I'm fine just pretending none of it ever happened ;) > Anyway, FWIW, I think we should for sure approve this series. Works for me. Sounds like Jeff was saying about the same thing in his reply, so approved from me. Tsukasa: I haven't built/tested these and my box is kind of tied up with Linux right now, but feel free to commit them assuming someone's tested them. Thanks! > > Thanks, > Andrew > > >> Seems pretty >> straight-forward to say anything users can't rely on parsing syntax >> highlighting colors being a stable interface, so maybe we can just punt >> on the grey area for now ;) >> >>> Thanks for doing this. >> >> Yep. These LGTM, but I'm still kind of buried thanks to the >> a few weeks of conference->COVID. They're probably fine, but hopefully >> Nelson has the time to chime in -- if not I'll leave them in the queue. >> >> Thanks! >> >>> >>> Andrew >>> >>> >>> >>>> >>>> Previous Patchsets: >>>> <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13) >>>> <https://sourceware.org/pipermail/binutils/2022-August/122172.html> (2022-08-03) >>>> Tracker on GitHub: >>>> <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1> >>>> >>>> >>>> [Changes: v1 -> v2] >>>> >>>> - Added PATCH 5-6/6 after T-Head extensions are merged. >>>> - Slightly improved the commit messages? >>>> >>>> >>>> [Styling] >>>> - Print real immediates with the `immediate' style (PATCH 1/6) >>>> - Print comma and tabs with the `text' style (PATCH 4/6) >>>> - Print T-Head literal operand with `immediate' style (PATCH 6/6) >>>> [Typing on printf] >>>> - Fix wrong type for "%x" format specifier (PATCH 2/6) >>>> - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) >>>> [Small Optimization] >>>> - Use smallest portable types possible to print (PATCH 3/6) >>>> >>>> Thanks, >>>> Tsukasa >>>> >>>> >>>> >>>> >>>> Tsukasa OI (6): >>>> RISC-V: Fix immediates to have "immediate" style >>>> RISC-V: Fix printf argument types corresponding %x >>>> RISC-V: Optimize riscv_disassemble_data printf >>>> RISC-V: Print comma and tabs as the "text" style >>>> RISC-V: Fix T-Head immediate types on printing >>>> RISC-V: Print XTheadMemPair literal as "immediate" >>>> >>>> opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- >>>> 1 file changed, 39 insertions(+), 32 deletions(-) >>>> >>>> >>>> base-commit: c21736aed1d4877e090df60362413669dbdc391d >>>> -- >>>> 2.34.1