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

[Jan Beulich]

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