Bug 1831094: Add a process reader and module reader for each platform#168
Bug 1831094: Add a process reader and module reader for each platform#168afranchuk wants to merge 2 commits intorust-minidump:mainfrom
Conversation
32c1c29 to
af7d19a
Compare
af7d19a to
72f80ad
Compare
|
Sorry I've been holding off reviewing this one but I was waiting for PR #167 first given the changes there are more urgent and both PRs end up touching the same functionality. |
|
Sounds good, I'll rebase once that is merged. |
|
@afranchuk I was talking to @gabrielesvelto yesterday. I don't want to hold up changes to Honestly -- If I have to redo the part where I go through the existing codebase and redirect all the I/O, that's not the major difficulty of the change anyway -- Everything else is 😆. |
gabrielesvelto
left a comment
There was a problem hiding this comment.
LGTM, I just left a couple of questions
| } | ||
| } | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
I know these were in the original code but are the #[inline] directives really useful?
There was a problem hiding this comment.
For this method at least, perhaps not. Maybe it's to encourage the compiler to elide the enum variant checks to have less branching on that? I can see an argument for wanting reading bytes to be optimized. But then again, there's plenty of error-checking that needs to be done here anyway, so there's still overhead for every call.
| macro_rules! read_scroll { | ||
| ( $T:ty , $mem:expr , $offset:expr , $ctx:expr ) => {{ | ||
| let bytes = $mem.read($offset, <$T>::size_with(&$ctx) as u64)?; | ||
| let (value, _) = <$T>::try_from_ctx(bytes.as_ref(), $ctx)?; | ||
| value | ||
| }}; | ||
| } | ||
|
|
||
| macro_rules! read_scroll_array { | ||
| ( $T:ty , $mem:expr , $offset:expr , $count:expr , $ctx:expr ) => {{ | ||
| let bytes = $mem.read($offset, (<$T>::size_with(&$ctx) * $count) as u64)?; | ||
| let mut vals = Vec::with_capacity($count); | ||
| let mut offset = 0; | ||
| for _ in 0..$count { | ||
| let (value, bytes_read) = <$T>::try_from_ctx(&bytes[offset..], $ctx)?; | ||
| offset += bytes_read; | ||
| vals.push(value); | ||
| } | ||
| vals | ||
| }}; | ||
| } |
There was a problem hiding this comment.
Is there any particular reason why you used scroll specifically for the macOS module reader but not the other platforms?
There was a problem hiding this comment.
It wasn't very intentional: we already had code for linux, so I didn't touch that much, and on windows the goblin interface provides some convenience methods to get what we need. The goblin macOS interface wasn't as convenient (it doesn't have a lazy interface), so I read it directly instead.
This better reflects what the type represents. This also absorbs the `ProcessReader` type, which was poorly named as well, and adds a variant which takes a MemReader by reference.
3d33022 to
0812de8
Compare
This renames the linux `MemReader` to be the `ProcessReader`, and adds new implementations for mac and windows. The `ModuleReader`s have the minimum features needed; they can be filled out more in the future.
0812de8 to
07c2bd2
Compare
This merges features from the firefox process_reader crate for bug 1831094, in a step toward using
minidump-writerto read crash annotations on its own.