[PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions

Thu May 27 14:57:15 GMT 2021

On 27.05.2021 15:46, H.J. Lu wrote:
> On Thu, May 27, 2021 at 6:15 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 27.05.2021 15:08, H.J. Lu wrote:
>>> On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 27.05.2021 14:29, H.J. Lu wrote:
>>>>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 26.05.2021 16:16, H.J. Lu wrote:
>>>>>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>
>>>>>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
>>>>>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
>>>>>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
>>>>>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> as in
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
>>>>>>>>>>>>>>> introduction wants to be separated from the specific use in the
>>>>>>>>>>>>>>> linker, not the lease because ...
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
>>>>>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
>>>>>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
>>>>>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>>>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
>>>>>>>>>>>>>>> goal of the earlier change.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
>>>>>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
>>>>>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
>>>>>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
>>>>>>>>>>>>>
>>>>>>>>>>>>> When paying specific attention to resulting code size, people may
>>>>>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>>>>>>>>>>>>> code. This not having worked properly is what initially triggered
>>>>>>>>>>>>> me touching this area. And of course assembler programmers could
>>>>>>>>>>>>> even cause data to have PC16 relocations attached to it.
>>>>>>>>>>>>
>>>>>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
>>>>>>>>>>>> changed.
>>>>>>>>>>>
>>>>>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
>>>>>>>>>>> contain small pieces of 16-bit code.
>>>>>>>>>>
>>>>>>>>>> And we never ran into any issues before.  My patch just restores
>>>>>>>>>> the old behavior for them.
>>>>>>>>>
>>>>>>>>> And my point is that the old behavior was wrong (and will be again if
>>>>>>>>> we go with your change).
>>>>>>>>
>>>>>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
>>>>>>>> old behavior only if input has 16-bit codes.
>>>>>>>>
>>>>>>>
>>>>>>> This is the patch I am going to check in.
>>>>>>
>>>>>> Sadly you've sent all but v1 only as attachments, making commenting
>>>>>> difficult. There are issues:
>>>>>> 1) The workaround gets triggered from the handling of the actual
>>>>>> .code* directives, so would come into effect even if no insn at all
>>>>>> was emitted. Imo this should be tied to the specific case of a PC16
>>>>>> relocation getting emitted for 16-bit code. This would also take
>>>>>> care of 4) below.
>>>>>
>>>>> We can do this:
>>>>>
>>>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
>>>>> index c17f4da16fe..8adf0fce0a6 100644
>>>>> --- a/gas/config/tc-i386.c
>>>>> +++ b/gas/config/tc-i386.c
>>>>> @@ -2695,10 +2695,6 @@ static void
>>>>>  set_code_flag (int value)
>>>>>  {
>>>>>    update_code_flag (value, 0);
>>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>>>> -  if (value == CODE_16BIT)
>>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>>>> -#endif
>>>>>  }
>>>>>
>>>>>  static void
>>>>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
>>>>>    cpu_arch_flags.bitfield.cpu64 = 0;
>>>>>    cpu_arch_flags.bitfield.cpuno64 = 1;
>>>>>    stackop_size = LONG_MNEM_SUFFIX;
>>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>>>> -  if (new_code_flag == CODE_16BIT)
>>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>>>> -#endif
>>>>>  }
>>>>>
>>>>>  static void
>>>>> @@ -8784,6 +8776,10 @@ output_branch (void)
>>>>>    else
>>>>>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
>>>>>    subtype |= code16;
>>>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>>>> +  if (code16)
>>>>> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>>>> +#endif
>>>>
>>>> This would get us closer to what I have described, but doesn't go
>>>> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
>>>> misleading and would probably want naming (and describing)
>>>> differently.
>>>
>>> Do you have any suggestions?
>>
>> GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
>> when the flag gets set (i.e. only when a PC16 reloc gets actually
>> generated).
> 
> How about GNU_PROPERTY_X86_FEATURE_2_PC16_CODE16?

Fine with me - I don't care much about the name as long as it's
not actively misleading.

> My proposed patch should be sufficient.  If not, do you have a testcase?

The decision whether to emit a relocation happens only during
relaxation, aiui. IOW it is my understanding that a forward
branch to a local symbol would also cause the flag to get set.

>>>>>> 2) Object files created by other tools (including older gas, which
>>>>>> would include an incremental build after having updated binutils),
>>>>>> or created with that note's emission suppressed (which, as said in
>>>>>> bug 27753, shouldn't be emitted by default anyway) will still cause
>>>>>> problems.
>>>>>
>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
>>>>> suppressed.
>>>>
>>>> Of course it can, via -mx86-used-note=no. Or did you change this
>>>> kind of silently as a "side effect" of your patch, without me
>>>> even noticing? That would be evil - as said earlier (and reported
>>>> as a bug), these notes shouldn't be there by default; even less
>>>> so should they be there unconditionally.
>>>
>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
>>> Otherwise, it won't work for seabios.
>>
>> Well, for both this and ...
>>
>>>> Plus even if it couldn't be suppressed, there would still be the
>>>> issue with object files created by other / older tools.
>>>
>>> This is a workaround/compromise.  There is no perfect solution.
>>> It works for seabios.
>>
>> ... this: Abusing relocations is something that may very well require
>> projects to adjust their build environment, e.g. by adding a command
>> line option to one of the tools involved.
> 
> When I was reading the binutils source, my impression was that x86 PC16
> relocations were intended for 16-bit codes in an ELF32/ELF64 container.
> Whatever we do, we shouldn't break existing 16-bit programs.

Well, if the treatment of PC16 isn't ABI-conforming, then perhaps
the ABI should be changed to express the intended behavior for this
reloc type (and make explicit that it's different from PC32 and PC8),
and there then perhaps ought to be a new reloc type with proper PC16
semantics (usable for 16-bit operand size XBEGIN as well as 16-bit
data items with PC-relative expressions)?

Jan