[PATCH] libopcodes/ppc: add support for disassembler styling

[PATCH] libopcodes/ppc: add support for disassembler styling

[Andrew Burgess]

This commit adds disassembler styling to the libopcodes ppc
disassembler.  This conversion was pretty straight forward, I just
converted the fprintf_func calls to fprintf_styled_func calls and
added an appropriate style.

For testing the new styling I just assembled then disassembled the
source files in gas/testsuite/gas/ppc and manually checked that the
styling looked reasonable.

I think the only slightly weird case was how things like '4*cr1+eq'
are styled.  As best I can tell, this construct, used for example in
this instruction:

  crand   4*cr1+lt,4*cr1+gt,4*cr1+eq

is used to access a field of a control register (NOTE: I know very
little about the PPC ISA, so please correct me if I'm wrong).  As
such, I have styled the entire construct as a register.  In some cases
constructs similar to the above can be simplified to just 'eq', as
this is still referencing a register field, this is still styled as a
register.

If the user does not request styled output from objdump, then there
should be no change in the disassembler output after this commit.
---
 opcodes/disassemble.c |   1 +
 opcodes/ppc-dis.c     | 107 ++++++++++++++++++++++++++++++------------
 2 files changed, 78 insertions(+), 30 deletions(-)

diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
index bd37f042b31..9c991b8121e 100644
--- a/opcodes/disassemble.c
+++ b/opcodes/disassemble.c
@@ -705,6 +705,7 @@ disassemble_init_for_target (struct disassemble_info * info)
 #endif
 #if defined (ARCH_powerpc) || defined (ARCH_rs6000)
       disassemble_init_powerpc (info);
+      info->created_styled_output = true;
       break;
 #endif
 #ifdef ARCH_riscv
diff --git a/opcodes/ppc-dis.c b/opcodes/ppc-dis.c
index 45e8faeef5e..1447afaecc3 100644
--- a/opcodes/ppc-dis.c
+++ b/opcodes/ppc-dis.c
@@ -829,12 +829,24 @@ print_got_plt (struct sec_buf *sb, uint64_t vma, struct disassemble_info *info)
 		    sym = (*info->symbol_at_address_func) (ent, info);
 		}
 	    }
+	  (*info->fprintf_styled_func) (info->stream, dis_style_text, " [");
 	  if (sym != NULL)
-	    (*info->fprintf_func) (info->stream, " [%s@%s]",
-				   bfd_asymbol_name (sym), sb->name + 1);
+	    {
+	      (*info->fprintf_styled_func) (info->stream, dis_style_symbol,
+					    "%s", bfd_asymbol_name (sym));
+	      (*info->fprintf_styled_func) (info->stream, dis_style_text, "@");
+	      (*info->fprintf_styled_func) (info->stream, dis_style_symbol,
+					    "%s", sb->name + 1);
+	    }
 	  else
-	    (*info->fprintf_func) (info->stream, " [%" PRIx64 "@%s]",
-				   ent, sb->name + 1);
+	    {
+	      (*info->fprintf_styled_func) (info->stream, dis_style_address,
+					    "%" PRIx64, ent);
+	      (*info->fprintf_styled_func) (info->stream, dis_style_text, "@");
+	      (*info->fprintf_styled_func) (info->stream, dis_style_symbol,
+					    "%s", sb->name + 1);
+	    }
+	  (*info->fprintf_styled_func) (info->stream, dis_style_text, "]");
 	  return true;
 	}
     }
@@ -943,8 +955,9 @@ print_insn_powerpc (bfd_vma memaddr,
       uint64_t d34;
       int blanks;
 
-      (*info->fprintf_func) (info->stream, "%s", opcode->name);
-      /* gdb fprintf_func doesn't return count printed.  */
+      (*info->fprintf_styled_func) (info->stream, dis_style_mnemonic,
+				    "%s", opcode->name);
+      /* gdb fprintf_styled_func doesn't return count printed.  */
       blanks = 8 - strlen (opcode->name);
       if (blanks <= 0)
 	blanks = 1;
@@ -976,39 +989,49 @@ print_insn_powerpc (bfd_vma memaddr,
 	  value = operand_value_powerpc (operand, insn, dialect);
 
 	  if (op_separator == need_comma)
-	    (*info->fprintf_func) (info->stream, ",");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, ",");
 	  else if (op_separator == need_paren)
-	    (*info->fprintf_func) (info->stream, "(");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, "(");
 	  else
