fix: Fetch ELF debug files as well just to be safe if it is a Windows minidump #555
Conversation
| } else { | ||
| None | ||
| }; | ||
|
|
There was a problem hiding this comment.
i fear this is the wrong place to fix this up, i expect it should be fixed in whoever calls this function to pass the right slice of FileTypes. but still looking for all the places where this could be caused
There was a problem hiding this comment.
https://github.com/getsentry/symbolicator/blob/master/crates/symbolicator/src/sources.rs#L462 might be closer to where this should be done
There was a problem hiding this comment.
In that case I think https://github.com/getsentry/symbolicator/blob/master/crates/symbolicator/src/services/symbolication.rs#L1716 might also be an option. This is assuming we're only seeing this issue with minidumps, though.
There was a problem hiding this comment.
I don't think so, MinidumpState::object_type is only used in
.list_files(). The other location is the one which ends up being used in .list_files().
|
there are a few integration tests left that i need to update and the changes required for those are enough of a rat's nest that i'm going to put them off until tomorrow |
crates/symbolicator/src/sources.rs
Outdated
| ObjectType::Macho => &[FileType::MachDebug, FileType::MachCode, FileType::Breakpad], | ||
| ObjectType::Pe => &[FileType::Pdb, FileType::Pe, FileType::Breakpad], | ||
| ObjectType::Elf => &[FileType::ElfDebug, FileType::ElfCode, FileType::Breakpad], | ||
| // There are instances where applications have been cross-compiled for some target |
There was a problem hiding this comment.
| // There are instances where applications have been cross-compiled for some target | |
| // There are instances where applications have been compiled for some target |
I don't think we can claim this is cross-compiling
Instructions for snapshot changesSentry runs a symbolicator integration test suite located at Follow these steps to update snapshots in Sentry:
|
| @@ -195,19 +218,12 @@ def _make_error_result(download_error, source="microsoft", bucket_type="http"): | |||
| "source": source, | |||
| }, | |||
| ] | |||
| if bucket_type != "http": | |||
There was a problem hiding this comment.
removed this because i fixed an error where i'd incorrectly left out the layout field on the non-http sources in the permissions tests, causing them to include dSYMs in their output.
| "location": "http://127.0.0.1:1234/msdl/wkernel32.pdb/FF9F9F7841DB88F0CDEDA9E1E9BFF3B51/wkernel32.pd_", | ||
| "source": "microsoft", | ||
|
|
||
| def _make_successful_result(filtered=False): |
There was a problem hiding this comment.
would recommend hiding whitespace changes for this block
|
i fixed the integration tests but my attempts to update snapshots in sentry has left them in pretty bad shape. those will have to be double checked before merging this PR in, but i think the changes here should be good. |
|
based on the CI results it looks like sentry doesn't need updating. i think we can infer based off of that that we can leave sentry as is and this should be safe to merge in without changes to that repo. i (and floris) did run some tests locally to be safe, and the results of those tests support the conclusion that no snapshot updates are needed in sentry. |
A first shot at fixing a regression where we're no longer fetchingELF DIFs for minidumps generated on Windows, likely caused by
#414.
Symbolicator now requests all known debug file types when encountering a minidump generated by a native executable. This means that minidumps produced by PEs, Machos and ELFs will trigger a search for PDBs, dSYMs, and ELFs if possible.