Skip to content

Fix handling of multidimensional arrays in type system and dotnet-pgo #51764

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 26, 2021
Merged

Fix handling of multidimensional arrays in type system and dotnet-pgo #51764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ public enum MethodSignatureFlags
public enum EmbeddedSignatureDataKind
{
RequiredCustomModifier = 0,
OptionalCustomModifier = 1
OptionalCustomModifier = 1,
ArrayShape = 2
}

public struct EmbeddedSignatureData
Expand Down
38 changes: 31 additions & 7 deletions src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Reflection.Metadata;
using System.Runtime.InteropServices;
using System.Diagnostics;
using System.Text;

using Internal.TypeSystem;
using System.Collections.Generic;
Expand Down Expand Up @@ -159,13 +160,36 @@ private TypeDesc ParseTypeImpl(SignatureTypeCode typeCode)
var elementType = ParseType();
var rank = _reader.ReadCompressedInteger();

// TODO: Bounds for multi-dimmensional arrays
var boundsCount = _reader.ReadCompressedInteger();
for (int i = 0; i < boundsCount; i++)
_reader.ReadCompressedInteger();
var lowerBoundsCount = _reader.ReadCompressedInteger();
for (int j = 0; j < lowerBoundsCount; j++)
_reader.ReadCompressedInteger();
if (_embeddedSignatureDataList != null)
{
var boundsCount = _reader.ReadCompressedInteger();
int []bounds = boundsCount > 0 ? new int[boundsCount] : Array.Empty<int>();
for (int i = 0; i < boundsCount; i++)
bounds[i] = _reader.ReadCompressedInteger();

var lowerBoundsCount = _reader.ReadCompressedInteger();
int []lowerBounds = lowerBoundsCount > 0 ? new int[lowerBoundsCount] : Array.Empty<int>();
for (int j = 0; j < lowerBoundsCount; j++)
lowerBounds[j] = _reader.ReadCompressedSignedInteger();

if (boundsCount != 0 || lowerBoundsCount != 0)
{
StringBuilder arrayShapeString = new StringBuilder();
arrayShapeString.Append(string.Join(",", bounds));
arrayShapeString.Append('|');
arrayShapeString.Append(string.Join(",", lowerBounds));
_embeddedSignatureDataList.Add(new EmbeddedSignatureData { index = string.Join(".", _indexStack) + "|" + arrayShapeString.ToString(), kind = EmbeddedSignatureDataKind.ArrayShape, type = null });
}
}
else
{
var boundsCount = _reader.ReadCompressedInteger();
for (int i = 0; i < boundsCount; i++)
_reader.ReadCompressedInteger();
var lowerBoundsCount = _reader.ReadCompressedInteger();
for (int j = 0; j < lowerBoundsCount; j++)
_reader.ReadCompressedSignedInteger();
}

if (elementType != null)
return _tsc.GetArrayType(elementType, rank);
Expand Down
60 changes: 52 additions & 8 deletions ...s/dotnet-pgo/TypeSystemMetadataEmitter.cs → ...adataEmitter/TypeSystemMetadataEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
using System.Reflection.PortableExecutable;
using Internal.TypeSystem;

