[pcshrosbree]

TL;DR: Editing a loaded Portable PDB document's Url before the method bodies that refer to it are read can make ModuleDef.Write throw (#566). This PR makes PdbState find loaded Portable PDB documents by their Document-row token first. It also makes Remove delete the token of the document it actually removes, so that the token lookup never returns a removed document. The change is 12 added and 3 removed lines in PdbState.cs; the details and test results follow. After a Url edit, Remove and GetExisting can still miss a document. Fixing those lookups needs a design choice; I've described the options in #587.

Fixes #566

At master, changing a loaded Portable PDB document's Url before the method bodies that refer to it are read can make ModuleDef.Write throw ArgumentException: An item with the same key has already been added, from PdbState.Add_NoLock(SymbolDocument). The stack trace in #566 shows this.

The cause: PdbState reads method bodies lazily. For each sequence point, it builds a partial document from the SymbolDocument and looks it up in docDict, whose keys hash Url. When an edit makes that lookup miss, dnlib creates a second copy of the document, and tokenToDocument.Add throws because the row's token is already registered.

This PR makes two changes in PdbState:

• Add_NoLock(SymbolDocument) looks the document up in tokenToDocument first. A document read from a Portable PDB already has an identity, its Document row, and PdbState records that token for each document it creates when it loads the PDB.
• Remove now deletes the token of the stored document it removes, and only when it removes one. Previously, Remove deleted its argument's token before trying to remove a document by value. Removing a document through an equal copy with no token therefore left the stored document's token behind. If removal by value failed, Remove still deleted the argument's token. With the first change alone, the token left behind after removal through an equal copy would make the lookup return the removed document.

RemoveAllDocuments already clears both dictionaries, and Add records the token of each document it inserts. So with both changes, the token lookup only returns documents that are still in the PdbState. Documents from a Windows PDB have no token in dnlib: DbiDocument.MDToken returns null, as does the COM reader's equivalent. The lookup therefore doesn't affect them. I read that code but didn't test with a Windows PDB.

I tested dnlib's net6.0 build with .NET SDK 10.0.401 on Linux x64, and read every written PDB back with System.Reflection.Metadata. I used two Debug C# libraries with Portable PDBs: a plain one (tested with the repro below), and one with two #line names that differ only by case, the case-sensitivity case from my review of #580.

• Changing PDB document URLs causes an exception #566's loop (a new Url for every document), on the plain library: master throws. With this PR, it writes, and both methods' sequence points point at the document with the new name.
• Other Url edits, on the library with #line names that differ only by case: master throws in each of the following cases. With this PR, each writes, and the sequence points of the method in the edited file point at its new name:
  • Edit one document's Url directly.
  • Call RemoveAllDocuments(true), edit one document's Url, then Add each document back (Changing PDB document URLs causes an exception #566's second variant).
  • Remove one document, edit its Url, then Add it back.
• Remove through an equal copy, on the same library:
  • If I add the copy back, the method's sequence points refer to the copy at master and with this PR. I checked this before attempting to write. The write then throws NullReferenceException in both versions, because the new PdbDocument has null CustomDebugInfos (New PdbDocument has null CustomDebugInfos, so writing a Portable PDB throws NullReferenceException #584).
  • If I add nothing back, master throws the Changing PDB document URLs causes an exception #566 exception when the method body is read. With this PR, the method gets a new document for the row, and the write succeeds.
• A failed Remove after a Url edit returns false at master and with this PR. At master it has already deleted the token, so the written PDB gets a second copy of the document under the old name. With this PR, it changes nothing, and the written PDB keeps the edited document.
• Cases that already write at master, on the same library: no edit; a CheckSum edit; a Language edit; RemoveAllDocuments() without adding anything back; Remove of one document. In each case, this PR writes a PDB byte-identical to master's.
• With Fix PDB document identity to include all document row fields #580: Fix PDB document identity to include all document row fields #580 adds the checksum, checksum algorithm and language to the document's hash. On Fix PDB document identity to include all document row fields #580's head alone, writing after a CheckSum or Language edit throws in the same way, as the review on Fix PDB document identity to include all document row fields #580 noted. With this PR applied on top of Fix PDB document identity to include all document row fields #580, those edits and the Url edits all write.
• Each change is needed. With only the lookup change, removing a document through an equal copy makes later lookups return the removed document, whether or not the copy is added back. With only the Remove change, the Changing PDB document URLs causes an exception #566 cases throw again.

One limitation remains for Document rows whose names differ only by case. At master, two such rows are merged into one document when the PDB loads, because Url is compared case-insensitively, and only the first row's token is recorded. In my test, which prepends /moved to each Url, dnlib can then no longer match the second row to the loaded document. With this PR, dnlib creates a new document with the old name for sequence points that refer to the second row, and the write succeeds. With this PR applied on top of #580, the two rows load as separate documents and each edit applies to its own row. I didn't record the second row's token as an alias, because Remove deletes only the stored document's own token, and the alias would then bring back a removed document.

After a Url edit, Remove and GetExisting can still miss the document or find a different one, because they use its current values for the lookup, but its dictionary entry is still filed under its old values. That needs a design choice, so I've described the options in #587.

dnlib has no test project, so this PR adds no test. Here is the repro. args[0] is an assembly with its Portable PDB beside it:

using System;
using dnlib.DotNet;
using dnlib.DotNet.Writer;

// args[0]: an assembly with its Portable PDB beside it; args[1]: the output path
var module = ModuleDefMD.Load(args[0], new ModuleCreationOptions { TryToLoadPdbFromDisk = true });
foreach (var doc in module.PdbState.Documents)
	doc.Url = "/moved" + doc.Url;
module.Write(args[1], new ModuleWriterOptions(module) { WritePdb = true }); // throws at master
Console.WriteLine("written");