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

[Jan Beulich]

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