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

[Jan Beulich <jbeulich@suse.com>]

On 19.02.2022 11:54, Andrew Burgess wrote:
> Jan Beulich via Binutils <binutils@sourceware.org> writes:
> 
>> On 17.02.2022 23:37, Andrew Burgess wrote:
>>> Jan Beulich via Binutils <binutils@sourceware.org> writes:
>>>> On 17.02.2022 17:15, Andrew Burgess wrote:
>>>>> Jan Beulich via Binutils <binutils@sourceware.org> writes:
>>>>>> On 16.02.2022 21:53, Andrew Burgess via Binutils wrote:
>>>>>>> +	      (*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?
>>>>
>>>> Well, an operand to a directive for example is not an immediate imo,
>>>> yes. A "load offset" (as your comment calls it) may also not be an
>>>> immediate. E.g. in x86 memory access instructions:
>>>>
>>>> 	mov	0x10(%rbx), %eax
>>>>
>>>> the 0x10 isn't an immediate, but a displacement. The difference may
>>>> be more relevant in something like
>>>>
>>>> 	mov	$0, 0x10(%rbx)
>>>>
>>>> where the $0 is an immediate operand, but the 0x10 isn't (and you
>>>> wouldn't want to mix the two).
>>>>
>>>> From that comment it's not clear to me where else you would think
>>>> "immediate" applies (or not), but in RISC-V's
>>>>
>>>> 	lw	x0, 0x10(x0)
>>>>
>>>> I wouldn't consider the 0x10 an immediate either, albeit this may
>>>> be a result of my x86 bias.
>>>
>>> I wonder if there's a name we could come up with that would allow me to
>>> classify the '$0' and '0x10' (in your example above) as the same style?
>>>
>>> I've kind-of lost the thread a bit, but maybe that's what the 'number'
>>> you suggested original was for?  If I replaced dis_style_immediate with
>>> dis_style_number, and just replaced thoughout, would that be less
>>> problematic?
>>
>> Yes, "number" was meant to be a possible replacement. Whether it's
>> helpful to style all forms of numbers the same is questionable
>> though. Note that to me in "$0" the '$' would not be covered by
>> "number" then, while it would be covered by "immediate".
>>
>>> Another possibility would be to have some aliases either in the original
>>> enum, as in:
>>>
>>>   dis_style_displacement = dis_style_immediate,
>>>
>>> or even at the top of i386-dis.c, as in:
>>>
>>>   #define dis_style_displacement dis_style_immediate
>>
>> But that's wrong. I'm not primarily after the naming in the sources,
>> but after the output not showing distinct things as similar.
>>
>>> I really think we should avoid adding too many distinct styles if we
>>> can.  My concern is less about disassembler users handling the different
>>> styles, and more about consistency between the disassemblers.  I figure
>>> it's easier to be consistent if we only have a small number of styles.
>>> If displacement is a different style to other immediate (yes, I'm still
>>> going to call numbers in instruction immediates!) then we end up with
>>> some architectures going one way, and others another.
>>
>> I agree with the goal of limiting the number of styles. But instead
>> of marking distinct items with similar non-basic (text) styles, I'd
>> rather see items left alone then. IOW with dis_style_immediate I'd
>> prefer (in the x86 example) to see only true immediate insn operands
>> be tagged that way, and all other numbers to remain dis_style_text.
> 
> Having reviewed the thread, I've tried to come up with the complete list
> of styles that I believe your are arguing for.  These incude a new
> directive style, and an address_offset style.  I've extended the text in
> several places to cover how to handle prefixes, as well as how styles
> should be used for directives.
> 
> I'd be grateful if you could read through this list, and give any
> examples of things which, if styled using the rules below, you feel
> would be unacceptable.

I think this all looks good (but of course once this can actually
be seen in use, more things may pop up), provided ...

>   /* This is the default style, use this for any additional syntax
>      (e.g. commas between operands, brackets, etc), or just as a default if
>      no other style seems appropriate.  */
>   dis_style_text,
> 
>   /* Use this for all instruction mnemonics, or aliases for mnemonics.
>      These should be things that correspond to real machine
>      instructions.  */
>   dis_style_mnemonic,
> 
>   /* For things that aren't real machine instructions, but rather
>      assembler directives, e.g. .byte, etc.  */
>   dis_style_assembler_directive,
> 
>   /* Use this for any register names.  This may or may-not include any
>      register prefix, e.g. '$', '%', at the discretion of the target,
>      though within each target the choice to include prefixes for not
>      should be kept consistent.  If the prefix is not printed with this
>      style, then dis_style_text should be used.  */
>   dis_style_register,
> 
>   /* Use this for any constant values used within instructions or
>      directives, unless the value is an absolute address, or an offset
>      that will be added to an address (no matter where the address comes
>      from) before use.  This style may, or may-not be used for any
>      prefix to the immediate value, e.g. '$', at the discretion of the
>      target, though within each target the choice to include these
>      prefixes should be kept consistent.  */
>   dis_style_immediate,
> 
>   /* The style for the numerical representation of an absolute address.
>      Anything that is an address offset should use the immediate style.
>      This style may, or may-not be used for any prefix to the immediate
>      value, e.g. '$', at the discretion of the target, though within
>      each target the choice to include these prefixes should be kept
>      consistent.  */
>   dis_style_address,
> 
>   /* The style for any constant value within an instruction or directive
>      that represents an offset that will be added to an address before
>      use.  This style may, or may-not be used for any prefix to the
>      immediate value, e.g. '$', at the discretion of the target, though
>      within each target the choice to include these prefixes should be
>      kept consistent.  */
>   dis_style_address_offset,

... it was more a copy-n-paste mistake to repeat the reference to $ etc
in these latter two? Or is this to cover e.g. Arm prefixing numbers by
# in a wider fashion than x86's use of $? In this case, using # as the
example character may avoid some confusion.

>   /* The style for a symbol's name.  The numerical address of a symbol
>      should use the address style above, this style is reserved for the
>      name.  */
>   dis_style_symbol,

There may be a remaining ambiguity here: What is the intended style to
be used for <symbol>+<offset>? Just dis_style_symbol or first
dis_style_symbol and then dis_style_address_offset?

Jan