Emit LocalScope and LocalVariable rows for named locals

The managed ilasm wrote no LocalScope or LocalVariable rows, so debuggers saw
no local names. Each lexical scope of a method body (the body itself and each
{ } block, including .try, catch, filter, finally and fault bodies) is now
recorded when it closes, with its IL offsets and the named locals it declares,
and the PDB gets the rows native ilasm writes: one LocalScope per scope that
declares a named local, spanning the offsets at its { and } (the whole body
for the method's own scope), followed by one LocalVariable per named local
with the local's slot as Index and attributes 0. Unnamed locals get no row.
The scopes are recorded whenever a PDB is requested, by any of the switches
that request one.

The rows are sorted by start offset, then longest first, as the table
requires; scopes with the same range keep source order. Two cases where
native ilasm writes rows the specification does not allow are left out: a
scope without instructions (zero length), and a second variable with the
same name or index in one scope. Within a scope, each name gets a row at the
slot of its first declaration, the declaration the name refers to, unless an
earlier row of the scope already describes that slot; later declarations of
a name, and unnamed locals, take no part. So a row never describes a local
that its name does not refer to. Import scopes and local constants are not
recorded, as in native ilasm.

Tests: LocalScopeTests and LocalTests, and seeded generated cases that compare the
local signature, named references and LocalScope/LocalVariable rows with a model
of native ilasm's rules.

Author: pcshrosbree

src/tools/ilasm/MANAGED-ILASM-FIXES.md

@@ -39,4 +39,6 @@
 | Security pseudoattributes on unsupported targets | Leaves `DynamicSecurityMethodAttribute` and `SuppressUnmanagedCodeSecurityAttribute` unchanged on targets where no transform is applied. | Reports an invalid target consistently with other recognized pseudoattributes. |
 | FieldOffset pseudoattribute ordering with local references | Defers attributes with local member-reference owners or constructors, allowing them to override explicit field offsets or attributes appearing later in the source. | Applies attributes in source order regardless of reference form; explicit field offsets always take precedence. |
 | `BackwardBranchOptimization_UsesShortInstructionLimit` | With `/OPTIMIZE`, leaves backward branches long when their short-form displacement is -126, -127, or -128. | Uses the actual short-form displacement and emits a short branch whenever it fits in a signed byte. |
-| `ExplicitSlot_OutsideTheSixteenBitRange_IsReportedAndTakesTheNextSlot`, `NextSlot_AfterSlot65535_IsReportedAndTheLocalIsNotDeclared` | Accepts any explicit local slot: `[65536]` and above become slots that no 16-bit local instruction operand can name, and a negative slot other than `[-1]` pads the slot table toward 2^32 entries and does not finish. A local without `[n]` after one at slot 65535 takes slot 65536, which no local instruction can name either, and the PDB records its index as 0. | Reports an explicit slot outside 0 to 65535 (other than `[-1]`, which means no explicit slot) and gives the local the next free slot. Reports a local whose next free slot is past 65535 and does not declare it. |
+| `ExplicitSlot_OutsideTheSixteenBitRange_IsReportedAndTakesTheNextSlot`, `NextSlot_AfterSlot65535_IsReportedAndTheLocalIsNotDeclared` | Accepts any explicit local slot: `[65536]` and above become slots that no 16-bit local instruction operand can name, and a negative slot other than `[-1]` pads the slot table toward 2^32 entries and does not finish. A local without `[n]` after one at slot 65535 takes slot 65536, which no local instruction can name either, and the PDB records its index as 0. Reads `[4294967295]` as `[-1]`, and a hexadecimal literal of more than 16 digits by its low bits, so `[0x10000000000000000]` is slot 0; neither is reported. | Reports an explicit slot outside 0 to 65535 (other than `[-1]`, which means no explicit slot) and gives the local the next free slot. The range is checked on the literal as written, so a literal too large for 32 bits is reported as well. Reports a local whose next free slot is past 65535 and does not declare it. |
+| `BlockWithoutInstructions_HasNoScopeRow`, `MethodWithoutInstructions_HasNoScopeRow` | Writes a LocalScope row of length 0 for a `{ }` block, or a method body, that has no instructions and declares a named local; the Portable PDB specification requires a positive length. | Writes no LocalScope row for a scope without instructions. |
+| `NameDeclaredTwiceInOneScope_HasOneVariableRowForTheFirstDeclaration`, `SlotDeclaredTwiceInOneScope_HasOneVariableRowForTheFirstDeclaration`, `NameAndSlotEachDeclaredTwice_ANameWhoseSlotAlreadyHasARowHasNone`, `SlotOfALaterDeclarationOfAName_DoesNotKeepAnotherNameOut` | Writes a LocalVariable row for every named local of a scope, so a scope that declares a name or a slot twice has two rows with that name or index, which the Portable PDB specification does not allow. | Writes a row for each name of a scope at the slot its first declaration has, unless an earlier row of the scope already describes that slot. A name refers to its first declaration in the scope, so a row never describes a local that the name does not refer to. Later declarations of a name, and unnamed locals, take no part: they claim no slot and keep no other name out. |

src/tools/ilasm/src/ILAssembler/Actions/GrammarActions.BuildImage.cs

@@ -557,6 +557,13 @@ private ImmutableArray<ValidatedExport> ValidateExports(
             }
         }
 
