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

Respect IsDynamicCodeSupported in more places in Linq.Expressions #88539

Merged
merged 4 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.CompilerServices;
using System.Text;
using System.Threading;

namespace System.Dynamic.Utils
{
internal static class DelegateHelpers
{
// This can be flipped to true using feature switches at publishing time
// This can be flipped to false using feature switches at publishing time
internal static bool CanEmitObjectArrayDelegate => true;
Comment on lines -14 to 16
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to change this into

internal static bool CanEmitObjectArrayDelegate => RuntimeFeature.CanEmitObjectArrayDelegate;

If it remains a constant value it will still cause issues for iOS-like platforms as described in: #87924

My last comment: #87924 (comment) breaks down the benefits of having these conditional variables as feature switches.

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 think that would be a good enhancement to make in a future PR. It isn't necessary to fix the issue this PR is fixing - #81803.


// Separate class so that the it can be trimmed away and doesn't get conflated
Expand All @@ -21,14 +22,23 @@ private static class DynamicDelegateLightup
public static Func<Type, Func<object?[], object?>, Delegate> CreateObjectArrayDelegate { get; }
= CreateObjectArrayDelegateInternal();

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",
Justification = "Works around https://github.com/dotnet/linker/issues/2392")]
private static Func<Type, Func<object?[], object?>, Delegate> CreateObjectArrayDelegateInternal()
=> Type.GetType("Internal.Runtime.Augments.DynamicDelegateAugments")!
.GetMethod("CreateObjectArrayDelegate")!
.CreateDelegate<Func<Type, Func<object?[], object?>, Delegate>>();
}

private static class ForceAllowDynamicCodeLightup
{
public static Func<IDisposable>? ForceAllowDynamicCodeDelegate { get; }
= ForceAllowDynamicCodeDelegateInternal();

private static Func<IDisposable>? ForceAllowDynamicCodeDelegateInternal()
=> typeof(AssemblyBuilder)
.GetMethod("ForceAllowDynamicCode", BindingFlags.NonPublic | BindingFlags.Static)
?.CreateDelegate<Func<IDisposable>>();
}

