Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the redundant MDExceptionCodeLinux enum and use the values from minidump-common instead #62

Merged

Conversation

gabrielesvelto
Copy link
Contributor

No description provided.

@Jake-Shadle
Copy link
Collaborator

So interesting, there seems to be a difference between 1.64 and 1.65 on Windows, I could build and test successfully on Windows with this PR on 1.64.0 but then switched to 1.65.0 and got the same crash as CI did, so at least it's fully reproducible.

@Jake-Shadle
Copy link
Collaborator

So it looks like the dump_current_process test crashes inside RtlCaptureContext, I've actually had issues with that function in the past, I think I've found a workaround for now which is to build and run test in release, but I'll add an issue to look into this for later, but considering that code path only gets hit in the not-recommended-use-at-your-own-risk dump_local_context I wouldn't consider it super high priority, though still worrying that (seemingly) a rust update changed...something.

@Jake-Shadle Jake-Shadle merged commit 42c74f1 into rust-minidump:main Nov 4, 2022
@gabrielesvelto
Copy link
Contributor Author

That function does magic stuff with the stack so it's possible LLVM is hitting some edge-case where it generates incompatible code. Anyway WDYT about that clippy warning? I suppose we could Box<> the (large) errors but it would change the external API so I didn't want to go there just to appease it (even though it would have other benefits).

@Jake-Shadle
Copy link
Collaborator

I just wanted to unblock this change for now so I silenced it, but I think before the next release of this crate it would be good to actually amp up the clippy lints and actually address all of them or at least have better reasoning for why certain ones get silenced since overall it makes things better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants