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

[Andrew Burgess <aburgess@redhat.com>]

Jan Beulich via Binutils <binutils@sourceware.org> writes:

> 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.

This whole nonsense was a convoluted method of maintaining compatibility
with the existing disassembler when it comes to emitting trailing
whitespace.

I've now posted this separate patch:

  https://sourceware.org/pipermail/binutils/2022-May/121054.html

which fixes what I think are some inconsistencies in how the existing
disassembler handles whitespace.

With that patch merged this whole hunk will disappear from this patch.

I'm in the process of addressing the remaining points that you and
H.J. have raised.

Thanks,
Andrew


>
>> @@ -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