[debugger] Calling embedded api and avoid changing System.Diagnostics.Debugger class. #6106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thaystg
changed the title
jonpryor
reviewed
Jul 20, 2021
| @@ -247,8 +247,13 @@ static void ManualJavaObjectDispose (Java.Lang.Object obj) | |||
| static void Initialize () | |||
| { | |||
| if (mono_unhandled_exception == null) { | |||
| #if NETCOREAPP | |||
There was a problem hiding this comment.
This feels "wasteful", in that we're using Reflection to create a delegate for a method which is directly convertible to the desired type without using Reflection.
Thus, I'd prefer:
#if NETCOREAPP
mono_unhandled_exception = monodroid_debugger_unhandled_exception;
#else // NETCOREAPP
var mono_UnhandledException = typeof (System.Diagnostics.Debugger)
.GetMethod ("Mono_UnhandledException", BindingFlags.NonPublic | BindingFlags.Static);
if (mono_UnhandledException != null)
mono_unhandled_exception = (Action<Exception>) Delegate.CreateDelegate (typeof(Action<Exception>), mono_UnhandledException);
#endif // NETCOREAPP
jonpryor
reviewed
Jul 20, 2021
| var mono_UnhandledException = typeof (System.Diagnostics.Debugger) | ||
| .GetMethod ("Mono_UnhandledException", BindingFlags.NonPublic | BindingFlags.Static); | ||
| #endif |
There was a problem hiding this comment.
Code convention: #else and #endif statements should have a comment "describing" the condition they're associated with, e.g.
#endif // NETCOREAPP
jonpryor
reviewed
Jul 20, 2021
| @@ -20,6 +20,12 @@ static void get_runtime_types () | |||
| "Mono_UnhandledException", BindingFlags.NonPublic | BindingFlags.Static); | |||
| if (mono_unhandled_exception_method == null) | |||
| AndroidEnvironment.FailFast ("Cannot find System.Diagnostics.Debugger.Mono_UnhandledException"); | |||
| #endif | |||
| #if NETCOREAPP | |||
| mono_unhandled_exception_method = typeof (Android.Runtime.JNIEnv).GetMethod ( | |||
There was a problem hiding this comment.
Instead of duplicating this logic, we should instead make JNIEnv.mono_unhandled_exception internal, so that we can directly access it here:
mono_unhandled_exception_method = JNIEnv.mono_unhandled_exception.Method;
grendello
approved these changes
Jul 21, 2021
|
@jonpryor now that it's already merged on dotnet/runtime preview 7, I think this is ready for review. Do I need to bump runtime version anywhere to get the exported function working? |
Kinda; dotnet-maestro does bumps, e.g. PR #6110, but PR #6110 (currently) bumps to dotnet/runtime@bd35632, which doesn't include Additionally, PR #6110 has introduced issues, e.g. satellite assembly handling has changed (?!): #6110 (comment) I don't know when we can bump main/rc1. Which leaves .NET 6 Preview 7; we have a PR to bump that as well: PR #6112. PR #6112 fails to build for not currently understood reasons, and PR #6112 currently bumps to commit dotnet/runtime@ ba08d9a305e2e0debeed1c96b3137b44305466b8, which doesn't include dotnet/runtime@743bd89. @thaystg: Would you be able to investigate why PR #6112 is failing to build? Once we get #6112 building and bumped to something that includes dotnet/runtime@743bd894c, we could re-target this PR (#6106) to target the release/6.0.1xx-preview7 branch. |
…er_unhandled_exception * origin/main: Bump to dotnet/installer@19943da 6.0.100-rc.1.21372.10 (dotnet#6110) [xaprepare] don't install microsoft-net-runtime-android workload (dotnet#6114) [Mono.Android] Use `mono_unhandled_exception` for NET6 (dotnet#6104) Bump to dotnet/installer@9c463710 6.0.100-rc.1.21369 (dotnet#6072) Bump to xamarin/xamarin-android-binutils/2.37-XA.1@77618674 v2.37 (dotnet#6096) [Mono.Android.Export] Fix DynamicDependency to JavaArray (dotnet#6105) [xaprepare] always delete ~/android-toolchain/dotnet (dotnet#6098) [CI] Add nuget to msi conversion and VS insert stage (dotnet#6030) [build] delete platform-31 folder on test jobs (dotnet#6103)
I think the build is finished and passed, can we merge? |
|
Commit message: [One .NET] Use new API for exception debugger notification (#6106)
Context: https://github.com/dotnet/runtime/pull/56071
Context: https://github.com/xamarin/xamarin-android/pull/4877
Context: https://github.com/xamarin/xamarin-android/pull/4927#discussion_r490457925
Context: https://github.com/xamarin/xamarin-android/pull/4927#issuecomment-875864999
Context: https://github.com/xamarin/monodroid/commit/3e9de5a51bd46263b08365ef18bed1ae472122d8
Context: https://github.com/xamarin/monodroid/commit/b0f85970102d43bab9cd860a8e8884d136d766b3
Context: https://github.com/xamarin/monodroid/commit/12a012e00b4533d586ef31ced33351b63c9de883
What should happen when an exception is thrown and a debugger is
attached?
This is in fact a loaded question: there's what Xamarin.Android
(Legacy) *has* done, vs. what .NET 6 Android for .NET does, vs.
what "should" happen.
What "should" happen is easiest:
1. We should behave like a "normal" Desktop .NET app when a debugger
is attached, AND
2. We shouldn't corrupt JVM state.
Unfortunately, (1)+(2) is currently not possible, in part because
Java doesn't have an equivalent to Windows' [two attempt][0] debugger
notification infrastructure.
See xamarin/xamarin-android#4877 for details.
What Legacy Xamarin.Android does is also detailed in
xamarin/xamarin-android#4877, and relies on the
`Debugger.Mono_UnhandledException()` method in order to alert an
attached debugger that there is an exception to show to the user.
However, `Debugger.Mono_UnhandledException()` never made it to the
`dotnet/runtime` repo. It never existed there.
PR dotnet/runtime#56071 added a new
`mono_debugger_agent_unhandled_exception()` Mono embedding API which
is equivalent to `Debugger.Mono_UnhandledException()` for use with
.NET 6 + MonoVM.
Update `src/Mono.Android` and `src/monodroid` so that
`mono_debugger_agent_unhandled_exception()` is used to alert the
debugger that an exception has been thrown at a JNI boundary.
This should allow .NET 6 + Android to have equivalent exception
handling semantics as legacy Xamarin.Android.
[0]: https://docs.microsoft.com/en-us/windows/win32/debug/debugger-exception-handling |
jonpryor
pushed a commit
that referenced
this pull request
Jul 23, 2021
[One .NET] Use Mono embedding API for exception debugger notification (#6106) Context: dotnet/runtime#56071 Context: #4877 Context: #4927 (comment) Context: #4927 (comment) Context: xamarin/monodroid@3e9de5a Context: xamarin/monodroid@b0f8597 Context: xamarin/monodroid@12a012e What should happen when an exception is thrown and a debugger is attached? This is in fact a loaded question: there's what Xamarin.Android (Legacy) *has* done, vs. what .NET 6 Android for .NET does, vs. what "should" happen. What "should" happen is easiest: 1. We should behave like a "normal" Desktop .NET app when a debugger is attached, AND 2. We shouldn't corrupt JVM state. Unfortunately, (1)+(2) is currently not possible, in part because Java doesn't have an equivalent to Windows' [two attempt][0] debugger notification infrastructure. See #4877 for details. What Legacy Xamarin.Android does is also detailed in #4877, and relies on the `Debugger.Mono_UnhandledException()` method in order to alert an attached debugger that there is an exception to show to the user. However, `Debugger.Mono_UnhandledException()` never made it to the `dotnet/runtime` repo. It never existed there. Thus, what .NET 6 Android for .NET *currently* does is…*nothing*. If an exception is thrown and a debugger is attached, the debugger is *not* notified. Eventually you'll get an unhandled exception, long after it was originally thrown; see commit c1a2ee7. PR dotnet/runtime#56071 added a new `mono_debugger_agent_unhandled_exception()` Mono embedding API which is equivalent to `Debugger.Mono_UnhandledException()` for use with .NET 6 + MonoVM. Update `src/Mono.Android` and `src/monodroid` so that `mono_debugger_agent_unhandled_exception()` is used to alert the debugger that an exception has been thrown at a JNI boundary. This should allow .NET 6 + Android to have equivalent exception handling semantics as legacy Xamarin.Android. [0]: https://docs.microsoft.com/en-us/windows/win32/debug/debugger-exception-handling
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Waiting this to be merged dotnet/runtime#56071