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.

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

Start building a universal System.Linq.Expressions library #61952

Merged
merged 4 commits into from
Dec 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions eng/illink.targets
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@
<ILLinkArgs Condition="'$(ILLinkRewritePDBs)' == 'true' and Exists('$(ILLinkTrimAssemblySymbols)')">$(ILLinkArgs) -b true</ILLinkArgs>
<!-- pass the non-embedded descriptors xml file on the command line -->
<ILLinkArgs Condition="'$(ILLinkDescriptorsLibraryBuildXml)' != ''">$(ILLinkArgs) -x "$(ILLinkDescriptorsLibraryBuildXml)"</ILLinkArgs>
<ILLinkArgs Condition="'$(ILLinkSubstitutionsLibraryBuildXml)' != ''">$(ILLinkArgs) --substitutions "$(ILLinkSubstitutionsLibraryBuildXml)"</ILLinkArgs>
<!-- suppress warnings with the following codes:
IL2008: Could not find type A specified in resource B
IL2009: Could not find method A in type B specified in resource C
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,7 @@ public static partial class PlatformDetection
private static readonly Lazy<bool> s_LinqExpressionsBuiltWithIsInterpretingOnly = new Lazy<bool>(GetLinqExpressionsBuiltWithIsInterpretingOnly);
private static bool GetLinqExpressionsBuiltWithIsInterpretingOnly()
{
Type type = typeof(LambdaExpression);
if (type != null)
{
// The "Accept" method is under FEATURE_COMPILE conditional so it should not exist
MethodInfo methodInfo = type.GetMethod("Accept", BindingFlags.NonPublic | BindingFlags.Static);
return methodInfo == null;
}

return false;
return !(bool)typeof(LambdaExpression).GetMethod("get_CanCompileToIL").Invoke(null, Array.Empty<object>());
}

// Please make sure that you have the libgdiplus dependency installed.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<linker>
<assembly fullname="System.Linq.Expressions">
<type fullname="System.Linq.Expressions.LambdaExpression">
<method signature="System.Boolean get_CanCompileToIL()" body="stub" value="false" />
</type>
</assembly>
</linker>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<linker>
<assembly fullname="System.Linq.Expressions">
<type fullname="System.Linq.Expressions.LambdaExpression">
<method signature="System.Boolean get_CanCompileToIL()" feature="System.Linq.Expressions.CanCompileToIL" featurevalue="false" body="stub" value="false" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why new feature switch for code compilation mode? Also the defaults don't look correct for existing configurations (e.g. published trimmed console app).

Copy link
Member Author

Choose a reason for hiding this comment

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

#17973 (comment) has reasoning for why this needs to be different from RuntimeFeature.IsDynamicCodeSupported.

Also the defaults don't look correct for existing configurations (e.g. published trimmed console app).

Could you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

has reasoning for why this needs to be different from

I don't see the reasoning in the comment and this mode is not new in .NET7. In .NET6 we use the same options to detect NativeAOT like setup where IsDynamicCodeSupported indicates that code generation is possible with IsDynamicCodeCompiled specifying how it such generated code processed.

Could you clarify?

What does happen today when you publish trimmed console for desktops. I guess it has "CanCompileToIL" set to true and here we are setting it to false for everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see the reasoning in the comment and this mode is not new in .NET7. In .NET6 we use the same options to detect NativeAOT like setup where IsDynamicCodeSupported indicates that code generation is possible with IsDynamicCodeCompiled specifying how it such generated code processed.

We could make this so that the body of CanCompileToIL is implemented as RuntimeFeature.IsDynamicCodeSupported, but I'm a believer in separating behavioral changes from refactoring changes and this would be a behavioral change. It can happen in subsequent pull request as a thing that can be discussed separately from this refactoring.

What does happen today when you publish trimmed console for desktops. I guess it has "CanCompileToIL" set to true and here we are setting it to false for everything.

The substitution is set as feature="X" featurevalue="false" body="stub" value="false" - if I'm reading IL Linker sources right, this substitution will be ignored unless someone sets feature X to false. Is that correct reading? I wasn't able to build intuition behind what featurevalue=X featuredefault=Y means and always defer to reading linker source code. If my interpretation is correct, this will do the right thing for trimmed apps (IL compiler will be kept).

