[PATCH 0/2] RISC-V styled disassembler output fixes

[PATCH 0/2] RISC-V styled disassembler output fixes

[Andrew Burgess]

A recent(ish) commit added some non-styled output to the RISC-V
disassembler.

In the first patch I convert the non-styled output to be styled.

In the second patch I attempt to add a mechanism that will prevent
such code being introduced in the future.

Thanks,
Andrew

---

Andrew Burgess (2):
  opcodes/riscv: add styling support to print_reg_list
  opcodes/riscv: prevent future use of disassemble_info::fprintf_func

 opcodes/riscv-dis.c | 56 +++++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 14 deletions(-)


base-commit: f95540d91f4c3df373c0f9e212030154658d7f6f
-- 
2.25.4

[PATCH 1/2] opcodes/riscv: add styling support to print_reg_list

[Andrew Burgess]

I noticed that some unstyled output had crept into the risc-v
disassembler in this commit:

  commit 9132c8152b899a1683bc886f8ba76bedadb48aa1
  Date:   Tue Feb 27 11:48:11 2024 +0800

      RISC-V: Support Zcmp push/pop instructions.

this commit adds styling support.  The risc-v disassembler is now once
again, fully styled.
---
 opcodes/riscv-dis.c | 51 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index e6596c47423..d0d74c0a05a 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -223,26 +223,49 @@ print_reg_list (disassemble_info *info, insn_t l)
   bool numeric = riscv_gpr_names == riscv_gpr_names_numeric;
   unsigned reg_list = (int)EXTRACT_OPERAND (REG_LIST, l);
   unsigned r_start = numeric ? X_S2 : X_S0;
-  info->fprintf_func (info->stream, "%s", riscv_gpr_names[X_RA]);
+  info->fprintf_styled_func (info->stream, dis_style_register,
+			     "%s", riscv_gpr_names[X_RA]);
 
   if (reg_list == 5)
-    info->fprintf_func (info->stream, ",%s",
-			riscv_gpr_names[X_S0]);
+    {
+      info->fprintf_styled_func (info->stream, dis_style_text, ",");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[X_S0]);
+    }
   else if (reg_list == 6 || (numeric && reg_list > 6))
-    info->fprintf_func (info->stream, ",%s-%s",
-			riscv_gpr_names[X_S0],
-			riscv_gpr_names[X_S1]);
+    {
+      info->fprintf_styled_func (info->stream, dis_style_text, ",");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[X_S0]);
+      info->fprintf_styled_func (info->stream, dis_style_text, "-");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[X_S1]);
+    }
+
   if (reg_list == 15)
-    info->fprintf_func (info->stream, ",%s-%s",
-			riscv_gpr_names[r_start],
-			riscv_gpr_names[X_S11]);
+    {
+      info->fprintf_styled_func (info->stream, dis_style_text, ",");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[r_start]);
+      info->fprintf_styled_func (info->stream, dis_style_text, "-");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[X_S11]);
+    }
   else if (reg_list == 7 && numeric)
-    info->fprintf_func (info->stream, ",%s",
-			riscv_gpr_names[X_S2]);
+    {
+      info->fprintf_styled_func (info->stream, dis_style_text, ",");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[X_S2]);
+    }
   else if (reg_list > 6)
-    info->fprintf_func (info->stream, ",%s-%s",
-			riscv_gpr_names[r_start],
-			riscv_gpr_names[reg_list + 11]);
+    {
+      info->fprintf_styled_func (info->stream, dis_style_text, ",");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[r_start]);
+      info->fprintf_styled_func (info->stream, dis_style_text, "-");
+      info->fprintf_styled_func (info->stream, dis_style_register,
+				 "%s", riscv_gpr_names[reg_list + 11]);
+    }
 }
 
 /* Get Zcmp sp adjustment immediate.  */
-- 
2.25.4

[PATCH 2/2] opcodes/riscv: prevent future use of disassemble_info::fprintf_func

[Andrew Burgess]

The previous commit removed a use of disassemble_info::fprintf_func
which had been added to the RISC-V disassembler after the disassembler
had been switched to use ::fprintf_styled_func, for styled output.

To prevent future mistakes, I propose adding a #define to rename
fprintf_func to something which does not exist.  If this had been in
place then the before the previous commit libopcodes would have failed
to compile, like this:

  ../../src/opcodes/riscv-dis.c: In function ‘print_reg_list’:
  ../../src/opcodes/riscv-dis.c:229:7: error: ‘disassemble_info’ {aka ‘struct disassemble_info’} has no member named ‘please_use_fprintf_styled_func_instead’
    229 |   info->fprintf_func (info->stream, "%s", riscv_gpr_names[X_RA]);
        |       ^~

If this commit is accepted then I'll follow up with another commit
that adds the same #define to every disassembler that has been
converted to use styled output.

As the RISC-V disassembler is now fully styled, this commit should
make no difference at all.
---
 opcodes/riscv-dis.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index d0d74c0a05a..0082f08b7fb 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -32,6 +32,11 @@
 #include <stdint.h>
 #include <ctype.h>
 
+/* The RISC-V disassembler produces styled output using
+   disassemble_info::fprintf_styled_func.  This define prevent use of
+   disassemble_info::fprintf_func.  */
+#define fprintf_func please_use_fprintf_styled_func_instead
+
 /* Current XLEN for the disassembler.  */
 static unsigned xlen = 0;
 
-- 
2.25.4

Re: [PATCH 0/2] RISC-V styled disassembler output fixes

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Both looks good, thanks for helping this, Andrew :-) Please commit if you
think it's time.

Thanks
Nelson

On Wed, Jun 5, 2024 at 8:22 PM Andrew Burgess <aburgess@redhat.com> wrote:

> A recent(ish) commit added some non-styled output to the RISC-V
> disassembler.
>
> In the first patch I convert the non-styled output to be styled.
>
> In the second patch I attempt to add a mechanism that will prevent
> such code being introduced in the future.
>
> Thanks,
> Andrew
>
> ---
>
> Andrew Burgess (2):
>   opcodes/riscv: add styling support to print_reg_list
>   opcodes/riscv: prevent future use of disassemble_info::fprintf_func
>
>  opcodes/riscv-dis.c | 56 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 14 deletions(-)
>
>
> base-commit: f95540d91f4c3df373c0f9e212030154658d7f6f
> --
> 2.25.4
>
>

Re: [PATCH 0/2] RISC-V styled disassembler output fixes

[Andrew Burgess]

Nelson Chu <nelson@rivosinc.com> writes:

> Both looks good, thanks for helping this, Andrew :-) Please commit if you
> think it's time.

Pushed.

Thanks,
Andrew