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

Store unhandled exception info on stack in Native AOT #84871

Merged
merged 4 commits into from
Apr 27, 2023

Conversation

agocke
Copy link
Member

@agocke agocke commented Apr 15, 2023

Allocates a span on the stack to hold the ToString of the exception, as well as a CrashRecord which contains a unique cookie that can be used to find the exception info.

Fixes #84636

@ghost
Copy link

ghost commented Apr 15, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: agocke
Assignees: agocke
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky
Copy link
Member

Would it make sense to also dump the ToString for the innermost InnerException? I'm just thinking how this would look like for various async goo and chances are the 1024 bytes are going to get eaten by uninteresting stuff.

Alternatively, would it make sense to call RhGetCurrentThreadStackBounds to see how much stack we have available and potentially allocate up to say, 50% of what's available? Not much is going to happen after this method so it feels it might be okay to get greedy?

@agocke agocke marked this pull request as ready for review April 18, 2023 20:47
@agocke
Copy link
Member Author

agocke commented Apr 18, 2023

@MichalStrehovsky What would you suggest for the inner exception info? Trying to concat both infos? Or just only store the inner exception if one exists?

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Apr 19, 2023

@MichalStrehovsky What would you suggest for the inner exception info? Trying to concat both infos? Or just only store the inner exception if one exists?

Ideally @tommcdon or @mikem8361 could guide us to what is more important from diagnostics perspective. We could have just the inner, or we could add another pointer in CrashDumpRecord that points to it.

Right now, the Exception.ToString will have both stacks and both messages (unless someone overrode the ToString since we don't seal it) - it might get truncated, we don't know. It really depends on how structured the data should be. Whether a duplication would be okay. Whether we should only do it if there was truncation. Etc.

It's why I asked for a "spec" on how this should look like.

@jkotas
Copy link
Member

jkotas commented Apr 19, 2023

more important from diagnostics perspective

It is hard to judge what's more important. I think the best we can do here is to try to format the complete message if there is enough space.

@tommcdon
Copy link
Member

Ideally @tommcdon or @mikem8361 could guide us to what is more important from diagnostics perspective. We could have just the inner, or we could add another pointer in CrashDumpRecord that points to it.

We have an upcoming .NET 8 backlog item on the diagnostics team to investigate what is needed to include an a Native AOT dump so that it is actionable. So I think it is a bit too soon for the diagnostics team to have a strong opinion on specific design details. Feel free to move forward with the change as we can make changes to the algorithm as needed.

@agocke
Copy link
Member Author

agocke commented Apr 25, 2023

Anything else that I should do here? Or is this ready to go?

@jkotas jkotas changed the title Attempt to fix #84636 Store unhandled exception info on stack in Native AOT Apr 25, 2023
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Looks good to me (modulo a few cleanup nits). We can adjust the implementation as necessary in future.

@agocke
Copy link
Member Author

agocke commented Apr 26, 2023

Yup, I missed the remaining GetExceptionsForCurrentThread pieces. Should be gone now.

@agocke
Copy link
Member Author

agocke commented Apr 27, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@agocke
Copy link
Member Author

agocke commented Apr 27, 2023

Test failures look unrelated.

@agocke agocke merged commit 5d65dd6 into dotnet:main Apr 27, 2023
@agocke agocke deleted the save-aot-exception branch April 27, 2023 16:56
@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2023
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.

Store unhandled exception info on stack in Native AOT
4 participants