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

[Jordan Justen <jordan.l.justen@intel.com>]

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.

-Jordan

> 
> > 
> >> 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