Re: [PATCH 3/3] opcodes/i386: partially implement disassembler style support

[Andrew Burgess <aburgess@redhat.com>]

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

> On 16.02.2022 21:53, Andrew Burgess via Binutils wrote:
>> @@ -9456,12 +9456,18 @@ print_insn (bfd_vma pc, instr_info *ins)
>>  	{
>>  	  name = prefix_name (ins, priv.the_buffer[0], priv.orig_sizeflag);
>>  	  if (name != NULL)
>> -	    (*ins->info->fprintf_func) (ins->info->stream, "%s", name);
>> +	    (*ins->info->fprintf_styled_func)
>> +	      (ins->info->stream, dis_style_mnemonic, "%s", name);
>>  	  else
>>  	    {
>>  	      /* Just print the first byte as a .byte instruction.  */
>> -	      (*ins->info->fprintf_func) (ins->info->stream, ".byte 0x%x",
>> -				     (unsigned int) priv.the_buffer[0]);
>> +	      (*ins->info->fprintf_styled_func)
>> +		(ins->info->stream, dis_style_mnemonic, ".byte");
>
> Perhaps better have dis_style_directive for this? It's certainly not
> an insn mnemonic.

Are you suggesting directive in addition to mnemonic?  Or as a
replacement for?

My goal with the style list was to try and keep the number of styles
pretty small, an instrution mnemonic like 'add' and a directive like
'.byte' seemed to have a similar enough function that styling them
identically felt OK.

>
>> +	      (*ins->info->fprintf_styled_func)
>> +		(ins->info->stream, dis_style_text, " ");
>> +	      (*ins->info->fprintf_styled_func)
>> +		(ins->info->stream, dis_style_immediate, "0x%x",
>> +		 (unsigned int) priv.the_buffer[0]);
>
> I wonder if the naming (dis_style_immediate) isn't misleading. As per
> the comment next to its definition it really appears to mean any kind
> of number (like is the case here), not just immediate operands of
> instructions. Hence maybe dis_style_number (as replacement for or in
> addition to dis_style_immediate)?

You mentioned this before in the previous thread, and I didn't really
understand then either.

Can you give an example of something that's a number, but not an
immediate?  e.g. I wonder (given the instruction/directive distinction
you draw above), I wonder if you're conserned about: '.byte 0x4', maybe
you don't like referring to this 0x4 here as an immediate?

>
>> @@ -9497,10 +9503,15 @@ print_insn (bfd_vma pc, instr_info *ins)
>>        /* Handle ins->prefixes before fwait.  */
>>        for (i = 0; i < ins->fwait_prefix && ins->all_prefixes[i];
>>  	   i++)
>> -	(*ins->info->fprintf_func) (ins->info->stream, "%s ",
>> -				    prefix_name (ins, ins->all_prefixes[i],
>> -						 sizeflag));
>> -      (*ins->info->fprintf_func) (ins->info->stream, "fwait");
>> +	{
>> +	  (*ins->info->fprintf_styled_func)
>> +	    (ins->info->stream, dis_style_mnemonic, "%s",
>> +	     prefix_name (ins, ins->all_prefixes[i], sizeflag));
>> +	  (*ins->info->fprintf_styled_func)
>> +	    (ins->info->stream, dis_style_mnemonic, " ");
>
> Does the style matter for blanks? If so, why "mnemonic" here, but ...
>
>> @@ -9744,13 +9759,17 @@ print_insn (bfd_vma pc, instr_info *ins)
>>  	if (name == NULL)
>>  	  abort ();
>>  	prefix_length += strlen (name) + 1;
>> -	(*ins->info->fprintf_func) (ins->info->stream, "%s ", name);
>> +	(*ins->info->fprintf_styled_func)
>> +	  (ins->info->stream, dis_style_mnemonic, "%s", name);
>> +	(*ins->info->fprintf_styled_func)
>> +	  (ins->info->stream, dis_style_text, " ");
>
> ... "text" here? If the style didn't matter, a single call (as it was
> before) would seem to suffice in both cases.

No the style doesn't really matter.  I guess it might be possible that a
user could choose to style the foreground colour, in which case these
two spaces would appear different... I'd be tempted to say white space
should be printed with 'text' style.  I'll take another pass though this
patch and clean this up.

Thanks,
Andrew