[4/5] x86-64: further tighten convert-load-reloc checking

Message ID 547f08ff-9d6f-4b3a-8011-a713f88c17f2@suse.com
State New
Headers
Series x86: further GOT{,PCREL} related adjustments |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jan Beulich Feb. 3, 2025, 11:41 a.m. UTC
  REX2.M affects what insn we're actually dealing with, so we better check
this to avoid transforming (future) insns we must not touch.
  

Comments

H.J. Lu Feb. 3, 2025, 10:41 p.m. UTC | #1
On Mon, Feb 3, 2025 at 7:41 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> REX2.M affects what insn we're actually dealing with, so we better check
> this to avoid transforming (future) insns we must not touch.
>
> --- a/bfd/elf64-x86-64.c
> +++ b/bfd/elf64-x86-64.c
> @@ -2282,14 +2282,14 @@ elf_x86_64_convert_load_reloc (bfd *abfd
>           if (to_reloc_pc32)
>             return true;
>
> -         if (opcode == 0x85)
> +         if (opcode == 0x85 && !(rex2 & (REX2_M << 4)))
>             {
>               /* Convert "test %reg, foo@GOTPCREL(%rip)" to
>                  "test $foo, %reg".  */
>               modrm = 0xc0 | (modrm & 0x38) >> 3;
>               opcode = 0xf7;
>             }
> -         else if ((opcode | 0x38) == 0x3b)
> +         else if ((opcode | 0x38) == 0x3b && !(rex2 & (REX2_M << 4)))
>             {
>               /* Convert "binop foo@GOTPCREL(%rip), %reg" to
>                  "binop $foo, %reg".  */
>

Please add a testcase to show it makes a difference.

Thanks.
  
Jan Beulich Feb. 4, 2025, 10:04 a.m. UTC | #2
On 03.02.2025 23:41, H.J. Lu wrote:
> On Mon, Feb 3, 2025 at 7:41 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> REX2.M affects what insn we're actually dealing with, so we better check
>> this to avoid transforming (future) insns we must not touch.
>>
>> --- a/bfd/elf64-x86-64.c
>> +++ b/bfd/elf64-x86-64.c
>> @@ -2282,14 +2282,14 @@ elf_x86_64_convert_load_reloc (bfd *abfd
>>           if (to_reloc_pc32)
>>             return true;
>>
>> -         if (opcode == 0x85)
>> +         if (opcode == 0x85 && !(rex2 & (REX2_M << 4)))
>>             {
>>               /* Convert "test %reg, foo@GOTPCREL(%rip)" to
>>                  "test $foo, %reg".  */
>>               modrm = 0xc0 | (modrm & 0x38) >> 3;
>>               opcode = 0xf7;
>>             }
>> -         else if ((opcode | 0x38) == 0x3b)
>> +         else if ((opcode | 0x38) == 0x3b && !(rex2 & (REX2_M << 4)))
>>             {
>>               /* Convert "binop foo@GOTPCREL(%rip), %reg" to
>>                  "binop $foo, %reg".  */
>>
> 
> Please add a testcase to show it makes a difference.

Hmm, not sure how such a testcase would look like. At least some of
the involved opcodes have no meaning (yet) with REX2. But maybe I
can construct something. Still I view it as unreasonable that such
obvious omissions in earlier changes can't be corrected without
investing a lot of time in trying to make up a situation where
things would fail. Once again: Proof of _no failure_ should have
been added when these optimizations were introduced. And that proof
should have been extended when APX support was added. What you're
effectively doing is to ask me to cover for earlier omissions.

Jan
  
H.J. Lu Feb. 4, 2025, 10:10 a.m. UTC | #3
On Tue, Feb 4, 2025 at 6:04 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 03.02.2025 23:41, H.J. Lu wrote:
> > On Mon, Feb 3, 2025 at 7:41 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> REX2.M affects what insn we're actually dealing with, so we better check
> >> this to avoid transforming (future) insns we must not touch.
> >>
> >> --- a/bfd/elf64-x86-64.c
> >> +++ b/bfd/elf64-x86-64.c
> >> @@ -2282,14 +2282,14 @@ elf_x86_64_convert_load_reloc (bfd *abfd
> >>           if (to_reloc_pc32)
> >>             return true;
> >>
> >> -         if (opcode == 0x85)
> >> +         if (opcode == 0x85 && !(rex2 & (REX2_M << 4)))
> >>             {
> >>               /* Convert "test %reg, foo@GOTPCREL(%rip)" to
> >>                  "test $foo, %reg".  */
> >>               modrm = 0xc0 | (modrm & 0x38) >> 3;
> >>               opcode = 0xf7;
> >>             }
> >> -         else if ((opcode | 0x38) == 0x3b)
> >> +         else if ((opcode | 0x38) == 0x3b && !(rex2 & (REX2_M << 4)))
> >>             {
> >>               /* Convert "binop foo@GOTPCREL(%rip), %reg" to
> >>                  "binop $foo, %reg".  */
> >>
> >
> > Please add a testcase to show it makes a difference.
>
> Hmm, not sure how such a testcase would look like. At least some of
> the involved opcodes have no meaning (yet) with REX2. But maybe I
> can construct something. Still I view it as unreasonable that such
> obvious omissions in earlier changes can't be corrected without
> investing a lot of time in trying to make up a situation where
> things would fail. Once again: Proof of _no failure_ should have
> been added when these optimizations were introduced. And that proof
> should have been extended when APX support was added. What you're
> effectively doing is to ask me to cover for earlier omissions.
>
> Jan

Again.  No test, no issue, no change.
  
Jan Beulich Feb. 4, 2025, 10:17 a.m. UTC | #4
On 04.02.2025 11:10, H.J. Lu wrote:
> On Tue, Feb 4, 2025 at 6:04 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 03.02.2025 23:41, H.J. Lu wrote:
>>> On Mon, Feb 3, 2025 at 7:41 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> REX2.M affects what insn we're actually dealing with, so we better check
>>>> this to avoid transforming (future) insns we must not touch.
>>>>
>>>> --- a/bfd/elf64-x86-64.c
>>>> +++ b/bfd/elf64-x86-64.c
>>>> @@ -2282,14 +2282,14 @@ elf_x86_64_convert_load_reloc (bfd *abfd
>>>>           if (to_reloc_pc32)
>>>>             return true;
>>>>
>>>> -         if (opcode == 0x85)
>>>> +         if (opcode == 0x85 && !(rex2 & (REX2_M << 4)))
>>>>             {
>>>>               /* Convert "test %reg, foo@GOTPCREL(%rip)" to
>>>>                  "test $foo, %reg".  */
>>>>               modrm = 0xc0 | (modrm & 0x38) >> 3;
>>>>               opcode = 0xf7;
>>>>             }
>>>> -         else if ((opcode | 0x38) == 0x3b)
>>>> +         else if ((opcode | 0x38) == 0x3b && !(rex2 & (REX2_M << 4)))
>>>>             {
>>>>               /* Convert "binop foo@GOTPCREL(%rip), %reg" to
>>>>                  "binop $foo, %reg".  */
>>>>
>>>
>>> Please add a testcase to show it makes a difference.
>>
>> Hmm, not sure how such a testcase would look like. At least some of
>> the involved opcodes have no meaning (yet) with REX2. But maybe I
>> can construct something. Still I view it as unreasonable that such
>> obvious omissions in earlier changes can't be corrected without
>> investing a lot of time in trying to make up a situation where
>> things would fail. Once again: Proof of _no failure_ should have
>> been added when these optimizations were introduced. And that proof
>> should have been extended when APX support was added. What you're
>> effectively doing is to ask me to cover for earlier omissions.
> 
> Again.  No test, no issue, no change.

Again - please adjust your attitude.

Jan
  

Patch

--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -2282,14 +2282,14 @@  elf_x86_64_convert_load_reloc (bfd *abfd
 	  if (to_reloc_pc32)
 	    return true;
 
-	  if (opcode == 0x85)
+	  if (opcode == 0x85 && !(rex2 & (REX2_M << 4)))
 	    {
 	      /* Convert "test %reg, foo@GOTPCREL(%rip)" to
 		 "test $foo, %reg".  */
 	      modrm = 0xc0 | (modrm & 0x38) >> 3;
 	      opcode = 0xf7;
 	    }
-	  else if ((opcode | 0x38) == 0x3b)
+	  else if ((opcode | 0x38) == 0x3b && !(rex2 & (REX2_M << 4)))
 	    {
 	      /* Convert "binop foo@GOTPCREL(%rip), %reg" to
 		 "binop $foo, %reg".  */