test

[rcj1]

Reverts #127336

[github-actions]

🤖 Copilot Code Review — PR #127490

Note

This review was generated by Copilot. Multi-model review: GPT-5.3-Codex contributed findings (Claude Sonnet 4.6 timed out and did not contribute).

Holistic Assessment

Motivation: This PR reverts #127336 ("Optimize runtime async suspend/resume machinery") and bundles an unrelated fix to `ClrDataModule.cs`. The revert undoes an unsafe pointer-based optimization of the async suspend/resume TLS state. The PR description ("Reverts #127336") provides no reason for the revert — no linked failure, crash report, or regression issue. The PR is labeled `NO-MERGE`, `NO-REVIEW`, and is a draft with the title "test", so it appears to be experimental.

Approach: The revert itself is structurally sound — it removes the `ref struct RuntimeAsyncStackState` pointer chain, simplifies `RuntimeAsyncAwaitState` to hold notifiers/contexts directly, and makes methods read TLS instead of taking state by ref. However, this is not a clean `git revert`; it introduces a new type (`ExecutionAndSyncBlockStore`) and bundles a separate `ClrDataModule` fix, which violates the "one concern per PR" principle.

Summary: ⚠️ Needs Human Review. The async revert logic appears correct but has naming concerns, and the ClrDataModule fix uses an unexplained magic number. A human reviewer should verify: (1) why the revert is needed (no linked issue), (2) whether the `_legacyModulePointer != 1` guard is the right fix, and (3) whether these changes should be split into separate PRs.

---

Detailed Findings

⚠️ Scope — Unrelated `ClrDataModule.cs` fix bundled with revert

The second commit (`Update ClrDataModule.cs`) adds a `_legacyModulePointer != 1` guard that has nothing to do with reverting #127336. Per dotnet/runtime conventions, reverts and unrelated fixes should be in separate PRs to keep review scope focused and reduce regression risk.

⚠️ ClrDataModule — Magic number `1` without documentation or consistent guarding

`ClrDataModule.cs:92`: The new guard `_legacyModulePointer != 1` prevents calling `Marshal.QueryInterface` on an invalid pointer value. However:

• The value `1` has no named constant or comment explaining why it can occur. While `END_SENTINEL = 0x1` exists in `DacEnumerableHash.cs`, there is no documented contract that `ComWrappers.TryGetComInstance` can produce a `1` value for `_legacyModulePointer`.
• The constructor (line 45-49) calls `Marshal.Release(_legacyModulePointer)` without the same `!= 1` guard. If `1` is a realistic sentinel value, the constructor's `Release` call would also be unsafe. (Flagged by both primary and GPT-5.3-Codex reviews)
• Consider defining a named constant (e.g., `const nint InvalidSentinel = 1;`) and guarding both call sites consistently.

💡 Naming — `ExecutionAndSyncBlockStore` is misleading

`AsyncHelpers.CoreCLR.cs:18`: The new struct `ExecutionAndSyncBlockStore` stores `ExecutionContext` and `SynchronizationContext`. However, "SyncBlock" has a well-established meaning in the CLR (the object header sync block table — see `ObjectHeader.CoreCLR.cs`, `SyncTable.cs`). This name is likely to confuse runtime developers. Consider `ExecutionAndSyncContextStore` or similar. (Flagged by both primary and GPT-5.3-Codex reviews)

💡 Naming — Public fields with underscore prefix

`AsyncHelpers.CoreCLR.cs:23-25`: `ExecutionAndSyncBlockStore` has `public` fields `_previousExecutionCtx`, `_previousSyncCtx`, and `_thread` with underscore prefixes. The underscore prefix conventionally denotes private fields in C#. While this is an `internal` struct and the fields are only used by `Push()`/`Pop()`, they could be private since no external code accesses them directly.

💡 Style — Cosmetic whitespace changes in `corelib.h`

`corelib.h:781,785`: The `THREAD` `START_CALLBACK` and `CTOR` definitions have changed column alignment. These are cosmetic/noise changes unrelated to the revert.

✅ Async revert correctness — State management changes look correct

The core revert logic is structurally sound:

• The `RuntimeAsyncStackState` pointer chain is properly removed.
• Notifiers (`CriticalNotifier`, `Notifier`, `ValueTaskSourceNotifier`, `TaskNotifier`) and contexts are now stored directly in `RuntimeAsyncAwaitState` (TLS), which is the pre-optimization design.
• `HandleSuspended()` correctly reads from TLS, extracts all state into locals, clears the TLS fields, then uses the locals — avoiding stale references.
• The `??=` → explicit null check change is valid (avoids potential issues with compound assignment on struct refs).
• `Unsafe.As` casts are properly replaced with regular casts.
• `metasig.h` correctly removes the 4 old signatures and the 2 non-generic variants properly reuse existing `SM_RetTask`/`SM_RetValueTask` definitions.

✅ IL generation — Thunk generation updated consistently

Both `AsyncThunks.cs` (managed IL generation) and `asyncthunks.cpp` (native IL generation) are updated consistently:

• `ExecutionAndSyncBlockStore.Push()/Pop()` replaces `RuntimeAsyncAwaitState.Push()/Pop()`
• `FinalizeTaskReturningThunk` calls emit 0 args instead of 1
• The `asyncHelpersType` local removal in `AsyncThunks.cs` causes repeated `context.SystemModule.GetKnownType(...)` lookups, but this is emitter-time code (not hot path) so the impact is negligible.

✅ Thread safety — TLS access pattern remains correct

All `RuntimeAsyncAwaitState` access goes through `[ThreadStatic] t_runtimeAsyncAwaitState`, which is thread-local by definition. No cross-thread sharing concerns.

✅ No new public API surface detected

All changes are to `internal` types and private methods. No ref assembly changes.

Generated by Code Review for issue #127490 · ◷

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.