</type>
<type fullname="System.Dynamic.Utils.DelegateHelpers">
<method signature="System.Boolean get_CanEmitObjectArrayDelegate()" feature="System.Linq.Expressions.CanEmitObjectArrayDelegate" featurevalue="false" body="stub" value="false" />
</type>
<type fullname="System.Linq.Expressions.Interpreter.CallInstruction">
<method signature="System.Boolean get_CanCreateArbitraryDelegates()" feature="System.Linq.Expressions.CanCreateArbitraryDelegates" featurevalue="false" body="stub" value="false" />
</type>
Comment on lines +6 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these be RuntimeFeature(s) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Property living in Expressions was the last direction #17973 (comment) discussed. I'm just following that, given the API is unreviewed.

get_CanEmitObjectArrayDelegate and get_CanCreateArbitraryDelegates probably won't be APIs ever. CanCompileToIL might.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand exactly what they try to do but it seems to me they react to runtime limitations, right? Should not this be then call to runtime to get the actual value with a feature switch on top of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

get_CanEmitObjectArrayDelegate and get_CanCreateArbitraryDelegates could probably be replaced with RuntimeFeature.IsDynamicCodeSupported eventually, but it would be a behavior change and this pull request didn't intend to do behavior changes, just replace #defines with something that can be overriden.

Replacing get_CanCreateArbitraryDelegates with IsDynamicCodeSupported would make the iDevices version take the codepaths that correspond to FEATURE_DLG_INVOKE not being defined in the csproj and that's not how iDevices build was set up in #54970. If you would like me to make that change, I can do it, but I cannot investigate any possible fallouts from it because I don't work on or own iDevices. An observable difference will be slower perf when invoking method call instructions in the interpreter. An upside will be that some of the tests that were blocked in #54970 will work. Ideally all of this (configuring the library so that it properly works, and revisiting tests blocked in #54970) should happen in subsequent pull requests. I definitely plan to revisit the blocked tests.

get_CanEmitObjectArrayDelegate cannot be replaced with RuntimeFeature.IsDynamicCodeSupported at this moment because it requires a runtime API that is not implemented on Mono and would therefore break iDevices (it's an API to create a strongly typed Delegate that boxes all it's arguments, puts them into an array, dispatches to a Func<object?[], object?> delegate to do the actual work, and unboxes the result - doing all of that without runtime codegen).