internal static Delegate CreateObjectArrayDelegate(Type delegateType, Func<object?[], object?> handler)
{
if (CanEmitObjectArrayDelegate)
Expand Down Expand Up @@ -186,6 +196,23 @@ private static Delegate CreateObjectArrayDelegateRefEmit(Type delegateType, Func

if (thunkMethod == null)
{
static IDisposable? CreateForceAllowDynamicCodeScope()
{
if (!RuntimeFeature.IsDynamicCodeSupported)
{
// Force 'new DynamicMethod' to not throw even though RuntimeFeature.IsDynamicCodeSupported is false.
// If we are running on a runtime that supports dynamic code, even though the feature switch is off,
// for example when running on CoreClr with PublishAot=true, this will allow IL to be emitted.
// If we are running on a runtime that really doesn't support dynamic code, like NativeAOT,
// CanEmitObjectArrayDelegate will be flipped to 'false', and this method won't be invoked.
return ForceAllowDynamicCodeLightup.ForceAllowDynamicCodeDelegate?.Invoke();
}

return null;
}

using IDisposable? forceAllowDynamicCodeScope = CreateForceAllowDynamicCodeScope();

eerhardt marked this conversation as resolved.
Show resolved Hide resolved
int thunkIndex = Interlocked.Increment(ref s_ThunksCreated);
Type[] paramTypes = new Type[parameters.Length + 1];
paramTypes[0] = typeof(Func<object[], object>);
Expand Down Expand Up @@ -270,8 +297,8 @@ private static Delegate CreateObjectArrayDelegateRefEmit(Type delegateType, Func
ilgen.BeginFinallyBlock();
for (int i = 0; i < parameters.Length; i++)
{
if (parameters[i].ParameterType.IsByRef)
{
if (parameters[i].ParameterType.IsByRef)
{
Type byrefToType = parameters[i].ParameterType.GetElementType()!;

// update parameter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal abstract partial class CallInstruction : Instruction
/// </summary>
public abstract int ArgumentCount { get; }

private static bool CanCreateArbitraryDelegates => true;
private static bool CanCreateArbitraryDelegates => RuntimeFeature.IsDynamicCodeSupported;

#region Construction

Expand Down
52 changes: 44 additions & 8 deletions src/libraries/System.Linq.Expressions/tests/CompilerTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Reflection;
using System.Runtime.CompilerServices;
using System.Text.RegularExpressions;
using Microsoft.DotNet.RemoteExecutor;
Expand Down Expand Up @@ -442,16 +443,51 @@ public static void CompileWorksWhenDynamicCodeNotSupported()

using RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke(static () =>
{
ParameterExpression param = Expression.Parameter(typeof(int));
CompileWorksWhenDynamicCodeNotSupportedInner();
}, options);
}

Func<int, int> typedDel =
Expression.Lambda<Func<int, int>>(Expression.Add(param, Expression.Constant(4)), param).Compile();
Assert.Equal(304, typedDel(300));
// run the same test code as the above CompileWorksWhenDynamicCodeNotSupported test
// to ensure this test works correctly on all platforms - even if RemoteExecutor is not supported
[Fact]
public static void CompileWorksWhenDynamicCodeNotSupportedInner()
{
ParameterExpression param = Expression.Parameter(typeof(int));

Delegate del =
Expression.Lambda(Expression.Add(param, Expression.Constant(5)), param).Compile();
Assert.Equal(305, del.DynamicInvoke(300));
}, options);
Func<int, int> typedDel =
Expression.Lambda<Func<int, int>>(Expression.Add(param, Expression.Constant(4)), param).Compile();
Assert.Equal(304, typedDel(300));

Delegate del =
Expression.Lambda(Expression.Add(param, Expression.Constant(5)), param).Compile();
Assert.Equal(305, del.DynamicInvoke(300));

// testing more than 2 parameters is important because because it follows a different code path in Compile.
Expression<Func<int, int, int, int, int, int>> fiveParameterExpression = (a, b, c, d, e) => a + b + c + d + e;
Func<int, int, int, int, int, int> fiveParameterFunc = fiveParameterExpression.Compile();
Assert.Equal(6, fiveParameterFunc(2, 2, 1, 1, 0));

Expression<Func<int, int, int>> callExpression = (a, b) => Add(a, b);
Func<int, int, int> callFunc = callExpression.Compile();
Assert.Equal(29, callFunc(20, 9));

MethodCallExpression methodCallExpression = Expression.Call(
instance: null,
typeof(CompilerTests).GetMethod("Add4", BindingFlags.NonPublic | BindingFlags.Static),
Expression.Constant(5), Expression.Constant(6), Expression.Constant(7), Expression.Constant(8));

Func<int> methodCallDelegate = Expression.Lambda<Func<int>>(methodCallExpression).Compile();
Assert.Equal(26, methodCallDelegate());
}

private static int Add(int a, int b)
{
return a + b;
}

private static int Add4(int a, int b, int c, int d)
{
return a + b + c + d;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
<!-- Used by VS4Mac via reflection to symbolize stack traces -->
<method name="GetMethodFromNativeIP" />
</type>
<type fullname="System.Reflection.Emit.AssemblyBuilder">
<!-- Used by System.Linq.Expressions via reflection -->
<method name="ForceAllowDynamicCode" />
</type>
<type fullname="System.Runtime.Serialization.SerializationInfo">
<!-- Used by System.Runtime.Serialization.Formatters via reflection -->
<method name="UpdateValue" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ namespace System.Reflection.Emit
{
public abstract partial class AssemblyBuilder : Assembly
{
[ThreadStatic]
private static bool t_allowDynamicCode;

protected AssemblyBuilder()
{
}
Expand Down Expand Up @@ -81,12 +84,40 @@ public override string[] GetManifestResourceNames() =>

internal static void EnsureDynamicCodeSupported()
{
if (!RuntimeFeature.IsDynamicCodeSupported)
if (!RuntimeFeature.IsDynamicCodeSupported && !t_allowDynamicCode)
{
ThrowDynamicCodeNotSupported();
}
}

/// <summary>
/// Allows dynamic code even though RuntimeFeature.IsDynamicCodeSupported is false.
/// </summary>
/// <returns>An object that, when disposed, will revert allowing dynamic code back to its initial state.</returns>
/// <remarks>
/// This is useful for cases where RuntimeFeature.IsDynamicCodeSupported returns false, but
/// the runtime is still capable of emitting dynamic code. For example, when generating delegates
/// in System.Linq.Expressions while PublishAot=true is set in the project. At debug time, the app
/// uses the non-AOT runtime with the IsDynamicCodeSupported feature switch set to false.
/// </remarks>
internal static IDisposable ForceAllowDynamicCode() => new ForceAllowDynamicCodeScope();

private sealed class ForceAllowDynamicCodeScope : IDisposable
{
private readonly bool _previous;

public ForceAllowDynamicCodeScope()
{
_previous = t_allowDynamicCode;
t_allowDynamicCode = true;
}

public void Dispose()
{
t_allowDynamicCode = _previous;
}
}

private static void ThrowDynamicCodeNotSupported() =>
throw new PlatformNotSupportedException(SR.PlatformNotSupported_ReflectionEmit);
}
Expand Down
Loading