From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by ml01.01.org (Postfix) with ESMTP id 1646D1A1DF8 for ; Wed, 3 Aug 2016 13:53:31 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 03 Aug 2016 13:53:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,467,1464678000"; d="scan'208";a="1007932549" Received: from mnakanis-mobl.amr.corp.intel.com (HELO localhost) ([10.252.138.185]) by orsmga001.jf.intel.com with ESMTP; 03 Aug 2016 13:53:30 -0700 MIME-Version: 1.0 To: Ard Biesheuvel , Message-ID: <147025761018.14160.4581842911325157321@jljusten-ivb> From: Jordan Justen In-Reply-To: <61A48C30-DDE7-40EF-B266-B06014C35991@linaro.org> Cc: "Shi, Steven" , "Kinney, Michael D" , "edk2-devel-01" , "afish@apple.com" , "Gao, Liming" , mischief@offblast.org References: <1467967364-11556-1-git-send-email-steven.shi@intel.com> <06C8AB66E78EE34A949939824ABE2B31033826FE@shsmsx102.ccr.corp.intel.com> <06C8AB66E78EE34A949939824ABE2B310338275F@shsmsx102.ccr.corp.intel.com> <06C8AB66E78EE34A949939824ABE2B3103383852@shsmsx102.ccr.corp.intel.com> <147025523821.2704.13575318169553931780@jljusten-ivb> <61A48C30-DDE7-40EF-B266-B06014C35991@linaro.org> User-Agent: alot/0.3.7 Date: Wed, 03 Aug 2016 13:53:30 -0700 Subject: Re: [PATCH v2 2/7] BaseTools-GenFw:Add new x86_64 Elf relocation types for PIC/PIE code X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Aug 2016 20:53:31 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2016-08-03 13:47:09, Ard Biesheuvel wrote: > = > > On 3 aug. 2016, at 22:13, Jordan Justen wro= te: > > = > > 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 wrote: > >>>> = > >>>> CoffAddFixup() must be used for absolute symbol references only. The= se > >>>> 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 ent= ry > >>>> (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 ent= ry > >>>> containing the address of 'n', and the instructions perform a IP > >>>> relative load of the contents of the GOT entry to retrieve the addre= ss > >>>> 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 multi= ple fixups for the same GOT relocation entry. How about we introduce a simp= le IsDuplicatedCoffFixup() to check whether a converting fixup offset is du= plicated before we use CoffAddFixup() to really add it? If it is new, we ad= d 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