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

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, May 31, 2022 at 10:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> 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.

OK.

Thanks.

-- 
H.J.