Re: [PATCH v2 2/7] BaseTools-GenFw:Add new x86_64 Elf relocation types for PIC/PIE code

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 3 aug. 2016, at 22:53, Jordan Justen <jordan.l.justen@intel.com> wrote:
> 
>> On 2016-08-03 13:47:09, Ard Biesheuvel wrote:
>> 
>>> On 3 aug. 2016, at 22:13, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>> 
>>> Where does this patch stand? I think Ard was asking for it to be
>>> split, but also wondering if it was really required...
>>> 
>>> Some devs are still reporting unsupported relocation errors during the
>>> build. (mischief on irc, for example.)
>> 
>> The patch is not correct in its current form. PLT relocations (0x4)
>> can be handled ok, so we can split that off and merge it. The GOT
>> based relocation handling needs non-trivial rework, and may
>> currently create corrupt binaries. Which unhandled reloc types are
>> being reported?
> 
> "unsupported ELF EM_X86_64 relocation 0x4"
> 
> You might join irc if you want to get some more info from mischief
> about it.
> 

That's the plt one, which we can fix easily. I'll whip up a patch in the morning (11 pm here now)


> 
>> 
>>> 
>>>> On 2016-08-02 05:00:43, Ard Biesheuvel wrote:
>>>> On 2 August 2016 at 13:40, Shi, Steven <steven.shi@intel.com> wrote:
>>>>>> 
>>>>>> CoffAddFixup() must be used for absolute symbol references only. These
>>>>>> instructions contain relative symbol references, which are
>>>>>> recalculated in WriteSections64().
>>>>>> 
>>>>>> The only absolute symbol reference is the GOT entry for 'n', and your
>>>>>> code (in WriteRelocations64()) calculates the address of the GOT entry
>>>>>> (which is always in .text BTW) and adds a fixup for it, i.e.,
>>>>>> 
>>>>>> +              CoffAddFixup(
>>>>>> +                (UINT32)(UINTN)((UINT64)
>>>>>> mCoffSectionsOffset[RelShdr->sh_info] + GoTPcRelPtrOffset),
>>>>>> +                EFI_IMAGE_REL_BASED_DIR64);
>>>>>> 
>>>>>> This code adds a fixup to the PE/COFF .reloc section for the GOT entry
>>>>>> containing the address of 'n', and the instructions perform a IP
>>>>>> relative load of the contents of the GOT entry to retrieve the address
>>>>>> of 'n'.
>>>>>> 
>>>>>> By adding two fixups, the PE/COFF loader will apply the load offset
>>>>>> twice, resulting in an incorrect value.
>>>>> OK, I get your point now. Yes, the current patch could generate multiple fixups for the same GOT relocation entry. How about we introduce a simple IsDuplicatedCoffFixup() to check whether a converting fixup offset is duplicated before we use CoffAddFixup() to really add it? If it is new, we add it, otherwise just skip it.
>>>> 
>>>> That could work, but you have to be aware that fixups are best emitted
>>>> in the order they need to be applied in the binary, or it will become
>>>> very inefficient. (Please refer to the PE/COFF spec section that
>>>> explains the layout of the .reloc section)
>>>> 
>>>> What it comes down to is that relocations are grouped by target page,
>>>> and for every place in the page that requires a relocation to be
>>>> applied, a 4 bit type is emitted followed by a 12-bit offset, which is
>>>> the offset into the current page. If you emit fixups for the current
>>>> instruction, followed by one for the GOT, it will basically take two
>>>> 'page switches' every time.
>>>> 
>>>> So it would be better to simply emit the relocations, but introduce a
>>>> sorting pass that merges all duplicates as well.
>>>> 
>>>> Thanks,
>>>> Ard.
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel