Skip to content

Write a MemoryInfoListStream on linux.#70

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

Write a MemoryInfoListStream on linux.#70
gabrielesvelto merged 8 commits intorust-minidump:mainfrom
afranchuk:linux-memory-info-list-stream

Conversation

@afranchuk
Copy link
Copy Markdown
Contributor

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

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

Comment thread src/linux/ptrace_dumper.rs Outdated
@afranchuk afranchuk requested a review from gabrielesvelto May 1, 2023 13:42
@afranchuk
Copy link
Copy Markdown
Contributor 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