-	    (*info->fprintf_func) (info->stream, "%*s", op_separator, " ");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, "%*s",
+					  op_separator, " ");
 
 	  /* Print the operand as directed by the flags.  */
 	  if ((operand->flags & PPC_OPERAND_GPR) != 0
 	      || ((operand->flags & PPC_OPERAND_GPR_0) != 0 && value != 0))
-	    (*info->fprintf_func) (info->stream, "r%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "r%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_FPR) != 0)
-	    (*info->fprintf_func) (info->stream, "f%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "f%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_VR) != 0)
-	    (*info->fprintf_func) (info->stream, "v%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "v%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_VSR) != 0)
-	    (*info->fprintf_func) (info->stream, "vs%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "vs%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_ACC) != 0)
-	    (*info->fprintf_func) (info->stream, "a%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "a%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_RELATIVE) != 0)
 	    (*info->print_address_func) (memaddr + value, info);
 	  else if ((operand->flags & PPC_OPERAND_ABSOLUTE) != 0)
 	    (*info->print_address_func) ((bfd_vma) value & 0xffffffff, info);
 	  else if ((operand->flags & PPC_OPERAND_FSL) != 0)
-	    (*info->fprintf_func) (info->stream, "fsl%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "fsl%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_FCR) != 0)
-	    (*info->fprintf_func) (info->stream, "fcr%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "fcr%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_UDI) != 0)
-	    (*info->fprintf_func) (info->stream, "%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text,
+					  "%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_CR_REG) != 0
 		   && (operand->flags & PPC_OPERAND_CR_BIT) == 0
 		   && (((dialect & PPC_OPCODE_PPC) != 0)
 		       || ((dialect & PPC_OPCODE_VLE) != 0)))