+        /// <summary>
+        /// Gets whether a PDB is requested: <see cref="Options.Debug"/>, <see cref="Options.DebugMode"/> or
+        /// <see cref="Options.Pdb"/> is set. Information that only the PDB uses, such as the lexical scopes of
+        /// method bodies, is kept only when this is <see langword="true"/>.
+        /// </summary>
+        private bool GeneratesPdb => _options.Debug || _options.DebugMode is not null || _options.Pdb;
+
         /// <summary>
         /// Builds the Portable PDB and the debug directory that references it, when a PDB is requested.
         /// </summary>
@@ -592,8 +599,7 @@ private ImmutableArray<ValidatedExport> ValidateExports(
 
             // As in native ilasm, only /DEBUG (any mode) or /PDB produces a PDB. Without them, sequence
             // points from .line directives are parsed and validated but not emitted.
-            bool generatePdb = _options.Debug || _options.DebugMode is not null || _options.Pdb;
-            if (!generatePdb)
+            if (!GeneratesPdb)
             {
                 return null;
             }
@@ -667,7 +673,8 @@ private string GetPdbFilePath()
         }
 
         /// <summary>
-        /// Adds the Document rows and one MethodDebugInformation row per MethodDef row to the PDB metadata.
+        /// Adds the Document rows, one MethodDebugInformation row per MethodDef row, and each method's LocalScope and
+        /// LocalVariable rows (<see cref="AddLocalScopes"/>) to the PDB metadata.
         /// </summary>
         /// <remarks>
         /// The documents are added in <see cref="PdbDocumentTable"/> order. A method without sequence points, or
@@ -702,29 +709,115 @@ private void BuildPdbMetadata()
                 if (sequencePoints.Count == 0 || !method.HasBody)
                 {
                     _pdbBuilder.AddMethodDebugInformation(default, default);
-                    continue;
+                }
+                else
+                {
+                    int firstDocument = sequencePoints[0].DocumentIndex;
+                    bool singleDocument = true;
+                    for (int i = 1; i < sequencePoints.Count && singleDocument; i++)
+                    {
+                        singleDocument = sequencePoints[i].DocumentIndex == firstDocument;
+                    }
+
+                    BlobBuilder sequencePointsBlob = EncodeSequencePoints(
+                        sequencePoints,
+                        method.DebugInfo.LocalSignature,
+                        documentHandles,
+                        singleDocument);
+                    // docs/design/specs/PortablePdb-Metadata.md, MethodDebugInformation table: Document is "the row id of the
+                    // single document containing all sequence points of the method, or 0 if the method doesn't have sequence
+                    // points or spans multiple documents", and "_InitialDocument_ is only present if the _Document_ field of
+                    // the _MethodDebugInformation_ table is nil"; the blob then names the documents (EncodeSequencePoints).
+                    _pdbBuilder.AddMethodDebugInformation(
+                        singleDocument ? documentHandles[firstDocument] : default,
+                        _pdbBuilder.GetOrAddBlob(sequencePointsBlob));
                 }
 
