Re: [PATCHv2] libopcodes: extend the styling within the i386 disassembler

[Jan Beulich <jbeulich@suse.com>]

On 09.05.2022 14:54, Andrew Burgess via Binutils wrote:
> @@ -248,6 +254,8 @@ struct instr_info
>  
>    enum x86_64_isa isa64;
>  
> +  int (*printf) (instr_info *ins, enum disassembler_style style,
> +		 const char *fmt, ...) ATTRIBUTE_FPTR_PRINTF_3;
>  };

Why do you go through a function pointer? Afaics it's only ever set
to i386_dis_printf(), so I don't see why you couldn't call the
function directly.

> @@ -9748,24 +9839,28 @@ print_insn (bfd_vma pc, instr_info *ins)
>  	if (name == NULL)
>  	  abort ();
>  	prefix_length += strlen (name) + 1;
> -	(*ins->info->fprintf_styled_func)
> -	  (ins->info->stream, dis_style_mnemonic, "%s ", name);
> +	ins->printf (ins, dis_style_mnemonic, "%s ", name);
>        }
>  
>    /* Check maximum code length.  */
>    if ((ins->codep - ins->start_codep) > MAX_CODE_LENGTH)
>      {
> -      (*ins->info->fprintf_styled_func)
> -	(ins->info->stream, dis_style_text, "(bad)");
> +      ins->printf (ins, dis_style_text, "(bad)");
>        return MAX_CODE_LENGTH;
>      }
>  
> -  ins->obufp = ins->mnemonicendp;
> -  for (i = strlen (ins->obuf) + prefix_length; i < 6; i++)
> -    oappend (ins, " ");
> -  oappend (ins, " ");
> -  (*ins->info->fprintf_styled_func)
> -    (ins->info->stream, dis_style_mnemonic, "%s", ins->obuf);
> +  i = strlen (ins->obuf);
> +  if (ins->mnemonicendp == ins->obuf + i)

What is this condition for? It doesn't look to match any of what the
original code does. In particular it's unclear to me ...

> +    {
> +      i += prefix_length;
> +      if (i < 6)
> +	i = 6 - i + 1;
> +      else
> +	i = 1;
> +    }
> +  else
> +    i = 0;

... what this "else" would cover.

> @@ -10224,8 +10314,11 @@ static void
>  OP_STi (instr_info *ins, int bytemode ATTRIBUTE_UNUSED,
>  	int sizeflag ATTRIBUTE_UNUSED)
>  {
> -  sprintf (ins->scratchbuf, "%%st(%d)", ins->modrm.rm);
> -  oappend_maybe_intel (ins, ins->scratchbuf);
> +  oappend_maybe_intel (ins, "%st");
> +  oappend (ins, "(");

Any reason these last two aren't simply

  oappend_maybe_intel (ins, "%st(");

?

> +  sprintf (ins->scratchbuf, "%d", ins->modrm.rm);
> +  oappend_with_style (ins, ins->scratchbuf, dis_style_immediate);

This is not an immediate. The entire %st(N) is a register name (like
anything that starts with % in AT&T mode).

> @@ -10772,12 +10865,64 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
>    return 0;
>  }
>  
> +/* Add a style marker to *INS->obufp that encodes STYLE.  This assumes that
> +   the buffer pointed to by INS->obufp has space.  A style marker is made
> +   from the STYLE_MARKER_CHAR followed by STYLE converted to a single hex
> +   digit, followed by another STYLE_MARKER_CHAR.  This function assumes
> +   that the number of styles is not greater than 16.  */
> +
>  static void
> -oappend (instr_info *ins, const char *s)
> +oappend_insert_style (instr_info *ins, enum disassembler_style style)
>  {
> +  int num = (int) style;
> +
> +  /* We currently assume that STYLE can be encoded as a single hex
> +     character.  If more styles are added then this might start to fail,
> +     and we'll need to expand this code.  */
> +  if (num > 0xf)
> +    abort ();

You want to either also check for negative values or make "num" unsigned.

> @@ -10789,26 +10934,27 @@ append_seg (instr_info *ins)
>    switch (ins->active_seg_prefix)
>      {
>      case PREFIX_CS:
> -      oappend_maybe_intel (ins, "%cs:");
> +      oappend_maybe_intel_with_style (ins, "%cs", dis_style_register);

I was about to ask why dis_style_register needs specifying here, but I
notice the comment ahead of the function is misleading. There also are
cases where a leading '$' would be skipped. I wonder though whether it
wouldn't yield better readable code if those used a separate function,
thus eliminating the need for the explicit style parameter. E.g.
oappend_register() and oappend_immediate(). The "maybe_intel" part of
the name isn't really useful imo.

> @@ -13352,7 +13502,7 @@ OP_VexI4 (instr_info *ins, int bytemode ATTRIBUTE_UNUSED,
>  {
>    ins->scratchbuf[0] = '$';
>    print_operand_value (ins, ins->scratchbuf + 1, 1, ins->codep[-1] & 0xf);
> -  oappend_maybe_intel (ins, ins->scratchbuf);
> +  oappend_maybe_intel_with_style (ins, ins->scratchbuf, dis_style_text);
>  }
>  
>  static void
> @@ -13397,7 +13547,7 @@ VPCMP_Fixup (instr_info *ins, int bytemode ATTRIBUTE_UNUSED,
>        /* We have a reserved extension byte.  Output it directly.  */
>        ins->scratchbuf[0] = '$';
>        print_operand_value (ins, ins->scratchbuf + 1, 1, cmp_type);
> -      oappend_maybe_intel (ins, ins->scratchbuf);
> +      oappend_maybe_intel_with_style (ins, ins->scratchbuf, dis_style_text);
>        ins->scratchbuf[0] = '\0';
>      }
>  }
> @@ -13449,7 +13599,7 @@ VPCOM_Fixup (instr_info *ins, int bytemode ATTRIBUTE_UNUSED,
>        /* We have a reserved extension byte.  Output it directly.  */
>        ins->scratchbuf[0] = '$';
>        print_operand_value (ins, ins->scratchbuf + 1, 1, cmp_type);
> -      oappend_maybe_intel (ins, ins->scratchbuf);
> +      oappend_maybe_intel_with_style (ins, ins->scratchbuf, dis_style_text);
>        ins->scratchbuf[0] = '\0';
>      }
>  }

Why "text" for these three immediates, but ...

> @@ -13497,7 +13647,8 @@ PCLMUL_Fixup (instr_info *ins, int bytemode ATTRIBUTE_UNUSED,
>        /* We have a reserved extension byte.  Output it directly.  */
>        ins->scratchbuf[0] = '$';
>        print_operand_value (ins, ins->scratchbuf + 1, 1, pclmul_type);
> -      oappend_maybe_intel (ins, ins->scratchbuf);
> +      oappend_maybe_intel_with_style (ins, ins->scratchbuf,
> +				      dis_style_immediate);
>        ins->scratchbuf[0] = '\0';
>      }
>  }

... "immediate" here?

Jan