namespace Microsoft.Diagnostics.Tools.Pgo
namespace Internal.TypeSystem
{
class TypeSystemMetadataEmitter
{
Expand Down Expand Up @@ -134,10 +134,8 @@ public EntityHandle GetTypeRef(TypeDesc type)

EntityHandle typeHandle;

if (type.IsTypeDefinition)
if (type.IsTypeDefinition && type is MetadataType metadataType)
{
MetadataType metadataType = (MetadataType)type;

// Make a typeref
StringHandle typeName = _metadataBuilder.GetOrAddString(metadataType.Name);
StringHandle typeNamespace = metadataType.Namespace != null ? _metadataBuilder.GetOrAddString(metadataType.Namespace) : default(StringHandle);
Expand Down Expand Up @@ -293,9 +291,8 @@ private void EncodeType(BlobBuilder blobBuilder, TypeDesc type, EmbeddedSignatur
var arrayType = (ArrayType)type;
blobBuilder.WriteByte((byte)SignatureTypeCode.Array);
EncodeType(blobBuilder, type.GetParameterType(), signatureDataEmitter);
var shapeEncoder = new ArrayShapeEncoder(blobBuilder);
// TODO Add support for non-standard array shapes
shapeEncoder.Shape(arrayType.Rank, default(ImmutableArray<int>), default(ImmutableArray<int>));

signatureDataEmitter.EmitArrayShapeAtCurrentIndexStack(blobBuilder, arrayType.Rank);
}
else if (type.IsPointer)
{
Expand Down Expand Up @@ -386,6 +383,51 @@ public void Push()
}
}

public void EmitArrayShapeAtCurrentIndexStack(BlobBuilder signatureBuilder, int rank)
{
var shapeEncoder = new ArrayShapeEncoder(signatureBuilder);

bool emittedWithShape = false;

if (!Complete)
{
if (_embeddedDataIndex < _embeddedData.Length)
{
if (_embeddedData[_embeddedDataIndex].kind == EmbeddedSignatureDataKind.ArrayShape)
{
string indexData = string.Join(".", _indexStack);

var arrayShapePossibility = _embeddedData[_embeddedDataIndex].index.Split('|');
if (arrayShapePossibility[0] == indexData)
{
string[] boundsStr = arrayShapePossibility[1].Split(',', StringSplitOptions.RemoveEmptyEntries);
string[] loBoundsStr = arrayShapePossibility[2].Split(',', StringSplitOptions.RemoveEmptyEntries);
int[] bounds = new int[boundsStr.Length];
int[] loBounds = new int[loBoundsStr.Length];

for (int i = 0; i < boundsStr.Length; i++)
{
bounds[i] = Int32.Parse(boundsStr[i]);
}
for (int i = 0; i < loBoundsStr.Length; i++)
{
loBounds[i] = Int32.Parse(loBoundsStr[i]);
}

shapeEncoder.Shape(rank, ImmutableArray.Create(bounds), ImmutableArray.Create(loBounds));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - would it be useful to add assertions checking the bounds / lower bounds length against the array rank?

_embeddedDataIndex++;
return;
}
}
}
}

if (!emittedWithShape)
{
shapeEncoder.Shape(rank, ImmutableArray<int>.Empty, ImmutableArray<int>.Empty);
}
}

public void EmitAtCurrentIndexStack(BlobBuilder signatureBuilder)
{
if (!Complete)
Expand Down Expand Up @@ -413,6 +455,9 @@ public void EmitAtCurrentIndexStack(BlobBuilder signatureBuilder)
}
break;

case EmbeddedSignatureDataKind.ArrayShape:
return;

default:
throw new NotImplementedException();
}
Expand Down Expand Up @@ -472,7 +517,6 @@ void EncodeMethodSignature(BlobBuilder signatureBuilder, MethodSignature sig, Em

signatureEncoder.MethodSignature(sigCallingConvention, genericParameterCount, isInstanceMethod);
signatureBuilder.WriteCompressedInteger(sig.Length);
// TODO Process custom modifiers in some way
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my education - does deletion of this comment mean that this has been fixed independently or that we no longer care about custom modifiers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already wrote the code to handle custom modifiers a year or so ago, but never actually cleaned up this comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular that's what the embedded signature data stuff was written for originally. I will say that it is completely and totally incomprehensible as Michal has noted before, but it does work. And with handling of array shapes we hopefully won't ever have to touch it again.

EncodeType(signatureBuilder, sig.ReturnType, signatureDataEmitter);
for (int i = 0; i < sig.Length; i++)
EncodeType(signatureBuilder, sig[i], signatureDataEmitter);
Expand Down
1 change: 1 addition & 0 deletions .../aot/ILCompiler.TypeSystem.ReadyToRun.Tests/ILCompiler.TypeSystem.ReadyToRun.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
</ProjectReference>
</ItemGroup>
<ItemGroup>
<Compile Include="../../Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs" />
<Compile Include="ArchitectureSpecificFieldLayoutTests.cs" />
<Compile Include="CanonicalizationTests.cs" />
<Compile Include="ConstraintsValidationTest.cs" />
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/ILTestAssembly/Signature.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@
{
ret
}

.method public hidebysig instance int32 modopt([CoreTestAssembly]System.Void) [3,1 ... 4] Method4(int32 modopt(FooModifier)[2 ...,0...8], int32 modopt(FooModifier)[0...,...], int32 [,,,]) cil managed
{
ret
}
}

.class private auto ansi beforefieldinit Atom
Expand Down
78 changes: 77 additions & 1 deletion src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/SignatureTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;

using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;

using Internal.IL;
using Internal.TypeSystem;
using Internal.TypeSystem.Ecma;
Expand Down Expand Up @@ -37,7 +41,10 @@ private static string GetModOptMethodSignatureInfo(MethodSignature signature)
{
sb.Append(data.kind.ToString());
sb.Append(data.index);
sb.Append(((MetadataType)data.type).Name);
if (data.type != null)
sb.Append(((MetadataType)data.type).Name);
else
sb.Append("<null>");
}
return sb.ToString();
}
Expand Down Expand Up @@ -79,6 +86,19 @@ public void TestSignatureMatchesModoptOnPointerOrRefModifiedType()
Assert.Equal(MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(2), methodWithModOpt.GetEmbeddedSignatureData()[2].index);
}

