Uh oh!
There was an error while loading. Please reload this page.
Disable FEATURE_PGO on WebAssembly, iOS, TVOS, and MacCatalyst - #126493
Conversation
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/9be1dcea-b234-4f56-ad13-b3f3578acce2 Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
…feedback Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ec2398a4-0129-4cef-80da-5eef6c0ca54b Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
davidwrighton
commented
Apr 3, 2026
@copilot the build fails with errors like Please fix. |
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/10208c32-9727-496d-ae23-933873d32c46 Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/coreclr/vm/pgo.cpp:1257
- In the !FEATURE_PGO stub for PgoManager::getPgoInstrumentationResults, not all out parameters are initialized:
ppSchemaandpPgoSourceare left untouched. Callers may inspect these even on failure (e.g., to distinguish "no data" vs "mismatch"), so leaving them uninitialized risks undefined behavior. Initialize*ppSchemato NULL and*pPgoSourceto ICorJitInfo::PgoSource::Unknown (similar to the FEATURE_PGO implementation).
HRESULT PgoManager::getPgoInstrumentationResults(MethodDesc* pMD, BYTE **pAllocatedData, ICorJitInfo::PgoInstrumentationSchema** ppSchema, UINT32 *pCountSchemaItems, BYTE**pInstrumentationData, ICorJitInfo::PgoSource* pPgoSource)
{
*pAllocatedData = NULL;
*pCountSchemaItems = 0;
*pInstrumentationData = NULL;
return E_NOTIMPL;
Note This comment was generated by Copilot reviewing PR #126493. Code Review: Making |
| # | Check | Verdict |
|---|---|---|
| 1 | All g_pConfig PGO accessor call-sites guarded | ✅ |
| 2 | No PGO-only symbols reachable when FEATURE_PGO undefined | |
| 3 | pgo.cpp stubs match pgo.h declarations | |
| 4 | Architectural consistency under FEATURE_DYNAMIC_CODE_COMPILED | ✅ |
| 5 | NULL JIT helper table entries safe | ✅ |
✅ (1) All g_pConfig PGO accessors are properly guarded
Every call to TieredPGO(), TieredPGO_InstrumentOnlyHotCode(), and TieredPGO_ScalableCountThreshold() is either:
- Inside a
#ifdef FEATURE_PGOblock (tieredcompilation.cpp:267-301, tieredcompilation.cpp:1052-1061, jitinterface.cpp:10857-10868, jitinterface.cpp:12724-12728, eeconfig.cpp:790+, eventtrace.cpp:5489+), or - Inside function bodies that are themselves compiled only when
FEATURE_PGOis defined (pgo.cpp:357, jithelpers.cpp:2138/2164 — wrapped by the new#ifdef FEATURE_PGOat jithelpers.cpp:1779/2184).
No unguarded accessor call exists.
⚠️ (2) getPgoInstrumentationResults stub: missing output-parameter initializations
File:src/coreclr/vm/pgo.cpp:1252-1258
The !FEATURE_PGO stub for getPgoInstrumentationResults does not initialize *ppSchema or *pPgoSource:
// stub (lines 1253-1257)
*pAllocatedData = NULL;
*pCountSchemaItems = 0;
*pInstrumentationData = NULL;
returnE_NOTIMPL;
// ← *ppSchema and *pPgoSource never setCompare with the real implementation (pgo.cpp:831-835) which initializes both, and the sibling stubgetPgoInstrumentationResultsFromR2RFormat (pgo.cpp:1243-1246) which does set *ppSchema = NULL.
Practical impact is low because the only call site (jitinterface.cpp:12732-12767) has its own #ifdef FEATURE_PGO guard that skips calling the stub entirely, and the caller pre-initializes *pSchema = NULL and *pPgoSource = Unknown at lines 12720-12723. However, the stub is part of the public PgoManager API; any future direct caller would get uninitialized out-params.
Recommendation: Add *ppSchema = NULL; and *pPgoSource = ICorJitInfo::PgoSource::Unknown; to the stub for consistency and defensive correctness.
⚠️ (3) Pre-existing pgo.h declaration typo (not introduced by this PR)
File:src/coreclr/vm/pgo.h:26
staticHRESULTgetPgoInstrumentationResults(MethodDesc* pMD,
BYTE **pAllocatedDatapAllocatedData, ...);
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doubled parameter nameThis is pre-existing and cosmetic (parameter names in declarations don't affect compilation), but since this PR touched the corresponding stub signatures, it would be a good opportunity to fix it. This is not a blocker.
✅ (3 cont.) Other stub signatures match declarations correctly
| Method | Signature match | Out-params initialized |
|---|---|---|
getPgoInstrumentationResultsFromR2RFormat | ✅ matches pgo.h:28-36 | ✅ all 4 |
allocPgoInstrumentationBySchema | ✅ matches pgo.h:27 | ✅ |
CreatePgoManager | ✅ volatile qualifier matches pgo.h:38 | ✅ |
VerifyAddress | ✅ matches pgo.h:41 | N/A |
✅ (4) FEATURE_PGO under FEATURE_DYNAMIC_CODE_COMPILED is architecturally consistent
clrfeatures.cmake now sets FEATURE_PGO alongside FEATURE_TIERED_COMPILATION and FEATURE_REJIT inside the FEATURE_DYNAMIC_CODE_COMPILED block. This is sound because:
- PGO requires JIT recompilation (tiered compilation) → requires dynamic code
- Platforms that disable dynamic code (iOS, tvOS, WASM) cannot use PGO anyway
- NativeAOT has zero
FEATURE_PGOreferences (uses its own AOT PGO pipeline) - The JIT itself has zero
#ifdef FEATURE_PGOchecks — it receives PGO data through the JIT-EE interface, so it's naturally decoupled
✅ (5) NULL JIT helper table entries are safe
When FEATURE_PGO is off, all 10 PGO helpers (CORINFO_HELP_CLASSPROFILE32 through CORINFO_HELP_VALUEPROFILE64) are set to NULL in jithelpers.h:344-353. This is safe because:
- JIT-side guard: The JIT only emits calls to these helpers when
JIT_FLAG_BBINSTRis set (src/coreclr/jit/importercalls.cpp:9448). - VM-side guard:
CORJIT_FLAG_BBINSTRis only set forOptimizationTier0Instrumented/OptimizationTier1Instrumentedtiers, and theBBINSTR_IF_LOOPSpath is wrapped in#ifdef FEATURE_PGO(tieredcompilation.cpp:1052-1061). - Precedent: The same
NULLpattern is used for other conditional helpers (e.g.,CORINFO_HELP_STACK_PROBE).
No code path can reach a NULL helper dereference when FEATURE_PGO is off.
💡 Additional observation: *pSchema = NULL initialization in jitinterface.cpp
The PR adds *pSchema = NULL; at jitinterface.cpp:12720, which was missing in the baseline (7fa0e1ed). This is a good defensive fix — previously *pSchema could be left uninitialized on the !FEATURE_PGO path. Nice catch.
Overall Verdict: ✅ Ship-ready with minor suggestions
The PR is correct and safe. The conditional compilation is thorough — all PGO accessor calls, helper functions, and JIT flag settings are properly guarded. The architectural placement under FEATURE_DYNAMIC_CODE_COMPILED is sound. The two warnings above are low-severity:
- Minor: Add
*ppSchema = NULL; *pPgoSource = ICorJitInfo::PgoSource::Unknown;to thegetPgoInstrumentationResultsstub inpgo.cppfor defensive completeness. - Cosmetic: Fix the
pAllocatedDatapAllocatedDatatypo inpgo.h:26(pre-existing, not a regression).
Generated by Code Review for issue #126493 · ◷
jkotas
commented
Apr 9, 2026
/ba-g intermittent networking failures |
jkotas
commented
Apr 9, 2026
@copilot Fix this feedback (2) getPgoInstrumentationResults stub: missing output-parameter initializations |
… pgo.h parameter name typo Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/51af0fd3-5ce0-4f5f-bf48-eaa6f46fac97 Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Fixed in 283f203:
|
Uh oh!
There was an error while loading. Please reload this page.
Description
FEATURE_PGO(PGO infrastructure) is not meaningful on WebAssembly, iOS, TVOS, or MacCatalyst targets and should be excluded from those builds.FEATURE_PGOis now gated under theFEATURE_DYNAMIC_CODE_COMPILEDblock inclrfeatures.cmake, following the same pattern asFEATURE_REJIT.Changes
src/coreclr/clrfeatures.cmake: MoveFEATURE_PGOinside theFEATURE_DYNAMIC_CODE_COMPILEDblock next toset(FEATURE_REJIT 1). WASM, iOS, TVOS, and MacCatalyst are all implicitly excluded sinceFEATURE_DYNAMIC_CODE_COMPILEDis not set for those targets.src/coreclr/clrdefinitions.cmake: Gateadd_compile_definitions(FEATURE_PGO)on theFEATURE_PGOvariable (was unconditional).src/coreclr/vm/pgo.cpp: Fix stale!FEATURE_PGOstub implementations to match current declarations inpgo.h:getPgoInstrumentationResults: updated signature to useBYTE**and added the missingICorJitInfo::PgoSource*parameter; added missing*ppSchema = NULLout-parameter initialization.CreatePgoManager: updated parameter toPgoManager* volatile*.getPgoInstrumentationResultsFromR2RFormat, which is referenced fromreadytoruninfo.cpp(always compiled) but had no implementation in non-PGO builds.src/coreclr/vm/pgo.h: Fixed pre-existing parameter name typopAllocatedDatapAllocatedData→pAllocatedDatain thegetPgoInstrumentationResultsdeclaration.src/coreclr/vm/jitinterface.cpp: Guardg_pConfig->TieredPGO()call ingetPgoInstrumentationResultswith#ifdef FEATURE_PGO; returnsfalsefor non-PGO builds. Added*pSchema = NULLinitialization for out-parameter consistency.src/coreclr/vm/tieredcompilation.cpp: GuardTieredPGO() && TieredPGO_InstrumentOnlyHotCode()block in theOptimizationTier0path with#ifdef FEATURE_PGO.src/coreclr/inc/jithelpers.h: Gate all 10 PGO profiling JIT helper table entries (CLASSPROFILE32/64,DELEGATEPROFILE32/64,VTABLEPROFILE32/64,COUNTPROFILE32/64,VALUEPROFILE32/64) under#ifdef FEATURE_PGO/#else NULL #endif, following the pattern used byFEATURE_RESOLVE_HELPER_DISPATCH.src/coreclr/vm/jithelpers.cpp: Wrap the actual PGO profiling JIT helper implementations (JIT_ValueProfile32/64,JIT_ClassProfile32/64,JIT_DelegateProfile32/64,JIT_VTableProfile32/64,JIT_CountProfile32/64) in#ifdef FEATURE_PGO. No stub implementations are needed for!FEATURE_PGObuilds since the JIT helper table entries are nowNULL.Changes to public API
No public API changes.