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.129.124]) by sourceware.org (Postfix) with ESMTPS id DF51738376C3 for ; Thu, 26 May 2022 12:49:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DF51738376C3 Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-297-CTxp_ZvUN76KJ-dzl1-5NQ-1; Thu, 26 May 2022 08:49:02 -0400 X-MC-Unique: CTxp_ZvUN76KJ-dzl1-5NQ-1 Received: by mail-wm1-f72.google.com with SMTP id m26-20020a05600c3b1a00b00397220d6329so1043710wms.5 for ; Thu, 26 May 2022 05:49:01 -0700 (PDT) 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=hdMbdmXb+EbjUDYmYTqBW2neq5YTBIvz7Ett9XuCQ8M=; b=yQ+IVVFORm+1HAlQj0w9q/emOZL7PG0hMJE59gG77+bOYd2thaH2WTTYM9nzVVYiJ3 mnieTtNCvhFOQEPggHBUlUjfTQvJJSzeybyEXFAPZDXNVYtOiWq8dw3Z8Mbd2eZbnEZx sZBhnGQA/xQjqt/xau1FQ8FB+kgQlJLMDRqNJQchABjDEasHQrwiPmYK0y8C27g0B5cd 5d/s47FUJevoiG8+8Y1zBm62+R6ZsSk6pnxf88ae3Pww8VRGDjSuox36IngAv73xM0RX QRlgYzMDsk3RfRfoXXbwCjzB/GYE8KC7sFzGUEmK6MxESjZbMphCMCQ8hDALcz7BAl1K 8u1Q== X-Gm-Message-State: AOAM530pIn2NcTqwEgLcr1gDWNrXUQQmz/qnDTRwC1LJUEGns3TgtY0u rzPjkFEv7l/skSXc5R8uCiBGvN5oAg5R+AINeIJs4fKM3Eg8ZWArhkGS9IB9Ft3IVoGRA34Azog Ih7LF2gRsFMYnu8IXuw== X-Received: by 2002:a05:600c:6015:b0:397:54e1:8279 with SMTP id az21-20020a05600c601500b0039754e18279mr2193738wmb.100.1653569340685; Thu, 26 May 2022 05:49:00 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwiN7HA3u0GWGGvCbwPsB2tdeXFK6ZaXdTtOa8EHOLiRdCRkYI/ab9NUL+IElkDvJGN7rsrCw== X-Received: by 2002:a05:600c:6015:b0:397:54e1:8279 with SMTP id az21-20020a05600c601500b0039754e18279mr2193724wmb.100.1653569340426; Thu, 26 May 2022 05:49:00 -0700 (PDT) Received: from localhost (host109-152-215-36.range109-152.btcentralplus.com. [109.152.215.36]) by smtp.gmail.com with ESMTPSA id a11-20020a056000188b00b0020fcaba73bcsm1944043wri.104.2022.05.26.05.48.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 May 2022 05:48:59 -0700 (PDT) From: Andrew Burgess To: Jan Beulich Cc: binutils@sourceware.org Subject: Re: [PATCHv2] libopcodes: extend the styling within the i386 disassembler In-Reply-To: <0afe6d6b-08a4-b1b5-80a3-98e3b232dbc5@suse.com> References: <87tu9zkpdy.fsf@redhat.com> <20220509125414.3526554-1-aburgess@redhat.com> <0afe6d6b-08a4-b1b5-80a3-98e3b232dbc5@suse.com> Date: Thu, 26 May 2022 13:48:59 +0100 Message-ID: <874k1cihms.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=-4.5 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.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: Thu, 26 May 2022 12:49:06 -0000 Jan Beulich via Binutils writes: > On 09.05.2022 14:54, Andrew Burgess via Binutils wrote: >> @@ -248,6 +254,8 @@ struct instr_info >> >> enum x86_64_isa isa64; >> >> + int (*printf) (instr_info *ins, enum disassembler_style style, >> + const char *fmt, ...) ATTRIBUTE_FPTR_PRINTF_3; >> }; > > Why do you go through a function pointer? Afaics it's only ever set > to i386_dis_printf(), so I don't see why you couldn't call the > function directly. > >> @@ -9748,24 +9839,28 @@ print_insn (bfd_vma pc, instr_info *ins) >> if (name == NULL) >> abort (); >> prefix_length += strlen (name) + 1; >> - (*ins->info->fprintf_styled_func) >> - (ins->info->stream, dis_style_mnemonic, "%s ", name); >> + ins->printf (ins, dis_style_mnemonic, "%s ", name); >> } >> >> /* Check maximum code length. */ >> if ((ins->codep - ins->start_codep) > MAX_CODE_LENGTH) >> { >> - (*ins->info->fprintf_styled_func) >> - (ins->info->stream, dis_style_text, "(bad)"); >> + ins->printf (ins, dis_style_text, "(bad)"); >> return MAX_CODE_LENGTH; >> } >> >> - ins->obufp = ins->mnemonicendp; >> - for (i = strlen (ins->obuf) + prefix_length; i < 6; i++) >> - oappend (ins, " "); >> - oappend (ins, " "); >> - (*ins->info->fprintf_styled_func) >> - (ins->info->stream, dis_style_mnemonic, "%s", ins->obuf); >> + i = strlen (ins->obuf); >> + if (ins->mnemonicendp == ins->obuf + i) > > What is this condition for? It doesn't look to match any of what the > original code does. In particular it's unclear to me ... > >> + { >> + i += prefix_length; >> + if (i < 6) >> + i = 6 - i + 1; >> + else >> + i = 1; >> + } >> + else >> + i = 0; > > ... what this "else" would cover. This whole nonsense was a convoluted method of maintaining compatibility with the existing disassembler when it comes to emitting trailing whitespace. I've now posted this separate patch: https://sourceware.org/pipermail/binutils/2022-May/121054.html which fixes what I think are some inconsistencies in how the existing disassembler handles whitespace. With that patch merged this whole hunk will disappear from this patch. I'm in the process of addressing the remaining points that you and H.J. have raised. Thanks, Andrew > >> @@ -10224,8 +10314,11 @@ static void >> OP_STi (instr_info *ins, int bytemode ATTRIBUTE_UNUSED, >> int sizeflag ATTRIBUTE_UNUSED) >> { >> - sprintf (ins->scratchbuf, "%%st(%d)", ins->modrm.rm); >> - oappend_maybe_intel (ins, ins->scratchbuf); >> + oappend_maybe_intel (ins, "%st"); >> + oappend (ins, "("); > > Any reason these last two aren't simply > > oappend_maybe_intel (ins, "%st("); > > ? > >> + sprintf (ins->scratchbuf, "%d", ins->modrm.rm); >> + oappend_with_style (ins, ins->scratchbuf, dis_style_immediate); > > This is not an immediate. The entire %st(N) is a register name (like > anything that starts with % in AT&T mode). > >> @@ -10772,12 +10865,64 @@ putop (instr_info *ins, const char *in_template, int sizeflag) >> return 0; >> } >> >> +/* Add a style marker to *INS->obufp that encodes STYLE. This assumes that >> + the buffer pointed to by INS->obufp has space. A style marker is made >> + from the STYLE_MARKER_CHAR followed by STYLE converted to a single hex >> + digit, followed by another STYLE_MARKER_CHAR. This function assumes >> + that the number of styles is not greater than 16. */ >> + >> static void >> -oappend (instr_info *ins, const char *s) >> +oappend_insert_style (instr_info *ins, enum disassembler_style style) >> { >> + int num = (int) style; >> + >> + /* We currently assume that STYLE can be encoded as a single hex >> + character. If more styles are added then this might start to fail, >> + and we'll need to expand this code. */ >> + if (num > 0xf) >> + abort (); > > You want to either also check for negative values or make "num" unsigned. > >> @@ -10789,26 +10934,27 @@ append_seg (instr_info *ins) >> switch (ins->active_seg_prefix) >> { >> case PREFIX_CS: >> - oappend_maybe_intel (ins, "%cs:"); >> + oappend_maybe_intel_with_style (ins, "%cs", dis_style_register); > > I was about to ask why dis_style_register needs specifying here, but I > notice the comment ahead of the function is misleading. There also are > cases where a leading '$' would be skipped. I wonder though whether it > wouldn't yield better readable code if those used a separate function, > thus eliminating the need for the explicit style parameter. E.g. > oappend_register() and oappend_immediate(). The "maybe_intel" part of > the name isn't really useful imo. > >> @@ -13352,7 +13502,7 @@ OP_VexI4 (instr_info *ins, int bytemode ATTRIBUTE_UNUSED, >> { >> ins->scratchbuf[0] = '$'; >> print_operand_value (ins, ins->scratchbuf + 1, 1, ins->codep[-1] & 0xf); >> - oappend_maybe_intel (ins, ins->scratchbuf); >> + oappend_maybe_intel_with_style (ins, ins->scratchbuf, dis_style_text); >> } >> >> static void >> @@ -13397,7 +13547,7 @@ VPCMP_Fixup (instr_info *ins, int bytemode ATTRIBUTE_UNUSED, >> /* We have a reserved extension byte. Output it directly. */ >> ins->scratchbuf[0] = '$'; >> print_operand_value (ins, ins->scratchbuf + 1, 1, cmp_type); >> - oappend_maybe_intel (ins, ins->scratchbuf); >> + oappend_maybe_intel_with_style (ins, ins->scratchbuf, dis_style_text); >> ins->scratchbuf[0] = '\0'; >> } >> } >> @@ -13449,7 +13599,7 @@ VPCOM_Fixup (instr_info *ins, int bytemode ATTRIBUTE_UNUSED, >> /* We have a reserved extension byte. Output it directly. */ >> ins->scratchbuf[0] = '$'; >> print_operand_value (ins, ins->scratchbuf + 1, 1, cmp_type); >> - oappend_maybe_intel (ins, ins->scratchbuf); >> + oappend_maybe_intel_with_style (ins, ins->scratchbuf, dis_style_text); >> ins->scratchbuf[0] = '\0'; >> } >> } > > Why "text" for these three immediates, but ... > >> @@ -13497,7 +13647,8 @@ PCLMUL_Fixup (instr_info *ins, int bytemode ATTRIBUTE_UNUSED, >> /* We have a reserved extension byte. Output it directly. */ >> ins->scratchbuf[0] = '$'; >> print_operand_value (ins, ins->scratchbuf + 1, 1, pclmul_type); >> - oappend_maybe_intel (ins, ins->scratchbuf); >> + oappend_maybe_intel_with_style (ins, ins->scratchbuf, >> + dis_style_immediate); >> ins->scratchbuf[0] = '\0'; >> } >> } > > ... "immediate" here? > > Jan