Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When calculating the image slide, the difference between the image's preferred load address and the TEXT segment, we faithfullly ported the Breakpad implementation:
breakpad
https://github.com/google/breakpad/blob/335e61656fa6034fabc3431a91e5800ba6fc3dc9/src/client/mac/handler/dynamic_images.cc#L269-L272
minidump-writer
minidump-writer/src/mac/streams/module_list.rs
Lines 97 to 101 in 1a1e912
This was, however, a mistake, as that logic was just incorrect as shown by the crashpad implementation
crashpad
https://github.com/chromium/crashpad/blob/df86075acc33314e611b351b33bf1c671b8cbc2f/snapshot/mac/mach_o_image_reader.cc#L585-L589
When removing the if and unconditionally calculating the slide, the
images_match
test that I added started passing which gives me a bit more confidence that the implementation is (more) correct now than the one copied from Breakpad.While I was here, the test code needed a bit of refactoring in the minidumpwriter since that was easier than pulling out the logic into a reusable piece (it's maybe a good idea to do....but I also don't want us to end up in a crashpad scenario with a mountain of obtuse abstractions), so this addresses the Mac part of #37, and I'll do the same for Windows in a separate PR to close the issue.
Part of: #37
Resolves: #43