-	    (*info->fprintf_func) (info->stream, "cr%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "cr%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_CR_BIT) != 0
 		   && (operand->flags & PPC_OPERAND_CR_REG) == 0
 		   && (((dialect & PPC_OPCODE_PPC) != 0)
@@ -1019,13 +1042,27 @@ print_insn_powerpc (bfd_vma memaddr,
 	      int cc;
 
 	      cr = value >> 2;
-	      if (cr != 0)
-		(*info->fprintf_func) (info->stream, "4*cr%d+", cr);
 	      cc = value & 3;
-	      (*info->fprintf_func) (info->stream, "%s", cbnames[cc]);
+	      if (cr != 0)
+		(*info->fprintf_styled_func) (info->stream, dis_style_register,
+					      "4*cr%d+%s", cr, cbnames[cc]);
+	      else
+		(*info->fprintf_styled_func) (info->stream, dis_style_register,
+					      "%s", cbnames[cc]);
 	    }
 	  else
-	    (*info->fprintf_func) (info->stream, "%" PRId64, value);
+	    {
+	      /* An immediate, but what style?  */
+	      enum disassembler_style style;
+
+	      if ((operand->flags & PPC_OPERAND_PARENS) != 0)
+		style = dis_style_address_offset;
+	      else
+		style = dis_style_immediate;
+
+	      (*info->fprintf_styled_func) (info->stream, style,
+					    "%" PRId64, value);
+	    }
 
 	  if (operand->shift == 52)
 	    is_pcrel = value != 0;
@@ -1033,7 +1070,7 @@ print_insn_powerpc (bfd_vma memaddr,
 	    d34 = value;
 
 	  if (op_separator == need_paren)
-	    (*info->fprintf_func) (info->stream, ")");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, ")");
 
 	  op_separator = need_comma;
 	  if ((operand->flags & PPC_OPERAND_PARENS) != 0)
@@ -1043,11 +1080,13 @@ print_insn_powerpc (bfd_vma memaddr,
       if (is_pcrel)
 	{
 	  d34 += memaddr;
-	  (*info->fprintf_func) (info->stream, "\t# %" PRIx64, d34);
+	  (*info->fprintf_styled_func) (info->stream,
+					dis_style_comment_start,
+					"\t# %" PRIx64, d34);
 	  asymbol *sym = (*info->symbol_at_address_func) (d34, info);
 	  if (sym)
-	    (*info->fprintf_func) (info->stream, " <%s>",
-				   bfd_asymbol_name (sym));
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text,
+					  " <%s>", bfd_asymbol_name (sym));
 
 	  if (info->private_data != NULL
 	      && info->section != NULL
@@ -1069,11 +1108,19 @@ print_insn_powerpc (bfd_vma memaddr,
 
   /* We could not find a match.  */
   if (insn_length == 4)
-    (*info->fprintf_func) (info->stream, ".long 0x%x",
-			   (unsigned int) insn);
+    (*info->fprintf_styled_func) (info->stream,
+				  dis_style_assembler_directive, ".long");
   else
-    (*info->fprintf_func) (info->stream, ".word 0x%x",
-			   (unsigned int) insn >> 16);
+    {
+      (*info->fprintf_styled_func) (info->stream,
+				    dis_style_assembler_directive, ".word");
+      insn >>= 16;
+    }
+  (*info->fprintf_styled_func) (info->stream, dis_style_text, " ");
+  (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%x",
+				(unsigned int) insn);
+
+
   return insn_length;
 }
 
-- 
2.25.4

Re: [PATCH] libopcodes/ppc: add support for disassembler styling

[Alan Modra]

On Tue, Jul 12, 2022 at 02:36:09PM +0100, Andrew Burgess via Binutils wrote:
> This commit adds disassembler styling to the libopcodes ppc
> disassembler.  This conversion was pretty straight forward, I just
> converted the fprintf_func calls to fprintf_styled_func calls and
> added an appropriate style.

Thanks for doing this!  The patch is OK as-is, but see comments
below if you'd like to make some improvements.

> For testing the new styling I just assembled then disassembled the
> source files in gas/testsuite/gas/ppc and manually checked that the
> styling looked reasonable.
> 
> I think the only slightly weird case was how things like '4*cr1+eq'
> are styled.  As best I can tell, this construct, used for example in
> this instruction:
> 
>   crand   4*cr1+lt,4*cr1+gt,4*cr1+eq
> 
> is used to access a field of a control register (NOTE: I know very
> little about the PPC ISA, so please correct me if I'm wrong).  As
> such, I have styled the entire construct as a register.  In some cases
> constructs similar to the above can be simplified to just 'eq', as
> this is still referencing a register field, this is still styled as a
> register.

This field is also present in conditional branches, and looking at
those is perhaps is the best way to understand what ought to be done
here.

eg. this instruction
lab: beq 7,lab
disassembles with "objdump -d" to
	beq cr7,0 <lab>
and with "objdump -d -Mraw" to see the underlying hardware insn
	bc 12,4*cr7+eq,0 <lab>

So the field probably ought to disassemble the "4*" and "+" as text,
the "eq" the same as the opcode mnemonic, and the "cr7" as a
register.  At least, that seems a reasonable way to highlight the
"interesting bits" to me.

>  	  else if ((operand->flags & PPC_OPERAND_UDI) != 0)
> -	    (*info->fprintf_func) (info->stream, "%" PRId64, value);
> +	    (*info->fprintf_styled_func) (info->stream, dis_style_text,
> +					  "%" PRId64, value);

I think these are likely best disassembled as registers.  I really
don't know that much about the Xilinx embedded cpu, but I believe it
has these "user defined instructions" where the field is configurable
as a register or immediate.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] libopcodes/ppc: add support for disassembler styling

[Jan Beulich]

On 14.07.2022 04:09, Alan Modra via Binutils wrote:
> On Tue, Jul 12, 2022 at 02:36:09PM +0100, Andrew Burgess via Binutils wrote:
>> I think the only slightly weird case was how things like '4*cr1+eq'
>> are styled.  As best I can tell, this construct, used for example in
>> this instruction:
>>
>>   crand   4*cr1+lt,4*cr1+gt,4*cr1+eq
>>
>> is used to access a field of a control register (NOTE: I know very
>> little about the PPC ISA, so please correct me if I'm wrong).  As
>> such, I have styled the entire construct as a register.  In some cases
>> constructs similar to the above can be simplified to just 'eq', as
>> this is still referencing a register field, this is still styled as a
>> register.
> 
> This field is also present in conditional branches, and looking at
> those is perhaps is the best way to understand what ought to be done
> here.
> 
> eg. this instruction
> lab: beq 7,lab
> disassembles with "objdump -d" to
> 	beq cr7,0 <lab>
> and with "objdump -d -Mraw" to see the underlying hardware insn
> 	bc 12,4*cr7+eq,0 <lab>
> 
> So the field probably ought to disassemble the "4*" and "+" as text,
> the "eq" the same as the opcode mnemonic,

Or maybe that new type which was introduced for Arm64?

Jan

> and the "cr7" as a
> register.  At least, that seems a reasonable way to highlight the
> "interesting bits" to me.

Re: [PATCH] libopcodes/ppc: add support for disassembler styling

[Alan Modra]

On Thu, Jul 14, 2022 at 08:08:07AM +0200, Jan Beulich wrote:
> On 14.07.2022 04:09, Alan Modra via Binutils wrote:
> > On Tue, Jul 12, 2022 at 02:36:09PM +0100, Andrew Burgess via Binutils wrote:
> >> I think the only slightly weird case was how things like '4*cr1+eq'
> >> are styled.  As best I can tell, this construct, used for example in
> >> this instruction:
> >>
> >>   crand   4*cr1+lt,4*cr1+gt,4*cr1+eq
> >>
> >> is used to access a field of a control register (NOTE: I know very
> >> little about the PPC ISA, so please correct me if I'm wrong).  As
> >> such, I have styled the entire construct as a register.  In some cases
> >> constructs similar to the above can be simplified to just 'eq', as
> >> this is still referencing a register field, this is still styled as a
> >> register.
> > 
> > This field is also present in conditional branches, and looking at
> > those is perhaps is the best way to understand what ought to be done
> > here.
> > 
> > eg. this instruction
> > lab: beq 7,lab
> > disassembles with "objdump -d" to
> > 	beq cr7,0 <lab>
> > and with "objdump -d -Mraw" to see the underlying hardware insn
> > 	bc 12,4*cr7+eq,0 <lab>
> > 
> > So the field probably ought to disassemble the "4*" and "+" as text,
> > the "eq" the same as the opcode mnemonic,
> 
> Or maybe that new type which was introduced for Arm64?

Yes, dis_style_sub_mnemonic works too.

> > and the "cr7" as a
> > register.  At least, that seems a reasonable way to highlight the
> > "interesting bits" to me.

-- 
Alan Modra
Australia Development Lab, IBM

[PATCHv2 0/2] PPC Styled Disassembler Support

[Andrew Burgess]

Since v1:

  - Added patch #1, this is the exact same patch I'm proposing in my
    AArch64 thread; this adds dis_style_sub_mnemonic,

  - In patch #2, updated the formatting for things like '4*cr1+eq',
    the new styling is explained in the commit message,

  - Updated the styling of PPC_OPERAND_UDI operands to be register as
    suggested by Alan.

Thanks,
Andrew

---

Andrew Burgess (2):
  opcodes: add new sub-mnemonic disassembler style
  libopcodes/ppc: add support for disassembler styling

 binutils/objdump.c    |   2 +
 include/dis-asm.h     |   7 +++
 opcodes/disassemble.c |   1 +
 opcodes/ppc-dis.c     | 117 +++++++++++++++++++++++++++++++-----------
 4 files changed, 97 insertions(+), 30 deletions(-)

-- 
2.25.4

[PATCHv2 1/2] opcodes: add new sub-mnemonic disassembler style

[Andrew Burgess]

When adding libopcodes disassembler styling support for AArch64, it
feels like the results would be improved by having a new sub-mnemonic
style.  This will be used in cases like:

  add    w16, w7, w1, uxtb #2
                      ^^^^----- Here

And:

  cinc   w0, w1, ne
                 ^^----- Here

This commit just adds the new style, and prepares objdump to handle
the style.  A later commit will add AArch64 styling, and will actually
make use of the style.

As this style is currently unused, there should be no user visible
changes after this commit.
---
 binutils/objdump.c | 2 ++
 include/dis-asm.h  | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/binutils/objdump.c b/binutils/objdump.c
index 67824053527..4076587151c 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -2167,6 +2167,7 @@ objdump_color_for_disassembler_style (enum disassembler_style style)
 	{
 	case dis_style_symbol: color = 32; break;
         case dis_style_assembler_directive:
+	case dis_style_sub_mnemonic:
 	case dis_style_mnemonic: color = 33; break;
 	case dis_style_register: color = 34; break;
 	case dis_style_address:
@@ -2185,6 +2186,7 @@ objdump_color_for_disassembler_style (enum disassembler_style style)
 	{
 	case dis_style_symbol: color = 40; break;
         case dis_style_assembler_directive:
+	case dis_style_sub_mnemonic:
 	case dis_style_mnemonic: color = 142; break;
 	case dis_style_register: color = 27; break;
 	case dis_style_address:
diff --git a/include/dis-asm.h b/include/dis-asm.h
index 4f91df12498..f1a83dc84e5 100644
--- a/include/dis-asm.h
+++ b/include/dis-asm.h
@@ -62,6 +62,13 @@ enum disassembler_style
      instructions.  */
   dis_style_mnemonic,
 
+  /* Some architectures include additional mnemonic like fields within the
+     instruction operands, e.g. on aarch64 'add w16, w7, w1, lsl #2' where
+     the 'lsl' is an additional piece of text that describes how the
+     instruction should behave.  This sub-mnemonic style can be used for
+     these pieces of text.  */
+  dis_style_sub_mnemonic,
+
   /* For things that aren't real machine instructions, but rather
      assembler directives, e.g. .byte, etc.  */
   dis_style_assembler_directive,
-- 
2.25.4

[PATCHv2 2/2] libopcodes/ppc: add support for disassembler styling

[Andrew Burgess]

This commit adds disassembler styling to the libopcodes ppc
disassembler.  This conversion was pretty straight forward, I just
converted the fprintf_func calls to fprintf_styled_func calls and
added an appropriate style.

For testing the new styling I just assembled then disassembled the
source files in gas/testsuite/gas/ppc and manually checked that the
styling looked reasonable.

I think the only slightly weird case was how things like '4*cr1+eq'
are styled.  As best I can tell, this construct, used for example in
this instruction:

  crand   4*cr1+lt,4*cr1+gt,4*cr1+eq

is used to access a field of a control register.  I initially tried
styling this whole construct as a register[1], but during review it
was suggested that instead different parts of the text should have
different styles.  In this commit I propose styling '4*cr1+lt' like
this:

  4    - immediate,
  *    - text,
  cr1  - register
  +    - text
  lt   - sub-mnemonic

If the user does not request styled output from objdump, then there
should be no change in the disassembler output after this commit.

[1] https://sourceware.org/pipermail/binutils/2022-July/121771.html
---
 opcodes/disassemble.c |   1 +
 opcodes/ppc-dis.c     | 117 +++++++++++++++++++++++++++++++-----------
 2 files changed, 88 insertions(+), 30 deletions(-)

diff --git a/opcodes/disassemble.c b/opcodes/disassemble.c
index e2e5a2608d6..4090e94b623 100644
--- a/opcodes/disassemble.c
+++ b/opcodes/disassemble.c
@@ -710,6 +710,7 @@ disassemble_init_for_target (struct disassemble_info * info)
 #endif
 #if defined (ARCH_powerpc) || defined (ARCH_rs6000)
       disassemble_init_powerpc (info);
+      info->created_styled_output = true;
       break;
 #endif
 #ifdef ARCH_riscv
diff --git a/opcodes/ppc-dis.c b/opcodes/ppc-dis.c
index 45e8faeef5e..57e792b922b 100644
--- a/opcodes/ppc-dis.c
+++ b/opcodes/ppc-dis.c
@@ -829,12 +829,24 @@ print_got_plt (struct sec_buf *sb, uint64_t vma, struct disassemble_info *info)
 		    sym = (*info->symbol_at_address_func) (ent, info);
 		}
 	    }
+	  (*info->fprintf_styled_func) (info->stream, dis_style_text, " [");
 	  if (sym != NULL)
-	    (*info->fprintf_func) (info->stream, " [%s@%s]",
-				   bfd_asymbol_name (sym), sb->name + 1);
+	    {
+	      (*info->fprintf_styled_func) (info->stream, dis_style_symbol,
+					    "%s", bfd_asymbol_name (sym));
+	      (*info->fprintf_styled_func) (info->stream, dis_style_text, "@");
+	      (*info->fprintf_styled_func) (info->stream, dis_style_symbol,
+					    "%s", sb->name + 1);
+	    }
 	  else
-	    (*info->fprintf_func) (info->stream, " [%" PRIx64 "@%s]",
-				   ent, sb->name + 1);
+	    {
+	      (*info->fprintf_styled_func) (info->stream, dis_style_address,
+					    "%" PRIx64, ent);
+	      (*info->fprintf_styled_func) (info->stream, dis_style_text, "@");
+	      (*info->fprintf_styled_func) (info->stream, dis_style_symbol,
+					    "%s", sb->name + 1);
+	    }
+	  (*info->fprintf_styled_func) (info->stream, dis_style_text, "]");
 	  return true;
 	}
     }
@@ -943,8 +955,9 @@ print_insn_powerpc (bfd_vma memaddr,
       uint64_t d34;
       int blanks;
 
-      (*info->fprintf_func) (info->stream, "%s", opcode->name);
-      /* gdb fprintf_func doesn't return count printed.  */
+      (*info->fprintf_styled_func) (info->stream, dis_style_mnemonic,
+				    "%s", opcode->name);
+      /* gdb fprintf_styled_func doesn't return count printed.  */
       blanks = 8 - strlen (opcode->name);
       if (blanks <= 0)
 	blanks = 1;
@@ -976,39 +989,49 @@ print_insn_powerpc (bfd_vma memaddr,
 	  value = operand_value_powerpc (operand, insn, dialect);
 
 	  if (op_separator == need_comma)
-	    (*info->fprintf_func) (info->stream, ",");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, ",");
 	  else if (op_separator == need_paren)
-	    (*info->fprintf_func) (info->stream, "(");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, "(");
 	  else
-	    (*info->fprintf_func) (info->stream, "%*s", op_separator, " ");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, "%*s",
+					  op_separator, " ");
 
 	  /* Print the operand as directed by the flags.  */
 	  if ((operand->flags & PPC_OPERAND_GPR) != 0
 	      || ((operand->flags & PPC_OPERAND_GPR_0) != 0 && value != 0))
-	    (*info->fprintf_func) (info->stream, "r%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "r%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_FPR) != 0)
-	    (*info->fprintf_func) (info->stream, "f%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "f%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_VR) != 0)
-	    (*info->fprintf_func) (info->stream, "v%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "v%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_VSR) != 0)
-	    (*info->fprintf_func) (info->stream, "vs%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "vs%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_ACC) != 0)
-	    (*info->fprintf_func) (info->stream, "a%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "a%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_RELATIVE) != 0)
 	    (*info->print_address_func) (memaddr + value, info);
 	  else if ((operand->flags & PPC_OPERAND_ABSOLUTE) != 0)
 	    (*info->print_address_func) ((bfd_vma) value & 0xffffffff, info);
 	  else if ((operand->flags & PPC_OPERAND_FSL) != 0)
