Skip to content

[clr-android] Add CoreCLR debugger support for Android#10889

Draft
kotlarmilos wants to merge 1 commit intomainfrom
feature/coreclr-android-debugger
Draft

[clr-android] Add CoreCLR debugger support for Android#10889
kotlarmilos wants to merge 1 commit intomainfrom
feature/coreclr-android-debugger

Conversation

@kotlarmilos
Copy link
Member

Description

When building a CoreCLR Android app in Debug mode, the build now configures the gRPC-based remote debugger. It sets the profiler environment variables CORECLR_ENABLE_PROFILING, CORECLR_PROFILER, CORECLR_PROFILER_PATH so CoreCLR loads libremotemscordbitarget.so as a CoreCLR profiler at startup. It excludes the profiler library from JNI preload because it is loaded by the profiler. New MSBuild targets handle adb port forwarding and app launch for debug sessions.

Tested end-to-end on a Pixel 7a arm64 - profiler loads, gRPC connection establishes over adb, initialize return S_OK, and the host receives the CreateProcess callback.

Note: This depends on diagnostics Android changes which are in PR.

- Introduced targets for setting up and running the CoreCLR remote debugger.
- Added conditions to enable CoreCLR debugging based on project properties.
- Updated environment variables for CoreCLR profiling.
- Implemented tests to verify CoreCLR debugger environment variables and behavior in release builds.
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I think one thing that is missing is some kind of end-to-end test. Maybe it can't actually debug, but could a test do something like:

  • Build run an app with CoreCLR debugger enabled
  • Verify it launches
  • Use TcpClient or equivalent to just verify the debugger is listening

I think that would catch a lot of potential bugs.

Comment on lines +239 to +240
<Target Name="RunWithCoreClrDebugger"
DependsOnTargets="Install;_SetupCoreClrDebugger">
Copy link
Member

Choose a reason for hiding this comment

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

This is a new public target, what calls it?

I think I have some general questions on how this is supposed to integrate with IDEs.

Comment on lines +57 to +58
<AndroidEnableCoreClrDebugger Condition=" '$(AndroidEnableCoreClrDebugger)' == '' and '$(_AndroidRuntime)' == 'CoreCLR' and '$(Configuration)' != 'Release' ">true</AndroidEnableCoreClrDebugger>
<AndroidEnableCoreClrDebugger Condition=" '$(AndroidEnableCoreClrDebugger)' == '' ">false</AndroidEnableCoreClrDebugger>
Copy link
Member

Choose a reason for hiding this comment

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

Question about naming: should this just be DebuggerSupport? Same as feature switch? or a new name like EnableDebugger? We'd also unify with the existing Mono settings for the debugger.

Setting a single property would be ideal, and then it would do the same thing no matter what runtime is used.

Comment on lines +59 to +60
<CoreClrDebuggerPort Condition=" '$(CoreClrDebuggerPort)' == '' ">4711</CoreClrDebuggerPort>
<CoreClrDebuggerIsServer Condition=" '$(CoreClrDebuggerIsServer)' == '' ">1</CoreClrDebuggerIsServer>
Copy link
Member

Choose a reason for hiding this comment

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

Again, these it seems like we should choose names that don't have CoreClr in them and try to unify with what Mono has. Picking names like DebuggerPort and DebuggerIsServer -- and compare what Mono has right now.

public string? AndroidSequencePointsMode { get; set; }
public bool EnableSGenConcurrent { get; set; }
public string? CustomBundleConfigFile { get; set; }
public bool EnableCoreClrDebugger { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this could check:

  • Is it CoreCLR
  • Is the general purpose debugger MSBuild property set

Comment on lines +207 to +228
<AndroidAdb
ToolExe="$(AdbToolExe)"
ToolPath="$(AdbToolPath)"
AdbTarget="$(AdbTarget)"
Command="shell"
Arguments="setprop debug.coreclr.isserver $(CoreClrDebuggerIsServer)"
/>
<AndroidAdb
ToolExe="$(AdbToolExe)"
ToolPath="$(AdbToolPath)"
AdbTarget="$(AdbTarget)"
Command="shell"
Arguments="setprop debug.coreclr.ip 127.0.0.1"
/>
<AndroidAdb
ToolExe="$(AdbToolExe)"
ToolPath="$(AdbToolPath)"
AdbTarget="$(AdbTarget)"
Command="shell"
Arguments="setprop debug.coreclr.port $(CoreClrDebuggerPort)"
/>

Copy link
Member

Choose a reason for hiding this comment

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

These are a lot of adb calls, do they all need to be system properties? We also have the ability to bake environment variables into apps, or use dotnet run -e DOTNET_CORE_CLR_IP=127.0.0.1.

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.

3 participants