Refactor...a lot

[Jake-Shadle]

I'm opening this as a draft as my intention is to split this into multiple PRs tomorrow, I just wanted to post this to get any initial feedback and just generally spread awareness.

A little background, I had started doing my own reimplementation of Breakpad's signal handling and minidump writing before I was made aware that this crate existed, so I kind of stopped on the minidump writing part and folded this project into the code I had (see the PR EmbarkStudios/crash-handling#1), so most of the changes came about from using this crate in production code already, as it allows us to catch crashes when targeting musl, which was tedious due to all of the Breakpad C++ code.

The high level changes are basically:

• Use the crash_context::CrashContext structure which allows the crash context to be easily shared between multiple crates, as one of the goals of this crate and the general effort around it with mozilla/sentry was to de-monolith the old breakpad structure. crash_context defines its own ucontext_t and friends rather than relying on libc's, as well as its own getcontext, as libc's has some rather unfortunate differences between libc implementations, most notably for musl as eg. getcontext has been deprecated for decades and has no replacement, but...is still useful for testing. Since musl was my primary target initially, all this code means that this crate now works for musl, resolving Doesn't build on x86_64 musl #4.
• This crate only used minidump/minidump-common for testing purposes, but this meant that there was an enormous amount of duplication with regards to the minidump structures that this crate is filling out and writing, so I changed it and removed all of the duplicated types that are already available in minidump-common, and added scroll::Pwrite derives for all of the relevant structures that this crates writes. rust-minidump/rust-minidump@d7a5a24
• Shifted the linux specific code into a linux module and renamed the crate minidump-writer as I want to add Windows support next, and I assume Mac support as well will come, so having the crate named minidump_writer_linux felt too specific.
• Replaced a lot of verbose #cfg usage with cfg_if as it reduces messy boilerplate and makes things easier to read.

Things I probably shouldn't have changed:

• Cleanup the file structure, there were a ton of mod.rs files that made the codebase annoying to navigate
• Cleaned up a ton of clippy lints

Things I broke:

I haven't been compiling or testing on anything except x86/_64 so none of the arm/aarch64/mips code paths even compile right now, but I hope to address those tomorrow, or in follow up PRs.

[msirringhaus]

Urgh, I messed up my tabs and posted my original answer to the wrong PR. Here it is:

From a first 'quick' glance, I can say that its.... long :D
No, overall, it looks good. I need more time to go through it all again. I've just had two small comments right away, which are now inlined 'reviews'.

Apart from that:

1. About ucontext_t: Do you plan to stay with your own implementation, or upstream those changes to libc, so that it does 'the right thing' ™️ ? I think it would be beneficial to upstream in the long run.
2. Also, are you aware of WIP android64 support #12 ?
3. At the time of writing this crate rust-minidump was sort of unmaintained for a while, hence the duplications. But I think I remember that there where also some incompatible struct definitions (although this might have changed by now). This needs a more thorough analysis and testing.

Let's pick this up on Monday again and have a great weekend!

[Jake-Shadle]

Yah, again, sorry about that, I'll split it out into smaller PRs that are easier to review/digest.

1. Yes, I think that would be beneficial, this was mainly to be able to iterate quickly, but that being said, I am a bit worried about some things not being merged or waiting in review a long time, because of previous PRs like, and would still need to exist at least for a getcontext implementation so that it works in eg musl etc that don't bother implementing it since it has been deprecated from POSIX

• macOS getcontext/setcontext rust-lang/libc#2712
• linux musl bring ucontext api for related archs rust-lang/libc#2430
• Update mcontext_t definition on musl, add fpstate struct rust-lang/libc#1646

2. Yes, I had my eye on it today since I decided to implement aarch64-unknown-linux-gnu/musl support today so that I could test some of the aarch64 specific code paths I broke
3. Ahh I see, I don't think I ran into any of these incompatibilities (at least, all the tests pass on x86_64) but I suppose that depends on what/where the incompatibilities were.

[msirringhaus]

Thanks for your work! I guess we can close this one now?

[Jake-Shadle]

Yup!