-                int firstDocument = sequencePoints[0].DocumentIndex;
-                bool singleDocument = true;
-                for (int i = 1; i < sequencePoints.Count && singleDocument; i++)
+                AddLocalScopes((MethodDefinitionHandle)method.Handle, method.DebugInfo.LocalScopes);
+            }
+        }
+
+        /// <summary>
+        /// Adds a method's LocalScope rows, each followed by its LocalVariable rows, to the PDB metadata
+        /// (docs/design/specs/PortablePdb-Metadata.md, "LocalScope Table" and "LocalVariable Table").
+        /// </summary>
+        /// <param name="method">The method's MethodDef row. Methods must be added in MethodDef order.</param>
+        /// <param name="scopes">The method's lexical scopes (<see cref="EntityRegistry.MethodDebugInfo.LocalScopes"/>).</param>
+        /// <remarks>
+        /// <para>
+        /// As in native ilasm, a scope gets a row only when it declares a named local, and an unnamed local gets no
+        /// row; so the root scope, which spans the whole body, has a row only when the method-level <c>.locals</c>
+        /// name a local. Each variable's Index is its slot in the local signature, and its attributes are 0.
+        /// Import scopes and local constants are not recorded.
+        /// </para>
+        /// <para>
+        /// Two rules keep the rows valid where native ilasm writes rows the specification does not allow: a scope
+        /// without IL (a block with no instructions, or any scope of a method without a body) gets no row, because
+        /// a scope's length must be positive; and because a scope may not have two variables with the same name
+        /// or index, within a scope each name gets a LocalVariable row at the slot its first declaration has,
+        /// unless an earlier row of the scope already describes that slot. A name refers to its first declaration
+        /// in the scope, so a row never describes a local that its name does not refer to. Later declarations of
+        /// a name take no part, and neither do unnamed locals, which have no name to record: neither keeps a name
+        /// out of the rows.
+        /// </para>
+        /// <para>
+        /// The rows are sorted by start offset, then by length with the longest first, as the table requires;
+        /// scopes with the same range keep their source order, enclosing scope first. Scopes of a method nest or
+        /// are disjoint because blocks do, and the variables of a scope are added together right after it, so its
+        /// VariableList run holds exactly its variables.
+        /// </para>
+        /// </remarks>
+        private void AddLocalScopes(MethodDefinitionHandle method, List<EntityRegistry.LocalScopeRecord> scopes)
+        {
+            // Most methods name no local; they get no rows and need no sorting.
+            if (!scopes.Exists(static scope => !scope.Variables.IsEmpty))
+            {
+                return;
+            }
+
+            IEnumerable<EntityRegistry.LocalScopeRecord> ordered = scopes
+                .Where(scope => scope.Length > 0)
+                .OrderBy(scope => scope.StartOffset)
+                .ThenByDescending(scope => scope.Length)
+                .ThenBy(scope => scope.Order);
+            var names = new HashSet<string>(StringComparer.Ordinal);
+            var slots = new HashSet<int>();
+            var variables = new List<EntityRegistry.LocalVariableRecord>();
+            foreach (EntityRegistry.LocalScopeRecord scope in ordered)
+            {
+                names.Clear();
+                slots.Clear();
+                variables.Clear();
+                foreach (EntityRegistry.LocalVariableRecord variable in scope.Variables)
                 {
-                    singleDocument = sequencePoints[i].DocumentIndex == firstDocument;
+                    // Only the first declaration of a name takes part: it is the local the name refers to. A later
+                    // declaration of the name claims nothing, so it cannot keep another name's row out. The slot
+                    // is claimed only by a row that is written.
+                    if (names.Add(variable.Name) && slots.Add(variable.Slot))
+                    {
+                        variables.Add(variable);
+                    }
                 }
 
-                BlobBuilder sequencePointsBlob = EncodeSequencePoints(
-                    sequencePoints,
-                    method.DebugInfo.LocalSignature,
-                    documentHandles,
-                    singleDocument);
-
-                // docs/design/specs/PortablePdb-Metadata.md, MethodDebugInformation table: Document is "the row id of the
-                // single document containing all sequence points of the method, or 0 if the method doesn't have sequence
-                // points or spans multiple documents", and "_InitialDocument_ is only present if the _Document_ field of
-                // the _MethodDebugInformation_ table is nil"; the blob then names the documents (EncodeSequencePoints).
-                _pdbBuilder.AddMethodDebugInformation(
-                    singleDocument ? documentHandles[firstDocument] : default,
-                    _pdbBuilder.GetOrAddBlob(sequencePointsBlob));
+                if (variables.Count == 0)
+                {
+                    continue;
+                }
+
+                _pdbBuilder.AddLocalScope(
+                    method,
+                    importScope: default,
+                    variableList: MetadataTokens.LocalVariableHandle(_pdbBuilder.GetRowCount(TableIndex.LocalVariable) + 1),
+                    constantList: default,
+                    scope.StartOffset,
+                    scope.Length);
+                foreach (EntityRegistry.LocalVariableRecord variable in variables)
+                {
+                    _pdbBuilder.AddLocalVariable(
+                        LocalVariableAttributes.None,
+                        variable.Slot,
+                        _pdbBuilder.GetOrAddString(variable.Name));
+                }
             }
         }
 

src/tools/ilasm/src/ILAssembler/Actions/GrammarActions.Conversions.cs

@@ -200,7 +200,7 @@ public CurrentMethodContext(EntityRegistry.MethodDefinitionEntity definition)
                     }
                 }
 
