-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix Metadata aggregator: removing cumulative sum for GUIDs heap offset #115268
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
55e1080
03e6cb9
6ab179c
1ccfa80
f842df6
b53be5a
5600560
923e406
f9199a2
09cb0e5
0f4c421
8793445
eb4f138
d15e547
808f42b
c86c65a
734d43a
501eb4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,12 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Collections.Generic; | ||
using System.Reflection.Internal; | ||
using System.Reflection.Metadata.Tests; | ||
using System.Reflection.PortableExecutable; | ||
using System.Runtime.InteropServices; | ||
using System.Text; | ||
using Xunit; | ||
using RowCounts = System.Reflection.Metadata.Ecma335.MetadataAggregator.RowCounts; | ||
|
||
|
@@ -175,6 +179,69 @@ public void HeapSizes() | |
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)); | ||
// Sizes are not cumulative for GUIDs. They represent the number of available GUIDs. | ||
AssertExtensions.Throws<ArgumentException>("handle", () => TestGenerationHandle(aggregator, MetadataTokens.GuidHandle(4), expectedHandle: MetadataTokens.StringHandle(0), expectedGeneration: 0)); | ||
} | ||
|
||
[Fact] | ||
public void HeapSize_GuidHeapSizes() | ||
{ | ||
MetadataReader baseReader = MetadataReaderTests.GetMetadataReader(NetModule.AppCS); ; | ||
var bytes = CreateMinimalReaderMetadataAsBytes(Guid.NewGuid()); | ||
unsafe | ||
{ | ||
fixed (byte* pointer = bytes) | ||
{ | ||
MetadataReader mr1 = CreateMinimalReaderWithGuid(pointer, bytes.Length); | ||
MetadataReader mr2 = CreateMinimalReaderWithGuid(pointer, bytes.Length); | ||
IReadOnlyList<MetadataReader> metadataReaders = new List<MetadataReader> { mr1, mr2 }; | ||
|
||
var aggregator = new MetadataAggregator(baseReader, metadataReaders); | ||
|
||
var guidSizeReader0 = baseReader.GetHeapSize(HeapIndex.Guid) / 16; | ||
Assert.Equal(1, guidSizeReader0); | ||
var guidSizeReader1 = mr1.GetHeapSize(HeapIndex.Guid) / 16; | ||
Assert.Equal(1, guidSizeReader1); | ||
var guidSizeReader2 = mr2.GetHeapSize(HeapIndex.Guid) / 16; | ||
Assert.Equal(1, guidSizeReader2); | ||
|
||
TestGenerationHandle(aggregator, MetadataTokens.GuidHandle(1), expectedHandle: MetadataTokens.GuidHandle(1), expectedGeneration: 0); | ||
|
||
// GUID-heap allocation shouldn't be cumulative, since GUIDs are copied among generations. | ||
// The delta-reader above need indeed a single GUID allocation in each gen. | ||
AssertExtensions.Throws<ArgumentException>("handle", () => TestGenerationHandle(aggregator, MetadataTokens.GuidHandle(2), expectedHandle: MetadataTokens.GuidHandle(2), expectedGeneration: 0)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be able to read 3 guids since there is one in baseline, and one in each delta, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tmat in a real case yes, not in this test though. With new implementation, each generation here allocates a single GUID, hence only that one at index 1 could be read. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} | ||
} | ||
|
||
private static byte[] CreateMinimalReaderMetadataAsBytes(Guid mvid) | ||
{ | ||
var builder = new MetadataBuilder(); | ||
GuidHandle mvidHandle = builder.GetOrAddGuid(mvid); | ||
|
||
// module-row name is mandatory | ||
StringHandle name = builder.GetOrAddString("TestModule"); | ||
builder.AddModule( | ||
generation: 0, | ||
moduleName: name, | ||
mvid: mvidHandle, | ||
encId: default, | ||
encBaseId: default); | ||
|
||
// Let's serialize metadata | ||
var root = new MetadataRootBuilder(builder); | ||
var bb = new BlobBuilder(); | ||
root.Serialize(bb, methodBodyStreamRva: 0, mappedFieldDataStreamRva: 0); | ||
return bb.ToArray(); | ||
} | ||
|
||
private static unsafe MetadataReader CreateMinimalReaderWithGuid(byte* pointer, int length) | ||
{ | ||
var reader = new MetadataReader(pointer, length, MetadataReaderOptions.None); | ||
// to avoid minimal flag exception. | ||
reader.TableRowCounts[(int)TableIndex.EncMap] = 1; | ||
reader.IsMinimalDelta = true; | ||
return reader; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line https://github.com/dotnet/runtime/pull/115268/files#diff-463bcba9d713f9c49493793a38b92567be4f344112abe18ebd08a34d3a5a1659R13 should be updated