</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,6 @@ TypesMustExist : Type 'System.Linq.Expressions.Interpreter.LightLambda' does not
TypesMustExist : Type 'System.Runtime.CompilerServices.CallSiteOps' does not exist in the reference but it does exist in the implementation.
TypesMustExist : Type 'System.Runtime.CompilerServices.Closure' does not exist in the reference but it does exist in the implementation.
TypesMustExist : Type 'System.Runtime.CompilerServices.RuntimeOps' does not exist in the reference but it does exist in the implementation.
Total Issues: 16
MembersMustExist : Member 'public System.Boolean System.Linq.Expressions.LambdaExpression.CanCompileToIL.get()' does not exist in the reference but it does exist in the implementation.
MembersMustExist : Member 'public System.Boolean System.Linq.Expressions.LambdaExpression.CanInterpret.get()' does not exist in the reference but it does exist in the implementation.
Total Issues: 18
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<FeatureInterpret>true</FeatureInterpret>
<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-iOS;$(NetCoreAppCurrent)-tvOS;$(NetCoreAppCurrent)-MacCatalyst</TargetFrameworks>
<Nullable>enable</Nullable>
<IsInterpreting>false</IsInterpreting>
<IsInterpreting Condition="'$(TargetsiOS)' == 'true' or '$(TargetstvOS)' == 'true' or '$(TargetsMacCatalyst)' == 'true'">true</IsInterpreting>
<DefineConstants> $(DefineConstants);FEATURE_DLG_INVOKE;FEATURE_FAST_CREATE</DefineConstants>
<DefineConstants Condition=" '$(IsInterpreting)' != 'true' ">$(DefineConstants);FEATURE_COMPILE</DefineConstants>
<DefineConstants Condition=" '$(FeatureInterpret)' == 'true' ">$(DefineConstants);FEATURE_INTERPRET</DefineConstants>
<ILLinkSubstitutionsLibraryBuildXml Condition="'$(IsInterpreting)' == 'true'">ILLink\ILLink.Substitutions.IsInterpreting.LibraryBuild.xml</ILLinkSubstitutionsLibraryBuildXml>
<DefineConstants> $(DefineConstants);FEATURE_FAST_CREATE</DefineConstants>
</PropertyGroup>
<ItemGroup>
<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.xml" />
</ItemGroup>
<ItemGroup>
<Compile Include="$(CommonPath)System\Obsoletions.cs"
Link="Common\System\Obsoletions.cs" />
Expand Down Expand Up @@ -125,7 +126,7 @@
<Compile Include="System\Dynamic\UnaryOperationBinder.cs" />
<Compile Include="System\Dynamic\IInvokeOnGetBinder.cs" />
</ItemGroup>
<ItemGroup Condition=" '$(IsInterpreting)' != 'true'">
<ItemGroup>
<Compile Include="System\Linq\Expressions\Compiler\AssemblyGen.cs" />
<Compile Include="System\Dynamic\Utils\Helpers.cs" />
<Compile Include="System\Linq\Expressions\Compiler\AnalyzedTree.cs" />
Expand Down Expand Up @@ -157,7 +158,7 @@
<Compile Include="System\Runtime\CompilerServices\RuntimeOps.ExpressionQuoter.cs" />
<Compile Include="System\Runtime\CompilerServices\RuntimeOps.RuntimeVariableList.cs" />
</ItemGroup>
<ItemGroup Condition="'$(IsInterpreting)' == 'true' Or '$(FeatureInterpret)' == 'true'">
<ItemGroup>
<Compile Include="System\Dynamic\Utils\DelegateHelpers.cs" />
<Compile Include="System\Linq\Expressions\Interpreter\AddInstruction.cs" />
<Compile Include="System\Linq\Expressions\Interpreter\AndInstruction.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ namespace System.Dynamic
{
internal static partial class UpdateDelegates
{
#if FEATURE_COMPILE
[Obsolete("pregenerated CallSite<T>.Update delegate", error: true)]
internal static TRet UpdateAndExecute1<T0, TRet>(CallSite site, T0 arg0)
{
Expand Down Expand Up @@ -2895,6 +2894,5 @@ internal static void NoMatchVoid10<T0, T1, T2, T3, T4, T5, T6, T7, T8, T9>(CallS
site._match = false;
return;
}
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ namespace System.Dynamic
{
internal static partial class UpdateDelegates
{
#if FEATURE_COMPILE
<#
for (int i = 1; i <= 10; i++)
{
Expand Down Expand Up @@ -330,6 +329,5 @@ for (int i = 1; i <= 10; i++)
}
}
#>
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,38 @@

using System.Diagnostics.CodeAnalysis;
using System.Reflection;

#if !FEATURE_DYNAMIC_DELEGATE
using System.Reflection.Emit;
using System.Text;
using System.Threading;
#endif

namespace System.Dynamic.Utils
{
internal static class DelegateHelpers
{
internal static Delegate CreateObjectArrayDelegate(Type delegateType, Func<object?[], object?> handler)
// This can be flipped to true using feature switches at publishing time
internal static bool CanEmitObjectArrayDelegate => string.Empty != null; // HACK: complex enough so that ILLink doesn't constprop at library build
MichalStrehovsky marked this conversation as resolved.
Show resolved Hide resolved

// Separate class so that the it can be trimmed away and doesn't get conflated
// with the Reflection.Emit statics below.
private static class DynamicDelegateLightup
{
#if !FEATURE_DYNAMIC_DELEGATE
return CreateObjectArrayDelegateRefEmit(delegateType, handler);
#else
return Internal.Runtime.Augments.DynamicDelegateAugments.CreateObjectArrayDelegate(delegateType, handler);
#endif
public static Func<Type, Func<object?[], object?>, Delegate> CreateObjectArrayDelegate { get; }
= Type.GetType("Internal.Runtime.Augments.DynamicDelegateAugments")!
.GetMethod("CreateObjectArrayDelegate")!
.CreateDelegate<Func<Type, Func<object?[], object?>, Delegate>>();
}


#if !FEATURE_DYNAMIC_DELEGATE
internal static Delegate CreateObjectArrayDelegate(Type delegateType, Func<object?[], object?> handler)
{
if (CanEmitObjectArrayDelegate)
{
return CreateObjectArrayDelegateRefEmit(delegateType, handler);
}
else
{
return DynamicDelegateLightup.CreateObjectArrayDelegate(delegateType, handler);
}
}

private static readonly CacheDict<Type, MethodInfo> s_thunks = new CacheDict<Type, MethodInfo>(256);
private static readonly MethodInfo s_FuncInvoke = typeof(Func<object?[], object?>).GetMethod("Invoke")!;
Expand Down Expand Up @@ -290,7 +300,5 @@ private static Type ConvertToBoxableType(Type t)
{
return (t.IsPointer) ? typeof(IntPtr) : t;
}

#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ internal static ParameterInfo[] GetParametersCached(this MethodBase method)
return pis;
}

#if FEATURE_COMPILE
// Expression trees/compiler just use IsByRef, why do we need this?
// (see LambdaCompiler.EmitArguments for usage in the compiler)
internal static bool IsByRefParameter(this ParameterInfo pi)
Expand All @@ -91,6 +90,5 @@ internal static bool IsByRefParameter(this ParameterInfo pi)

return (pi.Attributes & ParameterAttributes.Out) == ParameterAttributes.Out;
}
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -911,8 +911,6 @@ public static MethodInfo GetInvokeMethod(this Type delegateType)
return delegateType.GetMethod("Invoke", BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)!;
}

#if FEATURE_COMPILE

internal static bool IsUnsigned(this Type type) => IsUnsigned(GetNonNullableType(type).GetTypeCode());

internal static bool IsUnsigned(this TypeCode typeCode)
Expand Down Expand Up @@ -946,8 +944,6 @@ internal static bool IsFloatingPoint(this TypeCode typeCode)
}
}

#endif

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
Justification = "The Array 'Get' method is dynamically constructed and is not included in IL. It is not subject to trimming.")]
public static MethodInfo GetArrayGetMethod(Type arrayType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ public static FieldInfo DateTime_MinValue
(s_Math_Pow_Double_Double = typeof(Math).GetMethod(nameof(Math.Pow), new[] { typeof(double), typeof(double) })!);

// Closure and RuntimeOps helpers are used only in the compiler.
#if FEATURE_COMPILE
private static ConstructorInfo? s_Closure_ObjectArray_ObjectArray;
public static ConstructorInfo Closure_ObjectArray_ObjectArray =>
s_Closure_ObjectArray_ObjectArray ??
Expand Down Expand Up @@ -194,6 +193,5 @@ public static FieldInfo DateTime_MinValue
public static MethodInfo RuntimeOps_Quote =>
s_RuntimeOps_Quote ??
(s_RuntimeOps_Quote = typeof(RuntimeOps).GetMethod(nameof(RuntimeOps.Quote))!);
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ private static TypeInfo NextTypeInfo(Type initialArg, TypeInfo curTypeInfo)
return nextTypeInfo;
}

