Fix Thread offset for non-desktop platforms without FEATURE_CORPROFILER#126710
Fix Thread offset for non-desktop platforms without FEATURE_CORPROFILER#126710steveisok wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
Updates CoreCLR ARM64 assembly constants to account for tvOS builds where profiler-related Thread fields are compiled out (when FEATURE_CORPROFILER disables PROFILING_SUPPORTED / PROFILING_SUPPORTED_DATA), ensuring OFFSETOF__Thread__m_pInterpThreadContext matches the actual struct layout and fixes the unified-build static_assert failure.
Changes:
- Add
PROFILING_SUPPORTED || PROFILING_SUPPORTED_DATAconditionals forOFFSETOF__Thread__m_pInterpThreadContextin Unix Debug/Release cases. - Introduce alternative offset values for non-profiling builds (e.g.,
0x2b8→0x228,0xb20→0xa90).
src/coreclr/vm/arm64/asmconstants.h
Outdated
| #if defined(PROFILING_SUPPORTED) || defined(PROFILING_SUPPORTED_DATA) | ||
| #define OFFSETOF__Thread__m_pInterpThreadContext 0xb20 | ||
| #else | ||
| #define OFFSETOF__Thread__m_pInterpThreadContext 0xa90 | ||
| #endif |
There was a problem hiding this comment.
This fixes the ARM64 offsets when profiling fields are compiled out, but the same Thread layout change applies to other 64-bit targets with FEATURE_CORPROFILER disabled (notably tvOSSimulator x64). src/coreclr/vm/amd64/asmconstants.h still hardcodes OFFSETOF__Thread__m_pInterpThreadContext without a PROFILING_SUPPORTED || PROFILING_SUPPORTED_DATA guard, so its ASMCONSTANTS_C_ASSERT is likely to fail in non-profiling builds. Please apply the same conditional offset selection there (and any other affected arch asmconstants) so all tvOS legs are covered.
|
#126550 turned off FEATURE_CORPROFILER on iOS but not tvOS, that's why we didn't see it on iOS |
That feels like a mistake. @AaronRobinsonMSFT was there a reason to keep tvos disabled? I see that as a package deal with iOS / MacCatlyst. |
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>
steveisok
force-pushed
the
fix/tvos-asmconstants-profiler-offset
branch
from
1cd4703 to
a1d0b19
Compare
steveisok
requested review from
a team,
BrzVlad,
MichalStrehovsky,
akoeplinger,
janvorli,
jeffhandley and
kg
as code owners
steveisok
changed the title
steveisok
changed the title
Summary
PR #126550 disabled
FEATURE_CORPROFILERfor tvOS but left Android, MacCatalyst, and iOS commented out as placeholders. This removed profiler fields from theThreadstruct (m_pProfilerFilterContext,m_profilerCallbackState,m_dwProfilerEvacuationCounters— 144 bytes total), shiftingm_pInterpThreadContextto a lower offset without updatingasmconstants.h.Changes
1.
src/coreclr/vm/arm64/asmconstants.hAdd
PROFILING_SUPPORTED || PROFILING_SUPPORTED_DATAconditionals to select the correctm_pInterpThreadContextoffset:The existing
ASMCONSTANTS_C_ASSERTvalidates these values at compile time.2.
src/coreclr/clrfeatures.cmakeUncomment the Android, MacCatalyst, and iOS exclusions so all non-desktop platforms consistently disable
FEATURE_CORPROFILER. Sinceasmconstants.hnow handles both cases, all mobile platforms can safely disable profiling without hitting the same offset mismatch.Per review feedback: #126550 should have treated tvOS the same as the other mobile platforms (all commented out, or all active). This PR completes the intended work by enabling all four mobile platforms to disable profiling together, with the assembly constants properly updated.
Build failure
Internal unified-build on
main(build 2946990) fails on 3/54 tvOS legs:Affected legs: tvOS_Shortstack_arm64, tvOSSimulator_Shortstack_x64, tvOSSimulator_Shortstack_arm64.
cc @AaronRobinsonMSFT @kotlarmilos @akoeplinger