From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 5C9573858D20 for ; Thu, 17 Feb 2022 16:15:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5C9573858D20 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-198-uS7_fEiWMZOa-DhhgrT85g-1; Thu, 17 Feb 2022 11:15:57 -0500 X-MC-Unique: uS7_fEiWMZOa-DhhgrT85g-1 Received: by mail-wr1-f70.google.com with SMTP id g11-20020adfa48b000000b001e57dfb3c38so2495928wrb.2 for ; Thu, 17 Feb 2022 08:15:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=Engyrs0NYaNQ7rPf12nNbUWRWh3T9r2jPhgunW4GyQE=; b=njN8POydvoCodSkaBXZyxjWO6oXYDPcm/COt/DYg8WWwEAcZSkvxuLk0RSz+5ncVP9 pTDU/XiaH2nRdbdCS/LQyh4qHdXQK8lb6G2L/qWN5vpLXguY+jP4/ydUGzIMQNeSxLDW 2nMfsof1aVM/nJ6da9j3z3GzciIyrRd8H1ROYCssuuCU8uuqjTYQGPMpoNLo0+sfbbv2 Ul1HJKBotnKjygoqG+f2D54Qc+wRS6GN5GVoWhl5MkduaSOdADymmoMbHzf9fLGSsTit +n46y34XD1W46CxY3jDQmnkNypnABmS3SplrUKbmW7JzJVKMok3GzRBVT7gGMHrUdvxO 6NgQ== X-Gm-Message-State: AOAM533Rc67MnRNMpPwxg9Gg8iNetXMCAosCGtUUn0u2a0mCT01lDop4 Fy8P0/V2rqbt3q+mn5Rk54GgF7BFcaT/gGxvsTjeVG2qwotvkXQvxSHeJ8KhGqAZVM0CUqjpSeg hVwiMH+sIDlOAYLMQQw== X-Received: by 2002:adf:bc14:0:b0:1e2:b035:9c46 with SMTP id s20-20020adfbc14000000b001e2b0359c46mr2854111wrg.386.1645114556138; Thu, 17 Feb 2022 08:15:56 -0800 (PST) X-Google-Smtp-Source: ABdhPJyxcpc+WsgFFUpjPE9R6f8QP2t5BJ0FTym6lNwjG61fuqupxrYueF0xeuhPV9SRxclLnrZUwQ== X-Received: by 2002:adf:bc14:0:b0:1e2:b035:9c46 with SMTP id s20-20020adfbc14000000b001e2b0359c46mr2854096wrg.386.1645114555813; Thu, 17 Feb 2022 08:15:55 -0800 (PST) Received: from localhost (host86-134-151-224.range86-134.btcentralplus.com. [86.134.151.224]) by smtp.gmail.com with ESMTPSA id h17sm1632867wmm.15.2022.02.17.08.15.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Feb 2022 08:15:55 -0800 (PST) From: Andrew Burgess To: Jan Beulich Cc: binutils@sourceware.org Subject: Re: [PATCH 3/3] opcodes/i386: partially implement disassembler style support In-Reply-To: <37ba6375-00cf-3041-ee79-21377558402c@suse.com> References: <11996f886e69218629abf81f8041269e8740a60e.1645043588.git.aburgess@redhat.com> <37ba6375-00cf-3041-ee79-21377558402c@suse.com> Date: Thu, 17 Feb 2022 16:15:54 +0000 Message-ID: <87o835eaf9.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Thu, 17 Feb 2022 16:16:01 -0000 Jan Beulich via Binutils 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