From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 460B8386F404 for ; Thu, 1 Oct 2020 13:23:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 460B8386F404 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 5250CB325; Thu, 1 Oct 2020 13:23:34 +0000 (UTC) Subject: Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg To: "H.J. Lu" Cc: Binutils References: <20200930233002.3044234-1-hjl.tools@gmail.com> From: Jan Beulich Message-ID: <7d3679b6-acf0-e530-8ccf-654c5ff8fb16@suse.com> Date: Thu, 1 Oct 2020 15:23:36 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3033.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 01 Oct 2020 13:23:36 -0000 On 01.10.2020 15:07, H.J. Lu wrote: > On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu wrote: >> >> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich wrote: >>> >>> On 01.10.2020 01:30, H.J. Lu via Binutils wrote: >>>> I am checking in this patch and backporting it to 2.35 branch. >>> >>> But this is wrong, as can be seen from e.g. ... >>>> + +[a-f0-9]+: 66 0f 38 f8 0d 00 00 00 00 movdir64b 0x0\(%rip\),%rcx #.* >>>> + +[a-f0-9]+: 67 66 0f 38 f8 0d 00 00 00 00 movdir64b 0x0\(%eip\),%ecx #.* >>>> + +[a-f0-9]+: 67 66 0f 38 f8 0d 00 00 00 00 movdir64b 0x0\(%eip\),%ecx #.* >>> >>> ... the middle line here not matching up with >>> >>>> + movdir64b foo(%rip),%rcx >>>> + movdir64b foo(%rip),%ecx >>>> + movdir64b foo(%eip),%ecx >>> >>> ... what was written here. Without your change this >>> >>> movdir64b (%rbp), %rax >>> movdir64b (%rbp), %eax >>> movdir64b (%ebp), %rax >>> movdir64b (%ebp), %eax >>> >>> movdir64b (%rip), %rax >>> movdir64b (%rip), %eax >>> movdir64b (%eip), %rax >>> movdir64b (%eip), %eax >>> >>> yields consistent results for both blocks - the middle two entries >>> get an error issued. >>> >>> Please revert, and once again please don't commit (let alone >>> backport) patches without giving people at least _a little bit_ of >>> time to look at them. >>> >> >> The fix is correct. This specific case came from >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97257 >> >> The address prefix changes the register operand in these instructions. >> (%rip) is a special case. >> > > Update the fix for DISP and SYMBOL. The description wrongly says: "When the address size prefix applies to both the memory and the register operand, we need to extract the address size prefix from the register operand if the memory operand has no real registers, like symbol, DISP or symbol(%rip)." And then similarly a code comment. To the assembler, %rip _is_ a real register (and %eip could be used in its stead, when 32-bit addressing is desired). Again - the previous change was wrong and needs undoing. I agree (largely, not fully anymore, despite my earlier indication of agreement) the plain symbol/disp case needs fixing; I'm unconvinced though that this is the place where the fix is to be made (but I didn't invest any time in looking for alternatives). As a general observation, such extended conditionals to address isolated cases are often, not always, a good sign that they're sitting in the wrong place. As an aside - I wonder how gcc gets away with not using foo(%eip) for -mx32. But I assume this is due to what I'd call overly heavy restrictions in the x32 ABI spec. Jan