Skip to content

Fix portable PDB debug reservation after path mapping - #20509

Open
michaelglass wants to merge 3 commits into
dotnet:mainfrom
michaelglass:fix/portable-pdb-pathmap-reservation
Open

Fix portable PDB debug reservation after path mapping#20509
michaelglass wants to merge 3 commits into
dotnet:mainfrom
michaelglass:fix/portable-pdb-pathmap-reservation

Conversation

@michaelglass

@michaelglass michaelglass commented Sep 10, 2026

Copy link
Copy Markdown

Description

--pathmap doesn't make portable-PDB output path-independent. Compiling identical source from two
directories whose paths differ in length, with --deterministic+ and both mapped to the same root,
produces different DLLs. Measured on a real repo: PDBs byte-identical, 55 DLL bytes differ.

Cause: in writeBinaryAux (ilwrite.fs) the debug-directory chunk is reserved from the original
pdb path (Unicode.GetByteCount f + 40 slack), but the bytes written are the mapped path. The
reservation — and the 4-byte-aligned layout after it — tracks a path that never appears in the output.

Fix: for portable PDBs reserve exactly what CodeView writes — 24-byte header + mapped path as UTF-8

  • null terminator. The native PDB writer's conservative reservation (and its bug 748444 slack) is
    deliberately unchanged; embedded PDBs reserve from the file name.

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change path Release notes path Description
`src/Compiler` docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

michaelglass added a commit to michaelglass/fsharp that referenced this pull request Sep 10, 2026
@michaelglass

Copy link
Copy Markdown
Author

@dotnet-policy-service agree

@michaelglass
michaelglass force-pushed the fix/portable-pdb-pathmap-reservation branch from c2fe907 to d16961b Compare September 10, 2026 10:44
michaelglass added a commit to michaelglass/fsharp that referenced this pull request Sep 10, 2026
@github-actions github-actions Bot added the ⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen label Sep 10, 2026
@github-actions

This comment has been minimized.

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 🕵️ AI review — verify independently.

yield "--target:library"
yield "--deterministic+"
yield $"--debug:{debugType}"
yield $"--pathmap:{workDir}={mappedRoot}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 🕵️ All three cases fail with FS2028 when TEMP contains spaces because runFscProcess joins unquoted arguments.

$env:TEMP = Join-Path ([IO.Path]::GetTempPath()) 'with spaces'
$env:TMP = $env:TEMP
New-Item -ItemType Directory -Force $env:TEMP | Out-Null
dotnet test --project tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -c Debug --no-build -p:BUILDING_USING_DOTNET=true -p:FSHARPCORE_USE_PACKAGE=false -- --filter-method '*Path mapping removes original PDB path length from the entire binary*'

@T-Gro
T-Gro self-requested a review September 10, 2026 14:20
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Sep 10, 2026
runToolProcess (behind runFscProcess / runFsiProcess) built the child
command line with String.concat " ", so any argument containing a space
was split into several arguments by the child's command-line parser.

This surfaced on the .NET Framework test legs, where the default
references passed via CompilerAssert.DefaultProjectOptions live under
"C:\Program Files (x86)\Reference Assemblies\...". fsc saw "C:\Program"
and "Files" as separate tokens and failed with

  error FS2020: The assembly 'Assemblies\Microsoft\Framework\
  .NETFramework\v4.7.2\Microsoft.CSharp.dll' is listed on the command
  line. Assemblies should be referenced using a command line flag such
  as '-r'.

The same splitting applies to any test whose paths come from a temp
directory containing spaces.

Quote arguments that contain whitespace. Arguments without whitespace
are passed through unchanged, so existing callers are unaffected. The
determinism test now builds under a temp root containing a space so the
behaviour stays covered.
@michaelglass
michaelglass force-pushed the fix/portable-pdb-pathmap-reservation branch from 03eda14 to e16b51c Compare September 10, 2026 15:11
@github-actions github-actions Bot added the ⚠️ Affects-Test-Tooling Tooling check: PR touches test framework infrastructure label Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Compiler-Output, Affects-Test-Tooling
Affects-Compiler-Output: Changes portable PDB debug-directory binary layout.
Affects-Test-Tooling: Changes subprocess argument handling in shared test harness.

Generated by PR Tooling Safety Check · gpt56 507K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen ⚠️ Affects-Test-Tooling Tooling check: PR touches test framework infrastructure AI-reviewed PR reviewed by AI review council

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

2 participants