Skip to content
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

Replace RuntimeHelpers.CreateSpan<T>(LdMemberToken) with new T[] { } #3380

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+73 −0
Open

Replace RuntimeHelpers.CreateSpan<T>(LdMemberToken) with new T[] { } #3380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b6b1ed
WIP handling of RuntimeHelpers.CreateSpan<T>
ds5678 Jan 24, 2025
f2c6aa1
Improve preprocessor directive
ds5678 Jan 24, 2025
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
12 changes: 12 additions & 0 deletions ICSharpCode.Decompiler.Tests/TestCases/Correctness/InitializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,18 @@ public static void StructInitializerWithinObjectInitializer()
});
}

#if !NET40 && ROSLYN
public static ReadOnlySpan<byte> ReadOnlySpanInitializer_ByteArray()
{
return new byte[] { 1, 2, 3 };
}

public static ReadOnlySpan<int> ReadOnlySpanInitializer_Int32Array()
{
return new int[] { 1, 2, 3 };
}
#endif

Comment on lines +378 to +389
Copy link
Author

Choose a reason for hiding this comment

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

Should I have put these in a new file and modified the options? It currently fails for Roslyn v1.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why these tests are "correctness" tests and not "pretty" tests?

It seems like this is a relatively new language feature... you can simply use the CSx preprocessor symbol that matches the earliest version of C# where this feature is supported.

public static void Bug270_NestedInitialisers()
{
NumberFormatInfo[] numberFormats = null;
Expand Down
6 changes: 6 additions & 0 deletions ICSharpCode.Decompiler/IL/Transforms/ExpressionTransforms.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,12 @@ protected internal override void VisitCall(Call inst)
replacement.AcceptVisitor(this);
return;
}
if (TransformArrayInitializers.TransformRuntimeHelpersCreateSpanInitialization(inst, context, out var replacement2))
{
context.Step("TransformRuntimeHelpersCreateSpanInitialization: single-dim", inst);
inst.ReplaceWith(replacement2);
return;
}
base.VisitCall(inst);
TransformAssignment.HandleCompoundAssign(inst, context);
}
Expand Down
55 changes: 55 additions & 0 deletions ICSharpCode.Decompiler/IL/Transforms/TransformArrayInitializers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,40 @@ internal static bool TransformSpanTArrayInitialization(NewObj inst, StatementTra
return false;
}

internal static bool TransformRuntimeHelpersCreateSpanInitialization(Call inst, StatementTransformContext context, out ILInstruction replacement)
Copy link
Author

Choose a reason for hiding this comment

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

Thoughts on the method name?

Copy link
Member

Choose a reason for hiding this comment

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

Not really, I am bad at naming things... 😅 ... luckily I had nine months to name my children...

{
replacement = null;
if (!context.Settings.ArrayInitializers)
return false;
if (MatchRuntimeHelpersCreateSpan(inst, context, out var elementType, out var field))
{
if (field.HasFlag(System.Reflection.FieldAttributes.HasFieldRVA))
{
var valuesList = new List<ILInstruction>();
var initialValue = field.GetInitialValue(context.PEFile, context.TypeSystem);
var elementTypeSize = elementType.GetSize();
if (elementTypeSize <= 0 || initialValue.Length % elementTypeSize != 0)
return false;

var size = initialValue.Length / elementTypeSize;
if (context.Settings.Utf8StringLiterals &&
elementType.IsKnownType(KnownTypeCode.Byte) &&
DecodeUTF8String(initialValue, size, out string text))
{
replacement = new LdStrUtf8(text);
return true;
}
if (DecodeArrayInitializer(elementType, initialValue, new[] { size }, valuesList))
{
var tempStore = context.Function.RegisterVariable(VariableKind.InitializerTarget, new ArrayType(context.TypeSystem, elementType));
replacement = BlockFromInitializer(tempStore, elementType, new[] { size }, valuesList.ToArray());
return true;
}
Comment on lines +162 to +174
Copy link
Author

Choose a reason for hiding this comment

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

This is the same as for Span<T>(void*, int) handler. Should split this out into a shared function?

Copy link
Member

Choose a reason for hiding this comment

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

You could do that, yes...

}
}
return false;
}

private static unsafe bool DecodeUTF8String(BlobReader blob, int size, out string text)
{
if (size > blob.RemainingBytes)
Expand Down Expand Up @@ -187,6 +221,27 @@ static bool MatchSpanTCtorWithPointerAndSize(NewObj newObj, StatementTransformCo
return true;
}

static bool MatchRuntimeHelpersCreateSpan(Call inst, StatementTransformContext context, out IType elementType, out FieldDefinition field)
{
field = default;
elementType = null;
IType type = inst.Method.DeclaringType;
if (type.Namespace != "System.Runtime.CompilerServices" || type.Name != "RuntimeHelpers" || type.TypeParameterCount != 0)
Copy link
Author

Choose a reason for hiding this comment

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

Is this the right way to check? Should I have created a "known" type?

Copy link
Member

Choose a reason for hiding this comment

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

There already exists

ILSpy/ICSharpCode.Decompiler/IL/Transforms/TransformArrayInitializers.cs

Line 360 in a592169

static bool IsRuntimeHelpers(IType type) => type is { Name: "RuntimeHelpers", Namespace: "System.Runtime.CompilerServices" };

Although that's missing the check for TypeParameterCount, but you could add it.

I don't think it's necessary to add a known type for this...

return false;
if (inst.Arguments.Count != 1)
return false;
IMethod method = inst.Method;
if (method.Name != "CreateSpan" || method.TypeArguments.Count != 1)
return false;
Comment on lines +231 to +235
Copy link
Author

Choose a reason for hiding this comment

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

Should I be checking the full method signature? I noticed that the other handler doesn't check argument types.

Copy link
Member

Choose a reason for hiding this comment

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

checking more is usually better than checking less... note that the code in that file is very old and thus is not using some of the helpers and is not following the style of later transform implementations

Copy link
Member

Choose a reason for hiding this comment

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

Addendum: for methods we usually check the name, the number of parameters and then validate all the arguments, so checking the parameter types is usually not necessary.

elementType = method.TypeArguments[0];
if (!inst.Arguments[0].UnwrapConv(ConversionKind.StopGCTracking).MatchLdMemberToken(out var member))
return false;
if (member.MetadataToken.IsNil)
return false;
field = context.PEFile.Metadata.GetFieldDefinition((FieldDefinitionHandle)member.MetadataToken);
return true;
}

bool DoTransformMultiDim(ILFunction function, Block body, int pos)
{
if (pos >= body.Instructions.Count - 2)
Expand Down
Loading