Skip to content

Conversation

@VSadov
Copy link
Member

@VSadov VSadov commented Nov 19, 2025

Trivial change removing some TODOs.
All these TODOs are for known NYIs that are not blocking and are all tracked in some other way.

Ex:
Multicore JIT: #115097
R2R: #121559
PGO: #121755

Copilot AI review requested due to automatic review settings November 19, 2025 00:09
@VSadov VSadov requested review from agocke and jkotas November 19, 2025 00:10
Copilot finished reviewing on behalf of VSadov November 19, 2025 00:12
Copy link
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 removes TODO comments related to async feature support (Multicore JIT, R2R, PGO, and EnC) that are already tracked in separate issues and are not currently blocking. The changes are purely documentation updates with no code behavior modifications.

  • Removes tracked async variant TODOs from ReadyToRunInfo PGO and R2R entry point lookups
  • Removes tracked async variant TODO from multicore JIT method recording
  • Removes tracked async variant TODO from EnC (Edit and Continue) support

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/coreclr/vm/readytoruninfo.cpp Removes two TODO comments before early returns for async variants in PGO instrumentation data retrieval and R2R entry point lookups
src/coreclr/vm/multicorejit.cpp Removes TODO comment in the method preprocessing loop where async variant methods are skipped
src/coreclr/vm/class.cpp Removes TODO comment before the EnC error log for async methods

MethodReturnKind returnKind = ClassifyMethodReturnKind(sigParser, pModule, &offsetOfAsyncDetails, &isValueTask);
if (returnKind != MethodReturnKind::NormalMethod)
{
// TODO: (async) revisit and examine if this can be supported
Copy link
Member

@jkotas jkotas Nov 19, 2025

Choose a reason for hiding this comment

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

This means that EnC will be broken for all Task-returning methods. I expect we will see failures in diagnostic and VS debugger tests.

It sounds blocking to me.

Copy link
Member Author

@VSadov VSadov Nov 19, 2025

Choose a reason for hiding this comment

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

I think the disablement of EnC comes all the way back to the design where Roslyn would literally emit async variants into IL with a special disambiguating signature.

Since then the design has changed to not have methods with async calling convention in user code. It may make it a lot easier to implement EnC. Task-returning methods might just work, or almost work. The if (returnKind != MethodReturnKind::NormalMethod) condition could be made less strong to disable EnC only for MethodImpl.Async methods. I would not know how that is validated.

@tommcdon @steveisok -

  • any reason to keep this TODO? (as I understand EnC impact is known and accounted for)
  • what would be the acceptable state of EnC and diagnostics support in general for enablement of the feature in the repo?

I think we would not want to break diagnostic/VS, but waiting until everything works is not convenient either, including to the diagnostic and VS teams, I assume.
Can the breakage be mitigated/scoped to some acceptable level? (like disabling some tests temporarily,..)

Copy link
Member

Choose a reason for hiding this comment

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

what would be the acceptable state of EnC and diagnostics support in general for enablement of the feature in the repo?

It is not ok to break EnC for async v1. This TODO breaks async v1 and that's a problem.

It is ok for EnC to be broken for async v2 for now.

Copy link
Member Author

@VSadov VSadov Nov 20, 2025

Choose a reason for hiding this comment

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

EnC with async v1 or ordinary task-returning case may not be broken. We only add a thunk which is an implementation detail, unused if async2 is not involved. It may be possible to not disable anything, assuming the EnC target is not compiled as async2.

That is in theory. Someone needs to try this with appropriate tests.

Copy link
Member

@jkotas jkotas Nov 20, 2025

Choose a reason for hiding this comment

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

EnC with async v1 or ordinary task-returning case may not be broken

Async v1 EnC is broken with DOTNET_RuntimeAsync=1 set currently due to this TODO. I have just done the following to double check:

  1. build -s clr -c release with RuntimeAsync default flipped to 1 in clrconfigvalues.h
  2. Start VS2026 with VSDebugger_ValidateDotnetDebugLibSignatures=0 set in in the environment
  3. Create default console app, make it <SelfContained>true</SelfContained>, build it
  4. Overwrite the runtime in the app with your private (e.g. copy \runtime\artifacts\bin\coreclr\windows.x64.Release\*.* bin\Debug\net10.0\win-x64
  5. Set breakpoint at Console.WriteLine, F5 to run the app
  6. Make an edit that introduces Task-returning method:
Console.WriteLine("Hello, World!");

M();

async Task M()
{
}
  1. Step to apply the changes

Result:
image

Copy link
Member

Choose a reason for hiding this comment

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

I could imagine that narrowing the check to only return E_FAIL if IsMiAsync(dwImplFlags) is also TRUE might be a sufficient change to get @jkotas' ENC v1 test passing. It makes sense to me that with the check as-is the test fails.

Copy link
Member

@noahfalk noahfalk Nov 21, 2025

Choose a reason for hiding this comment

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

what would be the acceptable state of EnC and diagnostics support in general for enablement of the feature in the repo?

I assume enablement means:

  • DOTNET_RuntimeAsync=1 as the new default
  • Users still need to opt in to AsyncV2 in their project file
   <EnablePreviewFeatures>true</EnablePreviewFeatures>
   <Features>$(Features);runtime-async=on</Features>

If that is proposal then I agree with @jkotas - the ENC v1 test needs to be working but its OK if ENC doesn't work after the user does the project file feature opt-in.

Copy link
Member

Choose a reason for hiding this comment

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

return E_FAIL if IsMiAsync(dwImplFlags) is also TRUE might be a sufficient change

I think just checking for IsMiAsync(dwImplFlags) may be sufficient, but it needs to be tested.

We may want to log an issue to track enabling EnC for async v2 and link it from here.

Copy link
Member

Choose a reason for hiding this comment

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

any reason to keep this TODO? (as I understand EnC impact is known and accounted for)

I feel we can track this as a separate issue tracked separately.

If that is proposal then I agree with @jkotas - the ENC v1 test needs to be working but its OK if ENC doesn't work after the user does the project file feature opt-in.

+1 to this. We should have the goal of EnC parity but it shouldn't block enabling the feature.

if (pMethod->IsAsyncVariantMethod())
{
// TODO: (async) consider adding support for async variants in the future
skipped++;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
skipped++;
// TODO: (async) Multicore JIT https://github.com/dotnet/runtime/issues/115097
skipped++;

We may want to keep the TODOs with links to the tracking issues

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