Fix off-by-one error when calculating GUID handle generation in MetadataAggregator...

Commit migrated from https://github.com/dotnet/corefx/commit/366c75f583243969a6906410231595523f60bb72

diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataAggregator.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataAggregator.cs
index 0ae7a0b..8988cd4 100644 (file)
--- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataAggregator.cs
+++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataAggregator.cs
@@ -11,7 +11,7 @@ namespace System.Reflection.Metadata.Ecma335
     public sealed class MetadataAggregator
     {
         // For each heap handle and each delta contains aggregate heap lengths.
-        // heapSizes[heap kind][reader index] == Sum { 0..index | reader[i].XxxHeapLength }
+        // heapSizes[heap kind][reader index] == Sum { 0..index | reader[i].XxxHeap.Block.Length }
         private readonly ImmutableArray<ImmutableArray<int>> _heapSizes;
 
         private readonly ImmutableArray<ImmutableArray<RowCounts>> _rowCounts;
@@ -238,6 +238,13 @@ namespace System.Reflection.Metadata.Ecma335
             }
         }
 
+        /// <summary>
+        /// Given a handle of an entity in an aggregate metadata calculates 
+        /// a handle of the entity within the metadata generation it is defined in.
+        /// </summary>
+        /// <param name="handle">Handle of an entity in an aggregate metadata.</param>
+        /// <param name="generation">The generation the entity is defined in.</param>
+        /// <returns>Handle of the entity within the metadata generation <paramref name="generation"/>.</returns>
         public Handle GetGenerationHandle(Handle handle, out int generation)
         {
             if (handle.IsVirtual)
@@ -256,17 +263,20 @@ namespace System.Reflection.Metadata.Ecma335
 
                 var sizes = _heapSizes[(int)heapIndex];
 
-                generation = sizes.BinarySearch(heapOffset);
+                // #Guid heap offset is 1-based, other heaps have 0-based offset:
+                var size = (handle.Type == HandleType.Guid) ? heapOffset - 1 : heapOffset;
+
+                generation = sizes.BinarySearch(size);
                 if (generation >= 0)
                 {
-                    Debug.Assert(sizes[generation] == heapOffset);
+                    Debug.Assert(sizes[generation] == size);
 
                     // the index points to the start of the next generation that added data to the heap:
                     do
                     {
                         generation++;
                     }
-                    while (generation < sizes.Length && sizes[generation] == heapOffset);
+                    while (generation < sizes.Length && sizes[generation] == size);
                 }
                 else
                 {
@@ -278,7 +288,7 @@ namespace System.Reflection.Metadata.Ecma335
                     throw new ArgumentException(SR.HandleBelongsToFutureGeneration, nameof(handle));
                 }
 
-                // GUID heap accumulates - previous heap is copied to the next generation 
+                // GUID heap accumulates - previous heap is copied to the next generation
                 int relativeHeapOffset = (handle.Type == HandleType.Guid || generation == 0) ? heapOffset : heapOffset - sizes[generation - 1];
 
                 return new Handle((byte)handle.Type, relativeHeapOffset);

diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataTokens.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataTokens.cs
index b86fcea..b05924b 100644 (file)
--- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataTokens.cs
+++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataTokens.cs
@@ -164,8 +164,7 @@ namespace System.Reflection.Metadata.Ecma335
         /// to the specified <paramref name="handle"/>.
         /// </summary>
         /// <returns>
-        /// Zero based offset, or -1 if <paramref name="handle"/> can only be interpreted in a context of a specific <see cref="MetadataReader"/> or <see cref="MetadataBuilder"/>.
-        /// See <see cref="GetHeapOffset(MetadataReader, Handle)"/>.
+        /// 1-based index into the #Guid heap. Unlike other heaps, which are essentially byte arrays, the #Guid heap is an array of 16-byte GUIDs.
         /// </returns>
         public static int GetHeapOffset(GuidHandle handle) => handle.Index;
 
@@ -174,8 +173,7 @@ namespace System.Reflection.Metadata.Ecma335
         /// to the specified <paramref name="handle"/>.
         /// </summary>
         /// <returns>
-        /// Zero based offset, or -1 if <paramref name="handle"/> can only be interpreted in a context of a specific <see cref="MetadataReader"/> or <see cref="MetadataBuilder"/>.
-        /// See <see cref="GetHeapOffset(MetadataReader, Handle)"/>.
+        /// Zero based offset.
         /// </returns>
         public static int GetHeapOffset(UserStringHandle handle) => handle.GetHeapOffset();
 

diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs
index e637493..8c6bdbd 100644 (file)
--- a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs
+++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs
@@ -35,10 +35,10 @@ namespace System.Reflection.Metadata.Ecma335.Tests
             Assert.Equal(expected, string.Join(" | ", actual));
         }
 
-        private static void TestGenerationHandle(MetadataAggregator aggregator, Handle handle, Handle expectedHandle, int expectedGeneration)
+        private static void TestGenerationHandle(MetadataAggregator aggregator, Handle aggregateHandle, Handle expectedHandle, int expectedGeneration)
         {
             int actualGeneration;
-            var actualHandle = aggregator.GetGenerationHandle(handle, out actualGeneration);
+            var actualHandle = aggregator.GetGenerationHandle(aggregateHandle, out actualGeneration);
             Assert.Equal(expectedGeneration, actualGeneration);
             Assert.Equal(expectedHandle, actualHandle);
         }
@@ -152,13 +152,13 @@ namespace System.Reflection.Metadata.Ecma335.Tests
                     200,    // Gen3
                     400,    // Gen4
                 },
-                new int[] // #Guid
+                new int[] // #Guid (sizes are numbers of GUIDs on the heap, not bytes; GUIDs on the heap accumulate, previous content is copied to the next gen).
                 {
-                    2,      // Gen0
-                    4,      // Gen1
-                    8,      // Gen2
-                    16,     // Gen3
-                    32,     // Gen4
+                    1,      // Gen0: Guid #1
+                    2,      // Gen1: Guid #1, #2
+                    2,      // Gen2: Guid #1, #2
+                    2,      // Gen3: Guid #1, #2
+                    3,      // Gen4: Guid #1, #2, #3
                 }
             };
 
@@ -170,6 +170,11 @@ namespace System.Reflection.Metadata.Ecma335.Tests
             TestGenerationHandle(aggregator, MetadataTokens.UserStringHandle(12), expectedHandle: MetadataTokens.UserStringHandle(2), expectedGeneration: 2);
             TestGenerationHandle(aggregator, MetadataTokens.StringHandle(0), expectedHandle: MetadataTokens.StringHandle(0), expectedGeneration: 2);
 
+            // GUIDs on the heap accumulate, previous content is copied to the next gen, so the expected handle is the same as the given handle
+            TestGenerationHandle(aggregator, MetadataTokens.GuidHandle(1), expectedHandle: MetadataTokens.GuidHandle(1), expectedGeneration: 0);
+            TestGenerationHandle(aggregator, MetadataTokens.GuidHandle(2), expectedHandle: MetadataTokens.GuidHandle(2), expectedGeneration: 1);
+            TestGenerationHandle(aggregator, MetadataTokens.GuidHandle(3), expectedHandle: MetadataTokens.GuidHandle(3), expectedGeneration: 4);
+
             AssertExtensions.Throws<ArgumentException>("handle", () => TestGenerationHandle(aggregator, MetadataTokens.StringHandle(22), expectedHandle: MetadataTokens.StringHandle(0), expectedGeneration: 0));
         }
     }