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

[Jan Beulich <jbeulich@suse.com>]

On 31.05.2022 19:20, Andrew Burgess wrote:
> Jan Beulich via Binutils <binutils@sourceware.org> writes:
>> On 27.05.2022 19:44, Andrew Burgess via Binutils wrote:
>>> @@ -9299,11 +9304,117 @@ get_sib (instr_info *ins, int sizeflag)
>>>  }
>>>  
>>>  /* Like oappend (below), but S is a string starting with '%'.
>>> -   In Intel syntax, the '%' is elided.  */
>>> +   In Intel syntax, the '%' is elided.  STYLE is used when displaying this
>>> +   part of the output in the disassembler.
>>
>> As you're touching this comment anyway, can you add reference to
>> '$'?
> 
> Done.
> 
>>      Or alternatively (that's what I was envisioning with the
>> comment on v2) drop this function altogether, doing what it does
>> separately in oappend_register() and oappend_immediate()?
> 
> I didn't do this (though I will if you insist), I'd just prefer to keep
> the "magic" for how we handle the intel syntax (character skipping) in a
> single place.

I won't insist; I may do this subsequently though in a follow-on
patch.

>>> @@ -9404,8 +9515,7 @@ print_insn (bfd_vma pc, instr_info *ins)
>>>  
>>>    if (ins->address_mode == mode_64bit && sizeof (bfd_vma) < 8)
>>>      {
>>> -      (*ins->info->fprintf_styled_func) (ins->info->stream, dis_style_text,
>>> -					 _("64-bit address is disabled"));
>>> +      i386_dis_printf (ins, dis_style_text, _("64-bit address is disabled"));
>>
>> Just wondering: Couldn't there be an "error" style?
> 
> I've avoided an error style because I don't think the disassembler
> _should_ be emitting errors like this.
> 
> I'll go so far as to say that I consider this case a bug in the i386
> disassembler.
> 
> IMHO, if we pass some content to the disassembler then it should
> disassemble it to something, that might just be .word or .byte
> directives rather than real instructions, but we should disassemble to
> something.
> 
> In the above, isn't the "error" really just a reflection that the
> disassembler has been written using bfd_vma in places where either
> uint64_t or int64_t would have been a better choice?
> 
> If we did decide that the assembler should be able to handle errors
> other than memory errors, I think the correct solution would be to
> either add (yet) another callback which is like the memory error
> callback, but for different errors.  Or, modify the existing error
> callback to handle different types of error maybe....
> 
> ... anyway, I don't think we should do that, but I don't think we should
> add an error style either as I feel it will just encourage bad behaviour
> when writing the disassemblers.

That's certainly a fair view to have, albeit I'm not sure I fully
share it. In some cases I consider it more helpful for the
disassembler to at least provide a hint at what's wrong in a
given encoding.

> Patch below includes the updates you asked for above.

Thanks, lgtm. It'll want to be H.J. though to approve of this going
in.

Jan