[PATCH] x86: Check register operand for AddrPrefixOpReg

[PATCH] x86: Check register operand for AddrPrefixOpReg

[H.J. Lu]

I am checking in this patch and backporting it to 2.35 branch.

H.J.
---
If the address prefix changes the register operand, we need to check the
register operand when the memory operand is RIP-relative.

	PR gas/26685
	* config/tc-i386.c (process_suffix): Check the register operand
	for the address size prefix if the memory operand is symbol(%rip).
	* testsuite/gas/i386/x86-64-enqcmd.s: Add tests with RIP-relative
	addressing.
	* testsuite/gas/i386/x86-64-movdir.s: Likewise.
	* testsuite/gas/i386/x86-64-enqcmd-intel.d: Updated.
	* testsuite/gas/i386/x86-64-enqcmd.d: Likewise.
	* testsuite/gas/i386/x86-64-movdir-intel.d: Likewise.
	* testsuite/gas/i386/x86-64-movdir.d: Likewise.
---
 gas/config/tc-i386.c                         | 13 +++++++++
 gas/testsuite/gas/i386/x86-64-enqcmd-intel.d | 28 ++++++++++++++------
 gas/testsuite/gas/i386/x86-64-enqcmd.d       | 28 ++++++++++++++------
 gas/testsuite/gas/i386/x86-64-enqcmd.s       | 12 +++++++++
 gas/testsuite/gas/i386/x86-64-movdir-intel.d | 24 ++++++++++-------
 gas/testsuite/gas/i386/x86-64-movdir.d       | 24 ++++++++++-------
 gas/testsuite/gas/i386/x86-64-movdir.s       |  6 +++++
 7 files changed, 101 insertions(+), 34 deletions(-)

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 4891c45a1ab..094f555ea01 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -7178,6 +7178,19 @@ process_suffix (void)
 	  unsigned int op;
 	  enum { need_word, need_dword, need_qword } need;
 
+	  /* Check the register operand for the address size prefix if
+	     the memory operand is symbol(%rip).  */
+	  if (i.mem_operands == 1
+	      && i.reg_operands == 1
+	      && i.operands == 2
+	      && i.base_reg
+	      && i.base_reg->reg_num == RegIP
+	      && i.base_reg->reg_type.bitfield.qword
+	      && i.types[1].bitfield.class == Reg
+	      && i.op[1].regs->reg_type.bitfield.dword
+	      && !add_prefix (ADDR_PREFIX_OPCODE))
+	    return 0;
+
 	  if (flag_code == CODE_32BIT)
 	    need = i.prefix[ADDR_PREFIX] ? need_word : need_dword;
 	  else if (i.prefix[ADDR_PREFIX])
diff --git a/gas/testsuite/gas/i386/x86-64-enqcmd-intel.d b/gas/testsuite/gas/i386/x86-64-enqcmd-intel.d
index e483d570b92..64d325171b5 100644
--- a/gas/testsuite/gas/i386/x86-64-enqcmd-intel.d
+++ b/gas/testsuite/gas/i386/x86-64-enqcmd-intel.d
@@ -9,12 +9,24 @@
 Disassembly of section \.text:
 
 0+ <_start>:
