diff mbox

[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

Message ID CAKv+Gu-Vh9yXjKn_1u9NWo9EauVDLBAB_PT=iPKpz560pMua+w@mail.gmail.com
State New
Headers show

Commit Message

Ard Biesheuvel June 22, 2015, 7:28 p.m. UTC 
On 22 June 2015 at 19:26, Olivier Martin <Olivier.Martin@arm.com> wrote:
> So I am guessing it is fined to keep 0x260 used by the X64 linker script - and so reuse the existing GCC ld script. Debuger scripts would need to be updated to refect this change.
>
> If I compare the results with and without Eugene's patch:
>
> Without:
> --------
> /work/gcc-linaro-4.9-2015.02-3-x86_64_arm-linux-gnueabihf/bin/arm-linux-gnueabihf-readelf -S Build/ArmJuno-default/DEBUG_GCC49/AARCH64/ArmPkg/Drivers/TimerDxe/TimerDxe/DEBUG/ArmTimerDxe.dll
> There are 23 section headers, starting at offset 0x317a0:
>
> Section Headers:
>   [Nr] Name              Type             Address           Offset
>        Size              EntSize          Flags  Link  Info  Align
>   [ 0]                   NULL             0000000000000000  00000000
>        0000000000000000  0000000000000000           0     0     0
>   [ 1] .text             PROGBITS         0000000000000000  00010000
>        0000000000004468  0000000000000000  AX       0     0     8
>   [ 2] .rela.text        RELA             0000000000000000  00031d60
>        00000000000024d8  0000000000000018          21     1     8
>   [ 3] .rodata           PROGBITS         0000000000004468  00014468
>        00000000000018c8  0000000000000000   A       0     0     8
>   [ 4] .data             PROGBITS         0000000000015d30  00015d30
>        0000000000000190  0000000000000000  WA       0     0     8
>   [ 5] .rela.data        RELA             0000000000000000  00034238
>        0000000000000408  0000000000000018          21     4     8
>   [ 6] .bss              NOBITS           0000000000015ec0  00015ec0
>        0000000000000048  0000000000000000  WA       0     0     8
> (...)
>
> With:
> -----
> /work/gcc-linaro-4.9-2015.02-3-x86_64_arm-linux-gnueabihf/bin/arm-linux-gnueabihf-readelf -S Build/ArmJuno/DEBUG_GCC49/AARCH64/ArmPkg/Drivers/TimerDxe/TimerDxe/DEBUG/ArmTimerDxe.dll
> There are 20 section headers, starting at offset 0x317b0:
>
> Section Headers:
>   [Nr] Name              Type             Address           Offset
>        Size              EntSize          Flags  Link  Info  Align
>   [ 0]                   NULL             0000000000000000  00000000
>        0000000000000000  0000000000000000           0     0     0
>   [ 1] .text             PROGBITS         0000000000000000  00010000
>        0000000000004480  0000000000000000  AX       0     0     8
>   [ 2] .rela.text        RELA             0000000000000000  00031cb0
>        00000000000024d8  0000000000000018          18     1     8
>   [ 3] .data             PROGBITS         0000000000004480  00014480
>        0000000000001aa0  0000000000000000  WA       0     0     8
>   [ 4] .rela.data        RELA             0000000000000000  00034188
>        0000000000000408  0000000000000018          18     3     8
> (...)
>
> Some regions have disappeared such as .comment (ok!), but also .bss and .rodata (which I am more concerned).
> I have the impression the linker script combine these regions into .data (I confirm the size of these 3 regions is equal to 0x1aa0). It means it would change the PE/COFF binary. We can lose in memory protection if we decide later to make the '.rodata' section a ReadOnly region.
>
> I also do not understand why we have .rela.text between .text and .data sections. I would expect this section to be present after .data if your script was working as expected.
>

I have been spending some time investigating this myself. I agree that
-Ttext=0x0 seems to be redundant, so we should be able to reuse the
X86 script. However, there are some other considerations:
- the x86 script puts .rodata in .data, which is something I would
prefer to avoid since we are moving towards stricter permissions, and
putting .rodata in .data strips it from its constness (I don't think
we're quite there yet in UEFI 2.5 but it would be nice if
EfiRuntimeCode would map cleanly to R-X and EfiRuntimeData to RW-)
- if we put .text at 0x800 and align it to 0x800, we could potentially
drop the -mcmodel=large configuration, at least for PE32 modules,
i.e., something like

"""
SECTIONS {

  .text 0x800 : ALIGN(0x800) {
    *(.text .text.* .rodata .rodata.*)
  }
  .data : ALIGN(0x40) {
    *(.data .data.*)
    *(.bss .bss.* *COM*)
  }
  .rela ALIGN(0x20) : {
    *(.rela .rela.*)
  }

  /DISCARD/ : {
    *(.note.GNU-stack)
    *(.interp)
    *(.dynsym)
    *(.dynstr)
    *(.dynamic)
    *(.hash)
    *(.comment)
  }
}
"""

This should also solve Eugene's problem, since it results in the ELF
and PE/COFF binary to be laid out identically. It also results in all
code sections to be at the same alignment relative to 4K, which is
required for the small C model that uses ADRP/ADD and ADRP/LDR pairs
instead of 64-bit indirect addressing for globals.

I managed to build and run ArmVirtQemu.dsc with the above script,
-mcmodel=large removed and ADRP and related relocations ignored by the
BaseTools, and this patch applied on top of that




> -----Original Message-----
> From: Olivier Martin [mailto:Olivier.Martin@arm.com]
> Sent: 22 June 2015 17:43
> To: Cohen, Eugene; 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
>
> The text section at zero is there because GNU toolchain used to make this section at 0x8000(? - I am not sure of the value but it was not zero).
> So the debugging script would not even work for debugging the code section.
>
> I have just checked with the latest Linaro GCC toolchain and I have not managed to duplicate the issue. Both AArch32 and AArch64 GNU toolchains generate text section at 0x0 by default.
>
>
> -----Original Message-----
> From: Cohen, Eugene [mailto:eugene@hp.com]
> Sent: 18 June 2015 14:57
> 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
>
>> 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.
>
> Sure, I'm fine with that so long as it works.
>
>> So perhaps Olivier can explain where the -Ttext=0x0 comes from? It
>> looks like everything works fine without it ...
>
> Olivier, do you know if the text at zero is required or can we adopt the X86 placement in BaseTools/Scripts/gcc-arm-ld-script?
>
> Eugene
>
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, June 17, 2015 10:48 PM
> 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 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 ...
>
> --
> Ard.
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@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 <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.
>>
>> --
>> Ard.
>>
>>
>>
>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@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 <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.i
>>>> nf,
>>>> which outputs lines like
>>>>
>>>> add-symbol-file
>>>> /home/ard/build/uefi-next/Build/ArmVirtQemu-AARCH64/DEBUG_GCC48/AARC
>>>> H64/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
>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
>
> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782
>
>
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
>
> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590
> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782
>
>
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors
> network devices and physical & virtual servers, alerts via email & sms
> for fault. Monitor 25 devices for free with no restriction. Download now
> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
diff mbox

Patch

diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index 73d088a3bbdb..de81379105d1 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -313,14 +313,14 @@  [Rule.Common.SEC]

 [Rule.Common.PEI_CORE]
   FILE PEI_CORE = $(NAMED_GUID) {
-    TE     TE Align = 8                 $(INF_OUTPUT)/$(MODULE_NAME).efi
+    TE     TE Align = 4K                   $(INF_OUTPUT)/$(MODULE_NAME).efi
     UI     STRING ="$(MODULE_NAME)" Optional
   }

 [Rule.Common.PEIM]
   FILE PEIM = $(NAMED_GUID) {
      PEI_DEPEX PEI_DEPEX Optional       $(INF_OUTPUT)/$(MODULE_NAME).depex
-     TE       TE Align = 8              $(INF_OUTPUT)/$(MODULE_NAME).efi
+     TE       TE Align = 4K              $(INF_OUTPUT)/$(MODULE_NAME).efi
      UI       STRING="$(MODULE_NAME)" Optional
   }