[Fact]
public void TestSignatureMatchesForArrayShapeDetails()
{
MetadataType modOptTester = _testModule.GetType("", "ModOptTester");
MethodSignature methodWithModOpt = modOptTester.GetMethods().Single(m => string.Equals(m.Name, "Method4")).Signature;

Assert.Equal(6, methodWithModOpt.GetEmbeddedSignatureData().Length);
Assert.Equal(MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(0), methodWithModOpt.GetEmbeddedSignatureData()[0].index);
Assert.Equal(MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(1), methodWithModOpt.GetEmbeddedSignatureData()[2].index);
Assert.Equal(MethodSignature.GetIndexOfCustomModifierOnPointedAtTypeByParameterIndex(2), methodWithModOpt.GetEmbeddedSignatureData()[4].index);
Assert.Equal("OptionalCustomModifier0.1.1.2.1.1VoidArrayShape1.2.1.1|3,4|0,1<null>OptionalCustomModifier0.1.1.2.2.1FooModifierArrayShape1.2.2.1|0,9|2,0<null>OptionalCustomModifier0.1.1.2.3.1FooModifierArrayShape1.2.3.1||0<null>", GetModOptMethodSignatureInfo(methodWithModOpt));
}

[Fact]
public void TestSignatureMatches()
{
Expand All @@ -101,5 +121,61 @@ public void TestSignatureMatches()
int matchCount = matches.Select(b => b ? 1 : 0).Sum();
Assert.Equal(1, matchCount);
}

[Fact]
public void TestSerializedSignatureWithArrayShapes()
{
MetadataType modOptTester = _testModule.GetType("", "ModOptTester");
MethodDesc methodWithInterestingShapes = modOptTester.GetMethods().Single(m => string.Equals(m.Name, "Method4"));

// Create assembly with reference to interesting method
TypeSystemMetadataEmitter metadataEmitter = new TypeSystemMetadataEmitter(new System.Reflection.AssemblyName("Lookup"), _context);
var token = metadataEmitter.GetMethodRef(methodWithInterestingShapes);
Stream peStream = new MemoryStream();
metadataEmitter.SerializeToStream(peStream);
peStream.Seek(0, SeekOrigin.Begin);


// Create new TypeSystemContext with just created assembly inside
var lookupContext = new TestTypeSystemContext(TargetArchitecture.X64);
var systemModule = lookupContext.CreateModuleForSimpleName("CoreTestAssembly");
lookupContext.SetSystemModule(systemModule);

lookupContext.CreateModuleForSimpleName("Lookup", peStream);

// Use generated assembly to trigger a load through the token created above and verify that it loads correctly
var ilLookupModule = (EcmaModule)lookupContext.GetModuleForSimpleName("Lookup");
MethodDesc method4 = ilLookupModule.GetMethod(token);

Assert.Equal("Method4", method4.Name);
}


[Fact]
public void TestSerializedSignatureWithReferenceToMDIntArray()
{
var typeInInitialContext = _context.GetWellKnownType(WellKnownType.Int32).MakeArrayType(3);

// Create assembly with reference to interesting type
TypeSystemMetadataEmitter metadataEmitter = new TypeSystemMetadataEmitter(new System.Reflection.AssemblyName("Lookup"), _context);
var token = metadataEmitter.GetTypeRef(typeInInitialContext);
Stream peStream = new MemoryStream();
metadataEmitter.SerializeToStream(peStream);
peStream.Seek(0, SeekOrigin.Begin);


// Create new TypeSystemContext with just created assembly inside
var lookupContext = new TestTypeSystemContext(TargetArchitecture.X64);
var systemModule = lookupContext.CreateModuleForSimpleName("CoreTestAssembly");
lookupContext.SetSystemModule(systemModule);
lookupContext.CreateModuleForSimpleName("Lookup", peStream);

// Use generated assembly to trigger a load through the token created above and verify that it loads correctly
var ilLookupModule = (EcmaModule)lookupContext.GetModuleForSimpleName("Lookup");
TypeDesc int32ArrayFromLookup = ilLookupModule.GetType(token);
var typeInLookupContext = lookupContext.GetWellKnownType(WellKnownType.Int32).MakeArrayType(3);

Assert.Equal(typeInLookupContext, int32ArrayFromLookup);
}
}
}
10 changes: 8 additions & 2 deletions src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/TestTypeSystemContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,17 @@ public ModuleDesc GetModuleForSimpleName(string simpleName)
return CreateModuleForSimpleName(simpleName);
}

public ModuleDesc CreateModuleForSimpleName(string simpleName)
public ModuleDesc CreateModuleForSimpleName(string simpleName, Stream preLoadedFile = null)
{
string bindingDirectory = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location);
string filePath = Path.Combine(bindingDirectory, simpleName + ".dll");
ModuleDesc module = Internal.TypeSystem.Ecma.EcmaModule.Create(this, new PEReader(File.OpenRead(filePath)), containingAssembly: null);
Stream peStream = preLoadedFile;
if (peStream == null)
{
peStream = File.OpenRead(filePath);
}

ModuleDesc module = Internal.TypeSystem.Ecma.EcmaModule.Create(this, new PEReader(peStream), containingAssembly: null);
_modules.Add(simpleName, module);
return module;
}
Expand Down
Loading