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

Write a MemoryInfoListStream on linux. #70

Merged

Conversation

afranchuk
Copy link
Contributor

This makes it easier for consumers to read the minidumps, rather than needing to parse linux-specific information.

Closes #8.

@afranchuk
Copy link
Contributor Author

This relies on changes to minidump-common, so is a draft for now.

This makes it easier for consumers to read the minidumps, rather than
needing to parse linux-specific information.

Closes rust-minidump#8.
The sanitized stacks test was previously not running the `Context::With`
case. Now that the `With`/`Without` tests are programmatically toggled
(to prevent such errors as that!), it is clear that there is something
weird with the context's stack pointer, which seems to rarely (never?)
lie in mapped memory. For now, this test is disabled.
@afranchuk afranchuk force-pushed the linux-memory-info-list-stream branch from f25207f to 7ef6250 Compare April 19, 2023 18:08
@afranchuk afranchuk marked this pull request as ready for review April 19, 2023 18:09
@afranchuk
Copy link
Contributor Author

Since a new release of minidump-common was released, this can move out of draft status.

@Jake-Shadle
Copy link
Collaborator

Thanks, I'll take a look at this tomorrow!

Copy link
Collaborator

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, this is a big change, but the added/improved testing definitely helps!

@afranchuk
Copy link
Contributor Author

@Jake-Shadle Yeah, I'm not sure if you saw but the changes to the tests exposed a bug in the tests which was hiding a failure (7ef6250). For now, I had no solution to the failure.

Copy link
Contributor

@gabrielesvelto gabrielesvelto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine save for one small comment I left. The refactoring resulting from using the procfs crate is really nice and I also like how you macro-ized the tests.

src/linux/ptrace_dumper.rs Outdated Show resolved Hide resolved
@afranchuk
Copy link
Contributor Author

CI failure seems to be from network errors.

@gabrielesvelto
Copy link
Contributor

I'm re-triggering the jobs, I'll merge the changes as soon as they all turn green

@gabrielesvelto
Copy link
Contributor

@Jake-Shadle Yeah, I'm not sure if you saw but the changes to the tests exposed a bug in the tests which was hiding a failure (7ef6250). For now, I had no solution to the failure.

I'm reworking that stuff in #78. Right now situations in which the stack pointer falls out of a mapped range just don't work.

@gabrielesvelto gabrielesvelto merged commit a737690 into rust-minidump:main May 2, 2023
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.

Evaluate writing out a MemoryInfoListStream
3 participants