-                OpenScopes.Add(new LexicalScope(startOffset: 0));
+                OpenScopes.Add(new LexicalScope(startOffset: 0, order: 0));
             }
 
             public EntityRegistry.MethodDefinitionEntity Definition { get; }
@@ -222,6 +222,11 @@ public CurrentMethodContext(EntityRegistry.MethodDefinitionEntity definition)
             /// Gets the method's local slots, indexed by slot. The body's local signature has one entry per slot.
             /// </summary>
             public List<LocalSlot> LocalSlots { get; } = new();
+
+            /// <summary>
+            /// Gets or sets the source-order position of the next block to open; the root scope is 0.
+            /// </summary>
+            public int NextScopeOrder { get; set; } = 1;
         }
 
         private CurrentMethodContext? _currentMethod;

src/tools/ilasm/src/ILAssembler/Actions/GrammarActions.MethodBodies.Locals.cs

@@ -210,6 +210,10 @@ private void DeclareLocals(
             if (local.Name is not null)
             {
                 scope.Names.TryAdd(local.Name, index);
+                if (GeneratesPdb)
+                {
+                    scope.Variables.Add(new EntityRegistry.LocalVariableRecord(local.Name, index));
+                }
             }
         }
     }
@@ -218,15 +222,18 @@ private void DeclareLocals(
     /// Opens a lexical scope for a <c>{ }</c> block of the current method body, starting at the current offset.
     /// </summary>
     private void OpenLexicalScope(CurrentMethodContext method)
-        => method.OpenScopes.Add(new LexicalScope(CurrentMethodBodyOffset));
+        => method.OpenScopes.Add(new LexicalScope(CurrentMethodBodyOffset, method.NextScopeOrder++));
 
     /// <summary>
     /// Closes the innermost open lexical scopes of the method, at the current offset, until <paramref name="count"/>
     /// remain open. The slots that a closed scope declared are no longer in use, even when an enclosing scope that
-    /// is still open declared the same slot (<see cref="LocalSlot.InScope"/>).
+    /// is still open declared the same slot (<see cref="LocalSlot.InScope"/>). When a PDB is requested
+    /// (<see cref="GeneratesPdb"/>), the scope is recorded in the method's
+    /// <see cref="EntityRegistry.MethodDebugInfo.LocalScopes"/> for the PDB.
     /// </summary>
     private void CloseLexicalScopes(CurrentMethodContext method, int count)
     {
+        bool recordScopes = GeneratesPdb;
         while (method.OpenScopes.Count > count)
         {
             LexicalScope scope = method.OpenScopes[^1];
@@ -235,6 +242,15 @@ private void CloseLexicalScopes(CurrentMethodContext method, int count)
             {
                 method.LocalSlots[slot].InScope = false;
             }
+
+            if (recordScopes)
+            {
+                method.Definition.DebugInfo.LocalScopes.Add(new EntityRegistry.LocalScopeRecord(
+                    scope.StartOffset,
+                    CurrentMethodBodyOffset,
+                    scope.Order,
+                    scope.Variables.ToImmutableArray()));
+            }
         }
     }
 
@@ -308,12 +324,28 @@ private sealed class LocalSlot
     /// </summary>
     private sealed class LexicalScope
     {
+        /// <summary>Creates an open scope that has declared no locals yet.</summary>
         /// <param name="startOffset">The IL offset at which the scope starts.</param>
-        public LexicalScope(int startOffset) => StartOffset = startOffset;
+        /// <param name="order">The position of the scope in source order (<see cref="EntityRegistry.LocalScopeRecord.Order"/>).</param>
+        public LexicalScope(int startOffset, int order)
+        {
+            StartOffset = startOffset;
+            Order = order;
+        }
 
         /// <summary>Gets the IL offset at which the scope starts: 0 for the root scope, the offset at <c>{</c> for a block.</summary>
         public int StartOffset { get; }
 
+        /// <summary>Gets the position of the scope in source order: 0 for the root scope, then each block as it opens.</summary>
+        public int Order { get; }
+
+        /// <summary>
+        /// Gets the named locals declared in this scope, in declaration order, including a second declaration of a
+        /// name or a slot, from which the scope's LocalVariable rows are chosen. Filled only when a PDB is requested
+        /// (<see cref="GeneratesPdb"/>).
+        /// </summary>
+        public List<EntityRegistry.LocalVariableRecord> Variables { get; } = new();
+
         /// <summary>
         /// Gets the slot of each name declared in this scope. Names resolve from the innermost open scope outward.
         /// </summary>