[Rebase&FF] pecoff: add get_section functionality#1409
[Rebase&FF] pecoff: add get_section functionality#1409Javagedes wants to merge 2 commits intoOpenDevicePartnership:mainfrom
get_section functionality#1409Conversation
✅ QEMU Validation PassedAll QEMU validation jobs completed successfully.
Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/23168269121 Boot Time to EFI Shell
This comment was automatically generated by the Patina QEMU PR Validation Post workflow. |
github-actions
bot
added
impact:non-functional
Javagedes
force-pushed
the
personal/joeyvagedes/section-parse
branch
from
23caab7 to
5e68499
Compare
| for _ in 0..directory.number_of_named_entries { | ||
| if directory_entry.name_is_string() { | ||
| if directory_entry.name_offset() >= size { | ||
| let name_start_offset = (directory_entry.name_offset() + 1) as usize; |
There was a problem hiding this comment.
I know this was pre-existing, but it seems a little strange for the name offset to start a byte (+ 1) in when a UTF-16 character is 2 bytes.
In the comment below: // L"HII" = [0x0, 0x48, 0x0, 0x49, 0x0, 0x49], the low-byte would typically have the ASCII character for UTF-16 little endian:
// L"HII" = [0x48, 0x0, 0x49, 0x0, 0x49, 0x0]
There was a problem hiding this comment.
If you want, I can also take this up separately since it is unrelated to your PR.
There was a problem hiding this comment.
@joschock may have a better answer to this, but I assume that it has to do with the name_offset() being tied to the ID, and thus needing to do an offset by 1, similar to 0 or 1 based indexing. But I'm not familiar with this enough to answer confidently when it is working as-is.
There was a problem hiding this comment.
I have no memory of this place :) - I do not know what the +1 is. Maybe it is trying to skip the "length" field? But the length field is 2 bytes...
There was a problem hiding this comment.
I'm going to resolve this. @makubacki would you like me to create an issue to have this investigated?
There was a problem hiding this comment.
Sure. I'm also interested to look in more detail if you want to assign me.
Create a new function called `get_section` that returns the byte slice representing the requested PE/COFF section from the specified image. This logic is directly pulled from the `load_resource_section` and moved into it's own function.
This commit removes an allocation done in `get_section` by comparing the bytes of the two string, rather than allocating a new string based off the bytes.
Javagedes
force-pushed
the
personal/joeyvagedes/section-parse
branch
from
5e68499 to
7e819dc
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
4 participants
Description
This commit pulls the functionality for getting a sub-slice that represents a particular PE/COFF section from an unloaded image out of
load_resource_sectionand makes it a re-usable function. It also removes an unnecessary allocation when comparing section names.NOTE: The git diff does not do this change justice due to each line's "depth" being adjusted. There is no change in logic, except for the second commit, which removes the allocation.
How This Was Tested
CI continues to work. Boot on Q35.
Integration Instructions
N/A