-[ 	]*[a-f0-9]+:[ 	]*f2 0f 38 f8 01[ 	]*enqcmd rax,\[rcx\]
-[ 	]*[a-f0-9]+:[ 	]*67 f2 0f 38 f8 01[ 	]*enqcmd eax,\[ecx\]
-[ 	]*[a-f0-9]+:[ 	]*f3 0f 38 f8 01[ 	]*enqcmds rax,\[rcx\]
-[ 	]*[a-f0-9]+:[ 	]*67 f3 0f 38 f8 01[ 	]*enqcmds eax,\[ecx\]
-[ 	]*[a-f0-9]+:[ 	]*f2 0f 38 f8 01[ 	]*enqcmd rax,\[rcx\]
-[ 	]*[a-f0-9]+:[ 	]*67 f2 0f 38 f8 01[ 	]*enqcmd eax,\[ecx\]
-[ 	]*[a-f0-9]+:[ 	]*f3 0f 38 f8 01[ 	]*enqcmds rax,\[rcx\]
-[ 	]*[a-f0-9]+:[ 	]*67 f3 0f 38 f8 01[ 	]*enqcmds eax,\[ecx\]
+ +[a-f0-9]+:	f2 0f 38 f8 01       	enqcmd rax,\[rcx\]
+ +[a-f0-9]+:	67 f2 0f 38 f8 01    	enqcmd eax,\[ecx\]
+ +[a-f0-9]+:	f3 0f 38 f8 01       	enqcmds rax,\[rcx\]
+ +[a-f0-9]+:	67 f3 0f 38 f8 01    	enqcmds eax,\[ecx\]
+ +[a-f0-9]+:	f2 0f 38 f8 0d 00 00 00 00 	enqcmd rcx,\[rip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	f3 0f 38 f8 0d 00 00 00 00 	enqcmds rcx,\[rip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	f2 0f 38 f8 01       	enqcmd rax,\[rcx\]
+ +[a-f0-9]+:	67 f2 0f 38 f8 01    	enqcmd eax,\[ecx\]
+ +[a-f0-9]+:	f3 0f 38 f8 01       	enqcmds rax,\[rcx\]
+ +[a-f0-9]+:	67 f3 0f 38 f8 01    	enqcmds eax,\[ecx\]
+ +[a-f0-9]+:	f2 0f 38 f8 0d 00 00 00 00 	enqcmd rcx,\[rip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	f3 0f 38 f8 0d 00 00 00 00 	enqcmds rcx,\[rip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds ecx,\[eip\+0x0\]        #.*
 #pass
diff --git a/gas/testsuite/gas/i386/x86-64-enqcmd.d b/gas/testsuite/gas/i386/x86-64-enqcmd.d
index 337febf320f..5f6676636eb 100644
--- a/gas/testsuite/gas/i386/x86-64-enqcmd.d
+++ b/gas/testsuite/gas/i386/x86-64-enqcmd.d
@@ -9,12 +9,24 @@
 Disassembly of section \.text:
 
 0+ <_start>:
-[ 	]*[a-f0-9]+:[ 	]*f2 0f 38 f8 01[ 	]*enqcmd \(%rcx\),%rax
-[ 	]*[a-f0-9]+:[ 	]*67 f2 0f 38 f8 01[ 	]*enqcmd \(%ecx\),%eax
-[ 	]*[a-f0-9]+:[ 	]*f3 0f 38 f8 01[ 	]*enqcmds \(%rcx\),%rax
-[ 	]*[a-f0-9]+:[ 	]*67 f3 0f 38 f8 01[ 	]*enqcmds \(%ecx\),%eax
-[ 	]*[a-f0-9]+:[ 	]*f2 0f 38 f8 01[ 	]*enqcmd \(%rcx\),%rax
-[ 	]*[a-f0-9]+:[ 	]*67 f2 0f 38 f8 01[ 	]*enqcmd \(%ecx\),%eax
-[ 	]*[a-f0-9]+:[ 	]*f3 0f 38 f8 01[ 	]*enqcmds \(%rcx\),%rax
-[ 	]*[a-f0-9]+:[ 	]*67 f3 0f 38 f8 01[ 	]*enqcmds \(%ecx\),%eax
+ +[a-f0-9]+:	f2 0f 38 f8 01       	enqcmd \(%rcx\),%rax
+ +[a-f0-9]+:	67 f2 0f 38 f8 01    	enqcmd \(%ecx\),%eax
+ +[a-f0-9]+:	f3 0f 38 f8 01       	enqcmds \(%rcx\),%rax
+ +[a-f0-9]+:	67 f3 0f 38 f8 01    	enqcmds \(%ecx\),%eax
+ +[a-f0-9]+:	f2 0f 38 f8 0d 00 00 00 00 	enqcmd 0x0\(%rip\),%rcx        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd 0x0\(%eip\),%ecx        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd 0x0\(%eip\),%ecx        #.*
+ +[a-f0-9]+:	f3 0f 38 f8 0d 00 00 00 00 	enqcmds 0x0\(%rip\),%rcx        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds 0x0\(%eip\),%ecx        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds 0x0\(%eip\),%ecx        #.*
+ +[a-f0-9]+:	f2 0f 38 f8 01       	enqcmd \(%rcx\),%rax
+ +[a-f0-9]+:	67 f2 0f 38 f8 01    	enqcmd \(%ecx\),%eax
+ +[a-f0-9]+:	f3 0f 38 f8 01       	enqcmds \(%rcx\),%rax
+ +[a-f0-9]+:	67 f3 0f 38 f8 01    	enqcmds \(%ecx\),%eax
+ +[a-f0-9]+:	f2 0f 38 f8 0d 00 00 00 00 	enqcmd 0x0\(%rip\),%rcx        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd 0x0\(%eip\),%ecx        #.*
+ +[a-f0-9]+:	67 f2 0f 38 f8 0d 00 00 00 00 	enqcmd 0x0\(%eip\),%ecx        #.*
+ +[a-f0-9]+:	f3 0f 38 f8 0d 00 00 00 00 	enqcmds 0x0\(%rip\),%rcx        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds 0x0\(%eip\),%ecx        #.*
+ +[a-f0-9]+:	67 f3 0f 38 f8 0d 00 00 00 00 	enqcmds 0x0\(%eip\),%ecx        #.*
 #pass
diff --git a/gas/testsuite/gas/i386/x86-64-enqcmd.s b/gas/testsuite/gas/i386/x86-64-enqcmd.s
index f790b28fc20..d1f3aca5ce5 100644
--- a/gas/testsuite/gas/i386/x86-64-enqcmd.s
+++ b/gas/testsuite/gas/i386/x86-64-enqcmd.s
@@ -7,9 +7,21 @@ _start:
 	enqcmd (%ecx),%eax
 	enqcmds (%rcx),%rax
 	enqcmds (%ecx),%eax
+	enqcmd foo(%rip),%rcx
+	enqcmd foo(%rip),%ecx
+	enqcmd foo(%eip),%ecx
+	enqcmds foo(%rip),%rcx
+	enqcmds foo(%rip),%ecx
+	enqcmds foo(%eip),%ecx
 
 	.intel_syntax noprefix
 	enqcmd rax,[rcx]
 	enqcmd eax,[ecx]
 	enqcmds rax,[rcx]
 	enqcmds eax,[ecx]
+	enqcmd rcx,[rip+foo]
+	enqcmd ecx,[rip+foo]
+	enqcmd ecx,[eip+foo]
+	enqcmds rcx,[rip+foo]
+	enqcmds ecx,[rip+foo]
+	enqcmds ecx,[eip+foo]
diff --git a/gas/testsuite/gas/i386/x86-64-movdir-intel.d b/gas/testsuite/gas/i386/x86-64-movdir-intel.d
index 0f3a5abd610..fe92e80d715 100644
--- a/gas/testsuite/gas/i386/x86-64-movdir-intel.d
+++ b/gas/testsuite/gas/i386/x86-64-movdir-intel.d
@@ -9,13 +9,19 @@
 Disassembly of section \.text:
 
 0+ <_start>:
-[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*movdiri QWORD PTR \[rcx\],rax
-[ 	]*[a-f0-9]+:[ 	]*66 0f 38 f8 01[ 	]*movdir64b rax,\[rcx\]
-[ 	]*[a-f0-9]+:[ 	]*67 66 0f 38 f8 01[ 	]*movdir64b eax,\[ecx]
-[ 	]*[a-f0-9]+:[ 	]*0f 38 f9 01[ 	]*movdiri DWORD PTR \[rcx\],eax
-[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*movdiri QWORD PTR \[rcx\],rax
-[ 	]*[a-f0-9]+:[ 	]*0f 38 f9 01[ 	]*movdiri DWORD PTR \[rcx\],eax
-[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*movdiri QWORD PTR \[rcx\],rax
-[ 	]*[a-f0-9]+:[ 	]*66 0f 38 f8 01[ 	]*movdir64b rax,\[rcx\]
-[ 	]*[a-f0-9]+:[ 	]*67 66 0f 38 f8 01[ 	]*movdir64b eax,\[ecx\]
+ +[a-f0-9]+:	48 0f 38 f9 01       	movdiri QWORD PTR \[rcx\],rax
+ +[a-f0-9]+:	66 0f 38 f8 01       	movdir64b rax,\[rcx\]
+ +[a-f0-9]+:	67 66 0f 38 f8 01    	movdir64b eax,\[ecx\]
+ +[a-f0-9]+:	66 0f 38 f8 0d 00 00 00 00 	movdir64b rcx,\[rip\+0x0\]        #.*
+ +[a-f0-9]+:	67 66 0f 38 f8 0d 00 00 00 00 	movdir64b ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	67 66 0f 38 f8 0d 00 00 00 00 	movdir64b ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	0f 38 f9 01          	movdiri DWORD PTR \[rcx\],eax
+ +[a-f0-9]+:	48 0f 38 f9 01       	movdiri QWORD PTR \[rcx\],rax
+ +[a-f0-9]+:	0f 38 f9 01          	movdiri DWORD PTR \[rcx\],eax
+ +[a-f0-9]+:	48 0f 38 f9 01       	movdiri QWORD PTR \[rcx\],rax
+ +[a-f0-9]+:	66 0f 38 f8 01       	movdir64b rax,\[rcx\]
+ +[a-f0-9]+:	67 66 0f 38 f8 01    	movdir64b eax,\[ecx\]
+ +[a-f0-9]+:	66 0f 38 f8 0d 00 00 00 00 	movdir64b rcx,\[rip\+0x0\]        #.*
+ +[a-f0-9]+:	67 66 0f 38 f8 0d 00 00 00 00 	movdir64b ecx,\[eip\+0x0\]        #.*
+ +[a-f0-9]+:	67 66 0f 38 f8 0d 00 00 00 00 	movdir64b ecx,\[eip\+0x0\]        #.*
 #pass
diff --git a/gas/testsuite/gas/i386/x86-64-movdir.d b/gas/testsuite/gas/i386/x86-64-movdir.d
index 2deab8928e6..297c63fd002 100644
--- a/gas/testsuite/gas/i386/x86-64-movdir.d
+++ b/gas/testsuite/gas/i386/x86-64-movdir.d
@@ -9,13 +9,19 @@
 Disassembly of section \.text:
 
 0+ <_start>:
-[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*movdiri %rax,\(%rcx\)
-[ 	]*[a-f0-9]+:[ 	]*66 0f 38 f8 01[ 	]*movdir64b \(%rcx\),%rax
-[ 	]*[a-f0-9]+:[ 	]*67 66 0f 38 f8 01[ 	]*movdir64b \(%ecx\),%eax
-[ 	]*[a-f0-9]+:[ 	]*0f 38 f9 01[ 	]*movdiri %eax,\(%rcx\)
-[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*movdiri %rax,\(%rcx\)
-[ 	]*[a-f0-9]+:[ 	]*0f 38 f9 01[ 	]*movdiri %eax,\(%rcx\)
-[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*movdiri %rax,\(%rcx\)
-[ 	]*[a-f0-9]+:[ 	]*66 0f 38 f8 01[ 	]*movdir64b \(%rcx\),%rax
-[ 	]*[a-f0-9]+:[ 	]*67 66 0f 38 f8 01[ 	]*movdir64b \(%ecx\),%eax
+ +[a-f0-9]+:	48 0f 38 f9 01       	movdiri %rax,\(%rcx\)
+ +[a-f0-9]+:	66 0f 38 f8 01       	movdir64b \(%rcx\),%rax
+ +[a-f0-9]+:	67 66 0f 38 f8 01    	movdir64b \(%ecx\),%eax
+ +[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        #.*
+ +[a-f0-9]+:	0f 38 f9 01          	movdiri %eax,\(%rcx\)
+ +[a-f0-9]+:	48 0f 38 f9 01       	movdiri %rax,\(%rcx\)
+ +[a-f0-9]+:	0f 38 f9 01          	movdiri %eax,\(%rcx\)
+ +[a-f0-9]+:	48 0f 38 f9 01       	movdiri %rax,\(%rcx\)
+ +[a-f0-9]+:	66 0f 38 f8 01       	movdir64b \(%rcx\),%rax
+ +[a-f0-9]+:	67 66 0f 38 f8 01    	movdir64b \(%ecx\),%eax
+ +[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        #.*
 #pass
diff --git a/gas/testsuite/gas/i386/x86-64-movdir.s b/gas/testsuite/gas/i386/x86-64-movdir.s
index 6f9032dc4b3..33b60318813 100644
--- a/gas/testsuite/gas/i386/x86-64-movdir.s
+++ b/gas/testsuite/gas/i386/x86-64-movdir.s
@@ -6,6 +6,9 @@ _start:
 	movdiri %rax, (%rcx)
 	movdir64b (%rcx),%rax
 	movdir64b (%ecx),%eax
+	movdir64b foo(%rip),%rcx
+	movdir64b foo(%rip),%ecx
+	movdir64b foo(%eip),%ecx
 
 	.intel_syntax noprefix
 	movdiri [rcx],eax
@@ -14,3 +17,6 @@ _start:
 	movdiri qword ptr [rcx],rax
 	movdir64b rax,[rcx]
 	movdir64b eax,[ecx]
+	movdir64b rcx,[rip+foo]
+	movdir64b ecx,[rip+foo]
+	movdir64b ecx,[eip+foo]
-- 
2.26.2

Re: [PATCH] x86: Check register operand for AddrPrefixOpReg

[Jan Beulich]

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.

Jan

Re: [PATCH] x86: Check register operand for AddrPrefixOpReg

[H.J. Lu]

On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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.

-- 
H.J.

[PATCH] x86: Update register operand check for AddrPrefixOpReg

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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.

-- 
H.J.

[-- Attachment #2: 0001-x86-Update-register-operand-check-for-AddrPrefixOpRe.patch --]
[-- Type: application/x-patch, Size: 23092 bytes --]

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[Jan Beulich]

On 01.10.2020 15:07, H.J. Lu wrote:
> On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[H.J. Lu]

On Thu, Oct 1, 2020 at 6:23 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.10.2020 15:07, H.J. Lu wrote:
> > On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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).

GNU assembler serves GCC.  GCC doesn't use (%eip) and GCC always
uses (%rip) for RIP-relative addressing.

> 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.

This is a special case where the address size prefix applies to both the
memory and the register operands.  Since GCC always uses (%rip) for
RIP-relative addressing, we need to extract the address size from the
register operand in this case.

> 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.
>

--
H.J.

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[Jan Beulich]

On 01.10.2020 15:44, H.J. Lu wrote:
> On Thu, Oct 1, 2020 at 6:23 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 01.10.2020 15:07, H.J. Lu wrote:
>>> On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>
>>>> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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).
> 
> GNU assembler serves GCC.  GCC doesn't use (%eip) and GCC always
> uses (%rip) for RIP-relative addressing.

This may be its main purpose, but it's by far not its only one.

Jan

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[H.J. Lu]

On Thu, Oct 1, 2020 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.10.2020 15:44, H.J. Lu wrote:
> > On Thu, Oct 1, 2020 at 6:23 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 01.10.2020 15:07, H.J. Lu wrote:
> >>> On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>
> >>>> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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).
> >
> > GNU assembler serves GCC.  GCC doesn't use (%eip) and GCC always
> > uses (%rip) for RIP-relative addressing.
>
> This may be its main purpose, but it's by far not its only one.
>

GNU assembler accommodates other usages.  But it must work with GCC.
In this case, it needs to check the register operand for the address size prefix
when the memory operand is (%rip).

-- 
H.J.

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[Jan Beulich]

On 01.10.2020 17:08, H.J. Lu wrote:
> On Thu, Oct 1, 2020 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 01.10.2020 15:44, H.J. Lu wrote:
>>> On Thu, Oct 1, 2020 at 6:23 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 01.10.2020 15:07, H.J. Lu wrote:
>>>>> On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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).
>>>
>>> GNU assembler serves GCC.  GCC doesn't use (%eip) and GCC always
>>> uses (%rip) for RIP-relative addressing.
>>
>> This may be its main purpose, but it's by far not its only one.
>>
> 
> GNU assembler accommodates other usages.  But it must work with GCC.

We agree on this. What you don't want to accept is that gcc also
needs to emit correct code for gas to consume.

> In this case, it needs to check the register operand for the address size prefix
> when the memory operand is (%rip).

And we again agree on the checking aspect. What we disagree on is
what to do if the check finds a discrepancy. Without your recent
change, the checking you talk about did already happen. And the
assembler correctly diagnosed it. The assembler may not _ever_
guess upon what the programmer meant, when finding an ambiguity
(exceptions to this rule may be warranted for backwards
compatibility in very rare cases, but this clearly doesn't apply
here). We've had this discussion before in different contexts,
and I have to admit I'm struggling with how you drive things in
this regard. If there was a way for gcc to indicate "I've done my
best, but please fix it up for me", I could accept there being
such a (non-default) mode. I wouldn't like it very much, but I
could live with it. But for hand-written code (be it entire .s
files or inline assembly) inconsistencies and ambiguities need to
be diagnosed by default.

Note in particular that your change isn't even limited to x32
mode, where one _might_ view the adjustment as benign.

If this change of yours isn't going to get reverted, I will have
to enter a bug requesting its revert. Given there is already the
(false) bug report that triggered this fix, having to do so would
feel pretty odd, though.

Jan

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[H.J. Lu]

On Thu, Oct 1, 2020 at 11:58 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.10.2020 17:08, H.J. Lu wrote:
> > On Thu, Oct 1, 2020 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 01.10.2020 15:44, H.J. Lu wrote:
> >>> On Thu, Oct 1, 2020 at 6:23 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 01.10.2020 15:07, H.J. Lu wrote:
> >>>>> On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>>>
> >>>>>> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> 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).
> >>>
> >>> GNU assembler serves GCC.  GCC doesn't use (%eip) and GCC always
> >>> uses (%rip) for RIP-relative addressing.
> >>
> >> This may be its main purpose, but it's by far not its only one.
> >>
> >
> > GNU assembler accommodates other usages.  But it must work with GCC.
>
> We agree on this. What you don't want to accept is that gcc also
> needs to emit correct code for gas to consume.
>
> > In this case, it needs to check the register operand for the address size prefix
> > when the memory operand is (%rip).
>
> And we again agree on the checking aspect. What we disagree on is
> what to do if the check finds a discrepancy. Without your recent
> change, the checking you talk about did already happen. And the
> assembler correctly diagnosed it. The assembler may not _ever_
> guess upon what the programmer meant, when finding an ambiguity
> (exceptions to this rule may be warranted for backwards
> compatibility in very rare cases, but this clearly doesn't apply
> here). We've had this discussion before in different contexts,
> and I have to admit I'm struggling with how you drive things in
> this regard. If there was a way for gcc to indicate "I've done my
> best, but please fix it up for me", I could accept there being
> such a (non-default) mode. I wouldn't like it very much, but I
> could live with it. But for hand-written code (be it entire .s
> files or inline assembly) inconsistencies and ambiguities need to
> be diagnosed by default.
>
> Note in particular that your change isn't even limited to x32
> mode, where one _might_ view the adjustment as benign.

The address size prefix isn't limited to x32.   For RIP-relative
addressing, GCC always uses (%rip).  The assembler will never
guess correctly what the assembly code is trying to do in all
cases.  In this particular case, the assembler needs to look
at the register operand to extract the address size prefix.
I am going to check in the rest of my fix.

> If this change of yours isn't going to get reverted, I will have
> to enter a bug requesting its revert. Given there is already the
> (false) bug report that triggered this fix, having to do so would
> feel pretty odd, though.
>
> Jan



-- 
H.J.

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[Jan Beulich]

On 02.10.2020 13:33, H.J. Lu wrote:
> On Thu, Oct 1, 2020 at 11:58 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 01.10.2020 17:08, H.J. Lu wrote:
>>> GNU assembler accommodates other usages.  But it must work with GCC.
>>
>> We agree on this. What you don't want to accept is that gcc also
>> needs to emit correct code for gas to consume.
>>
>>> In this case, it needs to check the register operand for the address size prefix
>>> when the memory operand is (%rip).
>>
>> And we again agree on the checking aspect. What we disagree on is
>> what to do if the check finds a discrepancy. Without your recent
>> change, the checking you talk about did already happen. And the
>> assembler correctly diagnosed it. The assembler may not _ever_
>> guess upon what the programmer meant, when finding an ambiguity
>> (exceptions to this rule may be warranted for backwards
>> compatibility in very rare cases, but this clearly doesn't apply
>> here). We've had this discussion before in different contexts,
>> and I have to admit I'm struggling with how you drive things in
>> this regard. If there was a way for gcc to indicate "I've done my
>> best, but please fix it up for me", I could accept there being
>> such a (non-default) mode. I wouldn't like it very much, but I
>> could live with it. But for hand-written code (be it entire .s
>> files or inline assembly) inconsistencies and ambiguities need to
>> be diagnosed by default.
>>
>> Note in particular that your change isn't even limited to x32
>> mode, where one _might_ view the adjustment as benign.
> 
> The address size prefix isn't limited to x32.   For RIP-relative
> addressing, GCC always uses (%rip).

But the issue at hand is limited to x32, and the conversion you're
doing is also only half-way safe for x32.

>  The assembler will never
> guess correctly what the assembly code is trying to do in all
> cases.

Hence it shouldn't guess. It should tell the programmer to change
the code so guessing isn't needed.

>  In this particular case, the assembler needs to look
> at the register operand to extract the address size prefix.

And how does it know _that's_ the part which is correct (as per
the programmer's intentions), and not the other operand?

Jan

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[H.J. Lu]

On Fri, Oct 2, 2020 at 5:18 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.10.2020 13:33, H.J. Lu wrote:
> > On Thu, Oct 1, 2020 at 11:58 PM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 01.10.2020 17:08, H.J. Lu wrote:
> >>> GNU assembler accommodates other usages.  But it must work with GCC.
> >>
> >> We agree on this. What you don't want to accept is that gcc also
> >> needs to emit correct code for gas to consume.
> >>
> >>> In this case, it needs to check the register operand for the address size prefix
> >>> when the memory operand is (%rip).
> >>
> >> And we again agree on the checking aspect. What we disagree on is
> >> what to do if the check finds a discrepancy. Without your recent
> >> change, the checking you talk about did already happen. And the
> >> assembler correctly diagnosed it. The assembler may not _ever_
> >> guess upon what the programmer meant, when finding an ambiguity
> >> (exceptions to this rule may be warranted for backwards
> >> compatibility in very rare cases, but this clearly doesn't apply
> >> here). We've had this discussion before in different contexts,
> >> and I have to admit I'm struggling with how you drive things in
> >> this regard. If there was a way for gcc to indicate "I've done my
> >> best, but please fix it up for me", I could accept there being
> >> such a (non-default) mode. I wouldn't like it very much, but I
> >> could live with it. But for hand-written code (be it entire .s
> >> files or inline assembly) inconsistencies and ambiguities need to
> >> be diagnosed by default.
> >>
> >> Note in particular that your change isn't even limited to x32
> >> mode, where one _might_ view the adjustment as benign.
> >
> > The address size prefix isn't limited to x32.   For RIP-relative
> > addressing, GCC always uses (%rip).
>
> But the issue at hand is limited to x32, and the conversion you're
> doing is also only half-way safe for x32.

This is about the address size prefix in encoding and x32 uses it.

> >  The assembler will never
> > guess correctly what the assembly code is trying to do in all
> > cases.
>
> Hence it shouldn't guess. It should tell the programmer to change
> the code so guessing isn't needed.

GCC uses (%rip) for RIP relative addressing since there is no
need for the address size prefix for x32 and x86-64.  It is the same
as DISP and SYMBOL memory operand.   This is how GCC works.
Assembler just needs to check the register operand for the address
size prefix.

> >  In this particular case, the assembler needs to look
> > at the register operand to extract the address size prefix.
>
> And how does it know _that's_ the part which is correct (as per
> the programmer's intentions), and not the other operand?
>

In this case, just extract the address size prefix from the register
operand.

-- 
H.J.

Re: [PATCH] x86: Update register operand check for AddrPrefixOpReg

[H.J. Lu]

On Fri, Oct 2, 2020 at 5:27 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Oct 2, 2020 at 5:18 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 02.10.2020 13:33, H.J. Lu wrote:
> > > On Thu, Oct 1, 2020 at 11:58 PM Jan Beulich <jbeulich@suse.com> wrote:
> > >> On 01.10.2020 17:08, H.J. Lu wrote:
> > >>> GNU assembler accommodates other usages.  But it must work with GCC.
> > >>
> > >> We agree on this. What you don't want to accept is that gcc also
> > >> needs to emit correct code for gas to consume.
> > >>
> > >>> In this case, it needs to check the register operand for the address size prefix
> > >>> when the memory operand is (%rip).
> > >>
> > >> And we again agree on the checking aspect. What we disagree on is
> > >> what to do if the check finds a discrepancy. Without your recent
> > >> change, the checking you talk about did already happen. And the
> > >> assembler correctly diagnosed it. The assembler may not _ever_
> > >> guess upon what the programmer meant, when finding an ambiguity
> > >> (exceptions to this rule may be warranted for backwards
> > >> compatibility in very rare cases, but this clearly doesn't apply
> > >> here). We've had this discussion before in different contexts,
> > >> and I have to admit I'm struggling with how you drive things in
> > >> this regard. If there was a way for gcc to indicate "I've done my
> > >> best, but please fix it up for me", I could accept there being
> > >> such a (non-default) mode. I wouldn't like it very much, but I
> > >> could live with it. But for hand-written code (be it entire .s
> > >> files or inline assembly) inconsistencies and ambiguities need to
> > >> be diagnosed by default.
> > >>
> > >> Note in particular that your change isn't even limited to x32
> > >> mode, where one _might_ view the adjustment as benign.
> > >
> > > The address size prefix isn't limited to x32.   For RIP-relative
> > > addressing, GCC always uses (%rip).
> >
> > But the issue at hand is limited to x32, and the conversion you're
> > doing is also only half-way safe for x32.
>
> This is about the address size prefix in encoding and x32 uses it.
>
> > >  The assembler will never
> > > guess correctly what the assembly code is trying to do in all
> > > cases.
> >
> > Hence it shouldn't guess. It should tell the programmer to change
> > the code so guessing isn't needed.
>
> GCC uses (%rip) for RIP relative addressing since there is no
> need for the address size prefix for x32 and x86-64.  It is the same
> as DISP and SYMBOL memory operand.   This is how GCC works.
> Assembler just needs to check the register operand for the address
> size prefix.
>
> > >  In this particular case, the assembler needs to look
> > > at the register operand to extract the address size prefix.
> >
> > And how does it know _that's_ the part which is correct (as per
> > the programmer's intentions), and not the other operand?
> >
>
> In this case, just extract the address size prefix from the register
> operand.
>

I am backporting these 2 patches to 2.34 and 2.35 branches.

-- 
H.J.