[edk2] 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 <eugene@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 <eugene@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):

"""
+  . = 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.

------------------------------------------------------------------------------

On 16 June 2015 at 16:50, Cohen, Eugene <eugene@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.biesheuvel@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 <eugene@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 <eugene@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

------------------------------------------------------------------------------

On 16 June 2015 at 22:32, Cohen, Eugene <eugene@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.

On 18 June 2015 at 00:01, Cohen, Eugene <eugene@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.
>

I would still prefer to use the existing GCC ld script if possible
instead of rolling our own. Since the existing one does not have the
64 KB gap between the RX and RW segments, and it probably also dodges
the 'Multiple sections' error.

So perhaps Olivier can explain where the -Ttext=0x0 comes from? It
looks like everything works fine without it ...