PE: Support multiple debug directories and VCFeature, Repro, ExDllCharacteristics, POGO parsers #403
Conversation
|
@kkent030315 thanks for this PR, I think if we fix the basic nits this is ready to go, thank you! |
|
I'd also like to see this one go in for sure :) |
|
@m4b Thank you for the review! I'm going to mess with this PR very soon. |
- Added decent docs for `pe::debug` structs and constants - Added integration tests for `pe::debug::DebugData`
kkent030315
changed the title
|
@m4b Hi, this PR has been refactored, please take a look #403 (comment) for up-to-date changelists. |
kkent030315
changed the title
There was a problem hiding this comment.
initial review, did not dive in deeply; main concern was initially with the allocations; I can't recall offhand, but there is probably some trait you can implement to allow find to work with your iterator, to prevent the allocation; you'll likely need clone/copy on the iterator since it'll likely consume it, but that should be fine.
also would like to understand why the breaking change is necessary, can you give some motivation for that?
lastly, thank you so much (as usual!) for your incredible documentation on the code you commit, truly outstanding stuff!
src/pe/debug.rs
Outdated
| Self::parse_with_opts(bytes, idd, &options::ParseOptions::default()) | ||
| } | ||
|
|
||
| pub fn parse_with_opts( | ||
| bytes: &'a [u8], | ||
| idd: &ImageDebugDirectory, | ||
| idd: ImageDebugDirectoryIterator<'_>, |
There was a problem hiding this comment.
Yes, it is avoidable. This PR was initially designed and assumed subject to breaking change, while I'm pretty sure we could do that w/o actually breaking it. I give it a shot.
There was a problem hiding this comment.
@m4b Alright, everything looks improved and there are no explicit alloc's left there. On the other hand, a breaking pub change in debug::DebugData::parse_with_opts_and_fixup for TE seems inevitable at this time. We actually can let them alone, but that's out of our original purpose to support multiple debug data directories.
Perhaps, there's a two ideas:
- Let the TE breaking changes alone and release the rest as minor changes,
Then, do the TE breaking changes separately in a 0.10 rollup. - Or, rollup everything, including the breaking TE change, in this PR as-is.
Which one do you prefer?
This PR addresses the issue #314.
- The change is straightforward. It makes theDebugData::image_debug_directory: ImageDebugDirectoryintoVec<ImageDebugDirectory>as someone pointed out in the #314 (comment). So this is to be an breaking change.- There is another addition of VC feature metadataIMAGE_DEBUG_TYPE_VC_FEATURE(IMAGE_DEBUG_VC_FEATURE_ENTRY) in the debug directory.If anyone have suggestion of making this semantics without breaking the backward compatibility I am open to discuss.
update:
DebugDataand its parsers for comfortable code that makes multiple debug directories in mind.TE::fixup_debug_datahas been merged toDebugData::parse_*: AsDebugDatais now more compatible with TE (Terse Executable) by addingDebugData::parse_with_opts_and_fixupand fixup RVA field inDebugDataandImageDebugDirectoryIteratorin order if any fixup is required forImageDebugDirectory::address_of_raw_dataandImageDebugDirectory::pointer_to_raw_data.pe::debug.IMAGE_DEBUG_TYPE_VC_FEATURE.IMAGE_DEBUG_TYPE_REPRO.IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS.IMAGE_DEBUG_TYPE_POGO.DebugDataparsers.impl ImageDebugDirectoryand it has been merged toDebugData::parse_*.