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

Better createdump error messages #69663

Merged
merged 7 commits into from
May 26, 2022
Merged

Better createdump error messages #69663

merged 7 commits into from
May 26, 2022

Conversation

mikem8361
Copy link
Member

Redirect stderr when launching createdump to get any error text.

Added a new generate core dump ipc message that allows a error message string
to be returned for more detail on createdump errors.

Fixes or improves:
#54521
#64069
dotnet/diagnostics#3064
dotnet/diagnostics#2976

@ghost
Copy link

ghost commented May 23, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Redirect stderr when launching createdump to get any error text.

Added a new generate core dump ipc message that allows a error message string
to be returned for more detail on createdump errors.

Fixes or improves:
#54521
#64069
dotnet/diagnostics#3064
dotnet/diagnostics#2976

Author: mikem8361
Assignees: mikem8361
Labels:

area-Diagnostics-coreclr

Milestone: -

src/coreclr/vm/eventing/eventpipe/ds-rt-coreclr.h Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.h Outdated Show resolved Hide resolved
Redirect stderr when launching createdump to get any error text.

Added a new generate core dump ipc message that allows a error message string
to be returned for more detail on createdump errors.
Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

A couple of additional smaller comments. Apart from that I believe the EventPipe related changes look good!

src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.h Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
@mikem8361
Copy link
Member Author

Does anybody know how to code a empty ep_char16_t string that works across all our platforms, OS's and runtimes? (const ep_char16_t*)u"" fails the wasm linux builds.

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

I'll do another pass later today on the createdump half of things. The EventPipe side of things looks good to me 👍

src/coreclr/pal/src/thread/process.cpp Outdated Show resolved Hide resolved
src/native/eventpipe/ds-dump-protocol.c Outdated Show resolved Hide resolved
Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

Just a couple nits. Other than that this LGTM 👍

@mikem8361 mikem8361 merged commit 595f7ed into dotnet:main May 26, 2022
@mikem8361 mikem8361 deleted the errormsgs branch May 26, 2022 18:39
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2022
@@ -2399,16 +2431,36 @@ PROCCreateCrashDump(std::vector<const char*>& argv)
{
// Ignore any error because on some CentOS and OpenSUSE distros, it isn't
// supported but createdump works just fine.
ERROR("PROCCreateCrashDump: prctl() FAILED %d (%s)\n", errno, strerror(errno));
ERROR("PROCCreateCrashDump: prctl() FAILED %s (%d)\n", errno, strerror(errno));
Copy link
Member

Choose a reason for hiding this comment

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

Did you forget to swap errno and strerror(errno)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, looks like I did. I'll fix it in a future PR.

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

Successfully merging this pull request may close these issues.

4 participants