Add initial Windows implementation#19
Conversation
|
Oops, looks like minidump has changed, I'll fix this tomorrow. |
|
Oh, and add windows tests as well. |
|
Hmm, so I got the in process dump test working easily, but the external one doesn't work due to some problems internally in MiniDumpWriteDump failing to do |
|
Hi @Jake-Shadle. Aria asked me to help do a first-pass review of this, as she is currently very busy on another project. I should have some review comments ready for you soon. Sorry about the wait! |
|
No worries, thanks! |
marti4d
left a comment
There was a problem hiding this comment.
This is very nice, and it's great that you also wrote tests for it! Thanks a bunch for this, and sorry about the long wait for this review.
Most of my comments below are just minor suggestions and nitpicks -- You can probably ignore them if you don't agree; however, I have 3 concerns:
- I think there may be reversed logic in
windows/minidump_writer.rs#230 - You mention that
let mut user_streams = Vec::with_capacity(2);may interfere with in-process dumping. If that's the case, does it make sense that we are providing acurrent_process()function to allow a behavior that won't work properly? - I saw your comments above, and for the record I also see the failure of the
dump_external_processtest with the same code. Does it make sense to disable that test until it's fixed?
WRT to the failing test, I've done a bit of investigation and it looks like if you pass std::ptr::null() in place of exc_info during the call to MiniDumpWriteDump, it works... So the issue seems to be stemming from that.
It's getting a bit late for me right now, but tomorrow I will continue investigating --It looks like it should work, so it's fairly intriguing that it doesn't.
marti4d
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback so quickly! I have a few more optional suggestions, but as far as I'm concerned this code is ready to check in once we figure out why that test is broken.
I'm investigating that right now, so hopefully we can get that resolved quickly and merge this change 😀
This fixes the "ReadProcessMemory" failure in the MiniDumpWriteDump() call. The root issue is that Cargo is being used to run the test executable, and so `child.id()` returns Cargo's PID and not the PID of the test.exe executable. This has the test.exe pass its actual PID back to the parent so it can be debugged properly. Note that this doesn't fully fix the test -- There is still another error
|
@Jake-Shadle I left a PR to fix the broken test on your Embark Studios fork. Once you merge that in, I think this PR should be good-to-go! 😀 |
Fix dump_external_process test
marti4d
left a comment
There was a problem hiding this comment.
@Gankra Hey Aria! IMO this change is good-to-go and I'm confident to merge it in at anytime.
Thanks a lot, @Jake-Shadle. This is great :)
See EmbarkStudios/crash-handling#9 where it is being used.