#if !FEATURE_COMPILE

public delegate object VBCallSiteDelegate0<T>(T callSite, object instance);
public delegate object VBCallSiteDelegate1<T>(T callSite, object instance, ref object arg1);
public delegate object VBCallSiteDelegate2<T>(T callSite, object instance, ref object arg1, ref object arg2);
Expand All @@ -95,7 +93,6 @@ private static TypeInfo NextTypeInfo(Type initialArg, TypeInfo curTypeInfo)
public delegate object VBCallSiteDelegate6<T>(T callSite, object instance, ref object arg1, ref object arg2, ref object arg3, ref object arg4, ref object arg5, ref object arg6);
public delegate object VBCallSiteDelegate7<T>(T callSite, object instance, ref object arg1, ref object arg2, ref object arg3, ref object arg4, ref object arg5, ref object arg6, ref object arg7);


private static Type TryMakeVBStyledCallSite(Type[] types)
{
// Shape of VB CallSiteDelegates is CallSite * (instance : obj) * [arg-n : byref obj] -> obj
Expand Down Expand Up @@ -128,7 +125,6 @@ private static Type TryMakeVBStyledCallSite(Type[] types)
default: return null;
}
}
#endif

/// <summary>
/// Creates a new delegate, or uses a func/action
Expand Down Expand Up @@ -163,11 +159,14 @@ internal static Type MakeNewDelegate(Type[] types)

