[2/5] ix86: tighten convert-load-reloc checking

Message ID c1b7da76-57b2-4074-8429-b1940da251a7@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:40 a.m. UTC
  Just like was done recently for x86-64 (commit 4998f9ea9d35): Even if
the assembler avoids using the relaxable relocation for inapplicable
insns, the relocation type can still appear for other reasons. Be more
thorough in the opcode checking we do, to avoid bogusly altering other
insns.

Furthermore correct an opcode mask (even if with the added condition
that's now fully benign).
  

Comments

H.J. Lu Feb. 3, 2025, 10:34 p.m. UTC | #1
On Mon, Feb 3, 2025 at 7:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> Just like was done recently for x86-64 (commit 4998f9ea9d35): Even if
> the assembler avoids using the relaxable relocation for inapplicable
> insns, the relocation type can still appear for other reasons. Be more
> thorough in the opcode checking we do, to avoid bogusly altering other
> insns.
>
> Furthermore correct an opcode mask (even if with the added condition
> that's now fully benign).
>
> --- a/bfd/elf32-i386.c
> +++ b/bfd/elf32-i386.c
> @@ -1453,15 +1453,16 @@ elf_i386_convert_load_reloc (bfd *abfd,
>                   modrm = 0xc0 | (modrm & 0x38) >> 3;
>                   opcode = 0xf7;
>                 }
> -             else
> +             else if ((opcode | 0x38) == 0x3b)
>                 {
>                   /* Convert "binop foo@GOT(%reg1), %reg2" to
>                      "binop $foo, %reg2".  */
> -                 modrm = (0xc0
> -                          | (modrm & 0x38) >> 3
> -                          | (opcode & 0x3c));
> +                 modrm = 0xc0 | ((modrm & 0x38) >> 3) | (opcode & 0x38);
>                   opcode = 0x81;
>                 }
> +             else
> +               return true;
> +
>               bfd_put_8 (abfd, modrm, contents + roff - 1);
>               r_type = R_386_32;
>             }
>

Please add a testcase to show it makes a difference.

Thanks.
  
Jan Beulich Feb. 4, 2025, 7:21 a.m. UTC | #2
On 03.02.2025 23:34, H.J. Lu wrote:
> On Mon, Feb 3, 2025 at 7:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Just like was done recently for x86-64 (commit 4998f9ea9d35): Even if
>> the assembler avoids using the relaxable relocation for inapplicable
>> insns, the relocation type can still appear for other reasons. Be more
>> thorough in the opcode checking we do, to avoid bogusly altering other
>> insns.
>>
>> Furthermore correct an opcode mask (even if with the added condition
>> that's now fully benign).
>>
>> --- a/bfd/elf32-i386.c
>> +++ b/bfd/elf32-i386.c
>> @@ -1453,15 +1453,16 @@ elf_i386_convert_load_reloc (bfd *abfd,
>>                   modrm = 0xc0 | (modrm & 0x38) >> 3;
>>                   opcode = 0xf7;
>>                 }
>> -             else
>> +             else if ((opcode | 0x38) == 0x3b)
>>                 {
>>                   /* Convert "binop foo@GOT(%reg1), %reg2" to
>>                      "binop $foo, %reg2".  */
>> -                 modrm = (0xc0
>> -                          | (modrm & 0x38) >> 3
>> -                          | (opcode & 0x3c));
>> +                 modrm = 0xc0 | ((modrm & 0x38) >> 3) | (opcode & 0x38);
>>                   opcode = 0x81;
>>                 }
>> +             else
>> +               return true;
>> +
>>               bfd_put_8 (abfd, modrm, contents + roff - 1);
>>               r_type = R_386_32;
>>             }
>>
> 
> Please add a testcase to show it makes a difference.

The 64-bit counterpart has got a testcase demonstrating that it does.
Counter request: Please could you have added an exhaustive testcase
back at the time, demonstrating that no undue adjustments could ever
be done?

Underlying point: Experience tells me that adding (sensible) testcases
usually takes much more time than making the actual code change. Hence
why in a case like this I opted for not adding (another) one.

Jan
  
H.J. Lu Feb. 4, 2025, 7:26 a.m. UTC | #3
On Tue, Feb 4, 2025 at 3:21 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 03.02.2025 23:34, H.J. Lu wrote:
> > On Mon, Feb 3, 2025 at 7:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Just like was done recently for x86-64 (commit 4998f9ea9d35): Even if
> >> the assembler avoids using the relaxable relocation for inapplicable
> >> insns, the relocation type can still appear for other reasons. Be more
> >> thorough in the opcode checking we do, to avoid bogusly altering other
> >> insns.
> >>
> >> Furthermore correct an opcode mask (even if with the added condition
> >> that's now fully benign).
> >>
> >> --- a/bfd/elf32-i386.c
> >> +++ b/bfd/elf32-i386.c
> >> @@ -1453,15 +1453,16 @@ elf_i386_convert_load_reloc (bfd *abfd,
> >>                   modrm = 0xc0 | (modrm & 0x38) >> 3;
> >>                   opcode = 0xf7;
> >>                 }
> >> -             else
> >> +             else if ((opcode | 0x38) == 0x3b)
> >>                 {
> >>                   /* Convert "binop foo@GOT(%reg1), %reg2" to
> >>                      "binop $foo, %reg2".  */
> >> -                 modrm = (0xc0
> >> -                          | (modrm & 0x38) >> 3
> >> -                          | (opcode & 0x3c));
> >> +                 modrm = 0xc0 | ((modrm & 0x38) >> 3) | (opcode & 0x38);
> >>                   opcode = 0x81;
> >>                 }
> >> +             else
> >> +               return true;
> >> +
> >>               bfd_put_8 (abfd, modrm, contents + roff - 1);
> >>               r_type = R_386_32;
> >>             }
> >>
> >
> > Please add a testcase to show it makes a difference.
>
> The 64-bit counterpart has got a testcase demonstrating that it does.
> Counter request: Please could you have added an exhaustive testcase
> back at the time, demonstrating that no undue adjustments could ever
> be done?

If you find issues, please show them with tests.

>
> Underlying point: Experience tells me that adding (sensible) testcases
> usually takes much more time than making the actual code change. Hence
> why in a case like this I opted for not adding (another) one.
>

Then please postpone your change until you find time to write
some tests.
  
Jan Beulich Feb. 4, 2025, 7:58 a.m. UTC | #4
On 04.02.2025 08:26, H.J. Lu wrote:
> On Tue, Feb 4, 2025 at 3:21 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 03.02.2025 23:34, H.J. Lu wrote:
>>> On Mon, Feb 3, 2025 at 7:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> Just like was done recently for x86-64 (commit 4998f9ea9d35): Even if
>>>> the assembler avoids using the relaxable relocation for inapplicable
>>>> insns, the relocation type can still appear for other reasons. Be more
>>>> thorough in the opcode checking we do, to avoid bogusly altering other
>>>> insns.
>>>>
>>>> Furthermore correct an opcode mask (even if with the added condition
>>>> that's now fully benign).
>>>>
>>>> --- a/bfd/elf32-i386.c
>>>> +++ b/bfd/elf32-i386.c
>>>> @@ -1453,15 +1453,16 @@ elf_i386_convert_load_reloc (bfd *abfd,
>>>>                   modrm = 0xc0 | (modrm & 0x38) >> 3;
>>>>                   opcode = 0xf7;
>>>>                 }
>>>> -             else
>>>> +             else if ((opcode | 0x38) == 0x3b)
>>>>                 {
>>>>                   /* Convert "binop foo@GOT(%reg1), %reg2" to
>>>>                      "binop $foo, %reg2".  */
>>>> -                 modrm = (0xc0
>>>> -                          | (modrm & 0x38) >> 3
>>>> -                          | (opcode & 0x3c));
>>>> +                 modrm = 0xc0 | ((modrm & 0x38) >> 3) | (opcode & 0x38);
>>>>                   opcode = 0x81;
>>>>                 }
>>>> +             else
>>>> +               return true;
>>>> +
>>>>               bfd_put_8 (abfd, modrm, contents + roff - 1);
>>>>               r_type = R_386_32;
>>>>             }
>>>>
>>>
>>> Please add a testcase to show it makes a difference.
>>
>> The 64-bit counterpart has got a testcase demonstrating that it does.
>> Counter request: Please could you have added an exhaustive testcase
>> back at the time, demonstrating that no undue adjustments could ever
>> be done?
> 
> If you find issues, please show them with tests.

And I did, in the 64-bit counterpart. The code here is exactly the same
as the 64-bit one (up to and including the wrong [but benign] use of
0x3c as a mask).

>> Underlying point: Experience tells me that adding (sensible) testcases
>> usually takes much more time than making the actual code change. Hence
>> why in a case like this I opted for not adding (another) one.
> 
> Then please postpone your change until you find time to write
> some tests.

Ehem. Are you really asking to hold back bugfixes?

Jan
  
H.J. Lu Feb. 4, 2025, 8:03 a.m. UTC | #5
On Tue, Feb 4, 2025 at 3:58 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.02.2025 08:26, H.J. Lu wrote:
> > On Tue, Feb 4, 2025 at 3:21 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 03.02.2025 23:34, H.J. Lu wrote:
> >>> On Mon, Feb 3, 2025 at 7:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> Just like was done recently for x86-64 (commit 4998f9ea9d35): Even if
> >>>> the assembler avoids using the relaxable relocation for inapplicable
> >>>> insns, the relocation type can still appear for other reasons. Be more
> >>>> thorough in the opcode checking we do, to avoid bogusly altering other
> >>>> insns.
> >>>>
> >>>> Furthermore correct an opcode mask (even if with the added condition
> >>>> that's now fully benign).
> >>>>
> >>>> --- a/bfd/elf32-i386.c
> >>>> +++ b/bfd/elf32-i386.c
> >>>> @@ -1453,15 +1453,16 @@ elf_i386_convert_load_reloc (bfd *abfd,
> >>>>                   modrm = 0xc0 | (modrm & 0x38) >> 3;
> >>>>                   opcode = 0xf7;
> >>>>                 }
> >>>> -             else
> >>>> +             else if ((opcode | 0x38) == 0x3b)
> >>>>                 {
> >>>>                   /* Convert "binop foo@GOT(%reg1), %reg2" to
> >>>>                      "binop $foo, %reg2".  */
> >>>> -                 modrm = (0xc0
> >>>> -                          | (modrm & 0x38) >> 3
> >>>> -                          | (opcode & 0x3c));
> >>>> +                 modrm = 0xc0 | ((modrm & 0x38) >> 3) | (opcode & 0x38);
> >>>>                   opcode = 0x81;
> >>>>                 }
> >>>> +             else
> >>>> +               return true;
> >>>> +
> >>>>               bfd_put_8 (abfd, modrm, contents + roff - 1);
> >>>>               r_type = R_386_32;
> >>>>             }
> >>>>
> >>>
> >>> Please add a testcase to show it makes a difference.
> >>
> >> The 64-bit counterpart has got a testcase demonstrating that it does.
> >> Counter request: Please could you have added an exhaustive testcase
> >> back at the time, demonstrating that no undue adjustments could ever
> >> be done?
> >
> > If you find issues, please show them with tests.
>
> And I did, in the 64-bit counterpart. The code here is exactly the same
> as the 64-bit one (up to and including the wrong [but benign] use of
> 0x3c as a mask).

I also requested tests for the 64-bit change.

>
> >> Underlying point: Experience tells me that adding (sensible) testcases
> >> usually takes much more time than making the actual code change. Hence
> >> why in a case like this I opted for not adding (another) one.
> >
> > Then please postpone your change until you find time to write
> > some tests.
>
> Ehem. Are you really asking to hold back bugfixes?
>

No test, no bug, no change.
  
Jan Beulich Feb. 4, 2025, 8:10 a.m. UTC | #6
On 04.02.2025 09:03, H.J. Lu wrote:
> On Tue, Feb 4, 2025 at 3:58 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.02.2025 08:26, H.J. Lu wrote:
>>> On Tue, Feb 4, 2025 at 3:21 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 03.02.2025 23:34, H.J. Lu wrote:
>>>>> On Mon, Feb 3, 2025 at 7:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> Just like was done recently for x86-64 (commit 4998f9ea9d35): Even if
>>>>>> the assembler avoids using the relaxable relocation for inapplicable
>>>>>> insns, the relocation type can still appear for other reasons. Be more
>>>>>> thorough in the opcode checking we do, to avoid bogusly altering other
>>>>>> insns.
>>>>>>
>>>>>> Furthermore correct an opcode mask (even if with the added condition
>>>>>> that's now fully benign).
>>>>>>
>>>>>> --- a/bfd/elf32-i386.c
>>>>>> +++ b/bfd/elf32-i386.c
>>>>>> @@ -1453,15 +1453,16 @@ elf_i386_convert_load_reloc (bfd *abfd,
>>>>>>                   modrm = 0xc0 | (modrm & 0x38) >> 3;
>>>>>>                   opcode = 0xf7;
>>>>>>                 }
>>>>>> -             else
>>>>>> +             else if ((opcode | 0x38) == 0x3b)
>>>>>>                 {
>>>>>>                   /* Convert "binop foo@GOT(%reg1), %reg2" to
>>>>>>                      "binop $foo, %reg2".  */
>>>>>> -                 modrm = (0xc0
>>>>>> -                          | (modrm & 0x38) >> 3
>>>>>> -                          | (opcode & 0x3c));
>>>>>> +                 modrm = 0xc0 | ((modrm & 0x38) >> 3) | (opcode & 0x38);
>>>>>>                   opcode = 0x81;
>>>>>>                 }
>>>>>> +             else
>>>>>> +               return true;
>>>>>> +
>>>>>>               bfd_put_8 (abfd, modrm, contents + roff - 1);
>>>>>>               r_type = R_386_32;
>>>>>>             }
>>>>>>
>>>>>
>>>>> Please add a testcase to show it makes a difference.
>>>>
>>>> The 64-bit counterpart has got a testcase demonstrating that it does.
>>>> Counter request: Please could you have added an exhaustive testcase
>>>> back at the time, demonstrating that no undue adjustments could ever
>>>> be done?
>>>
>>> If you find issues, please show them with tests.
>>
>> And I did, in the 64-bit counterpart. The code here is exactly the same
>> as the 64-bit one (up to and including the wrong [but benign] use of
>> 0x3c as a mask).
> 
> I also requested tests for the 64-bit change.

The 64-bit change went in already, with testcase (commit 4998f9ea9d35).

>>>> Underlying point: Experience tells me that adding (sensible) testcases
>>>> usually takes much more time than making the actual code change. Hence
>>>> why in a case like this I opted for not adding (another) one.
>>>
>>> Then please postpone your change until you find time to write
>>> some tests.
>>
>> Ehem. Are you really asking to hold back bugfixes?
> 
> No test, no bug, no change.

I would hope you're kidding, but experience with you tells me you're likely
meaning this seriously.

Jan
  

Patch

--- a/bfd/elf32-i386.c
+++ b/bfd/elf32-i386.c
@@ -1453,15 +1453,16 @@  elf_i386_convert_load_reloc (bfd *abfd,
 		  modrm = 0xc0 | (modrm & 0x38) >> 3;
 		  opcode = 0xf7;
 		}
-	      else
+	      else if ((opcode | 0x38) == 0x3b)
 		{
 		  /* Convert "binop foo@GOT(%reg1), %reg2" to
 		     "binop $foo, %reg2".  */
-		  modrm = (0xc0
-			   | (modrm & 0x38) >> 3
-			   | (opcode & 0x3c));
+		  modrm = 0xc0 | ((modrm & 0x38) >> 3) | (opcode & 0x38);
 		  opcode = 0x81;
 		}
+	      else
+		return true;
+
 	      bfd_put_8 (abfd, modrm, contents + roff - 1);
 	      r_type = R_386_32;
 	    }