From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from de-smtp-delivery-102.mimecast.com (de-smtp-delivery-102.mimecast.com [194.104.109.102]) by sourceware.org (Postfix) with ESMTPS id 7F5483856DED for ; Wed, 18 May 2022 12:27:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7F5483856DED Received: from EUR05-AM6-obe.outbound.protection.outlook.com (mail-am6eur05lp2108.outbound.protection.outlook.com [104.47.18.108]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id de-mta-3-cj3pMK-1OXG8xXyuxeF8mw-1; Wed, 18 May 2022 14:27:24 +0200 X-MC-Unique: cj3pMK-1OXG8xXyuxeF8mw-1 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Yz7GR7f6Gu+BNPdhtosorZl87ZtqlejadkIgMaW5QFa1gX0aPNOt/MCMIGShEfM8seNaoEXh3X4yY6lJZ5udNPDnlW3ATT/aPI8CQVCPrpC3PaVvcYE+TQT59+McZ9HpGxedTbLOB//RhpE9RD5lWaJoEecADaDZgcWfKNEET+AEwxWVleiopVmY0F+bEWGca9DwNR/jDDvj+Xp5ksqQUd+3ifzW/3caSsItVNhPONaVcbpGtYRSIqXzd6Wve1rqHdiIYLtbRLZHRcPDfr8aT1R/tYNUVCrj+K5AA8kdZFw4QuXrJ9gwZo0wIpCpkHVoSzOuEwz2NHVxQRRZRotceA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=N5vbzELGb31Q/7ieIBBZrKdRdTtUAzfhhI9iDbW1aW0=; b=AM7s7+A15FP24eRuAfuDr1xwOm/G174ngIFLNg9G7IKqvBv2P0EKOaWmwyvGbJc1eqKYc6+MKFgYeNouwdxsedEZCTifVGEaBv5BF4k44RXg7E0m9J13ca9+tD31sc/2F3mptWlRKuYnTdsNb9PJHFbqgV0vqRqcOouebXjYFZRTlW7f4Gs60fViFRi9XCpI8lmKYS7A0IgHb0cVKzCRw5T9BN24uTe9lgb5/fUSUeQZj9nGHpK+C4tM5cqEBQf4ik/74wd6iC6I/JXVM0MiloVHu2MCets/FJZ3Tn89SD7haVWYdr+cs1XCuRdZGKy6ulzf2lcVyHw6iF9Vh92/kA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none Received: from VE1PR04MB6560.eurprd04.prod.outlook.com (2603:10a6:803:122::25) by AM0PR04MB5076.eurprd04.prod.outlook.com (2603:10a6:208:c0::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5273.14; Wed, 18 May 2022 12:27:22 +0000 Received: from VE1PR04MB6560.eurprd04.prod.outlook.com ([fe80::91b8:8f7f:61ac:cc9b]) by VE1PR04MB6560.eurprd04.prod.outlook.com ([fe80::91b8:8f7f:61ac:cc9b%7]) with mapi id 15.20.5273.014; Wed, 18 May 2022 12:27:22 +0000 Message-ID: <0afe6d6b-08a4-b1b5-80a3-98e3b232dbc5@suse.com> Date: Wed, 18 May 2022 14:27:20 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCHv2] libopcodes: extend the styling within the i386 disassembler Content-Language: en-US To: Andrew Burgess References: <87tu9zkpdy.fsf@redhat.com> <20220509125414.3526554-1-aburgess@redhat.com> Cc: binutils@sourceware.org From: Jan Beulich In-Reply-To: <20220509125414.3526554-1-aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: FR0P281CA0059.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:49::7) To VE1PR04MB6560.eurprd04.prod.outlook.com (2603:10a6:803:122::25) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 2bd04c39-720f-4217-4b91-08da38c9c242 X-MS-TrafficTypeDiagnostic: AM0PR04MB5076:EE_ X-Microsoft-Antispam-PRVS: X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: Liu3oI+lfP3MtH8+MDIIWvfHivozRA3vubcYfaf39KfbphVQYoHFDJzuxx2tR7Rz28HMPeJvX87fIal9zRc9sTBwAvNbQCeMwOZ3MnzS1zVZyeiOj2BIZIIlWxJKKu7clB+X5wqmIuy2gyrQOP8RB5fBVMe4pWjroEwf6hUjKc96fG84h8cXTwnHlYbpYGvAFt487RIf9Qv6gFZMuL/UxrDaQ1v9DLWqFncuLtuj4dJFOKdotVtrfSsH3WXnyIDAULOO/6I4NigAalGZ/DPUUO4yoC1a3rClhyTb/SFFqU/FXkxVuTTuH5dnL4IvYfAUFA+N9EXfCB+DocuPMQp1E7T2uXxxFh+E06wsm0l2m5oDb6eNGidei0trfcnoL83yLh9VZs4eAyegmFPU1SVYHkuZBwRvbjPDL3Lmd7eSBXEBYv+BV6X+JYW8ZVcyqAK2+nVgnAuwMhOruyiYXo64xCqlgxhx0mpAT1Xm5b7DLCktn/E/+Igxn11pDW9CdB42iQ10x49ESfj30F9xw3Ds2P4Gu80rurJY/apyv+Hy5TM9rdjUyg7otKFMGhyjEgxe1yB5C6/X5xE52CalOUrfH1bpJmtprJjKotuDzb3Uc+XYnV4N9yGDgZ+EVYFiDCvHig/d7iB3IUMPiCi0yfDaawq8EcFVVF5almxMsxxx/9TT4AmSBinaV+DsIUgWi4gUQbSmwF33t7q2I1r27hpvH+IussHra4TY7Ws+JMMra2Is0WQXdVqrGcVDlNd8xckg X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:VE1PR04MB6560.eurprd04.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230001)(366004)(86362001)(83380400001)(5660300002)(316002)(4326008)(6916009)(66946007)(66556008)(66476007)(31696002)(31686004)(8676002)(36756003)(6486002)(508600001)(8936002)(26005)(6512007)(2906002)(186003)(53546011)(6506007)(38100700002)(2616005)(43740500002)(45980500001); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K2pxQ0ZUcy91STNZdG9HeXBOMzNiVmFzZm83QTZ1Kzh6YUhEZC83VThlOFpI?= =?utf-8?B?T2ZLdWZENjc0VkxLSlVWWkRyUlZNbm9rdkFpZklVRUpPY2lMVnIzbXBjYkxr?= =?utf-8?B?U0dVOHg3a1ZFTlYvQm9ydm80OFlNZGtyVDJBQ0xiUGxqclBMa2U2MFhoQTJ5?= =?utf-8?B?MFpBeVh1ajg3SW1CVGJROXJIWnlKUHhSMzdZUlhXQkMrSjc0eGNGczI2dDVr?= =?utf-8?B?RkR2YlVJVDM3OHJOWWFubGo5ZVJzSXJrM3BXVlJibFBqWDRNcmR4VVJWTlli?= =?utf-8?B?ZzJCZlZKMERBSTZtLzduakM4cldHNXd4RU42TFRrVjMzb2xSRXptY2RsQnB6?= =?utf-8?B?WVo0VUNMSUo2MjJzQlpZNlhwMi9GU2l1Z2NGNjhIdER3dW1YdU9Yem1oTWFK?= =?utf-8?B?aHRaS0hDaEhUOGd3MlczWk50Mlh3dzFpN2JrSXBuWmc1RkhDa3gyaUZ0NnUz?= =?utf-8?B?SjRLTzE3S3ZFZVRhVk9vRDlsOERYcG43bkFmUy90NlBlNzVIR3pqU2M1T3J0?= =?utf-8?B?Ym1ML1A2SEdiRjIrak9JSEtpV1FUM082MFdUSVFwYVN2Z2s0blRBMnBxTnNt?= =?utf-8?B?OHo1L0pjNTdDenpVRXFmQ1g3eGN1eFgyYnM0a3hYS3FNUXY1R1g2Sk5HRUJv?= =?utf-8?B?WXZSaXJWWXZKSkxYejl3Tk1rQm0veWx6Q0QraWkwWW1kdjQ5ZGdUcWwveWov?= =?utf-8?B?bWNSWHh5Q2Y3RTlVWEM3QnprREJYUjNsZ2lLb0xRaTJQYStBY1BUQ1YvUktM?= =?utf-8?B?UU5qVUlacnlOMVBCOXU5c2hkNHJJM3pCaWszWXQ1L2JYbi91NGhzejlLMFVF?= =?utf-8?B?UEtrekUxOHc1OTEzVVdCNEt3UkdMaFFNSTZwUEVHaGVWWHdSSHlVSHpTN0pl?= =?utf-8?B?b01ueVlrZkc3VVplYm5PT3lnT3VBQVZ3TzlWSjZiV1c5Q0RZQzBBTkVPQlF5?= =?utf-8?B?Q1ZnYVlWNldqcHdwK3d0SVVBVHE3N2NJQm5KeFZVaUl1RDZtQmRFemYwK3lu?= =?utf-8?B?OEcrVk5NUnlXUVI0OUNINW1UVE9NTXRwRUFDYnhCRjJ2SjljSWEwdng4TzVN?= =?utf-8?B?RElXZitpUXdiRWx1d0VDRzFETnVKbi82b09zaUNiMm9xR0VJRHQ4VUU1SUtG?= =?utf-8?B?aEcwMWw4MldaMGNXWGU2T0hNenRsakltWjhSRmNIeVd6bXJjOHJKTEpOWC9p?= =?utf-8?B?Y1FkcHUvb0xuQWJrd2h1dTRvSHYxdnhhUDArNjZldEh1S0hxd0JJSGcyaHBz?= =?utf-8?B?eVVHWlpNTWxVYm0xYzlwZGMxY2R6eFZ3RElZZFdNWVFFM29tU0NHQ2pJc1pv?= =?utf-8?B?VmMvalQrS0o5d05DUmxySHlXdjlrTmI5dDlPR2NYTkRzT0xwM0ZacDVtTDBk?= =?utf-8?B?M3hnT3hxeTJIMzNYamwyb2Jvdk13b1VlNzZNeUNXVkd2WXMwQmhwNzdYSEFT?= =?utf-8?B?UmdFSFRzOEdiSytLMy92NWpLZFpFVjYraTJCTHZwRUtLQStGQ1kzYWJUaHhp?= =?utf-8?B?S1NFTERZUm5LR3Y4b0hMMGJDTVNXS2tLSUZFRmFUTzNaMVpkY0lXeFNFQUIy?= =?utf-8?B?WDd6d0V0R1NRb1FZVXV1eHJVaFZqNlQ5aWNKZWkrT0xaMnd4ZEZjRWRWSTln?= =?utf-8?B?dStNODFnVExPYVl4Zk1MZ3ljOXBzZUdrQmVCZTRlQktMQ1pNZDY1eVE2WW1J?= =?utf-8?B?Y0o1YjZqSWVRMXFQV1ZreXJkZVZmQlVJT3NXVDE4bStlUjh4dnRqLzduRlBM?= =?utf-8?B?MGJOL1ltWVJTczM0QTlXRkdPVUZsdkZoaTNPRFdVcy9aRGd6aHJsaEVrNkZI?= =?utf-8?B?TFA3S1hRd3UzU1FuMWtMcVd2M2dvRmhKTExKVGdCRk85a3FaVllHSllMME0v?= =?utf-8?B?ZXdrcUp5Wm9PMHVyQXdNQ0dTV011TFZZVzBua3FwTVJNRWF6cFFXOWdiZzhN?= =?utf-8?B?UzNRVGRDSlo1YVJJUGt0SFVjQmNFUm5HNk5RcFBRZEkwM3MvT2FzVEg3Nmov?= =?utf-8?B?ckplK3ZpNDFyRXI3Rkxpck5iTkpoODNkZ1l1b1NKbldydisyWHpRNVl0T0lq?= =?utf-8?B?WlVsMHU1Q3lteFh1UnJEQ3dMMi9PeEVtTXJTVU9EdWhBdmZ5M1lBT3RQbnJ6?= =?utf-8?B?QW9tTmxwbXZrL2RMNUx6d2VReERjOFF3NmtxYjh0ZjlNc3U4QWJHcEdnRmFR?= =?utf-8?B?SlFIUmgydDI5UWZDc0FvZFhkNEJTNDBPVWppYzV0aHhjVm1UZWwrU2tPRnlF?= =?utf-8?B?Q1NkQTZ4QWgvYkx0Y2NwSnlyZUVVSWtWeVQ5bC8rb3V6VWhiOTE2OENQamtJ?= =?utf-8?B?bUgvU0UyMmlUejU5VWxHUEYrdE9lVFZKVWVWZmNqQWlzVXJHaDIvUT09?= X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: 2bd04c39-720f-4217-4b91-08da38c9c242 X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2022 12:27:22.6459 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: qaVlEuj4GrTdwMJWY5YzNaGIu4h+WZ83/2Y9GXfK/3BovfGiz0w7dNaPWBEIzKgKB6J815MbU3qeOhfcR47jmQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB5076 X-Spam-Status: No, score=-3032.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, 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, 18 May 2022 12:27:28 -0000 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. > @@ -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