-	    (*info->fprintf_func) (info->stream, "fsl%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "fsl%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_FCR) != 0)
-	    (*info->fprintf_func) (info->stream, "fcr%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "fcr%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_UDI) != 0)
-	    (*info->fprintf_func) (info->stream, "%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_CR_REG) != 0
 		   && (operand->flags & PPC_OPERAND_CR_BIT) == 0
 		   && (((dialect & PPC_OPCODE_PPC) != 0)
 		       || ((dialect & PPC_OPCODE_VLE) != 0)))
-	    (*info->fprintf_func) (info->stream, "cr%" PRId64, value);
+	    (*info->fprintf_styled_func) (info->stream, dis_style_register,
+					  "cr%" PRId64, value);
 	  else if ((operand->flags & PPC_OPERAND_CR_BIT) != 0
 		   && (operand->flags & PPC_OPERAND_CR_REG) == 0
 		   && (((dialect & PPC_OPCODE_PPC) != 0)
@@ -1019,13 +1042,37 @@ print_insn_powerpc (bfd_vma memaddr,
 	      int cc;
 
 	      cr = value >> 2;
-	      if (cr != 0)
-		(*info->fprintf_func) (info->stream, "4*cr%d+", cr);
 	      cc = value & 3;
-	      (*info->fprintf_func) (info->stream, "%s", cbnames[cc]);
+	      if (cr != 0)
+		{
+		  (*info->fprintf_styled_func) (info->stream,
+						dis_style_immediate, "4");
+		  (*info->fprintf_styled_func) (info->stream, dis_style_text,
+						"*");
+		  (*info->fprintf_styled_func) (info->stream,
+						dis_style_register,
+						"cr%d", cr);
+		  (*info->fprintf_styled_func) (info->stream, dis_style_text,
+						"+");
+		}
+
+	      (*info->fprintf_styled_func) (info->stream,
+					    dis_style_sub_mnemonic,
+					    "%s", cbnames[cc]);
 	    }
 	  else
-	    (*info->fprintf_func) (info->stream, "%" PRId64, value);
+	    {
+	      /* An immediate, but what style?  */
+	      enum disassembler_style style;
+
+	      if ((operand->flags & PPC_OPERAND_PARENS) != 0)
+		style = dis_style_address_offset;
+	      else
+		style = dis_style_immediate;
+
+	      (*info->fprintf_styled_func) (info->stream, style,
+					    "%" PRId64, value);
+	    }
 
 	  if (operand->shift == 52)
 	    is_pcrel = value != 0;
@@ -1033,7 +1080,7 @@ print_insn_powerpc (bfd_vma memaddr,
 	    d34 = value;
 
 	  if (op_separator == need_paren)
-	    (*info->fprintf_func) (info->stream, ")");
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text, ")");
 
 	  op_separator = need_comma;
 	  if ((operand->flags & PPC_OPERAND_PARENS) != 0)
@@ -1043,11 +1090,13 @@ print_insn_powerpc (bfd_vma memaddr,
       if (is_pcrel)
 	{
 	  d34 += memaddr;
-	  (*info->fprintf_func) (info->stream, "\t# %" PRIx64, d34);
+	  (*info->fprintf_styled_func) (info->stream,
+					dis_style_comment_start,
+					"\t# %" PRIx64, d34);
 	  asymbol *sym = (*info->symbol_at_address_func) (d34, info);
 	  if (sym)
-	    (*info->fprintf_func) (info->stream, " <%s>",
-				   bfd_asymbol_name (sym));
+	    (*info->fprintf_styled_func) (info->stream, dis_style_text,
+					  " <%s>", bfd_asymbol_name (sym));
 
 	  if (info->private_data != NULL
 	      && info->section != NULL
@@ -1069,11 +1118,19 @@ print_insn_powerpc (bfd_vma memaddr,
 
   /* We could not find a match.  */
   if (insn_length == 4)
-    (*info->fprintf_func) (info->stream, ".long 0x%x",
-			   (unsigned int) insn);
+    (*info->fprintf_styled_func) (info->stream,
+				  dis_style_assembler_directive, ".long");
   else
-    (*info->fprintf_func) (info->stream, ".word 0x%x",
-			   (unsigned int) insn >> 16);
+    {
+      (*info->fprintf_styled_func) (info->stream,
+				    dis_style_assembler_directive, ".word");
+      insn >>= 16;
+    }
+  (*info->fprintf_styled_func) (info->stream, dis_style_text, " ");
+  (*info->fprintf_styled_func) (info->stream, dis_style_immediate, "0x%x",
+				(unsigned int) insn);
+
+
   return insn_length;
 }
 
-- 
2.25.4

Re: [PATCHv2 2/2] libopcodes/ppc: add support for disassembler styling

[Alan Modra]

On Mon, Jul 18, 2022 at 05:31:34PM +0100, Andrew Burgess via Binutils wrote:
> This commit adds disassembler styling to the libopcodes ppc
> disassembler.  This conversion was pretty straight forward, I just
> converted the fprintf_func calls to fprintf_styled_func calls and
> added an appropriate style.
> 
> For testing the new styling I just assembled then disassembled the
> source files in gas/testsuite/gas/ppc and manually checked that the
> styling looked reasonable.
> 
> I think the only slightly weird case was how things like '4*cr1+eq'
> are styled.  As best I can tell, this construct, used for example in
> this instruction:
> 
>   crand   4*cr1+lt,4*cr1+gt,4*cr1+eq
> 
> is used to access a field of a control register.  I initially tried
> styling this whole construct as a register[1], but during review it
> was suggested that instead different parts of the text should have
> different styles.  In this commit I propose styling '4*cr1+lt' like
> this:
> 
>   4    - immediate,
>   *    - text,
>   cr1  - register
>   +    - text
>   lt   - sub-mnemonic

As I said before, I believe "4" should be text.  It is not an
immediate that the user can alter, and is left out entirely when
disassembling if the cr field is cr0.  OK with that change.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCHv2 2/2] libopcodes/ppc: add support for disassembler styling

[Andrew Burgess]

Alan Modra via Binutils <binutils@sourceware.org> writes:

> On Mon, Jul 18, 2022 at 05:31:34PM +0100, Andrew Burgess via Binutils wrote:
>> This commit adds disassembler styling to the libopcodes ppc
>> disassembler.  This conversion was pretty straight forward, I just
>> converted the fprintf_func calls to fprintf_styled_func calls and
>> added an appropriate style.
>> 
>> For testing the new styling I just assembled then disassembled the
>> source files in gas/testsuite/gas/ppc and manually checked that the
>> styling looked reasonable.
>> 
>> I think the only slightly weird case was how things like '4*cr1+eq'
>> are styled.  As best I can tell, this construct, used for example in
>> this instruction:
>> 
>>   crand   4*cr1+lt,4*cr1+gt,4*cr1+eq
>> 
>> is used to access a field of a control register.  I initially tried
>> styling this whole construct as a register[1], but during review it
>> was suggested that instead different parts of the text should have
>> different styles.  In this commit I propose styling '4*cr1+lt' like
>> this:
>> 
>>   4    - immediate,
>>   *    - text,
>>   cr1  - register
>>   +    - text
>>   lt   - sub-mnemonic
>
> As I said before, I believe "4" should be text.  It is not an
> immediate that the user can alter, and is left out entirely when
> disassembling if the cr field is cr0.  OK with that change.

Thanks for the feedback.  I made the change you suggest (the "4" is now
styled as text), and pushed these patches.

Thanks,
Andrew

[PATCH 0/2] AArch64 libopcodes styling

[Andrew Burgess]

This series adds libopcodes based styling to the AArch64 disassembler
output, this enables 'objdump -d --disassembler-color=extended-color'
to work for AArch64.

The first commit is a small refactor, while the second commit actually
adds the styling support.

There's a couple of questions that I still have, which I've detailed
in the commit message of the second patch.

All feedback welcome.

Thanks,
Andrew


---

Andrew Burgess (2):
  opcodes/aarch64: split off creation of comment text in disassembler
  libopcodes/aarch64: add support for disassembler styling

 gas/config/tc-aarch64.c                       |  21 +-
 gas/testsuite/gas/aarch64/alias.d             |  26 +-
 gas/testsuite/gas/aarch64/b_c_1.d             |  42 +-
 gas/testsuite/gas/aarch64/float-fp16.d        |  12 +-
 gas/testsuite/gas/aarch64/int-insns.d         |   4 +-
 gas/testsuite/gas/aarch64/no-aliases.d        |  26 +-
 .../gas/aarch64/programmer-friendly.d         |   2 +-
 include/opcode/aarch64.h                      |  10 +-
 opcodes/aarch64-dis.c                         | 185 +++++++--
 opcodes/aarch64-opc.c                         | 367 +++++++++++++-----
 10 files changed, 504 insertions(+), 191 deletions(-)

-- 
2.25.4

[PATCHv2 0/2] AArch64 libopcodes styling

[Andrew Burgess]

Changes since v1:

  - Original patch #1 has been merged,

  - New patch #1 adds a dis_style_sub_mnemonic style and updates
    objdump to handle this style,

  - Patch #2 has been updated to make use of the new style.

---

Andrew Burgess (2):
  opcodes: add new sub-mnemonic disassembler style
  libopcodes/aarch64: add support for disassembler styling

 binutils/objdump.c       |   2 +
 gas/config/tc-aarch64.c  |  11 +-
 include/dis-asm.h        |   7 +
 include/opcode/aarch64.h |   8 +-
 opcodes/aarch64-dis.c    | 186 ++++++++++++++----
 opcodes/aarch64-opc.c    | 399 ++++++++++++++++++++++++++++-----------
 6 files changed, 472 insertions(+), 141 deletions(-)

-- 
2.25.4

[PATCHv2 1/2] opcodes: add new sub-mnemonic disassembler style

[Andrew Burgess]

When adding libopcodes disassembler styling support for AArch64, it
feels like the results would be improved by having a new sub-mnemonic
style.  This will be used in cases like:

  add    w16, w7, w1, uxtb #2
                      ^^^^----- Here

And:

  cinc   w0, w1, ne
                 ^^----- Here

This commit just adds the new style, and prepares objdump to handle
the style.  A later commit will add AArch64 styling, and will actually
make use of the style.

As this style is currently unused, there should be no user visible
changes after this commit.
---
 binutils/objdump.c | 2 ++
 include/dis-asm.h  | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/binutils/objdump.c b/binutils/objdump.c
index 67824053527..4076587151c 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -2167,6 +2167,7 @@ objdump_color_for_disassembler_style (enum disassembler_style style)
 	{
 	case dis_style_symbol: color = 32; break;
         case dis_style_assembler_directive:
+	case dis_style_sub_mnemonic:
 	case dis_style_mnemonic: color = 33; break;
 	case dis_style_register: color = 34; break;
 	case dis_style_address:
@@ -2185,6 +2186,7 @@ objdump_color_for_disassembler_style (enum disassembler_style style)
 	{
 	case dis_style_symbol: color = 40; break;
         case dis_style_assembler_directive:
+	case dis_style_sub_mnemonic:
 	case dis_style_mnemonic: color = 142; break;
 	case dis_style_register: color = 27; break;
 	case dis_style_address:
diff --git a/include/dis-asm.h b/include/dis-asm.h
index 4f91df12498..f1a83dc84e5 100644
--- a/include/dis-asm.h
+++ b/include/dis-asm.h
@@ -62,6 +62,13 @@ enum disassembler_style
      instructions.  */
   dis_style_mnemonic,
 
+  /* Some architectures include additional mnemonic like fields within the
+     instruction operands, e.g. on aarch64 'add w16, w7, w1, lsl #2' where
+     the 'lsl' is an additional piece of text that describes how the
+     instruction should behave.  This sub-mnemonic style can be used for
+     these pieces of text.  */
+  dis_style_sub_mnemonic,
+
   /* For things that aren't real machine instructions, but rather
      assembler directives, e.g. .byte, etc.  */
   dis_style_assembler_directive,
-- 
2.25.4