edk2 icon indicating copy to clipboard operation
edk2 copied to clipboard

BaseTools: Update alignment for entry seg for Clang

Open dhaval-rivos opened this issue 1 year ago • 3 comments

It does 3 things:

  1. Use separate linker file for clang vs GCC for RISCV64.
  2. Use common page size instead of max page to ensure -z option align values are properly applied.
  3. Enforce alignment for .entry segment as per -z option.

When we want to have -z option aligned images, clang while alignes .text and .data segments correctly, .entry segment is by default not aligned unless explicitly specified. This patch makes it explicit to clang that entry seg should also be aligned to requirements. Somehow GCC does not require such explicit entry. Hence detachiong both ld files.

Description

While working on fixing an alignment error during image loading (RISCV64), I realized that clang and gcc behave differently. Specifically when "-z common-page-size=0x1000" is extended for all UEFI driver types, dxecore started to fail. The failure was due to incorrectly calculated global variable offsets. Upon digging further I found that in case of clang, .entry segment does not have any alignment requirement and this causes it to be loaded at PECOFF_HEADER_SIZE as per ld. This is not -z aligned and causes issue. How does it work with gcc as they share the same linker file: In case of gcc, .entry segment is placed AFTER .text. .text as per linker file is already -z aligned and everything just seem to work fine there.

I have couple of options:

  1. To add this fix in gccbase.ld and let both gcc/clang continue to share the same linker file.
  2. separate out linker for clang (like in this patch) and have clang deal with it.
  • [ ] Breaking change? No
  • [ ] Impacts security? No
  • [ ] Includes tests? No

How This Was Tested

Tested on both GCC and CLANG with and without -z aligned options

Integration Instructions

NA

dhaval-rivos avatar Oct 13 '24 12:10 dhaval-rivos

WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.

Non-collaborators requested:

  • @bexcran

Attn Admins:

  • @jljusten
  • @lgao4
  • @mdkinney
  • @jkmathews
  • @miki-intel-work

Admin Instructions:

  • Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
  • If they are no longer needed as reviewers, remove them from Maintainers.txt

ClangBase.lds is also used for IA32 and X64 arch. IA32 and X64 archs configure -z max-page-size=0x40. This change will imapct them. Please confirm.

lgao4 avatar Oct 17 '24 05:10 lgao4

ClangBase.lds is also used for IA32 and X64 arch. IA32 and X64 archs configure -z max-page-size=0x40. This change will imapct them. Please confirm.

Yes. It will impact x86 as well as it sounds like it uses the same linker file. Is someone from x86 side able to confirm this works as expected?

dhaval-rivos avatar Oct 17 '24 07:10 dhaval-rivos

@lgao4 wdyt? Are you/someone able to verify/confirm this fix is okay or other option I have is to separate out RISC-V ld. However I believe this fix should be common. @vlsunil

dhaval-rivos avatar Nov 11 '24 15:11 dhaval-rivos

@lgao4 wdyt? Are you/someone able to verify/confirm this fix is okay or other option I have is to separate out RISC-V ld. However I believe this fix should be common. @vlsunil

I suggest you also change below definition from max-page-size to common-page-size. If so, ClangBase.lds can be used for IA32, X64, RISC-V

DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-q,--gc-sections -z max-page-size=0x40

lgao4 avatar Dec 06 '24 01:12 lgao4

@lgao4 wdyt? Are you/someone able to verify/confirm this fix is okay or other option I have is to separate out RISC-V ld. However I believe this fix should be common. @vlsunil

I suggest you also change below definition from max-page-size to common-page-size. If so, ClangBase.lds can be used for IA32, X64, RISC-V

DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-q,--gc-sections -z max-page-size=0x40

Done. Please check. If okay I will rebase and you can help merge?

dhaval-rivos avatar Dec 08 '24 15:12 dhaval-rivos

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

mergify[bot] avatar Dec 11 '24 00:12 mergify[bot]

@lgao4 wdyt? Are you/someone able to verify/confirm this fix is okay or other option I have is to separate out RISC-V ld. However I believe this fix should be common. @vlsunil

I suggest you also change below definition from max-page-size to common-page-size. If so, ClangBase.lds can be used for IA32, X64, RISC-V DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-q,--gc-sections -z max-page-size=0x40

Done. Please check. If okay I will rebase and you can help merge?

Seemly, this patch can't be rebased. Could you update this patch based on the latest master?

lgao4 avatar Dec 11 '24 00:12 lgao4

@dhaval-rivos , and all,

After this patch, the UefiPayloadPkg ELF entry module binary will miss Dynamic section which causing bootloader rebasing ASSERT hang issue, do you have idea?

ASSERT hang: UefiPayloadPkg\PayloadLoaderPeim\ElfLib\Elf64Lib.c: // // It's abnormal a DYN ELF doesn't contain a dynamic section. // ASSERT (DynShdr != NULL); if (DynShdr == NULL) { return EFI_UNSUPPORTED; }

You may try to build UefiPayloadPkg by below steps to reproduce the issue: (I use Windows build, not sure if Linux build also could reproduce)

  1. install LLVM from https://github.com/llvm/llvm-project/releases/tag/llvmorg-19.1.7
  2. install VisualStudio2019 (other version should be ok too)
  3. switch to edk2 root and execute edksetup
  4. switch to UefiPayloadPkg
  5. execute py UniversalPayloadBuild.py -t VS2019
  6. remember to clean Conf file between each builds to ensure template files will be re-created

Also attaching the binary and ELF header dump for your reference: build_rebuild_revert.zip is without this patch build_rebuild_new.zip is with this patch

build_rebuild_revert.zip build_rebuild_new.zip

ChaselChiu avatar Jan 24 '25 05:01 ChaselChiu

Which linker script is used for this use case. The GCC one discards dynamic sections.

https://github.com/tianocore/edk2/blob/feb8d49834b12e1eab3f3052e21433b9d535ac4e/BaseTools/Scripts/GccBase.lds#L84

mdkinney avatar Jan 24 '25 06:01 mdkinney

Which linker script is used for this use case. The GCC one discards dynamic sections.

https://github.com/tianocore/edk2/blob/feb8d49834b12e1eab3f3052e21433b9d535ac4e/BaseTools/Scripts/GccBase.lds#L84

we use CLANGDWARF so my understanding is the clang linker script will be used.

ChaselChiu avatar Jan 24 '25 16:01 ChaselChiu

2 more data adding here, it looks like we may need to use 4KB alignment. Would you check from your side too?

the header dump having below warning message if the UPL binary built with this commit: C:\git\edk2\Build\UefiPayloadPkgX64>llvm-objdump --all-headers UniversalPayload.elf > new_llvm_19.1.7.txt llvm-objdump.exe: warning: 'UniversalPayload.elf': dynamic sections must be DT_NULL terminated

per @mdkinney suggestion, I based on this commit but modifying common-page-size to 0x1000, I found issue cannot reproduce. The above warning message gone and ELF dynamic section is present. attaching the 4k build binary and header dump for your reference.

build_new_4k.zip

ChaselChiu avatar Jan 24 '25 17:01 ChaselChiu

Can the 4KB alignment be applied to the UefiPayloadPkg build only with [BuildOptions] section?

mdkinney avatar Jan 24 '25 20:01 mdkinney

Can the 4KB alignment be applied to the UefiPayloadPkg build only with [BuildOptions] section?

yes, just verified and this works too. I sent a PR to set common-page-size=0x1000 in UPL entry build: https://github.com/tianocore/edk2/pull/10673

ChaselChiu avatar Jan 25 '25 03:01 ChaselChiu