if (needCustom)
{
#if FEATURE_COMPILE
return MakeNewCustomDelegate(types);
#else
return TryMakeVBStyledCallSite(types) ?? MakeNewCustomDelegate(types);
#endif
if (LambdaExpression.CanCompileToIL)
{
return MakeNewCustomDelegate(types);
}
else
{
return TryMakeVBStyledCallSite(types) ?? MakeNewCustomDelegate(types);
}
}

Type result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,26 +107,27 @@ private static bool IsByRef(DynamicMetaObject mo)
return mo.Expression is ParameterExpression pe && pe.IsByRef;
}

#if FEATURE_COMPILE
private const MethodAttributes CtorAttributes = MethodAttributes.RTSpecialName | MethodAttributes.HideBySig | MethodAttributes.Public;
private const MethodImplAttributes ImplAttributes = MethodImplAttributes.Runtime | MethodImplAttributes.Managed;
private const MethodAttributes InvokeAttributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.NewSlot | MethodAttributes.Virtual;
private static readonly Type[] s_delegateCtorSignature = { typeof(object), typeof(IntPtr) };
#endif

private static Type MakeNewCustomDelegate(Type[] types)
{
#if FEATURE_COMPILE
Type returnType = types[types.Length - 1];
Type[] parameters = types.RemoveLast();

TypeBuilder builder = AssemblyGen.DefineDelegateType("Delegate" + types.Length);
builder.DefineConstructor(CtorAttributes, CallingConventions.Standard, s_delegateCtorSignature).SetImplementationFlags(ImplAttributes);
builder.DefineMethod("Invoke", InvokeAttributes, returnType, parameters).SetImplementationFlags(ImplAttributes);
return builder.CreateTypeInfo()!;
#else
throw new PlatformNotSupportedException();
#endif
if (LambdaExpression.CanCompileToIL)
{
Type returnType = types[types.Length - 1];
Type[] parameters = types.RemoveLast();
Type[] delegateCtorSignature = { typeof(object), typeof(IntPtr) };

const MethodAttributes ctorAttributes = MethodAttributes.RTSpecialName | MethodAttributes.HideBySig | MethodAttributes.Public;
const MethodImplAttributes implAttributes = MethodImplAttributes.Runtime | MethodImplAttributes.Managed;
const MethodAttributes invokeAttributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.NewSlot | MethodAttributes.Virtual;

TypeBuilder builder = AssemblyGen.DefineDelegateType("Delegate" + types.Length);
builder.DefineConstructor(ctorAttributes, CallingConventions.Standard, delegateCtorSignature).SetImplementationFlags(implAttributes);
builder.DefineMethod("Invoke", invokeAttributes, returnType, parameters).SetImplementationFlags(implAttributes);
return builder.CreateTypeInfo()!;
}
else
{
throw new PlatformNotSupportedException();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ namespace System.Linq.Expressions.Interpreter
{
internal partial class CallInstruction
{
#if FEATURE_DLG_INVOKE
private const int MaxHelpers = 5;
#endif

#if FEATURE_FAST_CREATE
private const int MaxArgs = 3;
Expand Down Expand Up @@ -158,7 +156,6 @@ private static CallInstruction FastCreate<T0, T1>(MethodInfo target, ParameterIn
}
#endif

#if FEATURE_DLG_INVOKE
[return: DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)]
private static Type GetHelperType(MethodInfo info, Type[] arrTypes)
{
Expand Down Expand Up @@ -211,10 +208,8 @@ private static Type GetHelperType(MethodInfo info, Type[] arrTypes)
}
return t;
}
#endif
}

#if FEATURE_DLG_INVOKE
internal sealed class ActionCallInstruction : CallInstruction
{
private readonly Action _target;
Expand Down Expand Up @@ -577,6 +572,4 @@ public override int Run(InterpretedFrame frame)

public override string ToString() => "Call(" + _target.Method + ")";
}

#endif
}
Loading