Skip to content

Re-enable FEATURE_CORPROFILER for tvOS to match iOS/MacCatalyst#126715

Open
steveisok wants to merge 1 commit intodotnet:mainfrom
steveisok:fix/tvos-reenable-corprofiler
Open

Re-enable FEATURE_CORPROFILER for tvOS to match iOS/MacCatalyst#126715
steveisok wants to merge 1 commit intodotnet:mainfrom
steveisok:fix/tvos-reenable-corprofiler

Conversation

@steveisok
Copy link
Copy Markdown
Member

Summary

PR #126550 disabled FEATURE_CORPROFILER for tvOS (AND NOT CLR_CMAKE_TARGET_TVOS uncommented) but left iOS, MacCatalyst, and Android commented out (profiler still enabled). There's no reason to single out tvOS from its sibling Apple mobile platforms — they should be treated as a package deal.

Problem

The uncommented AND NOT CLR_CMAKE_TARGET_TVOS line removed profiler fields from the Thread struct on tvOS (m_pProfilerFilterContext, m_profilerCallbackState, m_dwProfilerEvacuationCounters — 144 bytes total), making the hardcoded OFFSETOF__Thread__m_pInterpThreadContext in asmconstants.h wrong (0x2b8 vs actual 0x228), breaking all 3 tvOS legs in unified-build.

Fix

Comment out the tvOS line to match iOS/MacCatalyst/Android, keeping the profiler enabled on tvOS until all mobile platforms are ready to disable it together. This restores the original Thread struct layout and makes asmconstants.h correct again without needing offset changes.

Build failure

Internal unified-build on main (build 2946990) fails on 3/54 tvOS legs:

asmconstants.h:304:23: error: static assertion failed due to requirement
  '696 == __builtin_offsetof(Thread, m_pInterpThreadContext)'

Affected legs: tvOS_Shortstack_arm64, tvOSSimulator_Shortstack_x64, tvOSSimulator_Shortstack_arm64.

cc @AaronRobinsonMSFT @akoeplinger

PR dotnet#126550 disabled FEATURE_CORPROFILER for tvOS but left iOS,
MacCatalyst, and Android commented out (profiler still enabled).
There is no reason to single out tvOS from its sibling Apple mobile
platforms. They should be treated as a package deal.

The uncommented 'AND NOT CLR_CMAKE_TARGET_TVOS' line removed profiler
fields from the Thread struct on tvOS, making the hardcoded
OFFSETOF__Thread__m_pInterpThreadContext in asmconstants.h wrong
(0x2b8 vs actual 0x228), breaking all 3 tvOS legs in unified-build.

Comment out the tvOS line to match iOS/MacCatalyst/Android, keeping
the profiler enabled until all mobile platforms disable it together.

Fixes internal unified-build tvOS failures on main (build 2946990).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 9, 2026 15:48
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

@steveisok steveisok requested a review from kotlarmilos April 9, 2026 15:49
@steveisok steveisok requested a review from a team April 9, 2026 15:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR re-enables FEATURE_CORPROFILER for tvOS to keep feature-flag behavior consistent across Apple mobile platforms and to restore the expected Thread layout used by asmconstants.h static offset assertions.

Changes:

  • Comment out the tvOS-specific AND NOT CLR_CMAKE_TARGET_TVOS exclusion so tvOS matches the (currently enabled) iOS/MacCatalyst/Android behavior.
  • Adjust the CMake conditional formatting to accommodate the commented-out tvOS clause cleanly.

@AaronRobinsonMSFT
Copy link
Copy Markdown
Member

There's no reason to single out tvOS from its sibling Apple mobile platforms — they should be treated as a package deal.

I had considered them distinct and I'd argue they should be since they are specifically different "targets". I removed tvOS because it isn't a mobile target. The real issue here is the defines aren't correct and likely missing places where tvOS should keep something alive but is relying on an iOS define. Given the general mess of how we define these I'm fine adding it back, but it does indicate there are missing defines somewhere.

@akoeplinger
Copy link
Copy Markdown
Member

I removed tvOS because it isn't a mobile target.

We've historically called it "mobile" as well since it's very similar to iOS.

@AaronRobinsonMSFT
Copy link
Copy Markdown
Member

AaronRobinsonMSFT commented Apr 9, 2026

I removed tvOS because it isn't a mobile target.

We've historically called it "mobile" as well since it's very similar to iOS.

That is absolutely fine and I think including tvOS here makes sense. But the point is tvOS is clearly not a correctly defined target because it relies on something the iOS target is including. In fact, I'd almost prefer to have something that fails if someone defined tvOS and not iOS. tvOS needs something more and therefore should be explicitly coupled to iOS so this doesn't happen again.

@steveisok
Copy link
Copy Markdown
Member Author

tvOS needs something more and therefore should be explicitly coupled to iOS so this doesn't happen again.

You've got a legit point. This has some history as mono used unix signals and tvOS's signal handlers don't give access to ucontext_t. Seeing that coreclr uses mach exceptions + interpreter ios & tvos are basically the same.

@akoeplinger
Copy link
Copy Markdown
Member

there are more differences, e.g. tvOS can't use fork() etc, so it makes sense to have it as a separate target (albeit very similar to iOS). I was just trying to point out why we generally lumped them into the "mobile" category, compared to desktop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants