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

[NativeAOT] Fix formatting of assertion failures and fail-fasts #91300

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Aug 29, 2023

Fix a long standing TODO. Formatting of assertion failures and fail-fasts in native AOT matches regular CoreCLR with this change.

Debug.Assert(false, "Something is not right.");

Before:

Unhandled Exception: System.Diagnostics.DebugProvider+DebugAssertException: Something is not right.
   at System.Diagnostics.DebugProvider.Fail(String, String) + 0x49
   at System.Diagnostics.Debug.Fail(String, String) + 0x5a
   at System.Diagnostics.Debug.Assert(Boolean, String, String) + 0x2d
   at System.Diagnostics.Debug.Assert(Boolean, String) + 0x28
   at Program.<Main>$(String[] args) + 0x1d
   at test!<BaseAddress>+0x1d3a67
   at test!<BaseAddress>+0x1d3af5

After:

Process terminated. Assertion failed.
Something is not right.
   at System.Diagnostics.DebugProvider.Fail(String, String) + 0x49
   at System.Diagnostics.Debug.Fail(String, String) + 0x5a
   at System.Diagnostics.Debug.Assert(Boolean, String, String) + 0x2d
   at System.Diagnostics.Debug.Assert(Boolean, String) + 0x28
   at Program.<Main>$(String[] args) + 0x1d
   at repro!<BaseAddress>+0x1d51c7
   at repro!<BaseAddress>+0x1d5255
Environment.FailFast("Fatal error!");

Before:

Process terminated. Fatal error!

After:

Process terminated. Fatal error!
   at System.RuntimeExceptionHelpers.FailFast(String, Exception, String, RhFailFastReason, IntPtr, IntPtr) + 0x335
   at System.Environment.FailFast(String) + 0x35
   at Program.<Main>$(String[] args) + 0x1b
   at repro!<BaseAddress>+0x1d40c7
   at repro!<BaseAddress>+0x1d4155

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 29, 2023
@ghost ghost assigned jkotas Aug 29, 2023
@jkotas jkotas changed the title [NativeAOT] Fix formatting or assertion failures and fail-fasts [NativeAOT] Fix formatting of assertion failures and fail-fasts Aug 30, 2023
@ghost
Copy link

ghost commented Aug 30, 2023

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

Issue Details

Fix a long standing TODO. Formatting of assertion failures and fail-fasts in native AOT matches regular CoreCLR with this change.

Debug.Assert(false, "Something is not right.");

Before:

Unhandled Exception: System.Diagnostics.DebugProvider+DebugAssertException: Something is not right.
   at System.Diagnostics.DebugProvider.Fail(String, String) + 0x49
   at System.Diagnostics.Debug.Fail(String, String) + 0x5a
   at System.Diagnostics.Debug.Assert(Boolean, String, String) + 0x2d
   at System.Diagnostics.Debug.Assert(Boolean, String) + 0x28
   at Program.<Main>$(String[] args) + 0x1d
   at test!<BaseAddress>+0x1d3a67
   at test!<BaseAddress>+0x1d3af5

After:

Process terminated. Assertion failed.
Something is not right.
   at System.Diagnostics.DebugProvider.Fail(String, String) + 0x49
   at System.Diagnostics.Debug.Fail(String, String) + 0x5a
   at System.Diagnostics.Debug.Assert(Boolean, String, String) + 0x2d
   at System.Diagnostics.Debug.Assert(Boolean, String) + 0x28
   at Program.<Main>$(String[] args) + 0x1d
   at repro!<BaseAddress>+0x1d51c7
   at repro!<BaseAddress>+0x1d5255
Environment.FailFast("Fatal error!");

Before:

Process terminated. Fatal error!

After:

Process terminated. Fatal error!
   at System.RuntimeExceptionHelpers.FailFast(String, Exception, String, RhFailFastReason, IntPtr, IntPtr) + 0x335
   at System.Environment.FailFast(String) + 0x35
   at Program.<Main>$(String[] args) + 0x1b
   at repro!<BaseAddress>+0x1d40c7
   at repro!<BaseAddress>+0x1d4155
