-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix EventPipe on Android CoreClr. #112270
Fix EventPipe on Android CoreClr. #112270
Conversation
EventPipe CMake was causing issues when building CoreClr due to creating two versions of ep-shared-config.h with different set of variables when targeting iOS/tvOS/Android. Both include paths ended up being added and together with EventPipe's unity build, this caused issues since the wrong ep-shared-config.h was picked up, incorrect enabled default listeners as result of having wrong values for FEATURE_PERFTRACING_DISABLE_DEFAULT_LISTEN_PORT and FEATURE_PERFTRACING_PAL_TCP. Mono doesn't have this issue since it only creates one ep-shared-config.h. Commit makes sure we only setup on ep-shared-config.h and eventpipe libraries adds the include folder to its library targets. Commit also adds support to set DOTNET_DiagnosticPorts as part of building Android sample to simplify enable/disable diagnostics, similar to how Mono Android sample works. Couple of smaller adjustments to ApkBuilder.
bfeec11
to
77b9c7a
Compare
// NOTE, using DOTNET_DiagnosticPorts requires app build using AndroidAppBuilder and RuntimeComponents to include 'diagnostics_tracing' component | ||
#ifdef DIAGNOSTIC_PORTS | ||
setenv ("DOTNET_DiagnosticPorts", DIAGNOSTIC_PORTS, true); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the same for AppleAppBuilder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean for NAOT on iOS? Not sure if that has been enabled yet. But once we do, then we could add this for iOS inline with what we do on Mono.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(NativeAOT for iOS shipped since .NET 8; CoreCLR for iOS Simulator landed on main last month)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I know, but what I don't know if NATO + iOS also includes EventPipe support, do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the CoreCLR for iOS Simulator I believe we should do similar work to make sure EventPipe is build using right flags + include similar support in AppleAppBuilder, but we could do that as a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I know, but what I don't know if NATO + iOS also includes EventPipe support, do you know?
Yes, it does, in theory...although I am not sure if it ever worked or if it was tested. The GC heap events were not wired so I never bothered to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventpipe infra changes look good to me
* main: (41 commits) Automated bump of chrome version (dotnet#112309) Add `GetDeclaringType` to `PropertyDefinition` and `EventDefinition`. (dotnet#111646) Update the System.ComponentModel.Annotations solution to build in VS (dotnet#112313) JIT: initial support for stack allocating arrays of GC type (dotnet#112250) [main] Update dependencies from dotnet/roslyn (dotnet#112260) Update Xcode casing (dotnet#112307) update the location of assert for REG_ZR check (dotnet#112294) Enable `SA1206`: Keyword ordering (dotnet#112303) Address feedback on dense FrozenDictionary optimization (dotnet#112298) Start regular pri-1 tests runs with native AOT (dotnet#111391) Observe exceptions from _connectionCloseTcs (dotnet#112190) Test failure - SendAsync_RequestVersion20_ResponseVersion20 (dotnet#112232) Fix init race in mono_class_try_get_[shortname]_class. (dotnet#112282) Remove repeated call to DllMain (dotnet#112285) Replace bitvector.h/cpp with ptrArgTP type in gc_unwind_x86.h/inl (dotnet#112268) JIT: Limit 3-opt to 1000 swaps per run (dotnet#112259) [main] Update dependencies from dotnet/icu, dotnet/runtime-assets (dotnet#112120) Update dependencies from https://github.com/dotnet/emsdk build 20250205.3 (dotnet#112223) Fix EventPipe on Android CoreClr. (dotnet#112270) Fix exception handling in the prestub worker (dotnet#111937) ...
EventPipe CMake was causing issues when building CoreClr due to creating two versions of ep-shared-config.h with different set of variables when targeting iOS/tvOS/Android. Both include paths ended up being added and together with EventPipe's unity build, this caused issues since the wrong ep-shared-config.h was picked up, incorrect enabled default listeners as result of having wrong values for FEATURE_PERFTRACING_DISABLE_DEFAULT_LISTEN_PORT and
FEATURE_PERFTRACING_PAL_TCP. Mono doesn't
have this issue since it only creates one ep-shared-config.h.
Commit makes sure we only setup on ep-shared-config.h and eventpipe libraries adds the include folder to its library targets.
Commit also adds support to set DOTNET_DiagnosticPorts as part of building Android sample to simplify enable/disable diagnostics, similar to how Mono Android sample works.
Couple of smaller adjustments to ApkBuilder.