8000 Write a MemoryInfoListStream on linux. by afranchuk · Pull Request #70 · rust-minidump/minidump-writer · GitHub
[go: up one dir, main page]

Skip to content

Write a MemoryInfoListStream on linux. - #70

Merged
gabrielesvelto merged 8 commits into
rust-minidump:mainfrom
afranchuk:linux-memory-info-list-stream
May 2, 2023
Merged

gabrielesvelto merged 8 commits into
rust-minidump:mainfrom
afranchuk:linux-memory-info-list-stream

Conversation

@afranchuk
Copy link
Copy Markdown
Collaborator

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

Closes #8.

@afranchuk
Copy link
Copy Markdown
Collaborator 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
afranchuk requested a review from Jake-Shadle 8000 as a code owner April 19, 2023 18:09
@afranchuk
Copy link
Copy Markdown
Collaborator Author

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

@Jake-Shadle
Copy link
Copy Markdown
Collaborator

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

@Jake-Shadle Jake-Shadle left a comment
Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator 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.

@gabrielesvelto gabrielesvelto left a comment
Copy link
Copy Markdown
Contributor

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.

Comment thread src/linux/ptrace_dumper.rs Resolved
@afranchuk
afranchuk requested a review from gabrielesvelto May 1, 2023 13:42
@afranchuk
Copy link
Copy Markdown
Collaborator Author

CI failure seems to be from network errors.

@gabrielesvelto
Copy link
Copy Markdown
Contributor

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

@gabrielesvelto
Copy link
Copy Markdown
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

0