Author: jkotas
Assignees: jkotas
Labels:

area-NativeAOT-coreclr, needs-area-label

Milestone: -

@jkotas jkotas removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 30, 2023
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichalStrehovsky
Copy link
Member

Btw, can you think of an elegant way to get rid of the first one/two frames that don't have a name because they come from the compiler-generate startup code? I've been thinking of storing the address of Main somewhere and stop the stack walk when it's reached (we'll also need to make sure Main is never inlined) but I didn't particularly liked it so I've never implemented it.

@jkotas
Copy link
Member Author

jkotas commented Aug 30, 2023

Maybe we can implement a general support for [StackTraceHidden] in stacktrace metadata and then apply [StackTraceHidden] to the auto-generated code that we want to hide?

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Aug 30, 2023

Maybe we can implement a general support for [StackTraceHidden] in stacktrace metadata and then apply [StackTraceHidden] to the auto-generated code that we want to hide?

Sounds much better! Moved #91309 from runtimelab and added to 9.0.

@jkotas jkotas added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 30, 2023
@jkotas
Copy link
Member Author

jkotas commented Aug 30, 2023

I would like to re-validate it with the fix for #91298

@janvorli
Copy link
Member

janvorli commented Sep 1, 2023

I've just found that while coreclr spells it "Unhandled exception", NativeAOT says "Unhandled Exception". May be nice to unify that too.

@janvorli
Copy link
Member

janvorli commented Sep 1, 2023

Actually, there is one more difference I've not noticed before. The coreclr has a period at the end of the "Unhandled exception" while native aot has colon there.

Fix a long standing TODO. Formatting of assertion failures and fail-fasts in native AOT matches regular CoreCLR with this change.

```csharp
Debug.Assert(false, "Something is not right.");
```

Before:
```
Unhandled Exception: System.Diagnostics.DebugProvider+DebugAssertException: Something is not right.
   at System.Diagnostics.DebugProvider.Fail(String, String) + 0x49
   at System.Diagnostics.Debug.Fail(String, String) + 0x5a
   at System.Diagnostics.Debug.Assert(Boolean, String, String) + 0x2d
   at System.Diagnostics.Debug.Assert(Boolean, String) + 0x28
   at Program.<Main>$(String[] args) + 0x1d
   at test!<BaseAddress>+0x1d3a67
   at test!<BaseAddress>+0x1d3af5
```

After:
```
Process terminated. Assertion failed.
Something is not right.
   at System.Diagnostics.DebugProvider.Fail(String, String) + 0x49
   at System.Diagnostics.Debug.Fail(String, String) + 0x5a
   at System.Diagnostics.Debug.Assert(Boolean, String, String) + 0x2d
   at System.Diagnostics.Debug.Assert(Boolean, String) + 0x28
   at Program.<Main>$(String[] args) + 0x1d
   at repro!<BaseAddress>+0x1d51c7
   at repro!<BaseAddress>+0x1d5255
```

```csharp
Environment.FailFast("Fatal error!");
```

Before:
```
Process terminated. Fatal error!
```

After:
```
Process terminated. Fatal error!
   at System.RuntimeExceptionHelpers.FailFast(String, Exception, String, RhFailFastReason, IntPtr, IntPtr) + 0x335
   at System.Environment.FailFast(String) + 0x35
   at Program.<Main>$(String[] args) + 0x1b
   at repro!<BaseAddress>+0x1d40c7
   at repro!<BaseAddress>+0x1d4155
```
@jkotas jkotas merged commit 6e48c37 into dotnet:main Sep 5, 2023
@jkotas jkotas removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 5, 2023
@jkotas jkotas deleted the native-asserts branch September 11, 2023 20:51
@ghost ghost locked as resolved and limited conversation to collaborators Oct 12, 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.

3 participants