> On Jun 17, 2015, at 3:01 PM, Cohen, Eugene <eug...@hp.com> wrote: > > Thanks for the great feedback -- you made me do more homework than I was > expecting which is not a bad thing. > > Since we understand the problem and the fix, I think it's time to get > Olivier's review and approval to move forward. >
Eugene, One question. Does this make the minimum PEIM size > 64K? The original reason for the 32 byte alignment in the PE/COFF images was to make PEIMs smaller, the default is 4K alignment for sections. Thanks, Andrew Fish > Thanks, > > Eugene > > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Tuesday, June 16, 2015 2:51 PM > To: edk2-devel@lists.sourceforge.net; Olivier Martin > Subject: Re: [edk2] [PATCH] BaseTools for AArch64 GCC: Ensure that the > correlation .text and .data is consistent between ELF and PE-COFF so the > debugger sees global variables correctly > > On 16 June 2015 at 22:32, Cohen, Eugene <eug...@hp.com> wrote: >>> OK. so does that mean we are using a builtin linker script for >>> AArch64? That sounds fragile to me ... >> >> Apparently we are using the builtin script. I learned that this can be >> queried with the -verbose switch for ld and have attached it. It includes >> this interesting alignment mechanism: >> >> /* Adjust the address for the data segment. We want to adjust up to >> the same address within the page on the next page up. */ >> . = ALIGN(CONSTANT (MAXPAGESIZE)) + (. & (CONSTANT (MAXPAGESIZE) - 1)); >> >> MAXPAGESIZE doesn't appear to be well documented but I can see patches to ld >> that show this being increased to 64KB which is consistent with my objdump >> output before the change. So I think this is the "offending" line in the >> original script since it shifted data out in ELF even though the PE-COFF >> converter packed it tightly, accounting for the section alignment >> requirements from ELF. >> >>> Could you please look at the X64 approach, and compare it to yours? >> >> It's really a question for the developer originating -Ttext=0x0 - maybe >> Olivier? I don't have the history on how the GCC linker configuration for >> edk2 came to be. >> >>> Perhaps you could share some numbers or other details to get a feel >>> for what exactly goes on here. >> >> I think I described in this in my email titled " AArch64 Debugger Global >> Variable Correlation Issues" - I gave the ELF dump showing data shifted out >> by 64KB. Strangely, this was shifted out not to the next 64KB aligned >> boundary but to 0x10000 beyond the last .test/.rodata section - I'm not sure >> why but perhaps somebody with more GNU ld experience can figure out why. >> > > That is *precisely* what the expression above aims to achieve. It > wants to preserve the relative alignment of all the sections, but make > sure that there is a 64 KB aligned boundary in between so that the two > regions can always be mapped with different permissions even on a 64K > pagesize OS. I.e., the expression aligns to the next boundary, and > then adds the unaligned fraction of ".", which effectively just adds > MAXPAGESIZE unless "." is already 64K aligned (if I am not mistaken). > This matches your observation, right? > > I assume MAXPAGESIZE is a build time constant for ld, so there is not > a lot of wiggle room here unless we switch to a custom linker script. > > -- > Ard. > > > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >> Sent: Tuesday, June 16, 2015 9:21 AM >> To: edk2-devel@lists.sourceforge.net >> Subject: Re: [edk2] [PATCH] BaseTools for AArch64 GCC: Ensure that the >> correlation .text and .data is consistent between ELF and PE-COFF so the >> debugger sees global variables correctly >> >> On 16 June 2015 at 16:50, Cohen, Eugene <eug...@hp.com> wrote: >>>> Could you please send patches inline? Attachments are a pain to review. >>> >>> Sure, I will do that. Because I use Outlook I prefer the attachment since >>> I can pull it into a nice patch viewer. >>> >> >> I can see how that would be preferred for just viewing, but for >> commenting inline it is far from optimal. >> >>>> Applying your patch and doing 'git show --find-copies-harder' gives me >>>> (tools_def.tempate omitted): >>> >>> Unfortunately git found a copy that is not appropriate for comparison. It >>> found the X86/X64 linker script that, while similar, is different than what >>> we were already doing for AArch64. (I'm not saying the X86 approach >>> doesn't work but my goal here was to minimize the impact to what was >>> already being done for AArch64.) >>> >> >> OK. so does that mean we are using a builtin linker script for >> AArch64? That sounds fragile to me ... >> Could you please look at the X64 approach, and compare it to yours? >> Personally, I'd prefer to stay as close as possible to what is being >> done on Intel, for obvious reasons ... >> >> >>> On AArch64 the .text section was being placed with this linker switch: >>> >>> -Ttext=0x0 >>> >>> But this was not sufficient to pack the .data section in a manner that is >>> consistent with the PE-COFF conversion that happens later in the build. So >>> when I converted this to a linker control file, I maintained the zero >>> starting address with: >>> >>> . = 0; >>> >> >> OK, that parts seems obvious. So how is the packing of the .data >> section being affected by your version of the linker script? >> Perhaps you could share some numbers or other details to get a feel >> for what exactly goes on here. >> >>>> So are you saying that the resulting PE/COFF is identical (for all >>>> intents and purposes), and only the ELF intermediate file deviates? >>> >>> Yes, from my testing the PE/COFF looks the same but the ELF is updated to >>> move .data such that its offset relative to .text is consistent with >>> PE/COFF. This fixes debugger correlation for stuff in the .data section >>> like global variables. >>> >>>> How does this affect >>>> ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf, >>>> which outputs lines like >>> >>>> From my testing it has no effect since this output is only outputting the >>>> PE-COFF image address offset by the size of the PE-COFF headers (0x260 in >>>> this case): >>> >>> (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)) >>> >>> The reason the SizeOfHeaders is added is because the debugger is unaware of >>> the PE-COFF conversion and added executable headers. >>> >> >> OK, that makes sense. >> >>> -----Original Message----- >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >>> Sent: Tuesday, June 16, 2015 5:56 AM >>> To: edk2-devel@lists.sourceforge.net >>> Subject: Re: [edk2] [PATCH] BaseTools for AArch64 GCC: Ensure that the >>> correlation .text and .data is consistent between ELF and PE-COFF so the >>> debugger sees global variables correctly >>> >>> On 4 June 2015 at 21:48, Cohen, Eugene <eug...@hp.com> wrote: >>>> Oops, left off the contribution agreement line, trying again >>>> >>>> Dear ArmPkg maintainers (and later BaseTools maintainer), >>>> >>>> This is a fix for debugger correlation of global variables for AArch64 >>>> built on GCC. >>>> >>>> Before this change looking at global variables with a debugger showed >>>> bogus memory locations. This is because the offset of the .data section in >>>> the ELF file did not reflect where it was placed in the PE-COFF (.efi) >>>> output. >>>> >>>> This change passes a linker control script so that the data section is >>>> packed next to .text so the ELF accurately reflects the relationship >>>> between the sections when converted to PE-COFF by GenFw. >>>> >>>> I have tested this with the Lauterbach debugger. I don't know how well it >>>> will work with other debuggers and debug scripts. >>>> >>>> If you would rather view the change as a github pull request: >>>> https://github.com/tianocore/edk2/pull/12 >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Eugene Cohen <eug...@hp.com> >>>> >>> >>> Could you please send patches inline? Attachments are a pain to review. >>> >>> Also, since the patch introduces a completely new linker script, it is >>> hard to review how yours deviates from the default. >>> Git is actually quite helpful since it can figure out if your newly >>> introduced file resembles an existing file, and only shows the diff >>> with respect to the original. >>> >>> Applying your patch and doing 'git show --find-copies-harder' gives me >>> (tools_def.tempate omitted): >>> >>> """ >>> diff --git a/BaseTools/Scripts/gcc4.4-ld-script >>> b/BaseTools/Scripts/gcc-arm-ld-script >>> similarity index 59% >>> copy from BaseTools/Scripts/gcc4.4-ld-script >>> copy to BaseTools/Scripts/gcc-arm-ld-script >>> index 68b2767590ac..e1589a4d03bf 100644 >>> --- a/BaseTools/Scripts/gcc4.4-ld-script >>> +++ b/BaseTools/Scripts/gcc-arm-ld-script >>> @@ -1,8 +1,10 @@ >>> -/* OUTPUT_FORMAT(efi-bsdrv-x86_64) */ >>> SECTIONS >>> { >>> - /* . = 0 + SIZEOF_HEADERS; */ >>> - . = 0x280; >>> + /* Start at 0 so we can meet more aggressive alignment requires >>> after the PE-COFF conversion >>> + like those for ARM exception vectors. This requires debugger >>> scripts to offset past >>> + the PE-COFF header (typically 0x260). When the PE-COFF >>> conversion occurs we will >>> + get proper alignment since the ELF section alignment is applied >>> in the conversion process. */ >>> + . = 0; >>> .text ALIGN(0x20) : >>> { >>> *(.text .stub .text.* .gnu.linkonce.t.*) >>> """ >>> >>> So are you saying that the resulting PE/COFF is identical (for all >>> intents and purposes), and only the ELF intermediate file deviates? >>> How does this affect >>> ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf, >>> which outputs lines like >>> >>> add-symbol-file >>> /home/ard/build/uefi-next/Build/ArmVirtQemu-AARCH64/DEBUG_GCC48/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll >>> 0x5F2C7260 >>> >>> where the .dll is and ELF file, and the line seems to incorporate the >>> header offset that you are removing. >>> >>> Regards, >>> Ard. >>> >>> ------------------------------------------------------------------------------ >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/edk2-devel >>> >>> ------------------------------------------------------------------------------ >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >> ------------------------------------------------------------------------------ >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >> ------------------------------------------------------------------------------ >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
