[2/5] ix86: tighten convert-load-reloc checking
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
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
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.
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
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.
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
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.
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
@@ -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;
}