BaseTools: Add support for preserving build ID #1609
Conversation
Adds a optional flag that will copy the build-id section from the input ELF file to the output firmware image. The Build ID is used for debugging and allows tools to match the firmware image with the correct debug symbols.
| // | ||
| if (mBuildIdFlag) { | ||
| if (mBuildIdFound) { | ||
| CreateSectionHeader (".bldid", mBuildIdOffset, mRelocOffset - mBuildIdOffset, |
There was a problem hiding this comment.
I'm on the fence here. It feels odd to change the section name from .build-id to .bldid. This is currently being done because of the direct section size name limit. Longer names can be used, but required introducing complexity to genfw. My initial thinking is that this image already doesn't conform to image standards so the rename is worth it to avoid adding complexity, though perhaps that is lazy rationalization. Thoughts?
There was a problem hiding this comment.
Is changing the section name going to affect any existing ELF parsing tools that work or kind of work on the franken-image? i.e. is WinDbg going to look for .build-id when we tell it this in actually an ELF image? Presumably not or you wouldn't be going for .bldid. Will it affect gdb debugging this and seeing this .bldid?
I would tend to leave the section name as is if it isn't a huge amount of complexity. But, I don't have a strong feeling on it. I would suggest taking this to edk2 and getting feedback there (we should upstream this anyway).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/202502 #1609 +/- ##
=================================================
Coverage ? 2.22%
=================================================
Files ? 1460
Lines ? 380899
Branches ? 4584
=================================================
Hits ? 8477
Misses ? 372349
Partials ? 73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // | ||
| if (mBuildIdFlag) { | ||
| if (mBuildIdFound) { | ||
| CreateSectionHeader (".bldid", mBuildIdOffset, mRelocOffset - mBuildIdOffset, |
There was a problem hiding this comment.
Is changing the section name going to affect any existing ELF parsing tools that work or kind of work on the franken-image? i.e. is WinDbg going to look for .build-id when we tell it this in actually an ELF image? Presumably not or you wouldn't be going for .bldid. Will it affect gdb debugging this and seeing this .bldid?
I would tend to leave the section name as is if it isn't a huge amount of complexity. But, I don't have a strong feeling on it. I would suggest taking this to edk2 and getting feedback there (we should upstream this anyway).
| } | ||
|
|
||
| if (!mBuildIdFound) { | ||
| Warning (NULL, 0, 0, NULL, "Build ID section is not found in %s.", mInImageName); |
There was a problem hiding this comment.
In InitializeElf64() you had:
if (mBuildIdFlag) {
mCoffNbrSections ++;
}Does mCoffNbrSections need to be decremented heere if the build-id section is not found?
| { | ||
| Elf_Shdr *Namedr = GetShdrByIndex(mEhdr->e_shstrndx); | ||
|
|
||
| return (BOOLEAN) (strcmp((CHAR8*)mEhdr + Namedr->sh_offset + Shdr->sh_name, ELF_BUILD_ID_SECTION_NAME) == 0); |
There was a problem hiding this comment.
It feels like there should be additional validation to check bounds here. Is that covered somewhere else? For example, in other places something like this checks the offset against the file size.
if (Shdr->sh_offset + Shdr->sh_size > mFileBufferSize) {
return FALSE;
}5 participants
Description
Adds an optional flag that will copy the build-id section from the input ELF file to the output firmware image. The Build ID is used for debugging and allows tools to match the firmware image with the correct debug symbols.
How This Was Tested
Tested on Q35 & SBSA with GCC5 toolchain
Integration Instructions
N/A