Add hot reload baseline reading and recorded EnC state#20026
Add hot reload baseline reading and recorded EnC state#20026NatElkins wants to merge 11 commits into
Conversation
e8cbbe5 to
0e8dd0d
Compare
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
Warning No PR link found in some release notes, please consider adding it.
|
9388a17 to
9727c67
Compare
…ethod CDI emission Adds an internal AbstractIL module implementing, byte for byte, the three Portable PDB CustomDebugInformation blob formats Roslyn persists per method for Edit and Continue (EnC Local Slot Map, EnC Lambda and Closure Map, EnC State Machine State Map), with serializers, deserializers, a portable PDB read-back helper, and an occurrence-key packing helper for deterministic syntax-offset slots. Plumbs an optional methodCustomDebugInfoRows side channel through the IL binary writer options into the portable PDB generator so a compilation can attach CDI rows to named methods. Names that do not identify exactly one method row are dropped. All existing writer call sites pass an empty map, so emitted PDBs are byte-identical to before. No in-tree caller populates the map yet; the consumer is the F# hot reload work in dotnet#19941, following the same pattern as dotnet#20017 (land isolated, test-covered infrastructure first, wire the feature later). Tests: blob round-trips, Roslyn golden-byte encodings, cross-validation against CDI blobs emitted by a real Roslyn compilation, fail-closed occurrence-key packing (including an int32-overflow regression where a wrapped negative key previously escaped the bound check), and end-to-end synthetic PDB emission proving correct MethodDef parenting, zero rows for an empty map, and no rows for absent or ambiguous names.
ProcessStartInfo.ArgumentList does not exist on net472, which the component tests also target on Windows CI. Build the quoted argument string by hand instead.
xunit 3.2.2 no longer discovers internal test classes, so the module was silently skipped after rebasing onto current main (pre-existing internal test modules like CompilerService.Caches are likewise undiscovered there). Public visibility restores discovery; 17 tests run and pass.
Add internal generated-name normalization and synthesized-name map replay support as a standalone slice. The new map state is side-channel based, all new compiler modules remain internal, and CompilerGlobalState preserves the existing no-map counter path while checking an accessor captured once per compiler state. Route existing IlxGen generated-name allocations through inert helper wrappers, add pure name-map and normalizer tests, add a normal compilation determinism guard over emitted generated names, and document the extracted seams in P5_REPORT.md. Verification: built FSharp.Compiler.Service, FSharp.Compiler.Service.Tests, FSharp.Compiler.ComponentTests, and FSharpSuite.Tests in Release; ran the migrated service test classes, the component determinism class, FSharpSuite DeterministicTests, and the FCS SurfaceArea class successfully.
…CI images The Roslyn cross-validation test shells out to dotnet build to produce a real Roslyn PDB. The process launch had two problems. It computed the host path by hand from __SOURCE_DIRECTORY__, which misses on CI images that carry no repo-local .dotnet at that depth, failing with Win32Exception before the build starts. It also left UseShellExecute at its default, which is true on net472 and rejects redirected streams, so every Desktop test leg failed deterministically with InvalidOperationException. Resolve the host like the rest of the test framework via TestFramework.initialConfig.DotNetExe, which prefers the repo-local .dotnet and falls back to PATH, and set UseShellExecute to false explicitly. Verified: FSharp.Compiler.ComponentTests builds clean; EncMethodDebugInformationTests 17 passed, 0 failed (net10.0); fantomas clean on the touched file.
…0260717/hotreload-baseline-reader
Add standalone baseline PE and portable PDB readers for hot reload, including token maps and MVID/PDB table snapshots. Carry the F# synthesized-name snapshot module CDI codec and portable PDB read path, with recorded snapshots taking precedence over IL reconstruction. Add focused component tests for snapshot round-trip, direct module CDI reading, baseline token maps, recorded fallback behavior, and EnC closure-name reconstruction.
…erload CI compiles the snapshot round-trip and baseline reader tests with FS0193: the explicit Assert.Equal<string> type application commits overload resolution to the scalar Equal(T, T) shape while both arguments are string arrays. Use Assert.Equal<string[]>, the form the neighboring name-map tests already use, so resolution lands on the structural array comparison everywhere. Verified: FSharp.Compiler.ComponentTests builds clean; EncMethodDebugInformationTests 22 passed, 0 failed (net10.0); fantomas clean on the touched file.
9727c67 to
3e1678d
Compare
Verified with the repository-wide Fantomas check.
github-actions
Bot
added
the
⚠️ Affects-Compiler-Output
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
Reviewed the two new PR-owned files (ILBaselineReader.fs, HotReloadBaseline.fs) plus the EncMethodDebugInformation.fs codec/read additions. The metadata table row-size computations were spot-checked against ECMA-335 II.22 and look correct, and the snapshot codec's allocation guards (bucket/name counts bounded by RemainingBytes) are in place. Two robustness/correctness concerns below; both are about failing closed on unusual-but-legal or truncated inputs.
|
|
||
| for i in 0..63 do | ||
| if rowCounts[i] > 0 then | ||
| rowCountSize <- rowCountSize + 4 |
There was a problem hiding this comment.
rowCountSize is derived from the number of tables with rowCounts[i] > 0, but the tables-stream header stores one 4-byte row-count entry per valid bit — i.e. popcount(valid), not the number of non-empty tables. ECMA-335 permits a table to have its Valid bit set while holding 0 rows; when that happens this undercounts the header, so TablesStart (line 575) lands too early and every subsequent table offset is shifted, silently corrupting all token/name reads. parseTablesStream already advances rowCountOffset to exactly the tables start while consuming the header — consider returning that offset (or counting set bits in valid) instead of re-deriving it from non-zero row counts.
| | Some expectedContentId -> portablePdbBytes |> Option.bind (toPortablePdbSnapshot expectedContentId) | ||
| | None -> None | ||
|
|
||
| Some(createCore moduleId metadata portablePdb (buildTokenMaps reader)) |
There was a problem hiding this comment.
The try-prefixed sibling readers (metadataSnapshotFromBytes, BaselineMetadataReader.Create, readModuleMvidFromBytes) all fail closed by returning None, but buildTokenMaps reader runs outside any guard here. It walks method/field/property signature blobs via readBlobFromHeap, which indexes ctx.Bytes[start] without a bounds check — a truncated or malformed assembly whose headers parse but whose blob offsets point past the (short) image will throw IndexOutOfRangeException straight out of tryReadFromAssemblyAndPdbBytes, defeating the option-returning contract implied by the name. Consider wrapping the buildTokenMaps/createCore body in the same IndexOutOfRangeException/ArgumentOutOfRangeException catch used by the readers it composes.
2 participants
Adds the baseline reading layer for F# hot reload: a standalone PE/metadata/portable PDB reader (
ILBaselineReader) for the heap sizes, row counts, MVID, and token tables an Edit and Continue generation chains from, plusHotReloadBaselinestate construction: token maps, reconstruction of synthesized-name snapshots from the binary, reading EnC method CustomDebugInformation, and closure-name table reconstruction from those rows.It also adds the recorded synthesized-name snapshot: a module-level CustomDebugInformation record (kind
49DDB47E-9C74-46EC-8626-0350676571EB) whose payload is the compiler's name map in allocation-slot order, with a deterministic codec (buckets sorted by key, names kept in slot order, no paths or timestamps). Reading prefers the recorded snapshot and falls back to reconstruction only when the record is missing or invalid; this is what makes "small IL diff for small source diff" work, since slot layout is not inferable from IL enumeration order in general. Ordinary compiles emit no module rows and are unaffected.Trust rules fail closed: a recorded snapshot is used only when exactly one valid row is present; closure-name reconstruction requires generation zero and matching type names, otherwise it yields an empty map and the session layer treats the edit as rude.
Tests: 22 covering the codec roundtrip (including mixed replay/occurrence buckets), module CDI read/write through the in-memory PDB writer, baseline token maps, recorded versus reconstructed selection and fallback, and closure-name reconstruction.
SurfaceAreaTestgreen; everything internal.Sequencing
This PR is part of splitting the F# hot reload work (#19941) into small, independently reviewable PRs. The planned order:
--test:HotReloadDeltascapture hook (F# hot reload: Edit-and-Continue delta emission behind --test:HotReloadDeltas #19941 in its final, much smaller form).Role in the train: the delta emitter (wave 3) matches fresh compiles against this baseline state; the session (wave 4) constructs it at startup from the on-disk assembly and PDB.
Refresh status (2026-07-17)