Skip to content

Fix PDB document identity to include all document row fields #580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SychicBoy wants to merge 1 commit into
base: master
Choose a base branch
from
+16 −5
Open

Fix PDB document identity to include all document row fields #580

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/DotNet/Pdb/PdbDocument.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ public PdbDocument(SymbolDocument symDoc) : this(symDoc, partial: false) {
if (symDoc is null)
throw new ArgumentNullException(nameof(symDoc));
Url = symDoc.URL;
Language = symDoc.Language;
CheckSumAlgorithmId = symDoc.CheckSumAlgorithmId;
CheckSum = symDoc.CheckSum;
if (!partial)
Initialize(symDoc);
}
Expand All @@ -83,11 +86,8 @@ internal static PdbDocument CreatePartialForCompare(SymbolDocument symDoc) =>
new PdbDocument(symDoc, partial: true);

internal void Initialize(SymbolDocument symDoc) {
Language = symDoc.Language;
LanguageVendor = symDoc.LanguageVendor;
DocumentType = symDoc.DocumentType;
CheckSumAlgorithmId = symDoc.CheckSumAlgorithmId;
CheckSum = symDoc.CheckSum;
customDebugInfos = new List<PdbCustomDebugInfo>();
foreach (var cdi in symDoc.CustomDebugInfos)
customDebugInfos.Add(cdi);
Expand All @@ -113,14 +113,25 @@ public PdbDocument(string url, Guid language, Guid languageVendor, Guid document
}

/// <inheritdoc/>
public override int GetHashCode() => StringComparer.OrdinalIgnoreCase.GetHashCode(Url ?? string.Empty);
public override int GetHashCode() {

@pcshrosbree pcshrosbree Oct 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

CheckSum, CheckSumAlgorithmId and Language all have public setters, and PdbState keeps loaded documents in a Dictionary<PdbDocument, PdbDocument> (docDict). When a method body is read later, PdbState.Add_NoLock(SymbolDocument) looks up each sequence point's document in that dictionary again. If a caller has edited a field that is part of the hash before that method body is read, the lookup misses. dnlib then creates a second copy of the document, and tokenToDocument.Add throws. Today this happens only for Url, which is #566: "Calling module.Write() after updating PDB document URLs causes a System.ArgumentException." This PR exposes the other three fields to the same problem; I reproduced it for CheckSum and Language.

Here is the edit-and-write step from my repro. path is a compiled assembly with its compiler-written Portable PDB beside it, and outPath is the output assembly. The only edit is the checksum of the document whose URL ends in /Calc.cs:

var module = ModuleDefMD.Load(path, new ModuleCreationOptions { TryToLoadPdbFromDisk = true });
foreach (var doc in module.PdbState.Documents)
    if (doc.Url.EndsWith("/Calc.cs"))
        doc.CheckSum = SHA256.HashData(Encoding.UTF8.GetBytes("edited source"));
module.Write(outPath, new ModuleWriterOptions(module) { WritePdb = true });

At master, this writes the new checksum. With this PR it throws:

System.ArgumentException: An item with the same key has already been added. Key: 30000001
  at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
  at dnlib.DotNet.Pdb.PdbState.Add_NoLock(SymbolDocument symDoc) in .../PdbState.cs:line 140
  at dnlib.DotNet.Pdb.PdbState.AddSequencePoints(CilBody body, SymbolMethod method) in .../PdbState.cs:line 261

Editing Language instead gives the same exception, and with no edit the write succeeds. After the checksum edit, PdbState.Remove(doc) also returns false and leaves the document in the list; at master it returns true.

A suggestion: a document read from a Portable PDB already has an identity, its Document row. If Add_NoLock(SymbolDocument) looks up the token first, the lazy lookup no longer depends on fields the caller can change:

PdbDocument Add_NoLock(SymbolDocument symDoc) {
    if (symDoc.MDToken.HasValue && tokenToDocument.TryGetValue(symDoc.MDToken.Value, out var existing))
        return existing;
    var doc = PdbDocument.CreatePartialForCompare(symDoc);
    // ... unchanged

I applied this on top of this PR. In my repro, the checksum, language and Url edits then all write successfully, and the #579 checks above still pass. So it also covers the in-place Url edit from #566; I didn't try #566's RemoveAllDocuments variant.

It doesn't fix Remove or GetExisting after an edit, because those still hash the current values. So it may still be worth a sentence in the XML docs saying these fields shouldn't change while the document is in a PdbState.

unchecked {
int hash = StringComparer.Ordinal.GetHashCode(Url ?? string.Empty);
hash += CheckSumAlgorithmId.GetHashCode();
hash += Utils.GetHashCode(CheckSum);
hash += Language.GetHashCode();
return hash;
}
}

/// <inheritdoc/>
public override bool Equals(object obj) {
var other = obj as PdbDocument;
if (other is null)
return false;
return StringComparer.OrdinalIgnoreCase.Equals(Url ?? string.Empty, other.Url ?? string.Empty);
return StringComparer.Ordinal.Equals(Url ?? string.Empty, other.Url ?? string.Empty)

@pcshrosbree pcshrosbree Oct 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line, and the one in GetHashCode, switch the URL comparison from OrdinalIgnoreCase to Ordinal. Was that intended? If so, I think it's the right choice for Portable PDBs, but could you mention it in the description?

To check it, I compiled a C# library with two #line directives, "/src/App/Program.cs" and "/src/App/program.cs". The two documents have the same (empty) checksum and the same language, so they differ only by case. The C# compiler wrote a separate Document row for each name; the PDB had five Document rows in total. At master, dnlib merged the pair into one: both methods' sequence points pointed at Program.cs, and the written PDB had four rows. With this PR, both rows survive a load and a write.

A VB library using #ExternalSource with /src/App/Program.vb and /src/App/program.vb got a single Document row for the pair from the compiler. That matches Roslyn, which compares document names case-sensitively for C# and case-insensitively for VB: DebugDocumentsBuilder is passed compilation.IsCaseSensitive. With this PR, dnlib keeps both case-distinct rows of the C# library. For VB I only inspected the compiler output; I didn't load that assembly with dnlib.

I didn't test two things at runtime:

  • what a debugger does when it matches a breakpoint against two names that differ only by case;
  • Windows PDBs. dnlib's managed reader already keys its documents with StringComparer.OrdinalIgnoreCase (documents = new Dictionary<string, DbiDocument>(...) in Managed/PdbReader.cs), so a Windows PDB loaded through that reader never gives PdbDocument two casings of one name. I only read that code and didn't run it, and I didn't look at the COM-based reader. The new case-sensitive comparison would still apply to documents added through the API to a module that writes a Windows PDB.

Whichever URL comparison you choose, a test covering names that differ only by case would help.

&& CheckSumAlgorithmId == other.CheckSumAlgorithmId
&& Utils.Equals(CheckSum, other.CheckSum)
&& Language == other.Language;
}
}
}