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
