From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) by sourceware.org (Postfix) with ESMTPS id 12E99382FC26 for ; Wed, 1 Jun 2022 15:57:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 12E99382FC26 Received: by mail-pf1-x436.google.com with SMTP id u2so2382700pfc.2 for ; Wed, 01 Jun 2022 08:57:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vhaPbDGgO9hARBrEaTX9mP/aJlk239GZdlSejjmjfi4=; b=bnmbyQoodA5JYcE0UviHbH7UlpncxcTW/m7ygKo+Mr4YrzoRJ+YeCur5y+58f2rQj1 lKzjxwVUGqrfUEY0AT7K5rCkbpLnxeupNx7RnF5e3AuUtSb4viQMZBuhldamapERUYSA ZCIPptXGNCCjnsbPaU1RU1ulDgOaG7GRviDvLNGeEcOGpdIGl6lvUzto0+DHP7oGOnpq eW43VJ/dJ7/xuNsbTDuYfDNb6jxPcdcFsrSDKUQQT1kR0EbRyEk16jevkwSTn09PC3A1 j1WGCN0H5wem50RYGwKlcxgNPM0TfMXqUsay9T9MMjsC8vzLJv2byDhd5dMz2ui44jM2 3mwQ== X-Gm-Message-State: AOAM53378OtZTIGax9rGC6ZxE2h0kx0Cgosp9jGrvlcqPFNjOcTvGJgX VzA9G7/i1JwrRcCMEQK7jTLJuEe5cmFYTVshTn6X4iz7 X-Google-Smtp-Source: ABdhPJzQDZdMetrgVZE8mY9LUlh5a4rte6cGai47Kf0RxcMJd2NoMNQvy2kWNiZif5HnALpcROjyUcL4f6dL/zcDOX0= X-Received: by 2002:a65:63d9:0:b0:374:6b38:c6b3 with SMTP id n25-20020a6563d9000000b003746b38c6b3mr73908pgv.195.1654099029081; Wed, 01 Jun 2022 08:57:09 -0700 (PDT) MIME-Version: 1.0 References: <20220509125414.3526554-1-aburgess@redhat.com> <20220527174443.223739-1-aburgess@redhat.com> <87bkvdd3g1.fsf@redhat.com> In-Reply-To: From: "H.J. Lu" Date: Wed, 1 Jun 2022 08:56:33 -0700 Message-ID: Subject: Re: [PATCHv3] libopcodes: extend the styling within the i386 disassembler To: Jan Beulich Cc: Andrew Burgess , Binutils Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3019.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Jun 2022 15:57:11 -0000 On Tue, May 31, 2022 at 10:59 PM Jan Beulich wrote: > > On 31.05.2022 19:20, Andrew Burgess wrote: > > Jan Beulich via Binutils 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.