[SychicBoy]

Updated document equality and hashing to compare the URL, checksum algorithm, checksum, and language. These fields are now populated before document lookup during loading.

Fixes #579.

[pcshrosbree]

Thanks for picking this up. I ran this PR and master (9ab9b58) through a small console repro on Linux with .NET 10, and the PR fixes #579:

• The repro from #579. At master, first.Equals(second) is True and PdbState.Add(second) returns first. With this PR, they compare unequal, and PdbState.Documents has 2 entries.
• Loading. I built a C# library with two #line documents and gave them different #pragma checksum values. The PDB has five Document rows in total. I changed one byte so that the two rows for those #line documents are both named /src/App/Program.cs. At master, dnlib merges those two rows into one document, both methods' sequence points get the first row's checksum, and the written PDB has four Document rows instead of five. With this PR, both rows survive the load and the write, and each method still points at its own row. I checked the output with System.Reflection.Metadata. The load fix works because the PR sets the three fields in the constructor, so the partial document used for the lookup already has them.
• Writing. For a ModuleDefUser with two such documents, master writes one Document row and this PR writes two.

dnlib has no test project, so a repro in the PR description could stand in for a test. I'm happy to share mine.

The PR also has two consequences that the description doesn't mention. Details are in the inline comments:

1. URL comparison is now case-sensitive. I think that's right for Portable PDBs, but it's a behaviour change.
2. The hash now includes the checksum, the checksum algorithm and the language, and all three can be set. In my repro, I edited the checksum or language of a loaded document, then called ModuleDef.Write before reading the method bodies that refer to it. The write threw, as it already does at master after a Url edit (#566).

Callers may also notice a change in PdbState.GetExisting(new PdbDocument { Url = url }). In my sample, this lookup finds each loaded document at master but returns null with this PR, because the probe document's checksum and language are left at their defaults. That follows from the fix, but it may be worth a line in the release notes.

A side note, unrelated to this PR: my write test hits a NullReferenceException at master too. new PdbDocument() and the six-argument constructor leave CustomDebugInfos as null (only the SymbolDocument constructor initializes it), and the Portable PDB writer dereferences it in Metadata.AddCustomDebugInformationList. The test sets the private field by reflection to work around it. I can open a separate issue for it.

[pcshrosbree]

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.

[pcshrosbree]

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.

[SychicBoy]

This PR fixes #579 by including the checksum algorithm, checksum, and language in document identity.

The mutable-key problem already exists with Url and is tracked by #566. The new fields have the same limitation.

This PR remains focused on #579, leaving mutable-document behavior as a separate concern.

[pcshrosbree]

Keeping #580 focused on #579 makes sense. I've opened #586 for #566. It looks up loaded Portable PDB documents by their Document-row token first. In my repro, I can now edit a document's Url before the method bodies are read and then write successfully, instead of getting the duplicate-key exception. In my tests, with #586 applied on top of #580, writing also succeeds after the CheckSum and Language edits from my review. In my Url-edit repro, Remove and GetExisting still miss the document; that needs a design choice, so I've described the options in #587.