From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x329.google.com (mail-ot1-x329.google.com [IPv6:2607:f8b0:4864:20::329]) by sourceware.org (Postfix) with ESMTPS id D606938369E2 for ; Fri, 9 Dec 2022 02:12:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D606938369E2 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ot1-x329.google.com with SMTP id p24-20020a0568301d5800b0066e6dc09be5so1988283oth.8 for ; Thu, 08 Dec 2022 18:12:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=yER0lijBBEzccGOMSnlaHV42a7uS0Arqo9segsa7iJ4=; b=BewTNx9Aq+Dbgp8xho5X78XfGln3CawJk7lwwaElUC6tuB3zavGiuLksW1ixN50j8B bNerMvG9nc/gtUX3IH270fkJmV8iJ80eRqzhFbaQGpdH9sTs7/RO+g5QO6RNU/RTfrqE KkLUGxk33mBWxEIdrOGdrI3DFmaW1IoY7/zDE4/L0KBawhlxw8fmiQ92yn29a4CyfG9l 6CXWYA/qWeHhiGSjoiwXBP3G7lJ5kFWuGiimzwq1GcDzifD6E9gA9JqD2o0mgArzAcNj yEqvSkAzDd5RW7BwVXOtgdE0WPGBwJuI6Lf3486Rvy2DyL4ziHIDB6McfCm4BuEeHZQk WK/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=yER0lijBBEzccGOMSnlaHV42a7uS0Arqo9segsa7iJ4=; b=AVc1cayp+sAbkt1Yk2v1x6NQpumJyhP09NvstOuwJ6S6fkiotYnPosoHs9XBiWWbI1 8IXDdzvpxuUIm9QFxj9W7ra69iNLoT5CoeIeN54POSsar88NQq+VKLSFmVw5bquN1HKa ZVyVgvTGd1hlScXEHTTbeAbmKiBQHszBBzGhjG/Op1LynarWeSXR17vQnx/q9m/JaFf9 qmJyK+0r/CSKgXj2n7wC5ZrVBLmsRIXb9pL160XfiyMjsUfco0wBdjFuUSTE0c+r12k1 UdbF25IACVsmLsosOwA5sY4Gajad3Lo3t0e8FY/NvNPirIixomTeQJErIJ5UIX5uhlTR QJRA== X-Gm-Message-State: ANoB5pl9crPb7h2TY4rh9TAsfh9OSwrMM8b9FqwdcOEsve5N+ZOuoqP9 s3Wfkx2b2LfXVn+1V5LkJAqV3eiXGDl8ZQ+BzAtGUmz+ X-Google-Smtp-Source: AA0mqf7opJvIgBueqOZ8rsWMkkcdRDCDfdRImBH7vNDiT1s8YaSDkcoQ5h0SRZyPelI+ykO0BDdFn6K3XjjXkI41Eos= X-Received: by 2002:a9d:12e5:0:b0:66e:b992:749f with SMTP id g92-20020a9d12e5000000b0066eb992749fmr8808328otg.52.1670551956052; Thu, 08 Dec 2022 18:12:36 -0800 (PST) MIME-Version: 1.0 References: <20221203041307.34407-1-hjl.tools@gmail.com> <88604f9d-1cc7-0c05-c92e-2561512dc96e@suse.com> <3977276e-762c-661e-6b0d-f757debb5ae0@suse.com> <1ae3e9f1-1207-50ea-7d25-ec7154f739bb@suse.com> <9508b3e4-fef3-2e37-27ae-7c9f48508116@suse.com> In-Reply-To: From: "H.J. Lu" Date: Thu, 8 Dec 2022 18:11:59 -0800 Message-ID: Subject: Re: [PATCH] x86: Allow 16-bit register source for LAR and LSL To: Jan Beulich Cc: binutils@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3017.4 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 autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, Dec 7, 2022 at 11:38 PM Jan Beulich wrote: > > On 08.12.2022 00:57, H.J. Lu wrote: > > On Wed, Dec 7, 2022 at 2:47 AM Jan Beulich wrote: > >> > >> On 06.12.2022 23:32, H.J. Lu wrote: > >>> On Tue, Dec 6, 2022 at 8:36 AM Jan Beulich wrote: > >>>> > >>>> On 06.12.2022 17:11, H.J. Lu wrote: > >>>>> On Mon, Dec 5, 2022 at 11:51 PM Jan Beulich wrote: > >>>>>> > >>>>>> On 06.12.2022 00:20, H.J. Lu wrote: > >>>>>>> On Mon, Dec 5, 2022 at 3:11 AM Jan Beulich wrote: > >>>>>>>> > >>>>>>>> On 03.12.2022 05:13, H.J. Lu wrote: > >>>>>>>>> Since LAR and LSL only access 16 bits of the source operand, regardless > >>>>>>>>> of operand size, allow 16-bit register source for LAR and LSL, and always > >>>>>>>>> disassemble LAR and LSL with 16-bit source operand. > >>>>>>>>> > >>>>>>>>> gas/ > >>>>>>>>> > >>>>>>>>> PR gas/29844 > >>>>>>>>> * testsuite/gas/i386/i386.s: Add tests for LAR and LSL. > >>>>>>>>> * testsuite/gas/i386/x86_64.s: Likewise. > >>>>>>>>> * testsuite/gas/i386/intelbad.s: Remove "lar/lsl eax, ax". > >>>>>>>>> * testsuite/gas/i386/i386-intel.d: Updated. > >>>>>>>>> * testsuite/gas/i386/i386.d: Likewise. > >>>>>>>>> * testsuite/gas/i386/intel-intel.d: Likewise. > >>>>>>>>> * testsuite/gas/i386/intel.d: Likewise. > >>>>>>>>> * testsuite/gas/i386/intelbad.l: Likewise. > >>>>>>>>> * testsuite/gas/i386/x86_64-intel.d: Likewise. > >>>>>>>>> * testsuite/gas/i386/x86_64.d: Likewise. > >>>>>>>>> > >>>>>>>>> opcodes/ > >>>>>>>>> > >>>>>>>>> PR gas/29844 > >>>>>>>>> * i386-dis.c (MOD_0F02): Removed. > >>>>>>>>> (MOD_0F03): Likewise. > >>>>>>>>> (dis386_twobyte): Restore larS and lslS. > >>>>>>>>> (mod_table): Remove MOD_0F02 and MOD_0F03. > >>>>>>>>> * i386-opc.tbl: Allow 16-bit register source for LAR and LSL. > >>>>>>>>> * i386-tbl.h: Regenerated. > >>>>>>>> > >>>>>>>> Please can you refrain from immediately committing patches which have > >>>>>>>> a risk of being controversial. > >>>>>>>> > >>>>>>>> In the case here, given there are uses of the 16-bit register operand > >>>>>>>> form in the Linux kernel, I can accept the assembler part of the change. > >>>>>>>> The lines in i386-opc.tbl, however, need a comment then, as allowing for > >>>>>>>> 16-bit registers despite a wider destination is explicitly not in line > >>>>>>>> with the SDM. (Interestingly AMD's PM is different in this regard.) > >>>>>>>> > >>>>>>>> For the disassembler part you're completely undoing what I did, which is > >>>>>>>> wrong - again with reference to the SDM. If you want to accommodate for > >>>>>>>> AMD's PM, then you need to vary disassembly according to command line > >>>>>>>> options specified, with the default being in line with the SDM (I can > >>>>>>>> dig out a pretty old version of the doc, but I believe it has always > >>>>>>>> been that way, i.e. even before AMD introduced their clones). > >>>>>>>> > >>>>>>>> I will revert this change unless you come forward with an adjustment > >>>>>>>> within the next couple of days. > >>>>>>>> > >>>>>>> > >>>>>>> Given that the only lower 16 bits are used, the 16-bit register source > >>>>>>> is more appropriate. I will raise the issue with the Intel SDM author. > >>>>>> > >>>>>> I see no point in changing the documentation when what's there has been > >>>>>> valid for well over 30 years. There are other cases in newer insns where > >>>>>> only the low 16 (or 8) bits are used, yet still the 32-bit register name > >>>>>> is specified ({,v}pinsr{b,w} come to mind immediately). Also what you've > >>>>>> done brought things out of sync with mov-to-sreg (and no, please don't > >>>>>> "restore" consistency by also changing disassembly there). > >>>>> > >>>>> The 16-bit register has been used in both assembler and disassembler > >>>>> for well over 30 years. I consider this a flaw in the spec. > >>>> > >>>> And a flaw in disassembly of move-to-sreg (and maybe other insns)? > >>>> Is the spec then also wrong with {,v}pinsr{b,w}? Also note that > >>>> while the assembler wants to provide backwards compatibility, the > >>>> same is rarely necessary for the disassembler. Hence what it may > >>>> or may not have done for over 30 years doesn't really matter. > >>>> > >>>> Please can we avoid introducing further inconsistencies, and rather > >>>> work towards more consistency (and not by then also corrupting > >>>> move-to-sreg and possible other insns)? > >>>> > >>> > >>> I have a different view on "inconsistencies". For LAR/LSL, they > >>> are different: > >>> > >>> 1. All operands are integer registers. > >>> 2. The operand size prefix doesn't apply to the source. > >>> 3. Only the 16 bits of the source are used. > >> > >> 2 and 3 are true for move-to-sreg and {,v}pinsr{b,w} as well. > >> > >>> 4. 16 bit source has been in use for more than 30 years. > >>> > >>> What counts are how the processor behaves > >> > >> Which means we have further things to fix: For example LTR and LLDT, > >> like move-to-sreg, also accept 32- or 64-bit register operands, > >> silently ignoring the upper bits. The assembler wants to accept such > >> for consistency (even if the SDM doesn't name these variants), and > >> the disassembler wants to express operand size by picking the correct > >> register name (didn't check if it maybe already does). > >> > >> This "picking the correct register name" applies to both operands of > >> LAR and LSL just as much. > > > > We have different opinions on what "the correct register name" > > should be. > > Indeed - hence why the patch should never have been committed. Recall > you did approve of the earlier patch, which now you've partially > reverted without even saying so in your commit? Which is made even > worse by the fact that to address PR gas/29844 there was no need at > all to also touch the disassembler, i.e. you've mixed in a single > commit two entirely separate changes (one controversial, the other > not). > > > When there are more than one integer registers > > and the operand size prefix is ignored on one, the register > > name with the proper size is the correct register name. > > Quite differently: Except where necessary (MOVSX, MOVZX, CRC32), > register sizes ought to match so there are as few anomalies as possible > throughout the entire ISA. With non-matching register sizes it'll be > easily ambiguous what an insn's operand size actually is. > I don't think we should change disassembler for them after more than 30 years. -- H.J.